Community
Participate
Working Groups
DiagramMappingsManagerImpl is responsible for computing and maintaining a hiearchy of Mappings depending on the layers which are activated. Right now the code creates a manager for each DDiagram instance, but conceptually two diagrams from the same diagram description with the same list of activated layer will have the same hiearchy table. We should investigate how to make sure we won't do the same computations twices in those cases. This part of the code is also very difficult to debug because the relationships between DiagramMappingManagers and DiagramDescriptionMappingManagers are bidirectionals and their lifecycle is not explicit. It might be a good occasion to clear that up a bit...
Just to document before leaving for vacations. So far it appears : DiagramMappingManager & co are eating a lot of memory when one use quite a lot of diagrams (1000s) Big memory leaks are very easy to get as these instances have a reference to DDiagram instances. A test case based on EcoreTools 2 with 500K model elements and 2000 diagrams will fail with an OutOfMemorry error when enabling a Viewpoint with the current code, even with 2Go allocated to this. The logic to "maintain" these instances state up to date is very complex (and I would say quite un-predictable, hard to debug or change in any way). These instances are listening to the ViewpointRegistry, to the SessionManager just to invalidate some computations and will recompute as soon as there is an update from them (even if we might not need it) Analysing the actual behavior of these classes and why they are used, their names are badly reflecting what they actually does. DiagramMappingsManagerRegistry => is actually a cache maintaining all the instances of DiagramMappingsManager and re-trigger their computation on when a session changes. Or when a new layer has been enabled. Or... DiagramDescriptionMappingsRegistry => a cache of DiagramDescriptionMappingsManager which also re-trigger their computation when the ViewpointRegistry changes. DiagramDescriptionMappingsManager => is the result of a computation representing all the mappings (Node/Edge/..) and their hiearchy which are available for a given set of enabled Viewpoints DiagramMappingsManager => is the result of a computation representing all the available mappings (Node/Edge/...) and their hiearchy which are available from a given set of activated layers. To compute / update its state, the DiagramMappingsManager needs a DiagramDescriptionMappingsManager DiagramMappingsManagerImpl => is an interesting case of code which scream "I am two implementations in a single class which will change its behavior depending on whether you are in "layer mode" or not". DiagramDescriptionMappingsManagerListener => exists so that the DiagramMappingsManager will know when the computation result it was based on has changed so that it can trigger its own re-computation. Preliminary analysis work has been done in this branch: https://github.com/cbrun/org.eclipse.sirius/tree/wip/cbr/440032_diagrammappingmanagers (which is not quite ready to be considered for inclusion.) I took an approach where DiagramMappingManager & DiagramDescriptionMappingsManager are not considered as self-updating themselves anymore and the so-called Registries will either return a cached version or recompute a new one when some client code ask for it. I choose a very simple eviction strategy : when a Viewpoint is getting enabled/disabled or when a session closes : drop everything. Once the computation is shared among diagram instances through the cache it is not a big deal to have to re-compute the hierarchy of Mappings compared to risking having stalled information or leaks. With this approach any code keeping a DiagramMappingManager instance as a field is probably wrong as the validity of such a computation is quite short lived. Instead it should refers to the Cache and ask for the computation when needed. I also renamed all those classes to be consistent with the approach. In its current state the branch gives good performance enhancements, when enabling/disabling a viewpoint, enabling/disabling a layer with several representations having the same combination of activated layers. Impact on the refresh operations themsevles remains to be measured.
Renaming the Classes in the middle of the work was probably a bad idea as it would make testing more difficult because of the API changes and reviewing would have been harder to. I pushed here https://github.com/cbrun/org.eclipse.sirius/tree/perfosprint_diagrammappingmanager_440032 a new branch which contains the changes without the renaming. It is rebased on the "performances" branch so that we can compare the results with the other performance related work. It is not ready for review yet.
