Community
Participate
Working Groups
I20100429-1549 and before Every once in a while, my target workbench (with a some projects like jdt.ui checked out from CVS) spends a lot of cycles (dozens of seconds) to refresh the Package Explorer. I've now set a conditional breakpoint at PackageExplorerContentProvider#elementChanged(ElementChangedEvent) line 128 and looked at a case where this method collected about 1800 runnables, which translates into 1800 refreshes in the TreeViewer, which is simply too much. It looks like the problem is that the optimization we implemented for large refreshes like this only works for archives, but not for external class folders. I have a lot of external folders, since PDE puts them on the classpath in the runtime workbench (they reference the bin folders from my host workbench). AFAIK, this scenario is not common for normal plug-in development, but it is used by EMF (see bug 182537). Analysis: ========= PackageExplorerContentProvider#processDelta(IJavaElementDelta, Collection) line 713 checks IJavaElementDelta#getFlags() against F_ARCHIVE_CONTENT_CHANGED, but that flag is not set when an external folder is refreshed. The next test is: if ((flags & (IJavaElementDelta.F_CONTENT | IJavaElementDelta.F_CHILDREN)) == IJavaElementDelta.F_CONTENT) // content change, without children info (for example resource added/removed to class folder package) => I don't think this will ever be true for class folders, since F_CONTENT is only valid for files. Next hope is that the tests at the beginning of PackageExplorerContentProvider#handleAffectedChildren(..) cause an early return, but they don't, since they only check for ADDED and REMOVED, but not CHANGED. In the end, every single class file is refreshed! My current thinking is that we should: - remove IJavaElementDelta.F_CONTENT from the cited test (and the second place where we have the same bug), and probably add a check for IPFR#isExternal() - check whether we should change the loops at the beginning of PackageExplorerContentProvider#handleAffectedChildren() to also count changes and refresh the parent if there were more than a few children changed -
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.
