Community
Participate
Working Groups
Created attachment 175190 [details] Profiler screenshot (1) Switching the Java editor is not fast enough. This is caused by too many context updates (and Computation invalidations). The number of updates should be reduced. See profiler screenshots (1) and (2).
Created attachment 175191 [details] Profiler screenshot (2) Back traces for method Computation.handleInvalid(...).
One million invocations of handleInvalid()??? Surely, switching between editors must be possible with at most 100.000 invocations. :-P I suspect that we're leaking RunAndTracks somewhere. How many editors did you open? Does the number of perspective switches have an effect on the invocation count?
There are a lot of places where listeners on the IEclipseContexts are leaked. To find these leaks, do the following: 1. Open the 4.0 workbench and set up the initial editors and perspectives (e.g. open two Java editors) 2. Set a breakpoint in EclipseContext.handleInvalid Note: The debugger will immediately suspend at that point. 3. Clear the console output 4. Run/evaluate the following code in the Display view: ArrayList<String> lines = new ArrayList<String>(); for (int i = 0; i < ls.length; i++) { lines.add(ls[i].toString()); } Collections.sort(lines); for (int i = 0; i < lines.size(); i++) { System.out.println(lines.get(i)); } 5. Copy the console output to a diff tool (e.g. WinMerge) 6. Remove the breakpoint and hit F8 7. Perform some actions in the 4.0 workbench (e.g. switch the Java editor 20 times) 8. Repeat steps 3, 4, and 5. 9. Compare the two outputs in the diff tool (e.g. WinMerge) Another trick: Add the following line to EclipseContext.handleInvalid: System.out.println("EclipseContext.listeners.size = " + ls.length); Then do some actions in the 4.0 workbench and watch how the number of listeners increases. Here are my findings using the methods described above: - Switching the Java editor: two listeners (ValueComptation) leaked --> handler::org.eclipse.ui.navigate.next --> handler::org.eclipse.ui.navigate.previous - Switching between the Java and Debug perspectives: TrackableComputationExt with a ContextInjectionListener leaked Now I'll use the YourKit profiler to find out more details about the leaked listeners (by advancing the 'Object Generation Number' and then using 'Object Allocation Recording').
Created attachment 175211 [details] Screenshot: Stack trace for leaking EventBroker (with injected logger) Here's one leak: When switching the perspective, a new EventBroker will be created. This EventBroker has an injected logger, for which a TrackableComputationExt with a ContextInjectionListener will be created. This listener will never be removed.
Stefan, do you see any differences in the leaks if you close down "reactive" views like 'Outline' or 'Properties'? Since they react to the active editor, I'm wondering if something's going wrong in that department when you're switching editors.
I opened bug 320887 for the PageSite problem.
(In reply to comment #4) > Screenshot: Stack trace for leaking EventBroker (with injected logger) Please don't follow this track; it might be wrong. One difficulty in finding the cause of this bug is the WeakReference in class Requestor. In the profiler could cannot see the referenced object.
I think one of the issues with setGlobalActionHandler(String, IAction) is that we deactivate the originally defined handler first and then we proceed to activate the new one. Hence, for every call, we do two invalidations, one for removing the [original] handler and another for adding the [new] handler.
Remy, it seems that my track with the EventBroker is right. The EventBrokers are held by PartServiceImpl. When switching the perspective, one or more PartServiceImpl objects will be created. However, there doesn't seem to be a dispose method for the PartServiceImpl. Method preDestroy (where I tried to set field 'eventBroker' to null) isn't invoked. Any ideas?
(In reply to comment #9) > When switching the perspective, one or more > PartServiceImpl objects will be created. I am observing this behaviour though am not sure why. It seems the editor's context is constantly making new implementations. I would've just expected there to be one of them instead of constantly spawning new ones. > However, there doesn't seem to be a > dispose method for the PartServiceImpl. Method preDestroy (where I tried to set > field 'eventBroker' to null) isn't invoked. Any ideas? It will get disposed when you actually _close_ the editor. At least, that is the case for me.
Remy, when a part is activated, the ContributedPartRenderer will try to get the EPartService from the context using the following name: org.eclipse.e4.ui.workbench.modeling.EPartService However, none of the contexts contains the desired value, so that eventually the PartServiceCreationFunction (fetched by the root context via the OSGiContextStrategy) creates a new one.
(In reply to comment #11) > However, none of the contexts contains the desired value, so that eventually > the PartServiceCreationFunction (fetched by the root context via the > OSGiContextStrategy) creates a new one. That might be a separate problem. I am getting EPSes instantiated during the switch itself (instead of activation). Thread [main] (Suspended (breakpoint at line 160 in PartServiceImpl)) PartServiceImpl.postConstruct() line: 160 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 79 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43 Method.invoke(Object, Object...) line: 618 MethodRequestor.execute() line: 47 InjectorImpl.processAnnotated(Class<Annotation>, Object, Class<?>, PrimaryObjectSupplier, PrimaryObjectSupplier, ArrayList<Class<?>>) line: 795 InjectorImpl.inject(Object, PrimaryObjectSupplier, PrimaryObjectSupplier) line: 104 InjectorImpl.internalMake(Class<?>, PrimaryObjectSupplier, PrimaryObjectSupplier) line: 292 InjectorImpl.make(Class<T>, PrimaryObjectSupplier) line: 219 ContextInjectionFactory.make(Class<T>, IEclipseContext) line: 152 PartServiceCreationFunction.compute(IEclipseContext) line: 32 ValueComputation.get() line: 122 EclipseContext.internalGet(EclipseContext, String, boolean) line: 254 EclipseContext.internalGet(EclipseContext, String, boolean) line: 262 EclipseContext.internalGet(EclipseContext, String, boolean) line: 262 EclipseContext.internalGet(EclipseContext, String, boolean) line: 262 EclipseContext.internalGet(EclipseContext, String, boolean) line: 262 EclipseContext.get(String) line: 215 ContextObjectSupplier.get(IObjectDescriptor[], Object[], IRequestor, boolean, boolean) line: 148 InjectorImpl.resolveArgs(Requestor, PrimaryObjectSupplier, PrimaryObjectSupplier, boolean, boolean) line: 378 InjectorImpl.resolveRequestorArgs(ArrayList<Requestor>, PrimaryObjectSupplier, PrimaryObjectSupplier, boolean, boolean) line: 333 InjectorImpl.resolveArguments(IRequestor) line: 307 FieldRequestor(Requestor).resolveArguments() line: 108 ContextObjectSupplier$ContextInjectionListener.update(IEclipseContext, int, Object[], IContextRecorder) line: 70 TrackableComputationExt.update(ContextChangeEvent) line: 90 EclipseContext.processScheduled(List<Scheduled>) line: 333 EclipseContext.setParent(IEclipseContext) line: 394 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 296 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 335 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 341 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 341 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 341 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 341 PerspectiveStackRenderer(LazyStackRenderer).showElementRecursive(MUIElement, List<MUIElement>) line: 341 PerspectiveStackRenderer(LazyStackRenderer).showTab(MUIElement) line: 150 PerspectiveStackRenderer.showTab(MUIElement) line: 109 LazyStackRenderer$1.handleEvent(Event) line: 75 UIEventHandler.handleEvent(Event) line: 41 EventHandlerWrapper.handleEvent(Event, Permission) line: 188 EventHandlerTracker.dispatchEvent(Object, Object, int, Object) line: 198 EventManager.dispatchEvent(Set, EventDispatcher, int, Object) line: 227 ListenerQueue.dispatchEventSynchronous(int, Object) line: 149 EventAdminImpl.dispatchEvent(Event, boolean) line: 139 EventAdminImpl.sendEvent(Event) line: 78 EventComponent.sendEvent(Event) line: 39 EventBroker.send(String, Object) line: 73 UIEventPublisher.notifyChanged(Notification) line: 58 PerspectiveStackImpl(BasicNotifierImpl).eNotify(Notification) line: 380 PerspectiveStackImpl.setSelectedElement(MPerspective) line: 134 PerspectiveStackImpl.setSelectedElement(MUIElement) line: 1 PerspectiveSwitcher$11.widgetSelected(SelectionEvent) line: 343
Your stack trace is almost the same as mine. Notice that the 'get' on the contexts fails, so that eventually a new PartServiceImpl is created.
(In reply to comment #13) > Notice that the 'get' on the > contexts fails, so that eventually a new PartServiceImpl is created. Yes, the computations are being invalidated...for some reasons. Try putting a print statement in SelectionServiceImpl's postConstruct() method. Even if you stay within the 'Package Explorer', just changing pressing the up/down arrow keys will cause a new implementation to be created. As the context chain itself shouldn't have changed at all, I am at a loss as to why the context function's computation was invalidated.
(In reply to comment #14) > Try putting a print statement in SelectionServiceImpl's postConstruct() method. > Even if you stay within the 'Package Explorer', just changing pressing the > up/down arrow keys will cause a new implementation to be created. As the > context chain itself shouldn't have changed at all, I am at a loss as to why > the context function's computation was invalidated. Just put a breakpoint in ValueComputation.doHandleInvalid() to find out. Then press up/down again and your breakpoint will hit. Remy, it seems to me that this bug is very serious and should be marked a blocker.
Pressing up/down will change the selection, and this will change 'output.selection' in the context. The ESelectionService is registered as a dependency on 'output.selection' and will be invalidated. IMHO, this is wrong. The service needs to be notified that one of the values it provides has been updated, but the service itself is still valid.
(In reply to comment #15) > Just put a breakpoint in ValueComputation.doHandleInvalid() to find out. Then > press up/down again and your breakpoint will hit. Oh, I see the breakpoint being hit, I just don't understand the context code at all to understand the why. ;) (In reply to comment #16) > The ESelectionService is registered as a > dependency on 'output.selection' and will be invalidated. IMHO, this is wrong. Yes, this is the kind of analysis that I am incapable of performing. :o
The PartServiceImpl has exactly the same problem, namely, that the ValueComputation is invalidated when it probably shouldn't. When switching the perspective, the contexts of the views will be reparented (e.g. from the Java perspective's context to the Debug perspective's context). This will cause the name "org.eclipse.e4.ui.workbench.modeling.EPartService" to be invalidated, so that a new one will be created the next time it is queried.
When a PartServiceImpl is created, it will register itself in method postConstruct() at the event broker, which in turn will register a UIEventHandler as a service; this handler references the PartServiceImpl, so that it cannot be GC'ed and thus its ContextInjectionListeners will not be removed. The only way for the UIEventHandler to be removed from the ServiceRegistry is when method PartServiceImpl.preDestroy() will be called. Method PartServiceImpl.preDestroy() will be called when the context is disposed. This will happen on workbench shutdown. Only then will PartServiceImpl become GC'able and the ContextInjectionListeners be removed. Possible solutions: (1) make sure that PartServiceImpl will be destroyed earlier than the dispose() method of the originating EclipseContext; possible? (2) prevent that the PartServiceImpl objects are re-created
Created attachment 175231 [details] Tests patch v1 This patch adds test for the ESS problem.
Created attachment 175232 [details] patch to experiment with This prints out the number of handleInvalid calls (why am I not seeing a million calls here?) and whenever a new selection service, part service, or event broker is created.
aha - I need Computation.handleInvalid, hang on... new patch will be up in a minute
Created attachment 175235 [details] patch to experiment with v2
This is still under investigation.
Created attachment 175242 [details] fix for too many SelectionServiceImpl etc. This fixes the too many SelectionServiceImpl/PartServiceImpl/EventBroker problem. However, I am still seeing monotonically increasing invocations of Computation.handleInvalid() on every editor switch (this is switching between editors that I had switched to before): handleInvalid: 442759 handleInvalid: 443577 handleInvalid: 444381 handleInvalid: 445171 handleInvalid: 445947 handleInvalid: 446709 handleInvalid: 447457 This means we're adding between 700 and 800 invocations every time you switch an editor. We must have another leak.
(In reply to comment #25) > Created an attachment (id=175242) [details] > fix for too many SelectionServiceImpl etc. This patch results in compile errors. Boris, have you changed Eclipse.currentComputation to public?
(In reply to comment #26) > (In reply to comment #25) > > Created an attachment (id=175242) [details] [details] > > fix for too many SelectionServiceImpl etc. > > This patch results in compile errors. Boris, have you changed > Eclipse.currentComputation to public? Sorry, updated patch is on its way.
Created attachment 175243 [details] fix for too many SelectionServiceImpl etc.v2 This time, using the public getter.
(In reply to comment #3) > - Switching the Java editor: > two listeners (ValueComptation) leaked > --> handler::org.eclipse.ui.navigate.next > --> handler::org.eclipse.ui.navigate.previous Boris, I think this is not yet fixed.
(In reply to comment #29) > Boris, I think this is not yet fixed. Yes, I know, my patch only fixed one of the leaks. I had hoped you could help me find another cause of leaks.
(In reply to comment #29) > Boris, I think this is not yet fixed. Well, I don't think this accounts for the hundreds of invocations we are seeing.
Setting target to 4.1 to make sure this bug appears in search results.
Created attachment 176897 [details] Tests patch to reproduce the problem v2 Updated patch with both a "complicated" and a simplified test. They pass with attachment 175243 [details] applied. Whether that is the right fix or not I cannot say.
Remy, I also have a (very simple) test case. Shortly before the 4.0 relase I was hunting this bug together with Boris. I can explain the underlying problem, but I cannot fix it because it seems to be a bug by design. I want to discuss this with Boris when he's back from vacation.
(In reply to comment #34) > Remy, I also have a (very simple) test case. Could you attach it to this bug?
Created attachment 176909 [details] Patch: DependenciesLeakTest.java The patch contains the test case for reproducing a listener leak caused by *tracked* dependencies. I will provide some more details about this...
Created attachment 176917 [details] Diagram with explanations Here are some more details about the problem.
(In reply to comment #28) > Created an attachment (id=175243) [details] > fix for too many SelectionServiceImpl etc.v2 > > This time, using the public getter. We haven't applied this patch yet, or have we?
(In reply to comment #38) > (In reply to comment #28) > > Created an attachment (id=175243) [details] [details] > > fix for too many SelectionServiceImpl etc.v2 > > > > This time, using the public getter. > > We haven't applied this patch yet, or have we? Negative.
Created attachment 177133 [details] Patch I nested updates There are more than one problem here; this patch addresses the problem that Boris and Remy found - what I would call the nested updates issue. The patch "fix for too many SelectionServiceImpl etc.v2" starts on the right track, but addresses the consequences, rather then the cause. The base issue is that we record too many dependencies. At some point in the past we decided not to record dependencies that occur when executing a method. That way, the dependencies on the method arguments will be created, but not dependencies on anything used by the method body. What I did not realize at that time that this stance should be applied more generally - for instance, it should apply to the code we execute in constructors and @PostConstruct methods. The attached patch takes existing concept of IContextRecorder and changes it into generally available set { resume | pause } recording on the PrimaryObjectSupplier. I moved one of the Remy's tests into the DI tests and added a test for the constructors.
Patch "Patch I nested updates" applied to CVS Head; next on to the comment 36 ...
Created attachment 177142 [details] Patch II - removing cached value computations This is a tentative patch for the problem identified by Stefan. The problem is caused by the assymmetry in the way value computation is cached and cleared. The value computation is cached in the originated context, but removed from the actual context where IComputedFunction was found. I think this patch makes sense, but I'd like to try it a bit more before applying.
(In reply to comment #42) > I think this patch makes sense, but I'd like to try it a bit more before > applying. Oleg, I have tried something similar, but both your patch and mine have a side effect: the parts (views/editors) no longer have an active rendering (CSS style is not updated).
Created attachment 177227 [details] Patch II v.2 - removing cached value computations (In reply to comment #43) > the parts (views/editors) no longer have an active rendering Good point; the previous version of the patch changed EclipseContext#invalidate() affecting updates of injected computed values. This version only modifies updates for the computed values themselves.
"Patch II v.2 - removing cached value computations" applied to CVS Head.
The DepenciesLeakTest now passes, but unfortunately the problem is not yet completely solved. The number of handleInvalid invocations still grows when switching editors (see comment 23 and comment 25).
Created attachment 179961 [details] Patch III - removing dependencies invalidated caches After more thinking and debuging, it seems that the fix proposed in the comment 42 was correct. To my surprise, when I tried it with the current code base I no longer get a side effects of missing CSS activation.
I applied "Patch III" to CVS Head with my fingers crossed. With the patch handler count stays constant when switching between two editors.
Using the context debug view from "org.eclipse.e4.core.contexts.debug" verified that no new listeners are created on repeated switch of the active editor (I20101027-0214).
