Bug 551062 - setViewerContents performance poor due to heavy use of getAdapter
Summary: setViewerContents performance poor due to heavy use of getAdapter
Status: NEW
Alias: None
Product: GEF
Classification: Tools
Component: GEF MVC (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows 10
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: gef-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-13 13:44 EDT by Mike Marchand CLA
Modified: 2020-04-15 03:49 EDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Marchand CLA 2019-09-13 13:44:47 EDT
I am using GEF 5.0.2 MVC and MVC.UI. I have a fairly large model of 6500 nodes and I am encountering some performance issues loading and unloading my model into GEF.

When I profile my application with YourKit, I am spending 205 seconds in setViewerContents(), 188s of which is spent in InfiniteCanvasViewer.getAdapter().

The two functions that are calling InfiniteCanvasViewer.getAdapter() excessively are ContentBehavior.getContentPartFactory() (spent 89s) and ContentBehavior.getContentPartPool() (spent 87s).

Has anyone else encountered this problem? I have some ideas to work around the problem with custom ContentBehavior and caching, but I would like to see if perhaps we might be able to optimize this behavior in GEF.
Comment 1 Matthias Wienand CLA 2019-09-19 03:18:53 EDT
Thanks for reporting! We should really cache both the content-part-factory and the content-part-pool (e.g. on activation). I will look into that if no one else is taking the ticket in the near future ;-) In the meantime, I would advice you to exchange it with a custom ContentBehavior and perform the caching yourself as you already suggested.
Comment 2 Hans Meier CLA 2020-04-15 03:49:48 EDT
We have similar performance issues, in particular when deleting multiple nodes at once. Caching the contentPartPools and contentPartFactories introduces a significant speedup (30s down to 17s).
(ContentBehaviorEx:)

private Map<IViewer, IContentPartFactory> contentPartFactories = new HashMap<>();
	private Map<IViewer, ContentPartPool> contentPartPools = new HashMap<>();

@Override
     protected void doActivate() {
     …
          IViewer viewer = host.getRoot().getViewer();

           // caching, critical for performance!
           contentPartFactories.put(viewer, viewer.getAdapter(IContentPartFactory.class));
           contentPartPools.put(viewer, viewer.getAdapter(ContentPartPool.class));
     …
     }

@Override
     protected IContentPartFactory getContentPartFactory() {
           IViewer viewer = getHost().getRoot().getViewer();

           if (!contentPartFactories.containsKey(viewer)) {
                contentPartFactories.put(viewer, viewer.getAdapter(IContentPartFactory.class));
           }
           return contentPartFactories.get(viewer);
     }

     @Override
     protected ContentPartPool getContentPartPool() {
           IViewer viewer = getHost().getRoot().getViewer();

           if (!contentPartPools.containsKey(viewer)) {
                contentPartPools.put(viewer, viewer.getAdapter(ContentPartPool.class));
           }
           return contentPartPools.get(viewer);
     }


However, using VisualVM I can identify another performance drain: AdaptableSupport.getAdapterKey().
This function is where 40% of self time and CPU time is spent. Most calls come from
AdapterInjector.performAdapterInjection(),
  AdapterInjector.isContextApplicable(), 
    InfiniteCanvasViewer.getAdapterKey(),
      AdaptableSupport.getAdapterKey().

Maybe it is possible to cache these adapters wherever used (not sure if the adapters really change all that often). However, if we could make some safe assumptions there might be a better solution to it:

If the "adapter" map doesn't change all the time and the values have a reasonable hashCode/equals implementation. Could thie map be a BiMap/BiDirectionalMap? That would leverage a significant speedup to the getAdapterKey() function.
Side quest: Is the ordering of this map actually needed?

// XXX: We keep a sorted map of adapters to have a deterministic order
     private ObservableMap<AdapterKey<?>, Object> adapters = FXCollections
                .observableMap(new TreeMap<AdapterKey<?>, Object>());