Community
Participate
Working Groups
3.3 M5 While looking into performance of our decorator, I noticed lots of overhead in computing the problem decorations in JDT, due to its use of IResource.findMarkers to determine the max problem severity. For example, one of our projects has 457 warnings. In order to determine that a warning decoration is needed for the project, 457 Marker objects get created. This could be reduced by providing a more specialized API that determines the max severity across markers on a resource.
Created attachment 60832 [details] Patch providing findMaxSeverity API
Opened bug 177390 for the change to JDT to use this. PDE may want to use it too for the error check before launch.
This would certainly be useful. The check for compiler errors before launching is done at the platform/debug level. PDE gets it for free. cc DarinW.
Here are some stats on the improvement: decorating ~600 items in project; 457 warnings across all items normal development state with all components loaded (many warnings, no errors) Merged callees for ProblemsLabelDecorator.getErrorTicksFromMarkers() before: 7427 GCed objects, 284632 GCed size after (using recursion on packages): 1560 GCed objects, 62200 GCed size after (visitor, no recursion): 648 GCed objects, 26048 GCed size - improvement: 91% less garbage Merged callees for JavaUILabelProvider.getImage(Object) (625 calls) before: 21637 GCed objects, 710728 GCed size after (visitor, no recursion): 14734 GCed objects, 447152 GCed size - improvement: 37% less garbage Regarding "using recursion on packages" vs. "visitor, no recursion": this has to do with the decision on when to use recursiveFindMaxSeverity vs. visitorFindMaxSeverity in MarkerManager.findMaxProblemSeverity. The stats show that it's better to always use the visitor, except for the file case. This is because asking for the child resources creates IResource objects (and paths, and arrays) which are not needed in the result.
The profile shows that 14K of the remaining 26K of garbage is in creating the non-sparse array from the sparse array representation in MarkerSet.getElements(). This could be avoided if it could iterate directly over the sparse representation (skipping nulls). This could also be done in findMarkers.
Note that when I took the profile, I had warmed things up first, so the java model and its decorated images had already been created once. My steps were: - close and reopen the package explorer - expand one of our projects (containing ~600 items, with 457 warnings across these) - start the profile recording all object allocations - expand src - expand each package - wait for garbage creation to settle down (according to the heap status indicator) - stop the profile The stats above are from examining the profiles before and after the code change.
I filed bug 177393 for the Platform/Debug case.
I considered providing an API that returned the counts for INFO, WARNING and ERROR severities. This would be a bit more general, e.g. it could perhaps be used in the Problems view's display of marker counts. However, this one wouldn't be able to shortcut when it hits an ERROR, like findMaxSeverity does.
I wrote a little benchmark to measure the time improvement. The test created a workspace with 8,000 files in 400 folders, with 10 markers on each resource (120,000 markers). 100 invocations of an equivalent method to find max severity using old API took an average of 13.48 seconds. With Nick's new method, the elapsed time was 3.64 seconds. I tried also optimizing the MarkerSetElement[] creation, but it didn't have any further impact on time (I guess the JIT optimizes this kind of short term garbage very well).
+1
Nick, one minor question: Is there a reason you commented out the expression in this line: if (/*depth == IResource.DEPTH_INFINITE && */target.getType() != IResource.FILE)
Re comment #11 - I assume this must have been a mistake, since the depth one and zero cases are broken with this expression commented out. The visitor doesn't handle any depth other than infinite.
Released. Correctness tests added to MarkerTest. Performance tests added to BenchWorkspace. Thanks Nick!
I had commented out the depth check while trying to determine whether it's more efficient to use the visitor or the recursive method (see stats in comment 4). However you're correct -- despite being faster, it's incorrect ;-) since the visitor doesn't handle depths other than infinite. So your fix to put the code back is good. Thanks for reviewing and releasing it, not to mention the extra benchmark.
I can confirm that having the depth check commented out was definitely a bug. Test case: - create packages a and a.b, with class Foo with an error in a.b - Foo and a.b should have an error marker, but a should not - a incorrectly gets an error marker with the depth check commented out It works OK when the depth check is reinstated.
John, in your benchmark did you use errors or warnings? If there were errors, the new API will short-circuit, which could hide any improvement from optimizing out the MarkerSetElement[] creation.
I used only warnings. The result was really a mystery to me, but it was slightly faster *without* optimizing out the MarkerSetElement[] creation. I will attach a patch for this additional optimization if you're curious. The benchmark is released in the BenchWorkspace class in org.eclipse.core.tests.resources.
Created attachment 60966 [details] Patch with additional optimization
The optimization looks good. I can't explain why you're not seeing any speedup with it. Without the optimization, your bench should be generating 100 * (8000 * (12 + 10*4)) ~= 40M of garbage. Of course, this should all get scavenged very quickly, but I'd still expect to see some overhead.
It's -possible- that the JIT does escape analysis and realizes (after inlining) that the array does not live beyond the method in which it's created, and thus stack-allocates it. But I don't think our JIT does escape analysis.
So in the "learn something new every day" category, one of our lead engineers on the VM team reports: "Yes, the JIT does very agressive escape analysis, and has done for a couple of releases." John, which VM were you using?
J9, of course! That's cool to hear. It goes to show that the only way to optimize is to write benchmarks to test your changes - you can never tell what changes will actually impact performance.
