Community
Participate
Working Groups
Build Identifier: I20110613-1736 We have been seeing instances where a trigger of DeltaProcessor.createExternalArchiveDelta() is hanging Eclipse for multiple minutes for projects with large classpaths (~5000 external jars). That function loops over all of the library jars in the classpath. In that loop, if a jar has changed it calls IJavaProject.resetResolvedClasspath() "in case it contains a chained jar" (this was introduced in 1.330 to fix http://bugs.eclipse.org/198572). The problem we are seeing is that on a big update where nearly all of the jars on the classpath are being updated, IProject.resolveClasspath() is reconstructing the resolved classpath needlessly over and over again, causing O(M*N) accesses to jar files, where N is the number of jars on the classpath and M is the number of changed jars. The attached patch removes the call to IProject.resolveClasspath() from inside the loop and instead keeps a boolean value that tracks whether JavaModelManager.isNonChainingJar() ever returns true and calls IProject.resolveClasspath() only if needed. isNonChainingJar() is backed by a HashSet. I tried to follow all of the code paths to see if there would be any side effects to delaying the resolving of chained jars completely outside of the loop. The bottom half of the loop seemsonly to be creating the delta tree, and the upper part of the loop is only scheduling rather than performing indexer updates, so it looked safe to pull out to me, but I may have missed something. I did some timing tests on a contrived example where I coppied a single jar hundreds to thousands of times and added all of those copies to the classpath, then updated all of those jar copies with a different jar, and then immediately called IJavaModel.refreshExternalArchives() (whether DeltaProcessor.createExternalArchiveDelta() actually sees changed jar files seems to depend on whether an earlier call to DeltaProcessor.resourceChanged() runs first over all the "classpath change deltas" before the "external archive delta" runs. @HEAD PATCHED 100 jars updated: 28 23 100 jars updated: 21 28 100 jars updated: 28 41 5000 jars updated: 11617 2419 5000 jars updated: 11644 2841 5000 jars updated: 12376 2686 That is a 4x speed up. In a more realistic project this change brought the number from ~2 minutes down to 4 seconds. Reproducible: Always Steps to Reproduce: 1. Duplicate a jar 5000 times 2. IProject.setRawClasspath() with those jars 3. Change those jars copies to a different jar 4. Call jproject.getJavaModel().refreshExternalArchives()
Created attachment 201207 [details] Moves the call to IJavaProject.resetResolvedClasspath() out of DeltaProcessor.createExternalArchiveDelta()'s loop
Jay, please follow up. Thanks.
In an isolated performance test, I see about 36-42% improvement in terms of elapsed process time when I tried with 10 and 100 jars respectively. Good work! I will take a look at the patch closely again and see if this will have any side-effects.
Patch looks good to me. Besides all existing tests pass. Satyam, what do you think? Do we need a new performance test for this scenario?
(In reply to comment #4) > Patch looks good to me. Besides all existing tests pass. > Satyam, what do you think? Do we need a new performance test for this scenario? It will be good to have a test for this.
I have a performance test at hand, which I will add to the patch after some clean-up and one more round of comparison runs.
Created attachment 202867 [details] Patch with performance test The same fix with new performance test. Here are some numbers from the test for refreshing 500 jars: ================= Without Fix: ================= Scenario 'org.eclipse.jdt.core.tests.performance.FullSourceWorkspaceModelTests#testRefreshExternalArchives()' (average over 10 samples): Used Java Heap: 10.11M (95% in [6.35M, 13.88M]) Measurable effect: 6.66M (1.3 SDs) (required sample size for an effect of 5% of mean: 1735) Working Set: -33.2K (95% in [-132.85K, 66.45K]) Measurable effect: 176.19K (1.3 SDs) (required sample size for an effect of 5% of stdev: 6401) Committed: -65.2K (95% in [-257.83K, 127.43K]) Measurable effect: 340.6K (1.3 SDs) (required sample size for an effect of 5% of stdev: 6400) Working Set Peak: 0 (95% in [0, 0]) Elapsed Process: 21.14s (95% in [20.35s, 21.94s]) Measurable effect: 1.4s (1.3 SDs) (required sample size for an effect of 5% of mean: 18) Kernel time: 13.94s (95% in [13.3s, 14.59s]) Measurable effect: 1.14s (1.3 SDs) (required sample size for an effect of 5% of mean: 28) Page Faults: 74 (95% in [26, 122]) Measurable effect: 85 (1.3 SDs) (required sample size for an effect of 5% of mean: 5273) CPU Time: 19.97s (95% in [19.05s, 20.88s]) Measurable effect: 1.62s (1.3 SDs) (required sample size for an effect of 5% of mean: 27) GDI Objects: 0 (95% in [0, 0]) ============= With Fix: ============= Scenario 'org.eclipse.jdt.core.tests.performance.FullSourceWorkspaceModelTests#testRefreshExternalArchives()' (average over 10 samples): Used Java Heap: 23.08M (95% in [20.53M, 25.63M]) Measurable effect: 4.51M (1.3 SDs) (required sample size for an effect of 5% of mean: 153) Working Set: 14.4K (95% in [-34.78K, 63.58K]) Measurable effect: 86.96K (1.3 SDs) (required sample size for an effect of 5% of stdev: 6400) Committed: 59.6K (95% in [-60.71K, 179.91K]) Measurable effect: 212.74K (1.3 SDs) (required sample size for an effect of 5% of stdev: 6401) Working Set Peak: 0 (95% in [0, 0]) Elapsed Process: 8.43s (95% in [8.31s, 8.55s]) Measurable effect: 216ms (1.3 SDs) Kernel time: 6.2s (95% in [5.97s, 6.43s]) Measurable effect: 408ms (1.3 SDs) (required sample size for an effect of 5% of mean: 18) Page Faults: 37 (95% in [22, 52]) Measurable effect: 26 (1.3 SDs) (required sample size for an effect of 5% of mean: 1991) CPU Time: 8.25s (95% in [7.94s, 8.55s]) Measurable effect: 541ms (1.3 SDs) (required sample size for an effect of 5% of mean: 18) GDI Objects: 0 (95% in [0, 0])
Released in HEAD with performance test. Thanks for the patch, Terry!
Thanks for getting to the patch so quickly, Jay. FYI,I patched this into the 3.7.0 JDT release and pushed it to all of our internal Google developers, so this will get a fair bit of usage before the 3.8 release.
Reopening the bug since it's causing the regression bug 357425.
Moving to M3. Will be closed again once bug 357425 is fixed.
(In reply to comment #11) > Moving to M3. Will be closed again once bug 357425 is fixed. Jay, does this need to stay open anymore ?
(In reply to comment #12) > Jay, does this need to stay open anymore ? No, it doesn't. As bug 357425 has been resolved, this bug can also be marked resolved.
Verified for 3.8 M3 using build N20111022-2000 For performance data I'm relying on comment 7. The regression reported in bug 357425 has been fixed and verified, too.
Can we get this patch and the one in bug 357425 applied to the 3.7 maintenance branch as well?
(In reply to comment #15) > Can we get this patch and the one in bug 357425 applied to the 3.7 maintenance > branch as well? Dani, what do you think? This is a performance improvement.
(In reply to comment #16) > (In reply to comment #15) > > Can we get this patch and the one in bug 357425 applied to the 3.7 maintenance > > branch as well? > > Dani, what do you think? This is a performance improvement. -1 to backport. It's not fixing a critical issue and already caused a regression. Also, it looks like the performance test does not work as it was probably not backported to the reference branch. If so, please do so now.
(In reply to comment #17) > Also, it looks like the performance test does not work as it was probably not > backported to the reference branch. If so, please do so now. The performance test is already present in R3_7_maintenance, but looks like perf_37x branch has not been created yet. I am creating it now.
(In reply to comment #18) > The performance test is already present in R3_7_maintenance, but looks like > perf_37x branch has not been created yet. I am creating it now. I have pushed the new performance test to perf_37x branch.
(In reply to comment #19) > I have pushed the new performance test to perf_37x branch. The perf_37x branch has been tagged with the performance test and maps files updated accordingly. We should see the test results from next build.
(In reply to comment #20) > (In reply to comment #19) > > I have pushed the new performance test to perf_37x branch. > > The perf_37x branch has been tagged with the performance test and maps files > updated accordingly. We should see the test results from next build. Nope. It looks like you tagged just today, hence we have to wait for the next baseline build (usually on Friday).
