Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory test #119

Closed
Luisav1 opened this issue Aug 10, 2023 · 9 comments
Closed

Memory test #119

Luisav1 opened this issue Aug 10, 2023 · 9 comments
Assignees

Comments

@Luisav1
Copy link
Contributor

Luisav1 commented Aug 10, 2023

From self code review #112.

@Luisav1 Luisav1 self-assigned this Aug 10, 2023
@Luisav1 Luisav1 mentioned this issue Aug 10, 2023
70 tasks
@Luisav1
Copy link
Contributor Author

Luisav1 commented Aug 22, 2023

On-hold because of CT issue #129.

@zepumph zepumph mentioned this issue Aug 23, 2023
71 tasks
@zepumph
Copy link
Member

zepumph commented Aug 23, 2023

#129 is closed. let's do it!

@jbphet
Copy link
Contributor

jbphet commented Aug 23, 2023

As part of the code review in #165 I ran a memory test. There is a screenshot of the results below. The bottom line is that there may be a slow leak, but nothing egregious, and what we do see could be explainable by pooling in Scenery.

Everything was done in a Chrome incognito window using a freshly built version of build-a-nucleus_all_phet_debug.html.

Explanation:

  • Snapshot 1 - loaded, no interaction
  • Snapshot 2 - after a minute of basic interaction, then a reset on both screens and a return to the home screen
  • Snapshot 3 - after a reload with the fuzz parameter added and one minute of fuzzing
  • Snapshots 4 thru 12 - snapshots at one minute intervals while fuzzing
  • Snapshot 13 - Stopped fuzzing using phet.chipper.queryParameter.fuzz = false, reset both screens, went to the home screen

image

@zepumph
Copy link
Member

zepumph commented Aug 28, 2023

When I create 10 of each nucleon and then reset, the comparison memory tool doesn't show any obvious memory leaks. Yay!

@zepumph
Copy link
Member

zepumph commented Aug 31, 2023

We found one problem with how we were recreating a Text, PatternStringProperty, AND a DerivedProperty each time the mass number changed. Oops. Fixed here.

@zepumph
Copy link
Member

zepumph commented Aug 31, 2023

After fuzzing for 45 minutes and capturing snapshots intermittently, I don't see problems anymore. Woo! Closing. Thanks @Luisav1 for all your help.

image

@zepumph zepumph closed this as completed Aug 31, 2023
@zepumph
Copy link
Member

zepumph commented Aug 31, 2023

Arg! Found another nested listener adding:

nucleonCountProperty.link( () => {
startRemovingNucleonNumberDisplay.start();
// At the end of both animations, reset the values and opacities of oldNucleonNumberDisplay and newNucleonNumberDisplay.
addNucleonNumberDisplay.finishEmitter.addListener( () => {
oldNucleonNumberProperty.value = nucleonCountProperty.value;
oldNucleonNumberDisplay.opacity = 1;
newNucleonNumberDisplay.opacity = 0;
} );
} );

@zepumph zepumph reopened this Aug 31, 2023
zepumph added a commit that referenced this issue Aug 31, 2023
@zepumph
Copy link
Member

zepumph commented Aug 31, 2023

?listenerLimit=1000 has been helpful for this nesting issue

@zepumph
Copy link
Member

zepumph commented Aug 31, 2023

After fuzzing for a while I saw that the maxListenerCount was 783, then when I restarted the sim with ?listenerLimit=782, it failed on startup. No further work to be done here.

@zepumph zepumph closed this as completed Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants