Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak for actions instantiated through AbstractActionWrapperHandler #174

Closed
lredor opened this issue Nov 27, 2023 · 3 comments · Fixed by #175
Closed

Memory leak for actions instantiated through AbstractActionWrapperHandler #174

lredor opened this issue Nov 27, 2023 · 3 comments · Fixed by #175
Assignees
Labels
component: diagrams type: bug Something isn't working
Milestone

Comments

@lredor
Copy link
Contributor

lredor commented Nov 27, 2023

This issue is a sub part of #144. It concerns actions instantiated through AbstractActionWrapperHandler: BringForwardAction, BringToFrontAction, DeleteFromDiagramAction, DeselectAllAction, HideDDiagramElementAction, HideDDiagramElementLabelAction, PinElementsEclipseAction, QuickOutlineAction, RefreshDDiagramElementAction, RefreshAction, RevealAllElementsAction, RevealElementsAction, SendBackwardAction, SendToBackAction, RevealOutlineLabelsAction, SynchronizedDiagramAction and UnpinElementsEclipseAction.

Steps to reproduce:

  • Initialize the data
    • Launch a Sirius runtime
    • Switch to Modeling perspective
    • Create a new Modeling Project
    • Replace the content of this project with the 3 files from this folder. This data is used in org.eclipse.sirius.tests.swtbot.layout.ZOrderActionsTest where the leak has been discovered.
    • Close the runtime
  • Launch the Sirius runtime with YourKit (sample of startup options in "YourKit Java Profiler" tab of launch config: exceptions=off,disablestacktelemetry,alloceach=100,allocsizelimit=4096 to have memory analysis enabled)
  • In Model Explorer view, expand the above project, then My.ecore and root
  • Open the diagram diagramWithNodesAndEdges
  • On edge1, launch the action Bring to Front (from contextual menu Format>Order)
  • Then click on the menu Edit>Undo
  • Close the editor without saving
  • Go to YourKit and take a snapshot of the memory
  • Expected: In YourKit, you can observed the following leak concerning DDiagramEditorImpl referenced through the selection of BrintToFrontAction.
    YourKit_AbstractActionWrapperHandlerCase
@lredor lredor added component: diagrams type: bug Something isn't working labels Nov 27, 2023
@lredor lredor self-assigned this Nov 27, 2023
@lredor
Copy link
Contributor Author

lredor commented Nov 27, 2023

Analysis: The selection of this kind of actions is changed just before the execution of the action, in org.eclipse.sirius.ui.tools.internal.commands.AbstractActionWrapperHandler.execute(ExecutionEvent) (here). The selection is never reset, except in the dispose() method (for example, here for BrintToFrontAction). But the dispose of this kind of action is never called. It seems to be normal as more or less explained in documentation of the org.eclipse.ui.handlers extension point.

So a solution is to reset the selection just after the execution.

lredor added a commit that referenced this issue Nov 27, 2023
The selection, of actions launched through AbstractActionWrapperHandler,
was never cleaned. This commit cleans up the selection just after using
it.

Bug: #174
lredor added a commit that referenced this issue Nov 27, 2023
As the previous commit, the command FindElementCommand has the same
memory leak pattern, but without inherited from
AbstractActionWrapperHandler.

Bug: #174
@lredor
Copy link
Contributor Author

lredor commented Nov 27, 2023

The PR #175 solves the problem. With this PR, in the initial steps to reproduce, the observed memory leak is no longer here. It remains another leaks on DDiagramEditorImpl but they will be fixed through another issues.

@lredor
Copy link
Contributor Author

lredor commented Nov 27, 2023

Additional analysis: There is only "one memory leak" per action. Indeed, only one instance of this kind of actions is created during the first need. For example, in the scenario of this issue, the BringToFrontAction was created like this (see below stacktrace). If another BringToFrontAction was launched later for another diagram (in another session), the old selection was replaced by the new one. The leak was not cumulative.

Thread [main] (Suspended (breakpoint at line 35 in org.eclipse.sirius.diagram.ui.tools.internal.actions.layout.BringToFrontAction)) 
 org.eclipse.sirius.diagram.ui.tools.internal.actions.layout.BringToFrontAction.<init>() line: 35 
 org.eclipse.sirius.diagram.ui.tools.internal.commands.BringToFrontCommand.<init>() line: 29 
 jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(java.lang.reflect.Constructor<?>, java.lang.Object[]) line: not available [native method] 
 jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(java.lang.Object[]) line: 77 
 jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(java.lang.Object[]) line: 45 
 java.lang.reflect.Constructor<T>.newInstanceWithCaller(java.lang.Object[], boolean, java.lang.Class<?>) line: 499 
 java.lang.reflect.Constructor<T>.newInstance(java.lang.Object...) line: 480 
 org.eclipse.core.internal.registry.osgi.EquinoxRegistryStrategy(org.eclipse.core.internal.registry.osgi.RegistryStrategyOSGI).createExecutableExtension(org.eclipse.core.runtime.spi.RegistryContributor, java.lang.String, java.lang.String) line: 204 
 org.eclipse.core.internal.registry.ExtensionRegistry.createExecutableExtension(org.eclipse.core.runtime.spi.RegistryContributor, java.lang.String, java.lang.String) line: 920 
 org.eclipse.core.internal.registry.ConfigurationElement.createExecutableExtension(java.lang.String) line: 246 
 org.eclipse.core.internal.registry.ConfigurationElementHandle.createExecutableExtension(java.lang.String) line: 63 
 ...
 org.eclipse.jface.internal.MenuManagerEventHelper.showEventPostHelper(org.eclipse.jface.action.MenuManager) line: 95 
 org.eclipse.jface.action.MenuManager.handleAboutToShow() line: 469 
 org.eclipse.jface.action.MenuManager$2.menuShown(org.eclipse.swt.events.MenuEvent) line: 495 
 ...
 org.eclipse.swt.internal.win32.OS.TrackPopupMenu(long, int, int, int, int, long, org.eclipse.swt.internal.win32.RECT) line: not available [native method] 
 org.eclipse.swt.widgets.Menu._setVisible(boolean) line: 237 
 org.eclipse.swt.widgets.Display.runPopups() line: 4113 
 org.eclipse.swt.widgets.Display.readAndDispatch() line: 3654 
 ...

lredor added a commit that referenced this issue Dec 4, 2023
The selection, of actions launched through AbstractActionWrapperHandler,
was never cleaned. This commit cleans up the selection just after using
it.

Bug: #174
lredor added a commit that referenced this issue Dec 4, 2023
As the previous commit, the command FindElementCommand has the same
memory leak pattern, but without inherited from
AbstractActionWrapperHandler.

Bug: #174
lredor added a commit that referenced this issue Dec 4, 2023
The selection, of actions launched through AbstractActionWrapperHandler,
was never cleaned. This commit cleans up the selection just after using
it.

Bug: #174
lredor added a commit that referenced this issue Dec 4, 2023
As the previous commit, the command FindElementCommand has the same
memory leak pattern, but without inherited from
AbstractActionWrapperHandler.

Bug: #174
@lredor lredor added this to the v7.4.0 milestone Dec 4, 2023
@lredor lredor closed this as completed Dec 4, 2023
@lredor lredor linked a pull request Dec 4, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: diagrams type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant