Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 457678

Summary: Improve RefreshLayoutCommand
Product: [Modeling] Sirius Reporter: Laurent Redor <laurent.redor>
Component: DiagramAssignee: Laurent Redor <laurent.redor>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: mickael.lanoe, pierre-charles.david
Version: unspecifiedKeywords: triaged
Target Milestone: 2.0.4   
Hardware: PC   
OS: Linux   
Whiteboard:
Bug Depends on:    
Bug Blocks: 458973    
Attachments:
Description Flags
SequenceProject.zip (for steps to reproduce)
none
Initial state - First snapshot screenshot
none
Snapshot with cache capability none

Description Laurent Redor CLA 2015-01-16 05:36:32 EST 
In sequence diagram, the RefreshLayoutCommand is used at each move to recompute correctly the location of each elements. This introduces a feeling of slowness in the user experience. The goal of this evolution is to improve the RefreshLayoutCommand to reduce this feeling.

Steps to reproduce:
* Import the project Sequence from SequenceProject.zip (this project uses Interaction MM)
* Open sequenceDiagram
* Move Actor1 to the left, from several pixels only (4-5) to avoid moving the others lifelines. This first move is here to ensure that everything is well initialized.
* Start CPU recording (with Yourkit for example)
* Move Actor1 to the left, from several pixels only (4-5) to avoid moving the others lifelines
* Stop CPU recording and take a memory snapshot

The scope of this analysis/improvement is the method RefreshLayoutCommand.doExecute(). It represents 100% of the analysis time.
Comment 1 Laurent Redor CLA 2015-01-16 06:12:38 EST 
Created attachment 249990 [details]
SequenceProject.zip (for steps to reproduce)
Comment 2 Laurent Redor CLA 2015-01-16 07:06:08 EST 
Created attachment 249993 [details]
Initial state - First snapshot screenshot

A first snapshot, with the current state (on branch 2.0.x), shows that:
* 20% of the time is passed in SequenceDiagram.getAllLifelines (532 calls)
* 18% of the time is passed in SequenceDiagram.getAllOperands (532 calls)
* look at Sirius-InitialState.snapshot.png to have more details
Comment 3 Laurent Redor CLA 2015-01-16 07:08:29 EST 
For the previous (comment 2) analysis, there are 2 solutions:
* Reduce the number of calls to these methods
* Add a cache for method getAll* from SequenceDiagram. Indeed, the values returned by these methods can certainly be cache during the layout.
Comment 4 Laurent Redor CLA 2015-01-16 07:55:37 EST 
Created attachment 249995 [details]
Snapshot with cache capability

With a cache capability, gerrit https://git.eclipse.org/r/#/c/39746/1, the previous problems, comment 2, are solved. 0% of the time is used for SequenceDiagram.getAllLifelines and SequenceDiagram.getAllOperands.

This gerrit is in DRAFT mode because it can still change based on future analyzes.
Comment 5 Laurent Redor CLA 2015-01-16 09:25:19 EST 
After the cache:
* we can observe that DDiagramElementImpl.getDiagramElementMapping() is called 18582 times and cost 2% of times.
* we can observe that getVerticalRange() is called often
** Message.getVerticalRange(): 12%, 527 calls
** SequenceMessageViewQuery..getVerticalRange(): 11%, 527 calls
** SequenceNodeQuery.getVerticalRange(): 9%, 2005 calls
** Operand.getVerticalRange(): 2%, 625 calls
** Execution.getVerticalRange(): 1%, 252 calls

Sometimes in code there are 2 calls to this method. Only one is necessary if we use a local variable. With these optimisations, https://git.eclipse.org/r/#/c/39747/1 and https://git.eclipse.org/r/#/c/39748/1:
* we can observe that DDiagramElementImpl.getDiagramElementMapping() is called 9268 times and cost 1% of times.
* we can not observe real change on getVerticalRange()
Comment 6 Laurent Redor CLA 2015-01-16 10:35:31 EST 
With analysis, I observe that in ParentOperandFinder.getParentOperand(Range), there is a possible optimisation. In a AND predicate, the second predicate is less costly that the first one. This is the gerrit https://git.eclipse.org/r/#/c/39749/1.
Before this gerrit, there are 76 calls to ParentOperandFinder.getParentOperand(Range) and it costs 10%. After, same number of calls, but it costs 9%
Comment 8 Laurent Redor CLA 2015-01-23 03:29:31 EST 
Another improvement has been commited to reduce the calls to event ends computation : http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?h=v2.0.x&id=c0abae26590c2df71d3c5ce038447dedb010f1d8
Comment 9 Laurent Redor CLA 2015-02-02 11:33:27 EST 
All enhancements are considered as sufficient.
Comment 10 Mickael LANOE CLA 2015-02-11 11:38:56 EST 
Validated
Comment 11 Pierre-Charles David CLA 2015-02-12 03:26:07 EST 
Validated by Mickael.
Comment 12 Pierre-Charles David CLA 2015-02-12 07:54:57 EST 
Available in Sirius 2.0.4.