Community
Participate
Working Groups
In Notes, when open a new tab or switch tab, IWorkbenchActivitySupport.setEnabledActivityIds() is called to set enabled activity ids. This call will indirectly invoke savePluginPreferences()(located in saveEnabledStates()) when ActivityPersistanceHelper activity listener response the activityManagerChanged event. The savePluginPreferences call in saveEnabledStates takes much time, which slow the open new tab and switch tab performance. Can the WorkbenchPlugin.getDefault().savePluginPreferences() call be moved to background job? Or be removed when saveEnabledStates() is invoked in activityManagerChanged()? Since the saveEnabledStates() is invoked in shutdown(), is it necessary to save plugin preference in every activityManagerChanged() event? org.eclipse.ui.workbench plugin ActivityPersistanceHelper.java public void activityManagerChanged( ActivityManagerEvent activityManagerEvent) { //process newly defined activities. if (activityManagerEvent.haveDefinedActivityIdsChanged()) { Set delta = new HashSet(activityManagerEvent .getActivityManager().getDefinedActivityIds()); delta.removeAll(activityManagerEvent .getPreviouslyDefinedActivityIds()); // whatever is still in delta are new activities - restore their // state loadEnabledStates(activityManagerEvent .getActivityManager().getEnabledActivityIds(), delta); } if (activityManagerEvent.haveEnabledActivityIdsChanged()) { saveEnabledStates(); } } protected void saveEnabledStates() { try { saving = true; IPreferenceStore store = WorkbenchPlugin.getDefault() .getPreferenceStore(); IWorkbenchActivitySupport support = PlatformUI.getWorkbench() .getActivitySupport(); IActivityManager activityManager = support.getActivityManager(); Iterator values = activityManager.getDefinedActivityIds().iterator(); while (values.hasNext()) { IActivity activity = activityManager.getActivity((String) values .next()); if (activity.getExpression() != null) { continue; } store.setValue(createPreferenceKey(activity.getId()), activity .isEnabled()); } WorkbenchPlugin.getDefault().savePluginPreferences(); //move this to background job to improve performance when saveEnabledStates is invoked in activityManagerChanged(). or remove it when saveEnabledStates() is invoked in activityManagerChanged()? Since the saveEnabledStates() is invoked in shutdown(), is it necessary to save plugin preference in every activityManagerChanged() event? } finally { saving = false; } }
It looks to me like this isn't something we need to persist to disk every time the active part changes. Persisting only during normal shutdown seems like the right answer to me. If the application crashes or exits without normal save, then these active activities won't be applicable in the next session anyway. Also, making Plugin.savePluginPreferences asynchronous for all callers is quite a risky way to address an isolated performance problem.
If interested, see http://wiki.eclipse.org/Platform_UI/How_to_Contribute PW
Created attachment 228828 [details] Code difference to remove WorkbenchPlugin.getDefault().savePluginPreferences() when saveEnabledStates is invoked in activityManagerChanged() Do not call WorkbenchPlugin.getDefault().savePluginPreferences() when saveEnabledStates is invoked in activityManagerChanged()
From your comments, seems Plugin.savePluginPreferences is not necessary every time the active part changes, just required in shutdown(). We don't need to make Plugin.savePluginPreferences asynchronous for all callers, just need to remove it or move it to background when saveEnabledStates is called from activityManagerChanged()? While still keep Plugin.savePluginPreferences in UI thread as now in shutdown(). Is this change risky? I have attached the code difference of this change for reference. This performance problem has widely impact to lots of Notes actions (from the important actions like Open Mail & Open Contact to the frequently used actions like Open Message, Send message, Send Meeting invitation, Reply, Compose, Switch tab etc.) So it would greatly improve the overall Notes performance if the change can be made. (In reply to comment #1) > It looks to me like this isn't something we need to persist to disk every > time the active part changes. Persisting only during normal shutdown seems > like the right answer to me. If the application crashes or exits without > normal save, then these active activities won't be applicable in the next > session anyway. > > Also, making Plugin.savePluginPreferences asynchronous for all callers is > quite a risky way to address an isolated performance problem.
Critical implies data loss, and that's not the case here. PW
(In reply to comment #3) > Created attachment 228828 [details] > Code difference to remove > WorkbenchPlugin.getDefault().savePluginPreferences() when saveEnabledStates > is invoked in activityManagerChanged() See http://wiki.eclipse.org/Platform_UI/How_to_Contribute for acceptable formats to submit fixes. PW
any update?
Right now we're waiting for you to submit a fix we can consume. See http://wiki.eclipse.org/Platform_UI/How_to_Contribute PW
It is just my draft idea, i don't have fix. Could Eclipse team investigate this and help to provide an official solution, and create a patch for Eclipse 3.4.2? Thank you very much!
This performance improvement is needed by Notes. Can Eclipse team provide an ETA for the patch? Thanks!
If you don't have a fix then then this bug won't be looked at. We don't support 3.4.x any more. PW
The code difference I attached is the proposed fix. Can anyone evaluate it? Is it possible to create patch for Eclipse 4.0?
(In reply to comment #12) > The code difference I attached is the proposed fix. Can anyone evaluate it? > Is it possible to create patch for Eclipse 4.0? If that is a proposed fix, that's great. You need to follow the steps at http://wiki.eclipse.org/Platform_UI/How_to_Contribute to either push the fix to Gerrit for review or attach a patch here. PW
Remove the save activity preferences call every time active part change may be too aggressive. So we propose an updated fix to move that call to background (move the Plugin.savePluginPreferences() to background when saveEnabledStates is invoked in activityManagerChanged()). Please find the updated fix and related files in attachments. The patch is based on the src code of Eclipse 3.4.2 and generated by diff command.
Created attachment 229275 [details] Updated fix (code diff against Eclipse 3.4.2. Left: original, Right: fix) Code difference file based on Eclipse 3.4.2. Left column: original code, Right column: fix
Created attachment 229276 [details] Source code of the fix (based on Eclipse 3.4.2)
Created attachment 229277 [details] Source of the original code (Eclipse 3.4.2)
Created attachment 229278 [details] patch for updated fix based on Eclipse 3.4.2, generated by diff command.
(In reply to comment #3) > Created attachment 228828 [details] > Code difference to remove > WorkbenchPlugin.getDefault().savePluginPreferences() when saveEnabledStates > is invoked in activityManagerChanged() Li, I've turned this into a Gerrit change: https://git.eclipse.org/r/#/c/11638/ You can get your patch from that if you want, and if it fixes your problem then it'll be merged into Kepler and can be patched back to 4.2.2+ PW
(In reply to comment #19) > Li, I've turned this into a Gerrit change: > https://git.eclipse.org/r/#/c/11638/ > > You can get your patch from that if you want, and if it fixes your problem > then it'll be merged into Kepler and can be patched back to 4.2.2+ Dani has pointed out that this prevents saving the preferences on explicit calls to preference page. Please post a stack trace of the call that persists the preference that is causing your problem (happening when you open a new tab or switch tabs as you said in comment #0 ) PW
The patch you merged was obsoleted as we think it is aggressive and may be risky. A new patch was attached to move WorkbenchPlugin.getDefault().savePluginPreferences() to background rather than removing it. Could you please merge the updated patch (https://bugs.eclipse.org/bugs/attachment.cgi?id=229275) and get it reviewed? The original Eclipse code (https://bugs.eclipse.org/bugs/attachment.cgi?id=229277) and the fix code (https://bugs.eclipse.org/bugs/attachment.cgi?id=229276) were also attached for your comparison.
(In reply to comment #21) > The patch you merged was obsoleted as we think it is aggressive and may be > risky. > > A new patch was attached to move > WorkbenchPlugin.getDefault().savePluginPreferences() to background rather > than removing it. > > Could you please merge the updated patch > (https://bugs.eclipse.org/bugs/attachment.cgi?id=229275) and get it reviewed? > > The original Eclipse code > (https://bugs.eclipse.org/bugs/attachment.cgi?id=229277) and the fix code > (https://bugs.eclipse.org/bugs/attachment.cgi?id=229276) were also attached > for your comparison. Putting the logic into the background and synchronize the code is not safer than what Paul proposed. Can you please attach the stack trace requested by Paul in comment 20. Thanks.
(In reply to comment #22) > (In reply to comment #21) > The patch you merged was obsoleted as we think > it is aggressive and may be > risky. > > A new patch was attached to move > > WorkbenchPlugin.getDefault().savePluginPreferences() to background rather > > than removing it. > > Could you please merge the updated patch > > (https://bugs.eclipse.org/bugs/attachment.cgi?id=229275) and get it > reviewed? > > The original Eclipse code > > (https://bugs.eclipse.org/bugs/attachment.cgi?id=229277) and the fix code > > (https://bugs.eclipse.org/bugs/attachment.cgi?id=229276) were also attached > > for your comparison. Putting the logic into the background and > synchronize the code is not safer than what Paul proposed. Can you please > attach the stack trace requested by Paul in comment 20. Thanks. Please find below the stack trace: New_configuration [Client Services] org.eclipse.equinox.launcher.Main at localhost:51197 Thread [main] (Suspended (breakpoint at line 268 in ActivityPersistanceHelper)) ActivityPersistanceHelper.saveEnabledStates() line: 268 ActivityPersistanceHelper$1.activityManagerChanged(ActivityManagerEvent) line: 75 ProxyActivityManager(AbstractActivityManager).fireActivityManagerChanged(ActivityManagerEvent) line: 52 ProxyActivityManager$1.activityManagerChanged(ActivityManagerEvent) line: 50 MutableActivityManager(AbstractActivityManager).fireActivityManagerChanged(ActivityManagerEvent) line: 52 MutableActivityManager.updateListeners(boolean, Map, Set, Set) line: 625 MutableActivityManager.setEnabledActivityIds(Set) line: 592 WorkbenchActivitySupport.setEnabledActivityIds(Set) line: 320 ActivityController.update() line: 203 PerspectiveListener.perspectiveActivated(IWorkbenchPage, IPerspectiveDescriptor) line: 321 PerspectiveListenerList$1.run() line: 75 SafeRunner.run(ISafeRunnable) line: 37 Platform.run(ISafeRunnable) line: 880 PerspectiveListenerList.fireEvent(SafeRunnable, IPerspectiveListener, IPerspectiveDescriptor, String) line: 58 PerspectiveListenerList.firePerspectiveActivated(IWorkbenchPage, IPerspectiveDescriptor) line: 73 WorkbenchWindow.firePerspectiveActivated(IWorkbenchPage, IPerspectiveDescriptor) line: 1330 WorkbenchPage.setPerspective(Perspective) line: 3680 WorkbenchPage.busySetPerspective(IPerspectiveDescriptor) line: 1032 WorkbenchPage.access$16(WorkbenchPage, IPerspectiveDescriptor) line: 1016 WorkbenchPage$18.run() line: 3801 BusyIndicator.showWhile(Display, Runnable) line: 70 WorkbenchPage.setPerspective(IPerspectiveDescriptor) line: 3799 PerspectiveAction.run() line: 140 TabFolderContributionItem.handleWidgetSelection(SelectionEvent, boolean) line: 451 TabFolderContributionItem.access$3(TabFolderContributionItem, SelectionEvent, boolean) line: 430 TabFolderContributionItem$3.widgetSelected(SelectionEvent) line: 205 TypedListener.handleEvent(Event) line: 240 EventTable.sendEvent(Event) line: 84 STabFolder(Widget).sendEvent(Event) line: 1053 STabFolder(Widget).sendEvent(int, Event, boolean) line: 1077 STabFolder(Widget).sendEvent(int, Event) line: 1062 STabFolder(Widget).notifyListeners(int, Event) line: 774 STabFolder.setSelection(int, boolean) line: 3272 STabFolder.onMouse(Event) line: 1991 STabFolder$1.handleEvent(Event) line: 230 EventTable.sendEvent(Event) line: 84 STabFolder(Widget).sendEvent(Event) line: 1053 Display.runDeferredEvents() line: 4166 Display.readAndDispatch() line: 3755 Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2384 Workbench.runUI() line: 2348 Workbench.access$4(Workbench) line: 2200 Workbench$5.run() line: 495 Realm.runWithDefault(Realm, Runnable) line: 288 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 490 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 RCPApplication.run(Object) line: 67 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 60 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 37 Method.invoke(Object, Object...) line: 611 EclipseAppContainer.callMethodWithException(Object, String, Class[], Object[]) line: 574 EclipseAppHandle.run(Object) line: 195 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 386 EclipseStarter.run(String[], Runnable) line: 179 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 60 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 37 Method.invoke(Object, Object...) line: 611 Main.invokeFramework(String[], URL[]) line: 564 Main.basicRun(String[]) line: 504 Main.run(String[]) line: 1251 Main.main(String[]) line: 1227 Daemon Thread [Signal Dispatcher] (Running) Daemon Thread [Attach API wait loop] (Running) Thread [OSGi Console] (Running) Daemon Thread [State Data Manager] (Running) Daemon Thread [Log Event Dispatcher] (Running) Daemon Thread [Framework Event Dispatcher] (Running) Daemon Thread [Start Level Event Dispatcher] (Running) Daemon Thread [Thread-7] (Running) Thread [Worker-0] (Running) Thread [Worker-1] (Running) Thread [pool-1-thread-1] (Running) D:\Lotus\Notes\jvm\bin\javaw.exe (Apr 8, 2013 7:16:58 PM)
Side note from a chat with Li Juan: the product's actions cause perspective activations and hence this code is triggered frequently. The stack trace shows that the #setEnabledActivityIds(Set) is called by the product. Hence, the suggestion I made, to simply move the save from the listener to that method, won't do the trick. The safest fix, which does not affect other clients, is to - document that IWorkbenchActivitySupport#setEnabledActivityIds(Set) saves the plug-in preferences. - add a new API IWorkbenchActivitySupport#setEnabledActivityIds(Set, boolean) which allows to specify whether to save or not. The product can call this new method, requesting no save. - explicitly do the save in the implementation (WorkbenchActivitySupport) depending on the flag. - no longer save in the listener (covered by Paul's change) If we all agree on this, I can upload a patch for testing and review.
(In reply to comment #24) > Side note from a chat with Li Juan: the product's actions cause perspective > activations and hence this code is triggered frequently. > > The stack trace shows that the #setEnabledActivityIds(Set) is called by the > product. Hence, the suggestion I made, to simply move the save from the > listener to that method, won't do the trick. > > > The safest fix, which does not affect other clients, is to > - document that IWorkbenchActivitySupport#setEnabledActivityIds(Set) saves > the plug-in preferences. > > - add a new API IWorkbenchActivitySupport#setEnabledActivityIds(Set, boolean) > which allows to specify whether to save or not. The product can call this > new method, requesting no save. > > - explicitly do the save in the implementation (WorkbenchActivitySupport) > depending on the flag. > > - no longer save in the listener (covered by Paul's change) > > > If we all agree on this, I can upload a patch for testing and review. Thanks Deni. This looks good to me. It is much safer.
Thanks Dani. The fix looks good for us. Could you please help to create a patch based on Eclipse 3.4.2 because our product is still using Eclipse 3.4.2? If you can't create a patch based on Eclipse 3.4.2, can you help to send us the src code and diff files of your changes? Then we can refer them to merge the fix into our product. Thanks very much. (In reply to comment #24) > Side note from a chat with Li Juan: the product's actions cause perspective > activations and hence this code is triggered frequently. > > The stack trace shows that the #setEnabledActivityIds(Set) is called by the > product. Hence, the suggestion I made, to simply move the save from the > listener to that method, won't do the trick. > > > The safest fix, which does not affect other clients, is to > - document that IWorkbenchActivitySupport#setEnabledActivityIds(Set) saves > the plug-in preferences. > > - add a new API IWorkbenchActivitySupport#setEnabledActivityIds(Set, boolean) > which allows to specify whether to save or not. The product can call this > new method, requesting no save. > > - explicitly do the save in the implementation (WorkbenchActivitySupport) > depending on the flag. > > - no longer save in the listener (covered by Paul's change) > > > If we all agree on this, I can upload a patch for testing and review.
(In reply to comment #26) > Thanks Dani. The fix looks good for us. Could you please help to create a > patch based on Eclipse 3.4.2 because our product is still using Eclipse > 3.4.2? > I'm waiting for an OK from Paul before I start to work on a patch.
(In reply to comment #24) > The safest fix, which does not affect other clients, is to > - document that IWorkbenchActivitySupport#setEnabledActivityIds(Set) saves > the plug-in preferences. > > - add a new API IWorkbenchActivitySupport#setEnabledActivityIds(Set, boolean) > which allows to specify whether to save or not. The product can call this > new method, requesting no save. > > - explicitly do the save in the implementation (WorkbenchActivitySupport) > depending on the flag. > > - no longer save in the listener (covered by Paul's change) OK Dani, sounds good. PW
Created attachment 229515 [details] Possible fix for 3.4.2 Further information revealed that the problem has only been seen in performance tests and that the delay (30 ms on the test machine, 3 ms on my fast machine) is not something that's noticeable by an end user and hence not worth fixing. This patch implements the solution from comment 24 with one simplification: the call to save the plug-in preferences can be entirely removed from ActivityPersistanceHelper#saveEnabledStates because the framework saves the plug-in preference anyway on shutdown, when the workbench bundle gets stopped. Feel free to use it in your product.
.
Comment on attachment 229515 [details] Possible fix for 3.4.2 As correctly noticed by Li Juan, this patch can result in more save calls than before, e.g. when the activities did not change. Hence, it should not be used. Given that the product manages the activities dynamically and does not surface the 'Capabilities' preference page, the easiest solution for that specific product is just to remove the save call entirely. As mentioned before, the framework will save the plug-in preferences during shutdown.
Created attachment 229569 [details] Fix to provide new API to determine whether save plugin preferences or not Fix to provide a new API to determine whether save the plugin preferences, which just take effect for callers of this new API. There is no change/impact to existing code, that is, the save call is still retained to protect against crash for existing code. Very much appreciated if someone can help review the changes. Thanks!
(In reply to comment #32) > Created attachment 229569 [details] > Fix to provide new API to determine whether save plugin preferences or not > > Fix to provide a new API to determine whether save the plugin preferences, > which just take effect for callers of this new API. > There is no change/impact to existing code, that is, the save call is still > retained to protect against crash for existing code. > > Very much appreciated if someone can help review the changes. Thanks! Sorry, I won't have time for this, especially since the suggested solution is fine for your product, (which does not show the 'Capability' preference page) and it is essentially the same as suggested by Paul.
