Memory leak in TickLabelSet

[pixelzoom]

Discovered in phetsims/fourier-making-waves#259.

TickLabelSet has a createLabel option, which is exercised in update, which is called when the ChartTransform changes, or when spacing is called viasetSpacing.

Since TickLabelSet is creating the labels, it is also responsible for their disposal -- which it is currently not doing. If those labels happen require disposal -- for example Text/RichText that is linked to LocalizedStringProperty, as in phetsims/fourier-making-waves#259 -- then a memory leak will occur.

[pixelzoom]

TickLabelSet implements a form of caching, where it reuses labels. Labels that are not reused are removed from the cache, but are not currently disposed, resulting in a potential leak. Here's the relevant code:

    // empty cache of unused values
    const toRemove = [];
    for ( const key of this.labelMap.keys() ) {
      if ( !used.has( key ) ) {
        toRemove.push( key );
      }
    }
    toRemove.forEach( t => {
      this.labelMap.delete( t );
    } );

[pixelzoom]

Fixed in 44f6124.

@samreid and I looked at this together, and it seems straightforward and safe. So I'll consider this reviewed, and close the issue.

[pixelzoom]

Reopening. In #65 (comment), @samreid noted:

2. The method invalidateTickLabelSet clears the labelMap without disposing of the values.

... and FWM is indeed using invalidateTickLabelSet, so leaking memory there.

[pixelzoom]

@samreid would you please review? It would be nice if this was done before I create the release branch for phetsims/fourier-making-waves#247, which will probably happen Monday 12/9.

2d55d7b and beff3cf fix memory leaks in invalidateTickLabelSet and dispose respectively, by disposing of cached labels.

7527ae2 improves the readability of the piece of code that gets a cached label or creates a new one. Nested ternary operators made this difficult to read.

95f208b (and this bit that was accidentally in the previous commit) simplifies the implementation of removing unused labels -- fewer loops (1 vs 2), and no need for a Set.

[samreid]

Looks good, and I tested at runtime in the bamboo harness to make sure disposal worked as expected. Anything else before closing?

[pixelzoom]

Thanks for the speedy review. Nothing else to do here. Closing.