-
Notifications
You must be signed in to change notification settings - Fork 7
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 for brand=phet (1.1 branch) #189
Comments
Test procedure:
|
Test results with 1.1.0-dev.13 brand=phet, built using Test configuration: 2019 MacBook Pro (MacBookPro16,1), macOS 10.15.6, Chrome 84.0.4147.135
It certainly looks like there's a memory leak here. I will investigate. |
Unfortunately labeling this as "blocks-sim-publication" for the prototype. This leak is bad enough that it's very likely to be encountered by the user -- especially on iPad -- and will crash the browser. |
The commonality that I see between all heap snapshot comparisons is that Tandem and DynamicTandem show large #New and #Deleted=0. For example: ProportionsCounts here is expected to be #Deleted=0, because counts are retained for all generations until we Reset All or Start Over (or reach the 1000-generation limit). |
@samreid identified a memory leak in Tandem, see phetsims/tandem#203. He created a patch (see the issue) which has not been committed yet. Snapshots for the first 400 generations BEFORE the patch: Snapshots for the first 400 generations AFTER the patch: And Tandem and DynamicTandem instances are no longer appearing as #Deleted=0: I'm going to continue testing with the patch in my local copy, to see if the sim continues to leak, or if it stabilizes. |
Test results with local unbuilt, natural-selection sha e32a316, with the patch from phetsims/tandem#203 applied. Test configuration: 2019 MacBook Pro (MacBookPro16,1), macOS 10.15.6, Chrome 84.0.4147.135
Compared to #189 (comment), this is huge improvement. There may be additional leak(s), I'll need to investigate further. I did let the sim continue to run and took several more snapshots before and after Reset All, and heap size seems to stabilize in the range of 84-100MB. |
Inspecting the above arrays further (screenshot below), all of the strings in these arrays are phetioIDs for DerivedProperty instances. Some exist for the lifetime of the sim, but others are dynamic. I suspect that iO is not cleaning up phetioIDs for DerivedProperty. In the snapshot following Reset All, these arrays account for 31% of the (retained) heap size. |
@samreid and I worked on this via Zoom and identified another big iO leak, see phetsims/axon#328. Test results with local unbuilt, brand=phet, natural-selection sha e32a316, with the patches from phetsims/tandem#203 and phetsims/axon#328 applied. Test configuration: 2019 MacBook Pro (MacBookPro16,1), macOS 10.15.6, Chrome 84.0.4147.135
Two things make me happy about these results: (1) We're testing with a stable population, and the memory footprint is immediately stable. So... Work is done here. All leaks were in PhET-iO. On hold until phetsims/tandem#203 and phetsims/axon#328 are addressed. Then I will sanity check with one more test. |
After fixes for phetsims/tandem#203 and phetsims/axon#328 were pushed... Testing results on 2019 MacBook Pro, macOS 10.15.6, Chrome 85.0.4183.83 (Incognito), with https://phet-dev.colorado.edu/html/natural-selection/1.1.0-dev.17/phet/natural-selection_en_phet.html?secondsPerGeneration=0.25
This looks great. Stable memory with stable population, consistent memory for Generation 0. Closing. |
This is one of the "standard GitHub issues" required by the code-review checklist.
The text was updated successfully, but these errors were encountered: