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.
mastodon-sc · Oct 23, 2023 · b785d9e · b785d9e
1 parent f39fd09
commit b785d9e
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 47 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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 <code>true</code>, 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 );
 	}
 }