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 enhancements related to issue #772 #775

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

benlamonica
Copy link

Unfortunately I don't have the time to continue pursuing the solution to this problem, but I thought I would send the small code changes that I made that improved performance in this extreme situation.

@benlamonica
Copy link
Author

Removed the hashCode/equals change, as it broke tests.

@cbeust
Copy link
Collaborator

cbeust commented Aug 17, 2015

@juherr Thoughts on this PR?

@juherr
Copy link
Member

juherr commented Aug 17, 2015

@cbeust I never feel good perf improvments without something concrete to evaluate it.

@benlamonica Could you add a test based on your ContentionTest from #772? How many time should it take before and after the fix? We can imagine the new test should fail if it takes too many time.

@cbeust
Copy link
Collaborator

cbeust commented Aug 17, 2015

@juherr Agreed.

Unfortunately, @benlamonica says that he can't work on this any more so it's up to us to follow up or close this, and I agree with you that I wouldn't feel comfortable merging this until we can run some numbers and prove that it improves performance significantly.

@juherr
Copy link
Member

juherr commented Aug 18, 2015

@cbeust I looked further and the test case is interesting. It showed me that it has many problems but I don't know how to fix them for the moment.
I suppose it will involve many deep changes.

For the current PR, I've no idea what to do. We just know it doesn't broke something (for the moment) and it helps @benlamonica. But it is clear we should keep #772 open.

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.

None yet

3 participants