Make it possible to reuse FontAwesome shapes without introducing memory leaks

[samreid]

From phetsims/joist#329 we removed caching from FontAwesomeNode because it was causing a memory leak. In phetsims/function-builder#102 we identified that some sims, such as Function Builder spend a lot of time (like 1000ms) during startup parsing the same svg path over and over.

The memory leak was caused because Path assumes the Shape is dynamic, and adds listeners to it like so:

        // Add Shape invalidation listener if applicable
        if ( this._shape ) {
          this._shape.onStatic( 'invalidated', this._invalidShapeListener );
        }

@pixelzoom and I discussed it and would like to investigate adding a new Path option {staticShape} which indicates that no listeners would be attached to the Shape. This would make it possible to reuse the shapes created by FontAwesomeNodes without inadvertently introducing memory leaks.

@pixelzoom also recommends making the shape creation function public so that clients can manage how the shape is created (if a different strategy is necessary).

It would be great to hear from @jonathanolson whether this proposed strategy would work well, or if there are better recommendations.

It would also be nice to factor out FontAwesomeShape, but it is unclear how to do this because the shapes must be transformed and it is unclear whether it is possible to transform a shape in-place. Another alternative would be to preprocess all of the Shapes, transform them and use getSVGPath so they don't have to be re-transformed each time the sim launches.

[jonathanolson]

A staticShape option sounds fine, although it might be more convenient to have an 'immutable' flag on the Shape itself.

If we have the flag on the Shape, it can fail out when attempts are made to mutate it, and could be performed at the end of creating a shape, e.g. new Shape().drawStuff().close().done().

Thoughts @pixelzoom and @samreid?

[samreid]

@jonathanolson it sounds good to me. Let's hear from @pixelzoom.

[pixelzoom]

+1 for marking the Shape as immutable.

[pixelzoom]

Here's what I had in mind:

// FontAwesomeShape.js

// keys are fontawesome icon names, values are {string} fontawesome SVG data
var ICONS = {... };
var SHAPE_MATRIX = Matrix3.createFromPool( ... ); // to create a unity-scale icon

function FontAwesomeShape( iconName ) {
  //TODO How to apply SHAPE_MATRIX? No way to pass a transform to Shape constructor, or transform in place. 
  Shape.call( this, ... );
  this.mutable = false; //TODO or set this via options?
}

return inherit( Shape, FontAwesomeShape );

// FontAwesomeNode.js

// keys are fontawesome icon names, values are {FontAwesomeShape} 
var SHAPE_CACHE = {};

function FontAwesomeNode( iconName ) {

  options = _.extend( {
    fill: 'black',
    pickable: false,
    enableCache: true // {boolean} use Shape caching for this instance?
  }, options );

  var shape;
  if ( options.enableCache ) {

    // cache the shape
    if ( !SHAPE_CACHE[ iconName ] ) {
      SHAPE_CACHE[iconName] = new FontAwesomeShape( iconName );
    }

    // get the shape from the cache
    shape = SHAPE_CACHE[ iconName ];
  }
  else {

    // don't use the cache
    shape = new FontAwesomeShape( iconName );
  }

  Path.call( this, shape, options );
}

return inherit( Path, FontAwesomeNode );

[samreid]

Regarding subclassing Shape for the icons, I augmented the code like so to generate the transformed SVG Strings:

  window.out = {};
  var getShapeByName = function( iconName ) {
    assert && assert( ICONS[ iconName ], 'Icon not found: ' + iconName );

    // At one point, shapes were cached to reduce the overhead of having to reinterpret the SVG each time the shape was
    // loaded, but this led to memory leaks (see https://github.com/phetsims/joist/issues/329).  As a result, the icons
    // are loaded anew each time.
    var shape = new Shape( ICONS[ iconName ] ).transformed( SHAPE_MATRIX );
    window.out[ iconName ] = shape.getSVGPath();
    return shape;
  };

  _.keys( ICONS ).forEach( function( k ) {
    getShapeByName( k );
  } );

However, this takes the file size from 11kb to 83kb because the strings look like this:

var ICONS2 = {
    ban_circle: 'M 32.00000000000000000000 -16.00000000000000000000 Q 32.00000000000000000000 -19.47500000000000142109 
/// etc

[pixelzoom]

Unless applying the transform is a performance issue, I'd prefer not to add additional preprocessing steps to integrating fontawesome SVG data. I think it's advantageous for the data to appear as it does in the fontawesome source.

[pixelzoom]

Before we get too much further into this, we should verify that there's going to be a performance payoff. In phetsims/function-builder#102, @samreid did some benchmarking that identified creation of FontAwesomeNode as a performance issue. Caching assumes that most of the cost is in parsing the SVG and creating the Shape. Is that true? If a significant portion of the cost is in creating the associated Path, then the payoff will be smaller.

[pixelzoom]

9/22/16 dev meeting: @pixelzoom @samreid and @jonathanolson will work on this, no need to involve other developers.

[jonathanolson]

Caching assumes that most of the cost is in parsing the SVG and creating the Shape. Is that true?

Pretty sure that was true the last time. Path generally just asks the Shape for its bounds, svg string, etc., all of which should be faster with one Shape.

@samreid, any preferences on how to make a Shape immutable? I have a slight preference for a method so it can be chained at the end (like is common for constructing Shapes), like .done() or .makeImmutable(). @pixelzoom suggested "this.mutable = false".

[pixelzoom]

Currently in FontAwesomeNode we have:

return new Shape( ICONS[ iconName ] ).transformed( SHAPE_MATRIX );

So add .done() to the end, like this?

return new Shape( ICONS[ iconName ] ).transformed( SHAPE_MATRIX ).done();

[pixelzoom]

@jonathanolson Any thoughts on this TODO in #270 (comment)?

  //TODO How to apply SHAPE_MATRIX? No way to pass a transform to Shape constructor, or transform in place. 

[jonathanolson]

So add .done() to the end, like this?

That would be the plan.

@jonathanolson Any thoughts on this TODO in #270 (comment)?

I don't like the overhead of creating two Shapes (with two copies of each segment in the Shape), but I believe it's currently the best way (given that each segment only has a transformed() immutable-style method).

Presumably if we add mutable transforms to Shape and segments in the future, we'll find this location in searches, so we can remove the TODO when doing the refactoring.

[pixelzoom]

I don't like the overhead of creating two Shapes (with two copies of each segment in the Shape), but I believe it's currently the best way (given that each segment only has a transformed() immutable-style method).

Please take a close look at the FontAwesomeShape example. How can FontAwesomeShape extend Shape if we need to create 2 Shapes?

[jonathanolson]

I saw the example, but I wasn't sure why we were creating FontAwesomeShape. I didn't see any methods added.

A global FontAwesomeNode.getShape( name ) sounds preferable?

[pixelzoom]

Why is that preferable? That would be like ArrowButton.createShape() instead of new ArrowShape(). Clients should be able to create and manage FontAwesomeShapes independently of FontAwesomeNode.

[samreid]

@samreid, any preferences on how to make a Shape immutable? I have a slight preference for a method so it can be chained at the end (like is common for constructing Shapes), like .done() or .makeImmutable(). @pixelzoom suggested "this.mutable = false".

.makeImmutable() seems clearest and most consistent with existing shape API.

[jonathanolson]

Why is that preferable? That would be like ArrowButton.createShape() instead of new ArrowShape(). Clients should be able to create and manage FontAwesomeShapes independently of FontAwesomeNode.

First, I'm not sure why we'd want clients to create the shapes. Every 'check_empty' shape would be identical except for duplicated instance objects (since it will be immutable), so we really only want a shared shape for each type (one shared shape instance for 'check_empty', ideally allocated when it is initially requested).

Additionally, I don't feel like managing FontAwesomeShapes without a require to FontAwesomeNode is needed, but I'd still be happy if we had a FontAwesomeShape object whose sole interface was FontAwesomeShape.getShape( name ) instead of FontAwesomeNode.getShape( name ). In neither case is the client required to ever touch a Node if they want a Shape instead.

So in general, it would be preferable because we won't be creating duplicate shapes (performance win), we won't need inheritance to accomplish it (simplicity), and we are less likely to cause JIT issues because our shapes will have the same hidden class as most other shapes (performance win).

The potential upside to inheritance seems to only come into play if we need to store additional mutable state on the shapes (or immutable state that new methods would access that aren't part of Shape). @pixelzoom, are there additional advantages to the inheritance pattern?

[pixelzoom]

@jonathanolson said:

First, I'm not sure why we'd want clients to create the shapes.

To be able to create and manage a Shape that is based on fontawesome data without having to use the caching feature. With caching, once a Shape is in the cache, it's in the cache for the lifetime of the sim. (Unless you want to complicate matters and add a "flush cache" feature.)

@jonathanolson said

we won't need inheritance to accomplish it (simplicity)

I don't agree at all in this case that forgoing inheritance is simpler. FontAwesomeShape is responsible for the SVG shape descriptions, FontAwesomeNode is responsible for the cache - a natural division of responsibilities. But if you want to spread responsibilities around where they don't belong, go for it.

Btw... Shapes itself is a nice example of where inheritance would have been appropriate (SVGShape?), but instead we have constructor parameters that are overloaded and poorly documented.

[pixelzoom]

I also suspect that the objection to FontAwesomeShape has more to do with #270 (comment) than with the use of inheritance. Each segment is immutable, so the only way to transform a Shape is to create another Shape. And that makes implementing FontAwesomeShape (as I've proposed it) currently impossible.

[pixelzoom]

Adding @samreid to assignees since this is now blocking https://github.com/phetsims/phet-io/issues/642, which he needs to complete by Thursday, 9/29.

How should we proceed?

[jonathanolson]

How should we proceed?

I can give Shape .makeImmutable(), I'll be fine with however you decide FontAwesome*.js should look.

[pixelzoom]

@jonathanolson Correct me if I'm wrong, but it doesn't look like the changes to Path (phetsims/scenery@ff21c54) handle this case correctly:

var shape = new Shape()....;
var path = new Path( shape, ... );
shape.makeImmutable();

In this case, Path should remove its Shape listener.

[jonathanolson]

In this case, Path should remove its Shape listener.

It seems like a fairly rare case (usually you would construct the Shape and then mark it as immutable before it ever gets to Path).

I decided when implementing that this wasn't worth (to me) Path having to add yet another listener to Shape, just to listen for when it was made immutable (addShapeBecameImmutableListener?).

Path's setShape could use a note about this interaction.

Let me know if that sounds acceptable?

[pixelzoom]

My feeling that if it's something that's possible to do, then it should be handled. And seems relatively easy to handle this case:

In Shape, call invalidate in makeImmutable:

    makeImmutable: function() {
      this._immutable = true;
      this.invalidate();
      return this; // for chaining
    },

In Path, remove the listener when a Shape becomes immutable:

   invalidateShape: function() {
      this.invalidatePath();

      var stateLen = this._drawables.length;
      for ( var i = 0; i < stateLen; i++ ) {
        this._drawables[ i ].markDirtyShape(); // subtypes of Path may not have this, but it's called during construction
      }

       // if the shape has become immutable, remove the 'invalidated' listener
       if ( this._shape.isImmutable() ) {
        this._shape.offStatic( 'invalidated', this._invalidShapeListener );
      }
    },

[jonathanolson]

Changes as described pushed.

[pixelzoom]

@samreid Do you want to take another look at this before it goes live in https://github.com/phetsims/phet-io/issues/642?

[samreid]

The change set looks correct, it seems OK to close.

[pixelzoom]

Reopening. This behavior (console) doesn't look correct:

> var shape = new phet.kite.Shape().lineTo( 100, 100 );
> var path = new phet.scenery.Path( shape );
> shape.makeImmutable()
assert.js:22 Assertion failed: Concurrent modifications from static listeners are forbidden

According to the commit message for @phetsims/scenery@e948509, the Path should remove the listener from the shape, and there should be no error here.

[jonathanolson]

Sorry about that, it may be that we're not using that ordering pattern in any sims.

Since those are static listeners, we can't modify the listener list while it is iterating. Making it non-static would cause extra allocations in places we don't want, so presumably we'll want another event for this purpose?

Alternatively, we could have making a Shape immutable an assertion failure if it has any invalidation listeners.

@pixelzoom and @samreid, thoughts?

[samreid]

Since I am aware of the issue I would be fine with the current behavior (as long as you document that makeImmutable must be called before new Path() is called). But it may be opaque to others and the assertion @jonathanolson mentioned may be best, if that's straightforward.

[pixelzoom]

Alternatively, we could have making a Shape immutable an assertion failure if it has any invalidation listeners.

Sounds good.

[jonathanolson]

@samreid, what is the correct way to check an Events object for the existence of a listener for a certain name? Might need a new method for that.

[samreid]

Events is deprecated and we should port to using Emitter instead. Will that work for this case or do we need something for Events?

[jonathanolson]

Added issue for handling Events => Emitter for Kite, as that's a prerequisite.

[pixelzoom]

I believe that all work is done here, or moved to new issues.

Closing.