Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvement in annotation lookup #848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

testn
Copy link
Contributor

@testn testn commented Nov 1, 2015

  • Annotation performs deep equals and hashCode. Wrapping it with AnnotationWrapper to use System.identityHashCode and == makes it significantly faster especially tests with a large number of data providers like the issues in Severe thread contention while running large test with parallel methods #772
  • Add NULL_MARKER to mark when a method does not have the annotation. It will save the lookup in such case.

@juherr
Copy link
Member

juherr commented Nov 1, 2015

Do you have any tests or benchmarks to share?

@testn
Copy link
Contributor Author

testn commented Nov 1, 2015

I ran a cutdown version of #772 by changing it to 6,000 data providers. It seems to reduce the test time by 6 seconds.

@juherr
Copy link
Member

juherr commented Nov 1, 2015

If the test only takes 6sec, I think you can add it :)

@testn
Copy link
Contributor Author

testn commented Nov 1, 2015

Well... what it cuts down is the time it takes to initialize the tests which take much more than the test time itself!

@juherr
Copy link
Member

juherr commented Nov 1, 2015

Ok, but you can share it even if it won't be run in the suite each time.
I think it is important to have something to check the fix now and something to check perf regressions later.

@testn
Copy link
Contributor Author

testn commented Nov 1, 2015

That's fine. It does not change the behavior. This is just to improve the caching behavior in JDK15AnnotationFinder without changing anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants