phetsims / bamboo

Charting library built with Scenery
http://scenerystack.org/
MIT License
3 stars 0 forks source link

Memory leak in TickLabelSet #64

Closed pixelzoom closed 6 hours ago

pixelzoom commented 6 hours ago

Discovered in https://github.com/phetsims/fourier-making-waves/issues/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 https://github.com/phetsims/fourier-making-waves/issues/259 -- then a memory leak will occur.

pixelzoom commented 6 hours ago

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 commented 6 hours ago

Fixed in https://github.com/phetsims/bamboo/commit/44f6124187fdc3e0ab9f71a4730ef557776032b8.

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