Community
Participate
Working Groups
When some behavior can be configured by the end-user through Eclipse preferences, the easiest way to make sure the code always does what the user wants is do read the preference's value from the preference store each time it is needed. Something like: SiriusTransPlugin.getPlugin().getPreferenceStore().getInt(CommonPreferencesConstants.PREF_GROUP_SIZE) However such an access is far for free in terms of performance, especially compared to raw field access. In performance-sensitive code (which includes code that may be called in deep loops from outside), this can hurt performance. A much better solution in terms of performance is to store the current value in a simple field, initialized once, and have a listener on the preference store to update it in the very rare cases when it will actually change. It is a little more complex to code, but I'll see if we can provide some helper code to make it easier, or at eh very least an example that can be easily followed as the recommended pattern.
See https://git.eclipse.org/r/32529 for an example of what I'm thinking about, in the case of GroupingContentProvider.
Other potential locations where such a change may be useful (beside GroupingContentProvider): * ViewQuery.getDefaultValue(EAttribute) * StyleConfigurationQuery.labelIconsMustBeHidden(DDiagramElement) * SiriusContainerEditPolicy.getArrangeCommand(ArrangeRequest) * RestoreToLastSavePointListener.isAllowedToReturnToSyncState() * SaveSessionWhenNoDialectEditorsListener.newMode() I'm not sure if all of these are called in performance-sensitive loops (except for labelIconsMustBeHidden(), for which I am sure it is), but it should be easy to check.
We could also centralize the auto-refresh checks.
(In reply to Maxime Porhel from comment #3) > We could also centralize the auto-refresh checks. Looking into this, 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.
It seems to be here to handle each element local refresh when the AUTO_REFRESH is disabled. 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)
(In reply to Pierre-Charles David from comment #4) > (In reply to Maxime Porhel from comment #3) > > We could also centralize the auto-refresh checks. > > Looking into this, 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".
See also bug 443307, where the issue comes up in tree item expansion.
I've created bug 443386 for the GroupingContentProvider case, so that each instance of this general issue can be handled separately.
I've created bug 444101 to look at the issue mentioned above the suspect adpaters installed by tree and table dialects's elements.
