Feature/clear cache on dispose

[flekschas]

This PR addresses #34 by exposing functions to clear the command, coordinate-scope, and composite cache.

While technically the library user could call these methods, it seems like it'd put quite a huge burden on them to learn when and how to clear the cache most efficiently. Internally, we kind of know then whenever a primitive or composite Renderable is disposed, it can't be rendered anymore and so we could safely removed it's cached draw commands.

Therefore, I've changed all Primitive, Composite, and CoordinateSystem instances to automatically remove themselves from the cache upon disposal. This means, upon properly disposing resources, the memory leak is fixed automatically. A downside is that this is a breaking API change because in order to auto-clear the cache, the resources need to have a reference to the candygraph instance. Another downside of this coupling might be that one cannot reuse resources across candygraph instances but I am not sure this was even possible before because we already coupled the resources via the regl instance used by the candygraph instance.

Let me know what you think of this. Happy to adjust it of course :)

[wwwtyro]

Thanks @flekschas! I'll read through this over the week and try to finish it up on the weekend. :)

[wwwtyro]

Still working through this, but wanted to give you a heads up that this seems to have broken the Logarithmic Y-Axis plot in the examples:

Also the last plot in the tutorial (another log plot).

[wwwtyro]

This looks good to me, thanks @flekschas! I think we just need to fix whatever broke with the plots and then land it.

[flekschas]

Thanks for the review! I'll take a look today or tomorrow to find out what broke the plots.

[flekschas]

@wwwtyro I fixed the log scale issue. I tried to be too smart by caching the coords-based commands using coord.kind. However, I didn't think of the fact that these two coordinate systems can come in 2 * 2^2 = 8 different flavors. Using coord.glsl as the coord-specific cache key fixes the issue:

[wwwtyro]

Awesome, thanks, I'll go over it again soon.

[wwwtyro]

Thanks for the fix, @flekschas! Seems like we're close, but it looks like there's an issue with kicking out DrawCommands prematurely (see comment in candygraph.ts).

[wwwtyro]

Nice catch

[wwwtyro]

In order to check that we're not needlessly recreating DrawCommands (which is expensive, on the order of 3 to 5 ms), I added the following logging to getCommand:

    let commands = this.commandCache.get(primitive.constructor);
    if (!commands) {
      commands = {};
      this.commandCache.set(primitive.constructor, commands);
    }

    let command = commands[coords.glsl];
    if (!command) {
      console.log("Rebuilding DrawCommand..."); // <========== Added logging
      command = primitive.command(coords.glsl + commonGLSL);
      commands[coords.glsl] = command;
    }

    return command;
  }

Testing the "Animated, relative time" example yields a lot of instances of this being logged per second. I think each call to trace.dispose() is triggering the DrawCommand to be kicked out of the cache and subsequently rebuilt. I'm not sure what the right fix is here, but I'll take a look too.

[flekschas]

Oh boy! Thanks a lot for finding this. I think I was too eager when it came to clearing the cache. I just realized that it might not be necessary to clear the command cache all the time as they are at most twelve (?) cached DrawCommand programs per coordinate system (one for each primitive). I also noticed that there it's totally fine to dispose a primitive but continuing to use the related draw command. Sorry I didn't realize all this earlier 🤦‍♂️

However, I do still believe that draw commands become obsolete when their coordinate system is disposed. I.e., after having disposed a coordinate system, one would not re-use associated draw commands ever again. Is that correct?

If this is correct, what do you think of the following approach:

1. Avoid clearing the command cache on disposal of primitives

2. Revert the command cache back to being primarily coordinate system based:

   private getCommand(coords: CoordinateSystem, primitive: Primitive) {
     let commands = this.commandCache.get(coords.glsl);
     if (!commands) {
       commands = new Map<Function, DrawCommand>();
       this.commandCache.set(coords.glsl, commands);
     }
   
     let command = commands.get(primitive.constructor);
     if (!command) {
       console.log("Rebuilding DrawCommand...");
       command = primitive.command(coords.glsl + commonGLSL);
       commands.set(primitive.constructor, command);
     }
   
     return command;
   }

3. Change the cache clearing functions to:

   public clearCoordinateCache = (coords?: CoordinateSystem): void => {
     if (!coords) {
       this.coordinateScopeCache.clear();
       this.clearCommandCache();
     } else {
       this.coordinateScopeCache.delete(coords);
       this.clearCommandCache(coords);
     }
   };
   
   public clearCompositeCache = (composite?: Composite): void => {
     if (!composite) {
       this.compositeScopeCache.clear();
     } else {
       this.compositeScopeCache.delete(composite.constructor);
     }
   };
   
   public clearCommandCache = (primitiveOrCoord?: Primitive | CoordinateSystem): void => {
     if (!primitiveOrCoord) {
       this.commandCache.clear();
     } else if (primitiveOrCoord instanceof Primitive) {
       for (const commands of this.commandCache.values()) {
         commands.delete(primitiveOrCoord.constructor);
       }
     } else {
       this.commandCache.delete(primitiveOrCoord.glsl);
     }
   };

With this approach the "Animated, relative time" does not excessively re-create draw commands and upon disposal of coordinate systems one still should avoid any memory leaks.

[wwwtyro]

after having disposed a coordinate system, one would not re-use associated draw commands ever again. Is that correct?

I think that this may be technically incorrect but practically correct. For example, someone could:

1. Create two coordinate systems, coordsA and coordsB, with identical glsl values (all linear/linear scale coordinate systems would have identical glsl, for example)
2. Render a primitive with coordsA, which would generate a draw command
3. Dispose coordsA, which would destroy the draw command
4. Render the primitive (or another glsl-equivalent one) with coordsB, requiring the same draw command to be recreated

It's difficult to imagine a case where this might happen, though, so it may be that you're practically correct. I'm not sure.

It does seem like destroying a coordinate system causing later rendering of a primitive to be slower violates the principle of least astonishment, though, so that gives me pause.

Let's back up. Right now:

1. There's no way to clear the command, coordinate scope, or composite scope caches. This is bad design, and proven untenable in the coordinate scope case per Potential memory leak #34.
2. We should be able to remove coordinate scope entries as coordinates systems are disposed, because that cache key is coordinate instances. This should be an easy fix.
3. We're using the composite constructor for the composite scope cache key, making removal of entries there more difficult. I don't recall why I used the composite constructor as the key, though, and I suspect it may be unnecessary. If so, that should also be an easy fix.
4. Removing entries for the draw command cache is the real mind-bender because it's nested and not based on any instances. I'm concerned that the difficulty here is indicative of a flaw in the underlying architecture.

What do you say we go ahead and fix #2, hopefully (mostly) solving your memory leak problems. I'll take a look at #3 and try to fix it tonight or tomorrow (if you don't beat me to it), and I'll probably spend a week digging into #4 to try to identify a good solution there (for which I'm very open to brainstorming!).

If, in the meantime, you find your memory leak is still not satisfactorily resolved, we can try either having you destroy/recreate the CG object when you switch views, or we can just shove a simple clearCache() function into candygraph.ts that obliterates everything as a temporary version zero band-aid.

Does that seem reasonable to you? Sorry for the trouble around this leak, but I really appreciate you working through it with me!