Bugzilla – Full Text Bug Listing
| Summary: | HandlerService takes up over 90% of time to activate an editor | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Oleg Besedin <ob1.eclipse> | ||||||||
| Component: | UI | Assignee: | Project Inbox <e4.ui-inbox> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | ob1.eclipse, pwebster, remy.suen | ||||||||
| Version: | unspecified | ||||||||||
| Target Milestone: | 1.0 M6 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Oleg Besedin
(In reply to comment #0) > Scenario: > - start e4 > - open two editors Are these text editors or Java editors? > The HandlerActivation's are never removed from the context. Hence, each editor > switch adds ~2K of new listener to the context. They should be removed when their 'participating' flag is set to 'false' on deactivation. Or at least...that's the intent. (In reply to comment #1) > Are these text editors or Java editors? I used Java editors. > They should be removed when their 'participating' flag is set to 'false' on > deactivation. Or at least...that's the intent. I haven't checked that in details yet. The HandlerActivation#changed() is never called when "participating" flag is set to false so it never gets cleared from the listener list. I'll look more into what can be done on the IEclipseContext side. Regardless of that, creating and disposing of all editor's handlers whenever the editor is activated and deactivated does not seems to be efficient. (In reply to comment #2) > The HandlerActivation#changed() is never called when "participating" flag > is set to false so it never gets cleared Sorry, that was wrong - I had code modified in my workspace. It gets called and gets cleared. The overall performance implications are still the same as in the comment 1 : activation and deactivation of handlers takes over 90% of the time to switch editors. I'll see if I can figure out the bottleneck. Created attachment 168401 [details]
LegacyHandlerService patch v1
I've released this patch which alters LegacyHandlerService's deactivateHandlers(Collection) method to first turn all handlers 'participating' flag to 'false' before asking the EHS to deactivate the handler. This prevents context churn caused by the deactivation of handler 1 to propagate to handlers 2 - N.
A couple of places that were using iterators to call deactivateHandler(IHandlerActivation) individually have been swapped to use the batch call.
Java editors should be closing much quicker now.
We have some recursive calls in HandlerActivation that causes two activation requests to be sent to the EHS... Object obj = HandlerServiceImpl.lookUpHandler(this.context, commandId); /* ... */ hs.activateHandler(commandId, proxy); ...we do a lookup, then we activate if applicable. But since we performed a lookup, we actually invoked a get(String) on the context, so when we set it in the EHS, the RAT is processed again. Or at least, that's how I believe I'm understanding the problem here. Though it's not clear how much this will help the problem since the context will take it as a no-op if the old value is identical to the new value (which it is in this case) during the second activation request. The significant part of the problem here is the combinatory explosion caused by the combination of the HandlerActivation and HandlerLookupFunction. The HandlerLookupFunction (HL) is essentially a singleton and takes an argument (handlerId) to determine what it should return. As the same instance of the HL is used by all HandlerActivation (HA) instances, the single HL instance has dependency on all handlerIDs, and all HAs have dependency on that single HL instance. When one HA is deactivated, its handler ID is removed from the context, causing invalidation of the HL, which causes *all other HAs* to be invalidated. So, it seems that using a singleton lookup function that takes arguments is a performance problem. (In reply to comment #6) > When one HA is deactivated, its handler ID is removed from the context, causing > invalidation of the HL, which causes *all other HAs* to be invalidated. I was wondering why I had hundreds of notifications after setting one handler in the context. :) (In reply to comment #6) > So, it seems that using a singleton lookup function that takes arguments is a > performance problem. Thinking about this, I guess it makes the 'cachedValue' field utterly useless. :) Created attachment 168600 [details]
Patch - don't use arguments with IContextFunctions
The patch removes only two uses of IContextFucntion.compute() that actually use arguments. Switching editors with this single change feels as fast as 3.x.
Funny, but code that does not use arguments looks simpler :-).
I'll think for a bit if we should remove IEclipseContext#get(String name, Object[] arguments) and arguments from IContextFunction#compute(IEclipseContext context, Object[] arguments).
Without the arguments, we'll have to watch that we don't lose any of the side effects. PW After thinking about it over the weekend, I think that we need to remove optional arguments from the IContextFunction#compute(). Overwise, it is way too easy to fall in the situation described in the comment 6. The only thing needed is to have IContextFunction be used in another context calculation, and we are in trouble. Aside from the patch above, there are two places that use arguments worth mentioning: - IServiceConstants.INPUT - adopts selection to the specified type Currently this functionality not used in the e4 SDK or demos - Several functions in the org.eclipse.e4.ui.bindings that follow the same pattern as the handle lookup function above Those functions are not used at present. Paul, what would you like me to do with those binding lookup functions? They are unused at present: BindingLookupFunction => EBindingLookup => BindingServiceImpl -> unused BindingPrefixLookupFunction => EBindingPrefixLookup => BindingServiceImpl -> unused CommandLookupFunction => ECommandLookup => BindingServiceImpl -> unused CommandSequencesLookupFunction => ECommandSequenceLookup => BindingServiceImpl -> unused PartialMatchCommandLookupFunction => EPartialMatchLookup => BindingServiceImpl -> unused Created attachment 168929 [details]
Complete patch
The comprehensive patch that removes optional arguments from IEclipseContext's #get(), changes 2 places that were actually using this functionality, and modifies a whole lot of places that weren't using the arguments but had them in signatures.
Patch applied to CVS Head. |