Community
Participate
Working Groups
Whenever a client calls getSelection() on a TreeViewer, it goes and asks the underlying SWT tree widget to get the selection and then builds a TreeSelection by figuring out the TreePath for each item. The thing is, the getSelection() is called quite often by all kinds of clients through the ISelectionService, and with every call, the tree viewer builds the same TreeSelection again and again whereas it already knows about the underlying SWT tree widget selection change events, and, it already has built the TreeSelection in order to fire the event to registered ISelectionChangedListeners in the first place. Therefore, the tree viewer can simply keep a given TreeSelection until it receives a newer widgetSelected event from the SWT tree control which will make repeated calls to getSelection() very fast. Please note that the larger the number of selected items is in a given tree the longer the getSelection() takes to build the TreePaths ..Etc.
Created attachment 41911 [details] cache the AbstractTreeViewer selection This issue is causing a major performance degradation in some scenarios in our application. I'm raising the priority hoping that this issue will get addresssed especially that the "fix" is simple. I'm submitting a patch, please review.
I don't think we can apply this patch for 3.2 - it is too risky at this point. In particular, your patch changes the behaviour for clients that set the selection directly on the SWT tree and then ask for the JFace TreeViewer's selection, since setting the selection on the SWT tree will not fire any events. We can consider optimizing this for 3.2.1, but we would have to remember the last Tree selection and the last TreeViewer selection, returning the cached TreeViewer selection if the current Tree selection is still the same as the cached Tree selection.
I understand your concerns, however, one can argue that the Tree widegt is "owned" by the TreeViewer so basically clients should not get hold (access or manipulate) the Tree widget directly, this should mitigate the risk and perhaps these clients should then call doGetSelection() instead of getSelection() to ensure that they will be getting the latest selected item.
(In reply to comment #3) > I understand your concerns, however, one can argue that the Tree widegt is > "owned" by the TreeViewer so basically clients should not get hold (access or > manipulate) the Tree widget directly, this should mitigate the risk and perhaps > these clients should then call doGetSelection() instead of getSelection() to > ensure that they will be getting the latest selected item. While this would help new clients, it still breaks existing clients. We also cannot add API for 3.2 any more, and we also don't like API changes in point releases like 3.2.1. I am not saying that we don't care about performance, in fact we do care very much. The only problem here is the timing in the release cycle. It would be interesting to know if this is "just" a performance problem, or if it is a performance regression when compared to 3.1. Did you do any tests against the 3.1 TreeViewer?
Please explain how critical this is for you. Ideally, you would provide numbers like "when I <select all> in the such-and-such scenario, it takes x seconds, whereas with the patch, it only takes y milliseconds". How many times is getSelection() called in your scenarios? I am asking because we could also optimize the implementation of ISelectionService.getSelection().
Created attachment 43143 [details] cache the selection in the selection service Yasser, could you please try running with this patch (only)? Does this improve your numbers?
(In reply to comment #6) > Created an attachment (id=43143) [edit] > cache the selection in the selection service > Yasser, could you please try running with this patch (only)? Does this improve > your numbers? I tried the patch and it seems to be fixing the issue :) I still need to re-run the standard performance tests to generate exact numbers in order to be 100% sure, and I'll enter a new comment with the results. Thanks Boris for your help.
Ran our standard performance tests and the results confirmed that the patch proposed by Boris achieves comparable reuslts to the originally proposed patch and that the performance numbers improved dramatically.
The actual numbers (as far as I know) are: >12 minutes without the patch, 64 seconds with the patch. This is in a scenario with more than 1000 selected nodes in the tree. John, Michael, Darin, Dani, McQ: Could you please review the patch and cast your votes for 3.2 RC7? I should note that the underlying issue is in TreeViewer which should be optimized as well, but is too risky at this point so needs to be addressed for 3.2.1. For 3.2, I propose to optimize just AbstractSelectionService since it solves the problem that was reported, and it looks like a reasonable optimization that we should keep even after optimizing TreeViewer.
I vote -1 for this fix for the following reasons: - changing the AbstractSelectionService code which is in since at least R3.1 and which is used by many different clients and not just in conjunction with the TreeViewer seems wrong and dangerous to me - adding a cache is always error-prone and not something I'd want to put into RC7 - we should at least have more testing with this fix and hence postpone it to 3.2.1 - having performance problems with >1000 selected nodes is very unfortunate but does not qualify as a stop-ship defect to me
At this point, one -1 means that this fix cannot be applied to 3.2. Moving to 3.2.1. Dani, if we put this in for 3.2.1 - do you still believe that caching the last selection in AbstractSelectionService is wrong and dangerous?
Less dangerous but still wrong: I think it's not the right thing to do for the service and would prefer to see a fix where at the problem's location i.e. the TreeViewer. Even with the patch a clients can run into the performance problem if they use ISelectionProvider.getSelection(String) instead of ISelectionProvider.getSelection() or query the selection from the selection provider or directly from the tree viewer itself. Here's a scenario where a client could get broken by the patch: Though (probably) not intended by the interface, a client could have implemented an ISelectionProvider which only offers support for get/setSelection - if his RCP code only used ISelectionService.getSelection() it would no longer get the newest updated selection from selection provider.
As a matter of fact, the very same "risky scenario" cited in Comment#2 applies here as well, namely, setting selection directly on the SWT Tree and then trying to get the current selection from the ISelectionService since setting the selection on the SWT tree will not fire any events.
The following comment is a mroe detailed description of the issue we are facing in our scenario: This problem existed in earlier versions of Eclipse -even during the Eclipse 2.1 days-, however we circumvented it in our code by caching the ISelection in our own TreeViewer (similar to the patch proposed for the CommonNavigator in Bugzilla 144294). Now that we moved to the CommonNavigator this issue is showing up again and there is nothing we could do about that on our end. The getSelection() gets called a lot in this scenario, the reason being we have several context-menu ObjectContribution actions that Eclipse -in response to the SelectionChange event tries to calculate their enablement -although these actions are not visible to the end-user (i.e., they are not a toolbar contribution button that need to be dimmed for example, nor are they retargetable actions, they are simply context-menu actions so calculating their enablement is unnecessary except in response to menu-about-to-show event, but that's a different issue in Eclipse, anyway, the action enablement code checks if selected elements implement IAdaptable and if an IActionFilter is registered for the selected elements, now, all our elements displayed in the tree are IAdaptables and they do adapt to our IActionFilter. Now, instead of passing the whole current selection to the IActionFilter (as it does to the ActionDelegate), Eclipse code iterates over the selected elements and asks the IActionFilter to calculate the enablement based on some attribute-value pair and only breaks from the loop if the IActionFilter answered false for any element in the selection. Now, the problem is that some o four actions can only be enabled if all the selected elements are of the very same type which means our IActionFilter have to ask for the current selection as a whole to verify that all the elements are of the same type. So, calling the ISelectionService to get the current Selection ends up being called for each iteration on each element (4646 in this scenario) and then this is multiplied by the number of action enablement criteria that needs to look at the current selection as a whole (we have something like 20 of those). The reason we can’t cache the selection in our IActionFilter is that the IActionFilter does not know the details of the context of its invocation by Eclipse, more precisely, it does not know whether or not a selection change occurred between any two successive calls to its testAttribute(...) function, for example, when it is called by Eclipse in a loop in response to single selection change event with multiple elements caching the selection would work fine, on the other hand, any two successive calls could be the result of two separate selection change events and thus cached selection will be stale, furthermore, the IActionFilter cannot simply register as a ISelectionListener with the ISelectionService and use the events it receives as an indicator that a cached selection is stale since the IActionFilter cannot guarantee that it will be notified of selection changes before Eclipse asks it to evaluate an enablement of an ObjectContribution action. Therefore, the originator of a selection change event (the selection provider) is the best guy to cache the selection since it is the one that owns the selection on the one hand and it knows why and when it should fire a selection change. Now, as you can see from the above explanation, the problem is not confined to select-all scenario but it will manifest itself also when the user tries to show a context-menu on a large selection because action enablement and visibility need to be calculated as well and the IActionFilter will be invoked...etc.
To put things in perspective, here are the numbers of the performance tests we have: 1) Number of our elements to be selected in the CommonNavigator TreeViewer: 4646 2) Time taken to select-all without the patch: 12 minutes (aborted the process) 3) Time taken to select-all with the patch: 62 seconds Please note that 62 seconds is still considered slow, however, it is caused by something else.
Dani, Could you propose or highlight a solution that you would consider +1'ing? This is a stop ship problem for an adopter; particularly from a major adopter that's willing to help investigate and propose fixes. What would it take for you to reconsider your position?
Since the problem seems to appear in the Common Navigator I could imagine a fix for 3.2 which is local to the Common Navigator and not in the selection service that affects the whole world.
I think we've just run out of time on this for 3.2. The final scheduled 3.2 build is in about nine hours, so it is very unlikely we'll find an optimization that we are comfortable with in that time. There will be time in 3.2.1 to explore and properly test a solution. Typically a user would "select all" in a tree by just selecting the roots, so this case seems somewhat artificial.
(In reply to comment #12) > Here's a scenario where a client could get broken by the patch: > Though (probably) not intended by the interface, a client could have > implemented an ISelectionProvider which only offers support for > get/setSelection - if his RCP code only used ISelectionService.getSelection() > it would no longer get the newest updated selection from selection provider. The whole point of ISelectionProvider/ISelectionService is to notify listeners about *changes* in the current selection. The Javadoc is not clear on this, but I would argue that clients should never rely on the current implementation which reaches through to the active part to get the current selection. In fact, I believe that ISelectionProvider just has sloppy API specs - the spec should require implementers to notifify listeners whenever their selection changes.
Just to be clear: I believe that we should cache the active part's selection in AbstractSelectionService (even if this "affects the whole world"), *if* it turns out that ISelectionService.getSelection() is called very often in real-world scenarios. I agree that it is too late to put anything into 3.2.
Moving to 3.3. Bug 144294 will resolve the performance issue for the 3.2.1 release in the Project Explorer view.
(In reply to comment #20) > *if* it > turns out that ISelectionService.getSelection() is called very often in > real-world scenarios. Does anybody on this bug have profiling data that supports this? Please reopen if this is the case.
Any chance to see this bug fixed in 3.4? Or at least do not use RESOLVED (deprecated) "REMIND" resolution, changing to the "OPEN"?
(In reply to comment #23) > Any chance to see this bug fixed in 3.4? > Or at least do not use RESOLVED (deprecated) "REMIND" resolution, changing to > the "OPEN"? I can WONTFIX the bug. Please reopen with profiling information from real-world scenarios if you think the caching is needed.
