Bugzilla – Full Text Bug Listing
| Summary: | [package explorer] Performance problem with refreshing external class folders | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Markus Keller <markus.kell.r> | ||||||||||||
| Component: | UI | Assignee: | Markus Keller <markus.kell.r> | ||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||
| Severity: | major | ||||||||||||||
| Priority: | P2 | CC: | daniel_megert, deepakazad, heiko.boettger, Olivier_Thomann, vkhomyackov | ||||||||||||
| Version: | 3.6 | Keywords: | performance | ||||||||||||
| Target Milestone: | 3.7.2 | Flags: | deepakazad:
review+
|
||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows XP | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Markus Keller
Created attachment 167029 [details]
Breakpoints
OK, there are quite a few design problems here, which we can't fix all so late in the game:
- we have no flag like F_ARCHIVE_CONTENT_CHANGED that shows when the whole class folder has been refreshed
- the content provider traverses deltas deeply and refreshes every single class file, even if all children of an element contain changes
- the content provider traverses deltas and posts refresh requests even if an element has not been created in the Tree (it doesn't talk to the viewer)
Here are steps to reproduce the problem:
- new workspace
- import org.eclipse.jdt.ui from CVS
- launch target workbench
- in target:
- import org.eclipse.jdt.junit from CVS
- create a Java working set WS and put the project into WS
- Package Explorer > Top Level Elements > Working Set
- exit target
- clean all projects
- set attached breakpoints
- debug target
- expand WS and select the org.eclipse.jdt.junit project
- File > Refresh (F5)
=> when you reach the first breakpoint, you can see how the PackageExplorerContentProvider processes all IClassFiles in the bin folder
- Resume (F8)
=> when you reach the second breakpoint, you can see how many pendingUpdates are going to be processed.
- Step Over (F6) once
=> On my machine, 7.5s are burned in calls to fViewer.refresh(..), most of that in getSelection(..) (4.3s) and setSelectiontoWidget(..) (2.6s)
Created attachment 167038 [details]
fix candidate
It looks scary to fix the architectural issues in RC1. The safest fix I could come up with is to leave the processing as is, but just avoid the expensive preservingSelection(..) calls by testing first whether there really exists a TreeItem for the element. That check is cheap (a hash table lookup), and with this patch, the time spent goes down to 0.35s.
In my runtime workbench, the UI lockup on full refresh goes down to less than 4s (for 36000 pending updates).
Created attachment 167167 [details]
fix candidate 2
First patch was not good (e.g. did not show new project).
This patch does not change postAdd(..).
Too late to fix this now, especially since we didn't get bugs about this and external class folders are available since 3.4. Punting to 3.7 (or 3.6.1). Created attachment 193605 [details]
Updated fix 2
The fundamental problems are:
- A full build of a project creates lots of individual class file changes rather than a clear indicator at the root (class folder) that everything should be refreshed (like for a JAR).
- JFace viewers have no mechanism to collect and batch updates, so each client has to implement this again.
Since we cannot fix these flaws now, I'll go with the fix candidate 2.
Fixed in HEAD. The fix caused 7 test failures in PackageExplorerTests because the MockPluginView doesn't really show any tree elements, and thus the query for realized elements failed. Fixed in HEAD of org.eclipse.jdt.ui.tests.packageview.MockPluginView. We're very late in the 3.7 cycle for such a general change in an important view like the Package Explorer / Project Explorer. I'll revert this for 3.7, but release it early in 3.8 with the option to backport it to 3.7.1. Created attachment 193718 [details]
Reverse patch with test fixes
postRefresh(..) also needs a closer look:
- Object[] elements should be moved into the Runnable
- behavior for null element could be wrong (if that ever happens). Fix would be to use "if (element == null || fViewer.testFindItems(element).length > 0)"
Fixed in master with commit 2b6ee74f15b0c815f9efa8b1b6d919f92bfba01e. Reopening for 3.7.2 consideration. Would also be nice to have a performance test for that. *** Bug 359569 has been marked as a duplicate of this bug. *** Fixed in R3_7_maintenance with commit de2543cd01b0d83870f6ea00adcf1e797e970ad1 and commit bb8e1514d4d96d51cfba8839a4ab4a21aacf9bb1. Performance test in master: commit e840eccd2cc9454521d8928c4b7835ab671c2ee8 Performance test in perf_37x: commit 848d30296606e490ced5d96b56391912e69dedf5 We should review the fix since it goes into 3.7.2. Deepak, please review. Fix looks good to me. Verified in M20120111-0800: - imported *.class files from rt.jar into a class folder (important: don't have any resources in the class folder) - expanded the class folder and a few packages in the Package Explorer - touched all *.class files with cygwin: find . -type f | xargs touch - refreshed the project Refresh locked the UI for about 30 seconds in 3.7.1. M20120111-0800 has no lag any more. |