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

test for memory leaks (brand=phet-io) #50

Closed
pixelzoom opened this issue Oct 9, 2018 · 6 comments
Closed

test for memory leaks (brand=phet-io) #50

pixelzoom opened this issue Oct 9, 2018 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 9, 2018

For instrumented sims, memory leak testing must be done separately for the phet-io brand, because according to the How to Instrument document:

Perform a full test for memory leaks. The benchmark dev release can be helpful here. This will help catch faulty tandem disposal. PhET-iO instantiates different objects and wires up listeners that are not present in the PhET-branded simulation. It needs to be tested separately for memory leaks.

@pixelzoom pixelzoom self-assigned this Oct 10, 2018
@pixelzoom
Copy link
Contributor Author

Slack dev-public:

Sam Reid [9:56 AM]
Memory leak testing Graphing Quadratics PhET-iO over the last 5 minutes doesn’t suggest any serious memory leaks (it’s just the normal slow rising pattern). I also don’t see any tandems being added after the sim starts up, so that’s a good indicator that there are no phet-io related memory leaks.

Chris Malley [9:59 AM]
Thanks, that matches what I see. I wasn’t aware of this “normal slow rising pattern”. When I tested Equality Explorer (5 screens), it started at 71MB and then oscillated around 82MB.

@pixelzoom
Copy link
Contributor Author

#14 (review of PhET-iO instrumentation) is a prerequisite to doing memory leak testing.

@pixelzoom pixelzoom removed their assignment Oct 25, 2018
@samreid
Copy link
Member

samreid commented Oct 31, 2018

The main part of the work in #14 is complete, removing "block" status. @chrisklus are running a 10-minute memory test for this. Initial heap size (after about a minute) is 73.6MB. We'll report back in 10 minutes with the second snapshot. Keep in mind that phetsims/unit-rates#207 may be related to this memory issue.

UPDATE: After 10 minutes, the memory is at 77.7MB:

image

@chrisklus and I will briefly review the deltas, but in my opinion, this is either (a) an acceptably low memory leak for publication or (b) not a memory leak at all, just the natural memory expansion during initial use.

@pixelzoom
Copy link
Contributor Author

Why are you doing a memory leak test before all PhET-iO related issues have been addressed? And did someone ask you to self-assign?

@samreid
Copy link
Member

samreid commented Oct 31, 2018

Everything seems OK for now, we can run another test after checking off all other PhET-iO issues. Putting on hold until all other phet-io issues have been addressed.

UPDATE: I provided a more thorough answer on slack:

We are checking through status of Graphing Quadratics and PhET-iO blocking issues, and wanted to do an intermediate test of memory. We’ll be continuing on Graphing Quadratics/PhET-iO until at least noon.

This was referenced Nov 5, 2018
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 15, 2018

For comparison, the latest brand=phet memory tests are in #106 (~41MB startup, ~52MB stable).

Test results for brand=phet-io:

  • 1.0.0-dev.46 with ?standalone&fuzz
  • Chrome 70.0.3538.102
  • macOS 10.11.6 on MacBook Pro
Minutes Heap Size (MB)
0 43.9
5 52.2
10 53.6
15 54.6
20 54.7
25 55.4
30 55.5
45 55.6

This looks fine. PhET-iO appears to add < 4MB of overhead to brand=phet, and that seems reasonable. Closing.

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