From b785d9eae9f5b7330a22235d23b160b602d17e49 Mon Sep 17 00:00:00 2001 From: Jean-Yves TINEVEZ Date: Sat, 26 Aug 2023 19:54:42 +0200 Subject: [PATCH] Rework the track color generator for better performance. I foresee that it will not be good to generate the track colors, by waling the full graph ONCE per opened view TIMES ONCE per change in the graph. So this version: - Is based on the RootProvider, and listens to it to only regenerate the cache when the set of roots changes. - There is only one common instance of the track color generator, use by all views, and it is registered as a manager in the window manager. I took some care of what happens if several threads try to query the track color concurrently, at least partially. - The color cache is regenerated only if there is a change in the root set AND if at least one view is using the 'Per track' coloring mode. --- .../org/mastodon/mamut/WindowManager.java | 5 ++ .../mastodon/mamut/views/MamutBranchView.java | 7 +- .../org/mastodon/mamut/views/MamutView.java | 7 +- .../mamut/views/table/MamutViewTable.java | 7 +- .../ui/coloring/TrackGraphColorGenerator.java | 79 +++++++++++-------- 5 files changed, 58 insertions(+), 47 deletions(-) diff --git a/src/main/java/org/mastodon/mamut/WindowManager.java b/src/main/java/org/mastodon/mamut/WindowManager.java index 65bee2bfd..618059589 100644 --- a/src/main/java/org/mastodon/mamut/WindowManager.java +++ b/src/main/java/org/mastodon/mamut/WindowManager.java @@ -57,11 +57,13 @@ import org.mastodon.feature.ui.FeatureColorModeConfigPage; import org.mastodon.mamut.feature.MamutFeatureProjectionsManager; import org.mastodon.mamut.managers.StyleManagerFactory; +import org.mastodon.mamut.model.Link; import org.mastodon.mamut.model.Model; import org.mastodon.mamut.model.Spot; import org.mastodon.mamut.views.MamutViewFactory; import org.mastodon.mamut.views.MamutViewI; import org.mastodon.model.tag.ui.TagSetDialog; +import org.mastodon.ui.coloring.TrackGraphColorGenerator; import org.mastodon.ui.coloring.feature.FeatureColorModeManager; import org.mastodon.ui.keymap.KeyConfigContexts; import org.mastodon.ui.keymap.KeymapSettingsPage; @@ -179,6 +181,9 @@ public WindowManager( final ProjectModel appModel ) final MamutFeatureProjectionsManager featureProjectionsManager = new MamutFeatureProjectionsManager( context.getService( FeatureSpecsService.class ), featureColorModeManager ); featureProjectionsManager.setModel( model, appModel.getSharedBdvData().getSources().size() ); managers.put( MamutFeatureProjectionsManager.class, featureProjectionsManager ); + final TrackGraphColorGenerator< Spot, Link > trackGraphColorGenerator = new TrackGraphColorGenerator<>( model.getGraph() ); + appModel.projectClosedListeners().add( () -> trackGraphColorGenerator.close() ); + managers.put( TrackGraphColorGenerator.class, trackGraphColorGenerator ); /* * Discover and handle view factories diff --git a/src/main/java/org/mastodon/mamut/views/MamutBranchView.java b/src/main/java/org/mastodon/mamut/views/MamutBranchView.java index d7a389e6e..71c02bee5 100644 --- a/src/main/java/org/mastodon/mamut/views/MamutBranchView.java +++ b/src/main/java/org/mastodon/mamut/views/MamutBranchView.java @@ -269,9 +269,8 @@ protected final ColoringModel registerBranchColoring( onClose( () -> featureModel.listeners().remove( coloringMenu ) ); // Handle track color generator. - final TrackGraphColorGenerator< Spot, Link > tgcg = new TrackGraphColorGenerator<>( appModel.getModel().getGraph() ); - appModel.getModel().getGraph().addGraphChangeListener( tgcg ); - onClose( () -> appModel.getModel().getGraph().removeGraphChangeListener( tgcg ) ); + @SuppressWarnings( "unchecked" ) + final TrackGraphColorGenerator< Spot, Link > tgcg = appModel.getWindowManager().getManager( TrackGraphColorGenerator.class ); // Adapt it so that is a color generator for the branch graph. final Model m = appModel.getModel(); final GraphColorGeneratorAdapter< Spot, Link, BranchSpot, BranchLink > branchTgcg = new GraphColorGeneratorAdapter<>( @@ -282,7 +281,6 @@ protected final ColoringModel registerBranchColoring( @SuppressWarnings( "unchecked" ) final ColoringModelMain.ColoringChangedListener coloringChangedListener = () -> { final GraphColorGenerator< BranchSpot, BranchLink > colorGenerator; - tgcg.pauseListener( true ); switch(coloringModel.getColoringStyle()) { case BY_FEATURE: @@ -292,7 +290,6 @@ protected final ColoringModel registerBranchColoring( colorGenerator = new TagSetGraphColorGenerator<>( tagSetModel, coloringModel.getTagSet() ) ; break; case BY_TRACK: - tgcg.pauseListener( false ); colorGenerator = branchTgcg; break; case NONE: diff --git a/src/main/java/org/mastodon/mamut/views/MamutView.java b/src/main/java/org/mastodon/mamut/views/MamutView.java index c39ff9cd5..36a3502c3 100644 --- a/src/main/java/org/mastodon/mamut/views/MamutView.java +++ b/src/main/java/org/mastodon/mamut/views/MamutView.java @@ -199,12 +199,10 @@ protected ColoringModelMain< Spot, Link, BranchSpot, BranchLink > registerColori onClose( () -> featureModel.listeners().remove( coloringMenu ) ); // Handle track color generator. - final TrackGraphColorGenerator< Spot, Link > tgcg = new TrackGraphColorGenerator<>( appModel.getModel().getGraph() ); - appModel.getModel().getGraph().addGraphChangeListener( tgcg ); - onClose( () -> appModel.getModel().getGraph().removeGraphChangeListener( tgcg ) ); + @SuppressWarnings( "unchecked" ) + final TrackGraphColorGenerator< Spot, Link > tgcg = appModel.getWindowManager().getManager( TrackGraphColorGenerator.class ); final ColoringModelMain.ColoringChangedListener coloringChangedListener = () -> { - tgcg.pauseListener( true ); final GraphColorGenerator< Spot, Link > colorGenerator; switch ( coloringModel.getColoringStyle() ) { @@ -215,7 +213,6 @@ protected ColoringModelMain< Spot, Link, BranchSpot, BranchLink > registerColori colorGenerator = new TagSetGraphColorGenerator<>( tagSetModel, coloringModel.getTagSet() ); break; case BY_TRACK: - tgcg.pauseListener( false ); colorGenerator = tgcg; break; case NONE: diff --git a/src/main/java/org/mastodon/mamut/views/table/MamutViewTable.java b/src/main/java/org/mastodon/mamut/views/table/MamutViewTable.java index d1c36096f..aec7b832e 100644 --- a/src/main/java/org/mastodon/mamut/views/table/MamutViewTable.java +++ b/src/main/java/org/mastodon/mamut/views/table/MamutViewTable.java @@ -253,9 +253,8 @@ private static final ColoringModel registerBranchColoring( runOnClose.add( () -> featureModel.listeners().remove( coloringMenu ) ); // Handle track color generator. - final TrackGraphColorGenerator< Spot, Link > tgcg = new TrackGraphColorGenerator<>( appModel.getModel().getGraph() ); - appModel.getModel().getGraph().addGraphChangeListener( tgcg ); - runOnClose.add( () -> appModel.getModel().getGraph().removeGraphChangeListener( tgcg ) ); + @SuppressWarnings( "unchecked" ) + final TrackGraphColorGenerator< Spot, Link > tgcg = appModel.getWindowManager().getManager( TrackGraphColorGenerator.class ); // Adapt it so that is a color generator for the branch graph. final Model m = appModel.getModel(); final GraphColorGeneratorAdapter< Spot, Link, BranchSpot, BranchLink > branchTgcg = new GraphColorGeneratorAdapter<>( @@ -266,7 +265,6 @@ private static final ColoringModel registerBranchColoring( @SuppressWarnings( "unchecked" ) final ColoringModelMain.ColoringChangedListener coloringChangedListener = () -> { final GraphColorGenerator< BranchSpot, BranchLink > colorGenerator; - tgcg.pauseListener( true ); switch ( coloringModel.getColoringStyle() ) { case BY_FEATURE: @@ -276,7 +274,6 @@ private static final ColoringModel registerBranchColoring( colorGenerator = new TagSetGraphColorGenerator<>( branchTagSetModel( appModel ), coloringModel.getTagSet() ); break; case BY_TRACK: - tgcg.pauseListener( false ); colorGenerator = branchTgcg; break; case NONE: diff --git a/src/main/java/org/mastodon/ui/coloring/TrackGraphColorGenerator.java b/src/main/java/org/mastodon/ui/coloring/TrackGraphColorGenerator.java index 0e54715c4..af5fc2e12 100644 --- a/src/main/java/org/mastodon/ui/coloring/TrackGraphColorGenerator.java +++ b/src/main/java/org/mastodon/ui/coloring/TrackGraphColorGenerator.java @@ -5,16 +5,14 @@ import org.mastodon.collection.RefList; import org.mastodon.collection.RefMaps; import org.mastodon.graph.Edge; -import org.mastodon.graph.GraphChangeListener; -import org.mastodon.graph.ReadOnlyGraph; +import org.mastodon.graph.ListenableReadOnlyGraph; import org.mastodon.graph.Vertex; -import org.mastodon.graph.algorithm.RootFinder; import org.mastodon.graph.algorithm.traversal.InverseDepthFirstIterator; import org.mastodon.model.HasLabel; import org.mastodon.views.trackscheme.util.AlphanumCompare; public class TrackGraphColorGenerator< V extends Vertex< E > & HasLabel, E extends Edge< V > > - implements GraphColorGenerator< V, E >, GraphChangeListener + implements GraphColorGenerator< V, E > { private final RefIntMap< V > cache; @@ -25,34 +23,54 @@ public class TrackGraphColorGenerator< V extends Vertex< E > & HasLabel, E exten private final RefList< V > toUpdate; - private final ReadOnlyGraph< V, E > graph; + private final ListenableReadOnlyGraph< V, E > graph; - private boolean pause; + private final RootProvider< V, E > rootsProvider; - public TrackGraphColorGenerator( final ReadOnlyGraph< V, E > graph ) + private boolean rootsUpToDate; + + public TrackGraphColorGenerator( final ListenableReadOnlyGraph< V, E > graph ) { + this.rootsProvider = new RootProvider<>( graph ); this.graph = graph; this.cache = RefMaps.createRefIntMap( graph.vertices(), 0 ); this.lut = GlasbeyLut.create(); this.iterator = new InverseDepthFirstIterator<>( graph ); this.toUpdate = RefCollections.createRefList( graph.vertices() ); - this.pause = false; - graphChanged(); + + rootsProvider.graphRebuilt(); + graph.addGraphListener( rootsProvider ); + + rootsProvider.listeners().add( () -> rootsUpToDate = false ); + rootsUpToDate = false; + rebuildRoots(); } @Override public int color( final V v ) { + // Should we clear the cache? + if ( !rootsUpToDate ) + rebuildRoots(); + + // Do we know of a color already for this vertex? if ( cache.containsKey( v ) ) return cache.get( v ); - /* - * Iterate until we find a spot with a color. The roots have already a - * color by then, so at worst we will have to iterate depth-first, - * upward, until we meet a root. Then we will return its color, but - * before, we will store this color in all the vertices we have - * encountered during iteration. - */ + return updateCache( v ); + } + + /** + * Iterates until we find a spot with a color. The roots have already a + * color by then, so at worst we will have to iterate depth-first, upward, + * until we meet a root. Then we will return its color, but before, we will + * store this color in all the vertices we have encountered during + * iteration. + */ + private synchronized int updateCache( final V v ) + { + if ( cache.containsKey( v ) ) + return cache.get( v ); iterator.reset( v ); toUpdate.clear(); toUpdate.add( v ); @@ -81,30 +99,27 @@ public int color( final E edge, final V source, final V target ) return color( source ); } - @Override - public void graphChanged() + /** + * Only one thread can regenerate the cache. If another one tries at the + * same time, it is paused (synchronized keyword) and will access the new + * value in the cache after the first has regenerated it. + */ + private synchronized void rebuildRoots() { - if ( pause ) + if ( rootsUpToDate ) return; - cache.clear(); - lut.reset(); final RefList< V > sortedRoots = RefCollections.createRefList( graph.vertices() ); - sortedRoots.addAll( RootFinder.getRoots( graph ) ); + sortedRoots.addAll( rootsProvider.get() ); sortedRoots.sort( ( v1, v2 ) -> AlphanumCompare.compare( v1.getLabel(), v2.getLabel() ) ); + cache.clear(); + lut.reset(); sortedRoots.forEach( r -> cache.put( r, lut.next() ) ); + rootsUpToDate = true; } - /** - * If true, the track colors will not be regenerated when the - * graph changes. This is intended to save a possibly costly operation when - * this color generator is not used. - * - * @param pause - * whether to pause regeneration of colors or not. - */ - public void pauseListener( final boolean pause ) + public void close() { - this.pause = pause; + graph.removeGraphListener( rootsProvider ); } }