Investigate possible memory leak

[jessegreenberg]

In PlinkoProbabilityIntroModel, PlinkoProbabilityIntroModel, and PlinkoProbabilityLabView, anonymous functions are added to balls as they are created. For example

//'exited' is triggered when the addedBall leaves the last peg on the Galton board.
addedBall.on( 'exited', function() {
  thisModel.histogram.addBallToHistogram( addedBall );
} );

These listeners are never removed with the balls so is this a potential memory leak?

[jessegreenberg]

I did some heap profiling, and found the following with Windows 10 Chrome, fuzzMouse=100:

Time (min) - HeapSize( MB ) 0 - 16.8 5 - 18.3 10 - 18.4 15 - 18.4 20 - 18.5 25 - 18.6 30 - 18.6 35 - 18.6 40 - 18.7 45 - 18.6

So there doesn't seem to be much of a leak.

[veillette]

This is what addBallToHistogram does,

    /**
     * Add an additional ball to the histogram to the appropriate bin and update all the relevant statistics
     * @param {IntroBall||LabBall} ball
     * @public
     */
    addBallToHistogram: function( ball ) {
      // @private
      this.bins[ ball.binIndex ].visibleBinCount++;
      this.updateStatistics( ball.binIndex );
      this.trigger0( 'histogramUpdated' );
      this.trigger0( 'statisticsUpdated' );
    },

because it only need a reference to the ball.binIndex, it should not create a memory leak.

Correct me if i'm wrong

[jessegreenberg]

I let the sim run overnight, and there is no indication of a memory leak. I expected there to be a memory leak since observers are not explicitly removed when balls are removed from the observable array. Since balls are allocated and destroyed I thought the observer would need to be disposed with 'off' to prevent a leak.

Perhaps I don't understand something though, why would https://github.com/phetsims/plinko-probability/issues/40#issuecomment-230053527 mean that there should not be a leak?

[jessegreenberg]

I also did a test where I hacked the step function to continuously create balls and let them reach the bottom histogram, which I let run for about 5 hours. Still no indication of a memory leak in this case. @veillette I am satisfied that there is no leak, feel free to close at any time.

[veillette]

Thanks for doing the test. FYI, I have a very shaky understanding of memory leaks and my comment was trying to ascertain why you did not observe a memory leak. I was hoping you could inform me about By the way, there are only four sims (area-builder, build-a-molecule, molecules-shapes, and curve-fitting) that have a .off but there are numerous simulations that have .on. I'm still unclear on how we can get away with it.

[veillette]

For testing purposes, I will look into a way of generating a memory leak in the first place , so I know what are the minimum ingredients to generate a memory leak and so and then how .off can fix it.

I'll leave this open and report back here if I find something.

[jessegreenberg]

@veillette I think in this case when the ball needs to be removed from the model, there are no other references to the ball or its observers so the garbage collector is able to remove it from memory. If the ball were referenced by another listener which was linked to an event that persisted beyond the lifetime of the ball, then a memory leak would occur.

For instance, I just created a memory leak by adding a test Emitter in the intro model and doing the following:

        //'exited' is triggered when the addedBall leaves the last peg on the Galton board.
        addedBall.on( 'exited', function() {
          thisModel.testEmitter.emit();
          thisModel.histogram.addBallToHistogram( addedBall );
        } );

        // this would create a memory leak because the ball is referenced by a listener attached to an emitter in the
        // model which is static and never changes.
        this.testEmitter.addListener( function() {
          addedBall.testProperty = 'this is a test property';
        } );

With this code, I ran the test where balls are created and removed frequently. I saw the following behavior with the heap, which indicates a memory leak:

[veillette]

Thanks, that makes a lot of sense.

Even though you didn't observe a memory leak, I added remove listeners to the listeners that were added to balls. I'll perform a memory leak check as well since #49 was a pretty extensive (and report here)

[Denz1994]

We were able to test for memory leaks by creating snapshots at different intervals:

• Snapshot 1 = 0 Balls
• Snapshot 2 = 1000 Balls
• Snapshot 3 = 2000 Balls
• Snapshot 4 = 7000 Balls
• Snapshot 5 = 10000 Balls
• Snapshot 6 = 12000 Balls
• Snapshot 7 = 20000 Balls
• Snapshot 8 = 30000 Balls

We are not generating any memory leaks, as our sim allocates a steady 15.4mb and maxes out at 15.5mb.

Settings applied:

• Lab Tab
• 26 Rows
• Continuous Balls
• Snapshot 1-4 = "Ball" view setting
• Snapshot 4-8 = "None" view setting
• All other settings = default

The intro tab didn't show any signs of memory leaks as well. Assigning to @jessegreenberg for review.

[Denz1994]

Additionally, I tried to catch leaks via various methods:

• Dropping balls and using reset all button to remove balls
• Dropping balls and moving row sliders mid drop
• Running sim and switching browser tabs
• Running sim and switching between intro and lab tabs

I was not able to produce any memoryleaks. The only concern was snapshot 6 (switching row sliders mid ball drop) which was a .4mb difference. In this case, I went from having 26 rows, all the balls on screen, and the histogram actively updating to switching to 5 rows with, no balls on screen, and an empty histogram. This behavior is expect however and I was not able to reproduce a similar memory gap in any other case.

[jessegreenberg]

Thanks all, sorry for the false alarm. Screenshots from @Denz1994 show that there is no leak. Thanks for removing the observers in https://github.com/phetsims/plinko-probability/issues/40#issuecomment-231352741 @veillette, that does seem safest overall.

Closing.