Memory Leak

[KatieWoe]

For https://github.com/phetsims/qa/issues/1094 Seen on PhET Brand with Win 11 + Chrome

[jessegreenberg]

Interesting, I did a memory test yesterday with PhET brand and saw it taper, see https://github.com/phetsims/mean-share-and-balance/issues/270#issuecomment-2158924153.

[marlitas]

Yeah I also did a memory test last week and saw it taper as well... https://github.com/phetsims/mean-share-and-balance/issues/261

Haven't changed anything since then except exchanging an artwork file which would not cause a leak. I'll see if I can replicate.

[marlitas]

Did a memory test locally and this doesn't really indicate a leak to me. There were several snapshots where the memory load stayed the same. I'm going to do a test on each individual screen just to confirm.

[jessegreenberg]

I just took another look, and we may have a slow leak related to Text or LocalizedStringProperty

The retained instances of TinyProperty, TinyEmitter, and BoundsProperty look like they go through a LocalizedStringProperty.

[jessegreenberg]

All of these Text instances point to a PatternStringProperty in the MeanCalculationPanel. That might be where this is coming from.

[jessegreenberg]

Ah, @marlitas I suspect it is the Text instances that are created inside of this multilink that need to be disposed https://github.com/phetsims/mean-share-and-balance/blob/950bb928ec3325dc9c0c57a77a0c2cca25dfba52/js/common/view/MeanCalculationPanel.ts#L193-L194, especially the ones that take a PatternStringProperty.

[marlitas]

I did a memory test on a single screen that has the Mean Calculation Panel and confirmed a leak there:

[marlitas]

I made some changes and ran the test again on a single screen. Looking good:

[marlitas]

I did a full test on the sim and this does not indicate a memory leak to me:

[marlitas]

I believe this is now addressed. @jessegreenberg can you review the changes?

I will say, I'm not sure if I'm the biggest fan of some of the workarounds that were needed to listen to the proper Properties for the patternStringProperties now that they are pulled out of the multilink, so I am curious if anything sticks out as supremely odd or less than ideal. And if you have any suggestions on how to improve I would greatly appreciate them. Thanks!

[jessegreenberg]

Yea, what you did here is what I expected.

https://github.com/phetsims/mean-share-and-balance/blob/a37634944f315a40b57461999be91fba4f248fbf/js/common/view/MeanCalculationPanel.ts#L230-L242

You could alternatively create an array of disposables - at the top of the Multilink, dispose all entries and remove from array. Then as you create new Text, add them to the array for next time. This lets you create fewer local variables, but its the same idea.

I'm not sure if I'm the biggest fan of some of the workarounds that were needed to listen to the proper Properties for the patternStringProperties now that they are pulled out of the multilink

What other workarounds were needed? I did see other changes in this commit but it looked like they were for other issues?

[marlitas]

I wasn't a big fan of this assertion after the optionize. There's only one model that has that Property though, so I couldn't think of a cleaner way to do it. However, if it didn't stand out, maybe it works?

  const options = optionize<MeanCalculationPanelOptions, SelfOptions, PanelOptions>()( {
      visibleProperty: visibleProperty,
      resize: false,
      calculatedMeanDisplayMode: 'decimal',
      zeroDataMessageProperty: null,
      isDisposable: false,
      meanWithRemainderProperty: null
    }, providedOptions );

    if ( options.calculatedMeanDisplayMode === 'remainder' ) {
      assert && assert( options.meanWithRemainderProperty !== null,
        'If calculatedMeanDisplayMode is "remainder" then we must provide a meanWithRemainderProperty.' );
    }

[jessegreenberg]

I see - No, that seems reasonable to me. There might be a clever way to do that with TypeScript but I don't know of it. Also brainstorming, you could create a subclass that requires the Property as an argument but sets calculateMeanDisplayMode: 'remainder' internally so that combo must go together. I don't think that is necessary but it is another option.

EDIT: There is also assertHasProperties if you prefer for readability

Was that needed for the memory leak or for #289?

[marlitas]

Also brainstorming, you could create a subclass that requires the Property as an argument but sets calculateMeanDisplayMode: 'remainder' internally so that combo must go together. I don't think that is necessary but it is another option.

I thought of that too, but wasn't sure if the assertion was glaring enough to do that. By the sounds of it, it doesn't seem overly concerning so I think it's fine to leave as is. Thanks for looking into it!

Was that needed for the memory leak or for https://github.com/phetsims/mean-share-and-balance/issues/289?

For the memory leak. So I could pull the patternStringProperties out of the multilink.

Mmmm yes let me look into assertHasProperties. Thanks!

[jessegreenberg]

Can you explain why they had to be removed from the multilink? My understanding was that the leak happens because when you create Text with this:

decimalRepresentationText = new Text( decimalTextPatternStringProperty, calculationsTextOptions );

The Text instance gets stuck in memory forever because it adds a listener to the decimalTextPatternStringProperty - and the decimalTextPatternStringProperty lives forever. So as long as the Text is disposed so it can unlink itself from the Property, the leak is patched.

[marlitas]

I looked into assertHasProperties and determined the assertion in the code now is best. I would need to pass in the full model through the SharingScreenView and that tight coupling doesn't seem worth it.

[marlitas]

The Text instance gets stuck in memory forever because it adds a listener to the decimalTextPatternStringProperty - and the decimalTextPatternStringProperty lives forever. So as long as the Text is disposed so it can unlink itself from the Property, the leak is patched.

I initially just disposed the Text instances, but the patternStringProperties were lingering. I believe because they still were tied to a LocalizedStringProperty. I went ahead and moved the creation of the patternStringProperties out of the multilink and that solved it.

Personally, I think this is ready to close, but @jessegreenberg feel free to reopen if you have more questions or concerns.