Community
Participate
Working Groups
Created attachment 173376 [details] Profiler screenshot: garbage-collected objects Steps to reproduce: 1. Open the e4 SDK Workbench 2. Enabled the heap status 3. Watch the heap status for a number of seconds Results: - Heap size grows by ~1 MB every 4 seconds - After 1 minute, a major garbage collections happens - This garbage collection will shrink the heap by ~14 MB Analysis: - Almost all background allocations are indirectly caused by ToolItemRenderer$7 (the runnable that is scheduled every 500 ms) - The two main causes are (1) Two updates of the EclipseContext per update (2) An invocation of Method.getAnnotation(...) - See screenshot for details. Ideas: - Cache the result of Method.getAnnotation(...) - Instead of addParamsToConcext(...) and removeParamsToConcext(...), use a temporary context that contains the necessary params. - Reduce the number of listeners in the root context; for example, is it possible to use a separate child-context for all global handlers? Perhaps the updating of this context could be optimized
Created attachment 173378 [details] Proposed patch to improve Method.getAnnotation() Some notes: The annotations themselves cannot be cached, since every call to Class.getDeclaredMethods() will return a fresh copy of methods. Therefore I propose to cache the declared methods. This also has some positive side-effects, as there are several invocations of Class.getDeclaredMethods() in InjectorImpl. I used a WeakHashMap for the cache. The patch reduces the memory allocations as expected.
Comment on attachment 173378 [details] Proposed patch to improve Method.getAnnotation() Committed this patch to HEAD. Thanks!
(In reply to comment #0) > - Instead of addParamsToConcext(...) and removeParamsToConcext(...), > use a temporary context that contains the necessary params. This sounds like a good idea to me, since it would leave the original context alone and thus wouldn't cause invalidations. Paul, Oleg, what do you think?
Created attachment 173381 [details] Proposed patch for HandlerServiceImpl This patch replaces addParmsToContext(...) and removeParmsFromContext(...) with a temporary context.
Note on the above patch: There is a problem with EclipseContext.dispose(). I get a 'Discouraged access' warning on this call because HandlerService is located in the commands bundle. This bundle is not in the list of plug-ins where the exported package should be visible. So, no dispose() for clients? See MANIFEST.MF of the 'di' bundle.
Released to HEAD
(In reply to comment #4) > Created an attachment (id=173381) [details] > Proposed patch for HandlerServiceImpl I had to revert the change. The problem is that handlers are expected to be execute in the active child chain. For example, at the Workbench Window level. if we add an temp context off of the Workbench Window context, then the handlers are not in the active child chain and won't be able to look up certain things correctly. PW
But, the temporary context had the right parent context. Shouldn't this have worked?
(In reply to comment #8) > But, the temporary context had the right parent context. Shouldn't this have > worked? The problem isn't the parent, it's the ACTIVE_CHILD. We have a number of algorithms that start with the current context, and then walk down the active child chain, and then do a lookup of something (handlers, etc). if the handler being executed needed any of those services or data, or tried to execute another handler provided by a part (for example) it would not see the correct information because it would never be able to reach the active part (as the trail dead ends with it). PW
There is a new version of #invoke() that takes two arguments: a real context and a "fake" context used to pass extra arguments to injected objects. I am not sure if that will work however as there is code like: HandlerUtil.getVariable(event, ECommandService.class.getName()); and EHandlerService handlerService = (EHandlerService) HandlerUtil.getVariable(event, EHandlerService.class.getName()); If handler service and command service were injected, they would have been added from the secondary context; but here the primary context is queried for those values. At any rate, I do agree that this is too much churn, especially with the timed updates on the tool items.
Created attachment 173565 [details] Illustrative patch - passing extra args to injection This is an illustration of how extra arguments can be passed to injection. It is not meant to be applied as-is, as, for instance, it will break ShowViewHandler.
Moving to 4.1
Removing outdated target milestone.
Most (if not all) of these issues were fixed during the performance work on Juno...
