-
Notifications
You must be signed in to change notification settings - Fork 2
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
CT: above our memoryLimit #207
Comments
Prior to publishing, this sim had extensive memory leak testing on 3/6/17, see #170, and the heap size never grew past 55MB. I looked through all commits since 3/6/17. The only significant change in master since then was to convert from TWEEN to Animation. So I guess I'll start there. |
According to @jonathanolson, CT fuzz test for requirejs is 120 seconds with query parameters: So if I run for > 2 minutes with |
With Chrome 70.0.3538.77 on macOS 10.11.6, running in requirejs mode with |
I don't see anything in the 1411 MB snapshot that looks related to Animation. But I do see a huge number of I took another snapshot at startup, 76MB. (That's bigger than the 55MB size after 25 minutes of runtime in the original memory tests, see #170). A second snapshot 10 seconds later was 123MB. Comparing snapshots, I again see a lot of Emitter instances. @jonathanolson is it possible that phetsims/scenery#879 is not resolved? |
This issue most definitely blocks publication of this (and possibly other) sims. |
Hmm, this looks related to phetsims/sun#362. At least one case in unit-rates is:
I'd like to tag for the developer meeting, because any of the ways of solving this generally have consequences:
Long-term, doing both things sounds correct, but I'm curious what others think. |
I've had two CT browser tabs crash as part of phetsims/aqua#54 while testing this. I'm bumping priority to high so we ensure it is talked about in dev meeting. |
I'll create an issue for immutable colors. |
11/1/18 dev meeting notes:
|
This isn't isolated to sim-specific code. I'll need to add dispose to |
Signed-off-by: Chris Malley <[email protected]>
In the above commit, I added The good news is that I can now take a heap snapshot without waiting 20 minutes, and CT shouldn't be complaining. Startup heap size was 86MB. After 1 minute, heaps size was 126MB. After 5 minutes, heap size was 98MB, which seems to indicate some GC. After 10 minutes, heap size was back up to 133MB. The bad news is that these heap sizes are still much larger than expected. In prior memory leak testing (#170), startup heap size was 45MB, and the sim stabilized at ~52MB after 25 minutes. We are nowhere near those numbers. Also worth noting is that the sim becomes sluggish/jerky almost immediately during fuzz testing, behavior that's indicative of a leak. @jonathanolson any ideas? |
Note to self: Each line of code that I added as a workaround for this issue has a TODO next to it. |
Adding dev meeting label to discuss timeframe for addressing this. Since we have a number of sims being published with new release branches (graphing-quadratics, graphing-lines, resistance-in-a-wire,...) it would be good to get to the bottom of this. |
I made the change for color listeners, so I'll retest for memory leaks right now. |
2 minutes of fuzzing and the sim is still <100MB, so the "major" memory leak was resolved by changes made. |
The preceding commit reduces requirejs phet-brand Unit Rates memory from 52.4MB to 43.6MB. |
After above commits, we are down to 43.0MB. |
Down to 42.6 MB |
@samreid looks like the source of the memory increase was identified. Can you summarize here in a comment, for posterity? |
The changes I made were predominantly about factoring out instantiated parametric types. For instance, phetsims/axon@4a9762e factors out |
For clarity, @jonathanolson and I (and mainly @jonathanolson) worked out that the source of the memory leak came from the commit when I moved Re his comment: |
In the above commits, I found that unit rates phet brand went down another .3 MB for I then looked at all usages of |
Somehow we went from @jonathanolson saying:
To this email from @zepumph:
I asked:
Then @samreid described specific commits, which was not what I'm looking for So to clarify.... What was the general problem, and what is the general solution that you're pursuing? Also highly recommended to create a general issue somewhere to be dealing with this, not deal with it in this sim-specific issue. |
And yes, I've read #207 (comment) and #207 (comment), but I don't understand. |
@samreid and I discussed on Slack. The cause of this issue is now moved to phetsims/tandem#71. Please comment and commit there. |
I tested and got startup heap size of 45MB on Chrome + macOS, so that correlates with what @samreid is seeing. Closing this issue. Further memory improvements related to IO types will continue in phetsims/tandem#71. |
(cherry picked from commit 4a9762e)
(cherry picked from commit eef4587)
(cherry picked from commit 8b02d30)
(cherry picked from commit 77b9f41)
(cherry picked from commit 3d75e4f)
(cherry picked from commit 6988a3a)
The text was updated successfully, but these errors were encountered: