Community
Participate
Working Groups
See https://bugs.eclipse.org/bugs/show_bug.cgi?id=442862#c4 I found some suspect code in DLineSpec, DCellSpec and DTargetColumnSpec. The getOrCreateListener() methods, which is called on activate() seems to install adapters on all semantic elements (target and associated elements) referenced by the table, which on every change on any of these will check the value of the AUTO_REFRESH setting directly from the preference store! This looks so bad at first glance that I am surprised we never found this in the profiles we took (granted, there are larger performance issues in tables that could explain that accesses to the preference store would not show up as hot spots). Anyway, I'm putting it here to remember to look further into this. There is also another problem (again, this needs further investigation to confirm): the activate() and deactivate() methods both refer to the getSemanticElements() reference to install (resp. uninstall the adapters). But the value of that reference is computed from a user-supplied expression, and is updated on each refresh. This means that: 1) if new semantic elements become associated to the table element after its creation, the adapter will not be installed on them. 2) if semantic elements disappear from the reference, deactivate() will not remove the adapter from them. The adapter is an instance of a anonymous internal class, which keeps a reference to the DTable/DLine/DCell/etc., and through is to all the representation, the session model, etc. The adapter also captures the DTableElementSynchronizer, which may retain other elements. If these issues are confirmed, we should create a separate ticket, this is quite different from the origin scope of "avoid accessing the preference store too often". In the Tree dialect, the same mechanism is present (see DTreeSpec and DTreeITemSpec) On the diagram side, this is done with listeners (precommit) and not adapters, see org.eclipse.sirius.diagram.ui.edit.api.part.AbstractBorderedDiagramElementEditPart.activate() and org.eclipse.sirius.diagram.ui.edit.internal.part.DiagramElementEditPartOperation.createEAdpaterSemanticElements(IDiagramElementEditPart)
We should indeed re-consider the way these "updates" are handled. * the memory cost is not null in managing so many "bindings" as tables tend to ù easily grow in number of lines/cells. * in a table at least, many DCells/Dline are actually listening to the same element. * such a granularity (the model elements) tend to make optimisation of "what we do when something changes" hard to plan. I am wondering if an approach where : 1- the change is inspected, the DTable model is browsed for .target elements 2- an element targeted by a .target reference has changed => we might need to re-compute the semantic elements references, we do so 3- we browse the dtable model to detect DLine/DColumns/DCells which will need a "local refresh" based on change and the .target and .semanticElements references would have better scalability characteristics or not.
A gerrit review is available: https://git.eclipse.org/r/#/c/41298/ The approach consist in update the DTableElements in pre-commit instead of install adapters on each semantic targets.
New Gerrit change created: https://git.eclipse.org/r/42015
Created attachment 250858 [details] Incremental refresh by using the xRef (first test)
Created attachment 250859 [details] Incremental refresh by using the xRef (second test)
Created attachment 250860 [details] Incremental refresh without using the xRef (first test)
Created attachment 250861 [details] Incremental refresh without using the xRef (second test)
Created attachment 250862 [details] Test case (based on ecore tool)
Results of the poc by using the InverseCrossReferencer to retrieve impacted DTableElements: During the first test the interpreter is being initialized. the incremental refresh took 84ms During the second test the interpreter was initialized. the incremental refresh took 4ms Results of the poc without using the InverseCrossReferencer but by iterating on each Dline: During the first test the interpreter is being initialized. the incremental refresh took 168ms During the second test the interpreter was initialized. the incremental refresh took 28ms All tests have been done with the "full-sirius-code class table"
The profiling is based on this path set for the test without Xref: https://git.eclipse.org/r/#/c/42015/2 and this patch set for test using the XRef: https://git.eclipse.org/r/#/c/41298/6
I gave it a shot by improving the implementation of both codes to avoid iterating 20K times over the list of changes adapting the "non-xref" implementation to correctly search into cells. I kept both code running at the same time calling the two methods in the same environment to have a more precise comparison. I also extended the test data to have "more .aird stuffs", to do so I did a few copies of the tables to see if we have a noticeable change of scalability when the .aird grow (as the xRef based implementation will be sensitive to that) I made sure to be on manual refresh and triggered Yourkit in tracing mode with "adaptive=false" (or as we are measuring very small times yourkit tends to "truncate" some of the calls) 10 subsequents invocations by changing an EClass name through the property view with two tables being opened, veryfing the tables are getting properly updated : total time spent in search for impacted DTableElements : - with Xref : 3ms (10 calls) - without Xref : 1300ms (10 calls) The difference is more important than I would have expected. When not using the xRef the time is spent in AbstractEList.EIterator (mostly constructor and hasNext()) and in HashSet.contains() All in all we should go for the xRef based one even if it might get slower and slower once the .aird grow, the overhead of browsing through the table is too significant to be ignored.
New Gerrit change created: https://git.eclipse.org/r/44025
New Gerrit change created: https://git.eclipse.org/r/44064
New Gerrit change created: https://git.eclipse.org/r/44063
Gerrit change https://git.eclipse.org/r/44063 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=d594bc99b7a4b82c947a647c50d9348c3914abdb
Gerrit change https://git.eclipse.org/r/41298 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=dbab8e4b34d85d8070cb1c77f91870ae9a462d5e
Gerrit change https://git.eclipse.org/r/44064 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=b9cd47cc3afa75deb0ee4bb2c63446126d688666
Fixed with the commits of the 3 last comments. The method used is not directly added in DialectServices but in non API DialectServices2 as it can change sooner when applying this mechanism to tree dialect and to diagram dialect (part of bug 443518 for diagram).
Created attachment 251779 [details] sampleProject.zip: Used to test the manual refresh The sampleProject allows to check that the refresh manual is always OK with the new implementation of this feature. Steps to reproduce for edition table: * Check that the preference "Sirius/Automatic Refresh" is false * Open session 444101.aird * Open table "new editionTable" * Open diagram "root package entities" * Split the edit view to see both editors * In the diagram, rename NewEClass2 into NewEClass3 --> The line of NewEClass1 with prefix "listen-" is automatically updated. --> The cell of column "Listen-Name" of the line "NewEClass1 -> NewEClass2" should be updated. This is a bug (already here before this issue). Another bugzilla will be created for it. Steps to reproduce for cross table: * Check that the preference "Sirius/Automatic Refresh" is false * Open session 444101.aird * Open table "new crossTable" * Open diagram "root package entities" * Split the edit view to see both editors * In the diagram, rename NewEClass2 into NewEClass3 --> The line and column "NewEClass1 extends ..." are automatically updated. The same modifications will be done shortly for tree dialect. Steps to reproduce for tree: * Check that the preference "Sirius/Automatic Refresh" is false * Open session 444101.aird * Open table "new Tree" * Open diagram "root package entities" * Split the edit view to see both editors * In the diagram, rename NewEClass2 into NewEClass3 --> For NewEClass1, the line with prefix "listen-" is automatically updated.
New Gerrit change created: https://git.eclipse.org/r/44231
New Gerrit change created: https://git.eclipse.org/r/44262
Gerrit change https://git.eclipse.org/r/44025 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=99cc27b118e6205c3b1bac289f82e99285d940af
Gerrit change https://git.eclipse.org/r/44231 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=a9623579d8f07472bef2ce26a9279d52ae925ee0
Gerrit change https://git.eclipse.org/r/44262 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=e96d5ca91de30daaa62abc35180e28f580ee50c2
Tha last 3 commits handle the case for Tree dialect (and some bugs on Table dialect discovered during Tree implementation).
Reopened with remarks on https://git.eclipse.org/r/#/c/41298 Gerrit : - TableDialectServices.getTableElementsToRefresh() : using a Set instead of a ArrayList could avoid having potential duplicate. And instead of having 2 loops, we could do all work in a single one. - TableDialectServices.getTableElementsFromInverseReferences() : we could use EObjectQuery.getInverseReferences(EReference) instead of writting this specific code. Have EObjectQuery.getInverseReferences(Set<EReference>) could be more usefull for our use case. - Instead of DialectServices2.refreshImpactedElements(), a better interface could be DialectServices2.getImpactedDRepresentationElements():Collection<DRepresentationElement> with a second method DialectServices2.refreshDRepresentationElements(Collection<DRepresentationElement>) - RefreshEditorsPrecommitListener.isImpactingNotification() do !notification.isTouch() test while touch notification are already filtered by SessionEventBroker - IProgressMonitor parameter is not used in TableDialectServices.refreshImpactedElements()
There is another comments no consider: the first of https://git.eclipse.org/r/#/c/44263/4/plugins/org.eclipse.sirius.diagram/src-core/org/eclipse/sirius/diagram/business/internal/dialect/DiagramDialectServices.java
New Gerrit change created: https://git.eclipse.org/r/44764
Remarks of comment 26 and comment 27 are handled with reviews [1] and [2] Except for "Instead of DialectServices2.refreshImpactedElements(), a better interface could be DialectServices2.getImpactedDRepresentationElements():Collection<DRepresentationElement> with a second method DialectServices2.refreshDRepresentationElements(Collection<DRepresentationElement>)" that is out of scope. [1] https://git.eclipse.org/r/#/c/44764 [2] https://git.eclipse.org/r/#/c/44765
Gerrit change https://git.eclipse.org/r/44764 was merged to [master]. Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=629ae3d6263d3f9972161029f9b05d4e4ff63e83
Remarks of comment 26 are done (see comment 29)
Eclipse Genie missed this commit (it has been added for bug 443518): 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
Created attachment 253769 [details] 444101.zip (replace old sampleProject.zip): Used to test the manual refresh
Verified with Sirius 3.0.0 RC1
Available in Sirius 3.0.0. See https://wiki.eclipse.org/Sirius/3.0.0.
