Community
Participate
Working Groups
While profiling the scenario for bug 441770, it became obvious that even for basic operations, we trigger too many EditPart.refresh() calls. For example, by instrumenting AbstractDDiagramEditPart.refresh() and AbstractDiagramElementContainerEditPart.refresh(), when creating a empty container on an otherwise empty diagram, I count: * 2 calls to DDiagramEditPart.refresh() on the same DDiagramEditPart instance. * 17 calls to AbstractDiagramElementContainerEditPart.refresh() on the same DNodeContainerEditPart instance. They appear in this order: DDiagramEditPart[7082f813].refresh() DNodeContainerEditPart[5c550cd3].refresh() (13 times) DDiagramEditPart[7082f813].refresh() DNodeContainerEditPart[5c550cd3].refresh() (4 times) When dropping a list container with 112 list items (from the sample model available on bug 441770), the numbers are even worse: DDiagramEditPart[7082f813].refresh() DNodeListEditPartWithAlpha[4aff8c01].refresh() DDiagramEditPart[7082f813].refresh() DNodeListEditPartWithAlpha[4aff8c01].refresh() (238 times) DDiagramEditPart[7082f813].refresh() DNodeListEditPartWithAlpha[4aff8c01].refresh() (4 times) Note that the DNodeListEditPartWithAlpha which is refresh()-ed 243 times is the list container itself, not the items (I did not instrument the items' refresh(), but fro what I saw in https://bugs.eclipse.org/bugs/show_bug.cgi?id=441770#c8, it would be 10s or 100s of thousands of calls). We should understand better why so many calls are triggered, and strive to reduce them to the minimum.
Cleanups that will be done in the context of bug #453902 could make it easier to understand what happens and why. Conversely, do not hesitate to simplify/refactor the code (referencing #453902 for this work) when working on this particular ticket if it makes it easier to analyze (and fix) the issues.
New Gerrit change created: https://git.eclipse.org/r/43165
Created attachment 251351 [details] Test case
New Gerrit change created: https://git.eclipse.org/r/43318
Non exhaustive list of things to rework/remove to limit the number of EditPart.refresh() : - Replace SemanticChangedCommand by a precommit in manual refresh : https://git.eclipse.org/r/#/c/43165/ - Analyse if on Mapping's StyleDescription change NotificationListener of DiagramElementEditPartOperation.createEApdaterDiagramElement() is useful. And if useful have a better mechanism like GMF View corresponding to the IStyleEditPart recreated to have IStyleEditPart recreated, for example SquareEditPart -> DotEditPart. - Have AbstractDiagramNodeEditPart.refreshVisuals() don't call DiagramNodeEditPartOperation.refreshVisuals(this); and have draw2d bounds updated only through GMF Bounds and not from DNode.width and not using AbstractDiagramNodeEditPartRefreshVisualsOperation.refreshSize() - EditStatusUpdater should be a postcommit - AbstractDiagramNodeEditPart.refresh() should not refresh its children, that don't seems usefull, children of a DNodeXEditPart are AbstractDiagramNameEditPart or others DNodeXEditParts which are refreshed on their GMF View changes - Code in AbstractDDiagramEditPart.handleNotificationEvent() seems do EditPart.refresh() about layer/filter changes while layer/filter changes impact only visibility of GMF View then only EditPart.refreshVisibility() should be called and automatically by GMF. - Too more listeners are registered in DiagramElementEditPartOperation.activate() and this could be - AbstractDiagramElementContainerEditPart.handleNotificationEvent() in some case can refresh its IStyleEditPart which seems useless and also 2 refresh of itself which following notification launch a refresh in super.handleNotificationEvent(). More cleanup works : - Have changes about color, i.e. RGBValues, in viewpoint model propagated in GMF model to avoid code like DiagramNodeEditPartOperation/DiagramElementEditPartOperation/DiagramEdgeEditPartOperation/DiagramContainerEditPartOperation.refreshForegroundColor()/refreshBackgroundColor()/refreshFont() as this can be managed directly by GMF.
To complete comment 5, I see a suspicious code in org.eclipse.sirius.diagram.ui.business.api.image.ImageSelectorService: when a basicLabelStyle is updated with a new image path, through method updateStyle(BasicLabelStyle, String), all the edit parts of the diagram is refreshed --> WorkspaceImageFigureRefresher.refreshAllEditPart
New Gerrit change created: https://git.eclipse.org/r/44263
New Gerrit change created: https://git.eclipse.org/r/44264
New Gerrit change created: https://git.eclipse.org/r/44674
New Gerrit change created: https://git.eclipse.org/r/44765
Gerrit change https://git.eclipse.org/r/44263 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=e7d07c250cebf1f413fcd0d9c3ad36fecaddd399
Gerrit change https://git.eclipse.org/r/44264 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=42c648b38445ffb924201f0fd35a8ec0bb0e32d8
Gerrit change https://git.eclipse.org/r/44765 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=42e7a7967977f1e7b8467747a7fccb4a81b4c89d
The initial review proposed by Esteban: https://git.eclipse.org/r/#/c/43165/ has been reworked and pushed with commits of comment 11 and comment 12.
Created attachment 251981 [details] TestProject.zip: Sample to verify changes in manual refresh This scenario is here to verify that the refresh is always done after the modifications of comment 11 Steps to reproduce: * Import the project 443518 from TestProject.zip * Open "root package entities" and "new Diagram" diagrams * Split the editors part to see both editor simultaneously * Check that you are in manual refresh mode (preference "Sirius/Automatic refresh" unchecked) * Rename p1 in p3 in "root package entities" diagram * --> Classes "p1-C1" and "p1-C2" are renamed in "new Diagram" diagram * Rename p3 in p5 in "root package entities" diagram * --> Classes "p3-C1" and "p3-C2" are renamed in "new Diagram" diagram, the background color of these classes have been changed and the icons too.
New Gerrit change created: https://git.eclipse.org/r/44821
Gerrit change https://git.eclipse.org/r/44821 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=a79e30ca75dba8aee052f5c2e45170ca443d6d1a
Assessment of the work done so far: No real gain on the number of refresh calls For the scenario of bug 441770 comment 3, there is no gain with what is done with commit of comment 11. There are 247 calls to DNodeListEditPartWithAlpha.refresh and 30537 calls to GraphicalEditPart.refresh(). With scenario of comment 15, there is also no gain on the number of refresh. But there is a gain on the number of adapters and on the number of transactionAboutToCommit. With the fix of comment 11, all the calls to org.eclipse.sirius.diagram.ui.edit.internal.part.DiagramElementEditPartOperation$1.transactionAboutToCommit(Notification) are removed.
About gerrit https://git.eclipse.org/r/#/c/43318/ which remove refreshVisuals() call in AbstractDiagramNodeEditPart.refresh(), we have test failures whose test LabelAlignmentRefreshTest.testLabelAlignmentRefresh() at diagram editor opening because the label is not displayed for DNodeEditPart Nodes2. Indeed when DNodeEditPart.refresh() is called, refreshChildren() is called after refreshVisuals() and its last through "DiagramNodeEditPartOperation.refreshVisuals(this);" refresh the child editpart figure, the SquareEditPart$SquareFigure in this case. To summarize the general issue is that parent EditPart manage draw2d figure refresh of its children EditParts while it should not, it is responsibility of each EditPart to refresh its draw2d figure.
Moving to 4.0, as the actual performance benefits are not yet measured (only the number of refresh() invocation).
Moving out of the 4.0 scope for now, along with all the other issues which were there "by default". This does not mean some of these will not be re-integrated at some point, but for now these issues are not part of the roadmap for 4.0. If you feel strongly about this removal from 4.0 and/or are ready to sponsor the corresponding work, feel free to comment.
Gerrit change https://git.eclipse.org/r/68909 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=0c05f5427005dc3ce41f5d69b7e85bb5691fe477
New Gerrit change created: https://git.eclipse.org/r/92390
Gerrit change https://git.eclipse.org/r/92390 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=10227967e1bbcd316139e9344930aaaf354475f0
The steps to reproduce of bug 571872 comment 1 is an interesting scenario for this kind of issue: only one modification (causing 5 notifications) and refresh doing the same thing called several times. In this issue, there is a text file [1] identifying the "stack" of each calls to DBorderItemLocator.setConstraint (called 18 times). In this scenario, as a part of comment 5, there are: * edit parts that react several times to the same notification * and edit parts that react, in the same way, to different notifications [1] https://bugs.eclipse.org/bugs/attachment.cgi?id=285800
