Startup/shutdown performance: unnecessary processing of activity events #1084

HeikoKlare · 2023-09-06T15:47:00Z

Issue

The activity management in MutableActivityManager and ActivityPersistenceHelper is inefficient when dealing with higher amounts of activities. Activity enablements are unnecessarily processed and persisted upon different events:

All activities enablements are persisted upon every activity enablement change in the preference store, even if only the enablement of an expression-controlled activity changes (which is not part of the persisted states):

eclipse.platform.ui/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/ActivityPersistanceHelper.java

Lines 60 to 62 in f4989c9

    
           if (activityManagerEvent.haveEnabledActivityIdsChanged()) { 
        
           	saveEnabledStates(); 
        
           }

Enablement changes of expression-controlled activities are processed (and persisted as stated in 1.) even if the workbench is already closing. This has no effect as the activity support is disposed anyway during workbench close. Since the activity management is disposed after the service locator, which produces further events due to the evaluation service being removed, it receives and processes these additional events unnecessarily.

Reproduction

This unnecessary processing probably has no noticable effect in many products, as the types and amounts of activities are not high enough to produce a perceivable performance impact. In larger products this can, however, have severe impacts. In our product the effect is an overhead during startup and shutdown of the workbench of multiple seconds.

I have created a very artificial demo project with a large amount of defined activities and a large amount of enablement changes during startup/shutdown to make the impact obvious:

ManyActivitiesPerformanceDemo.zip

Evaluation

I have executed that demo with the current state of implementation and compared it to improved implementations that address the mentioned issue (can be seen in PR #1085 and #1086).

On my machine, the results with the demo project across several runs were as follows:

Startup time:
Without unnecessary persistence: 350-500 ms
With unnecessary persistence: 1500-1700 ms

Shutdown time:
With early disposal: 350-450 ms
Without proper disposal: 1300-1450 ms

How to proceed

I have started two PRs that address the mentioned issues (#1085, #1086). The changes of those PRs are the basis for the evaluation results mentioned above. In case you already see any side effects of addressing those issue that I am currently not aware of or, in general, reasons why the current behavior is required, I would be glad if you can let me know.

The text was updated successfully, but these errors were encountered:

…pse-platform#1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to eclipse-platform#1084.

…m#1084 The ActivityPersistenceHelper persists all activity enablements upon every notification about a changed activity enablement it receives. Persistence is, however, only done and thus necessary for activities that are not expression-controlled. So the current implementation performs unnecessary persistence operations. With this change, the ActivityPersistenceHelper only persists the enablements when the enablement of a non-expression-controlled activity has changed. To this end, the received ActivityManagerEvent is extended by the information about whether such an activity was affected. This information is either directly passed to the event construction in case it has already been calculated in the calling context or is calculated lazily when accessing the value. Contributes to eclipse-platform#1084.

…pse-platform#1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to eclipse-platform#1084.

…m#1084 The ActivityPersistenceHelper persists all activity enablements upon every notification about a changed activity enablement it receives. Persistence is, however, only done and thus necessary for activities that are not expression-controlled. So the current implementation performs unnecessary persistence operations. With this change, the ActivityPersistenceHelper only persists the enablements when the enablement of a non-expression-controlled activity has changed. To this end, the received ActivityManagerEvent is extended by the information about whether such an activity was affected. This information is either directly passed to the event construction in case it has already been calculated in the calling context or is calculated lazily when accessing the value. Contributes to eclipse-platform#1084.

…pse-platform#1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to eclipse-platform#1084.

…m#1084 The ActivityPersistenceHelper persists all activity enablements upon every notification about a changed activity enablement it receives. Persistence is, however, only done and thus necessary for activities that are not expression-controlled. So the current implementation performs unnecessary persistence operations. With this change, the ActivityPersistenceHelper only persists the enablements when the enablement of a non-expression-controlled activity has changed. To this end, the received ActivityManagerEvent is extended by the information about whether such an activity was affected. This information is either directly passed to the event construction in case it has already been calculated in the calling context or is calculated lazily when accessing the value. Contributes to eclipse-platform#1084.

…pse-platform#1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to eclipse-platform#1084.

vogella · 2023-09-12T07:59:33Z

Thanks @HeikoKlare for working on this, improved startup performance is always welcomed. If you feel confident with your chanages, please merge them early in this release so that they get intensive usage during this development cycle.

HeikoKlare · 2023-09-12T08:14:28Z

Thanks @vogella! I will merge the PRs soon (probably this week). I will just find someone at least having a quick look at them first.

…m#1084 The ActivityPersistenceHelper persists all activity enablements upon every notification about a changed activity enablement it receives. Persistence is, however, only done and thus necessary for activities that are not expression-controlled. So the current implementation performs unnecessary persistence operations. With this change, the ActivityPersistenceHelper only persists the enablements when the enablement of a non-expression-controlled activity has changed. To this end, the received ActivityManagerEvent is extended by the information about whether such an activity was affected. This information is either directly passed to the event construction in case it has already been calculated in the calling context or is calculated lazily when accessing the value. Contributes to eclipse-platform#1084.

fedejeanne · 2023-09-13T14:36:01Z

I applied both of your PRs and these are my results

------------------------------------------------------------------
With PR-1085 and PR-1086
------------------------------------------------------------------
1st run
[With Activity Enablement Changes] Starting workbench took: 2242 ms.
[With Activity Enablement Changes] Closing workbench took: 3139 ms.
2nd run
[With Activity Enablement Changes] Starting workbench took: 2274 ms.
[With Activity Enablement Changes] Closing workbench took: 3103 ms.
3rd run
[With Activity Enablement Changes] Starting workbench took: 2316 ms.
[With Activity Enablement Changes] Closing workbench took: 3107 ms.
4th run
[With Activity Enablement Changes] Starting workbench took: 2430 ms.
[With Activity Enablement Changes] Closing workbench took: 3537 ms.
5th run
[With Activity Enablement Changes] Starting workbench took: 2509 ms.
[With Activity Enablement Changes] Closing workbench took: 3229 ms.

 

------------------------------------------------------------------
WIthout PRs
------------------------------------------------------------------
1st run
[With Activity Enablement Changes] Starting workbench took: 4321 ms.
[With Activity Enablement Changes] Closing workbench took: 6632 ms.
2nd run
[With Activity Enablement Changes] Starting workbench took: 4236 ms.
[With Activity Enablement Changes] Closing workbench took: 4537 ms.
3rd run
[With Activity Enablement Changes] Starting workbench took: 4191 ms.
[With Activity Enablement Changes] Closing workbench took: 4986 ms.
4th run
[With Activity Enablement Changes] Starting workbench took: 3829 ms.
[With Activity Enablement Changes] Closing workbench took: 5534 ms.
5th run
[With Activity Enablement Changes] Starting workbench took: 4299 ms.
[With Activity Enablement Changes] Closing workbench took: 4660 ms

Good performance improvement :-)

…pse-platform#1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to eclipse-platform#1084.

…m#1084 The ActivityPersistenceHelper persists all activity enablements upon every notification about a changed activity enablement it receives. Persistence is, however, only done and thus necessary for activities that are not expression-controlled. So the current implementation performs unnecessary persistence operations. With this change, the ActivityPersistenceHelper only persists the enablements when the enablement of a non-expression-controlled activity has changed. To this end, the received ActivityManagerEvent is extended by the information about whether such an activity was affected. This information is either directly passed to the event construction in case it has already been calculated in the calling context or is calculated lazily when accessing the value. Contributes to eclipse-platform#1084.

The ActivityPersistenceHelper persists all activity enablements upon every notification about a changed activity enablement it receives. Persistence is, however, only done and thus necessary for activities that are not expression-controlled. So the current implementation performs unnecessary persistence operations. With this change, the ActivityPersistenceHelper only persists the enablements when the enablement of a non-expression-controlled activity has changed. To this end, the received ActivityManagerEvent is extended by the information about whether such an activity was affected. This information is either directly passed to the event construction in case it has already been calculated in the calling context or is calculated lazily when accessing the value. Contributes to #1084.

…pse-platform#1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to eclipse-platform#1084.

The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to #1084.

HeikoKlare · 2023-09-29T13:42:56Z

Thanks for that validation, @fedejeanne!
FWIW: the differences in the measurements provided in #1084 (comment) and the ones I provided in the initial posting #1084 (comment) are not only due to different hardware but probably also because I reduced the set of plugins to those required for the test plugin, which reduces the overall execution times. When starting with a "complete" set of platform plugins, the measurements on my machine are comparable to those provided by @fedejeanne.

HeikoKlare · 2023-09-29T13:43:29Z

This issue is resolved by #1085 and #1086.

HeikoKlare added the performance label Sep 6, 2023

This was referenced Sep 6, 2023

Avoid unnecessary persistence of activity enablements #1084 #1085

Merged

Dispose activity management during shutdown completely and early #1084 #1086

Merged

Disable Event Processing During Workbench Shutdown vi-eclipse/Eclipse-Platform#14

Closed

HeikoKlare closed this as completed Sep 29, 2023

vogella mentioned this issue Nov 3, 2023

Closing Eclipse IDE shows flashing popup if closing is really fast #1269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Startup/shutdown performance: unnecessary processing of activity events #1084

Startup/shutdown performance: unnecessary processing of activity events #1084

HeikoKlare commented Sep 6, 2023 •

edited

Loading

vogella commented Sep 12, 2023

HeikoKlare commented Sep 12, 2023

fedejeanne commented Sep 13, 2023

HeikoKlare commented Sep 29, 2023

HeikoKlare commented Sep 29, 2023

Startup/shutdown performance: unnecessary processing of activity events #1084

Startup/shutdown performance: unnecessary processing of activity events #1084

Comments

HeikoKlare commented Sep 6, 2023 • edited Loading

Issue

Reproduction

Evaluation

How to proceed

vogella commented Sep 12, 2023

HeikoKlare commented Sep 12, 2023

fedejeanne commented Sep 13, 2023

HeikoKlare commented Sep 29, 2023

HeikoKlare commented Sep 29, 2023

HeikoKlare commented Sep 6, 2023 •

edited

Loading