-
Notifications
You must be signed in to change notification settings - Fork 171
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
AnnotationModel uses inefficient iteration over map, take 2 #980
Conversation
@szarnekow : just in case you have time, please check this patch. I hope this addresses your (right) concerns on original attempt to fix the issue in eclipse-platform/eclipse.platform.text#22. |
Note, build failure is due #981. |
bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java
Outdated
Show resolved
Hide resolved
Test Results 852 files ±0 852 suites ±0 1h 10m 26s ⏱️ - 1m 45s For more details on these failures, see this check. Results for commit b859a00. ± Comparison against base commit 1bd79e4. ♻️ This comment has been updated with latest results. |
bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java
Outdated
Show resolved
Hide resolved
AnnotationModel.cleanup() will lock on the map while iterating over map and iterate via forEach() instead to iterate over keys if possible. This halves iteration time spent in cleanup() on huge annotation models. Fixes eclipse-platform#892
@szarnekow : please check last version. |
Change looks safe, and I think it's an improvement. How much of a difference do you see in your benchmarks? |
Something around 40% less time spent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks Sebastian! It is really nice to have someone asking right questions :-) |
Synchronize on the map lock if possible and iterate via forEach() over the map.
This halves iteration time spent in cleanup() on huge annotation models.
Fixes #892
For test / use case that shows performance issue see this comment.