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 Leak #76

Closed
KatieWoe opened this issue Mar 7, 2022 · 11 comments
Closed

Memory Leak #76

KatieWoe opened this issue Mar 7, 2022 · 11 comments

Comments

@KatieWoe
Copy link

KatieWoe commented Mar 7, 2022

For phetsims/qa#781. Seen on Win 11 Chrome.
The memory footprint for this sim seems small, but there is a noticeable memory leak. This test was don on the PhET brand sim, with memory snapshots happening one minute apart.
memleak

@KatieWoe
Copy link
Author

KatieWoe commented Mar 7, 2022

These are the results on the PhET-iO Brand:
iomem

@samreid samreid assigned samreid and unassigned chrisklus Mar 11, 2022
@amanda-phet
Copy link
Contributor

This seems important to resolve sooner rather than later

@KatieWoe
Copy link
Author

This is in phetsims/qa#789 as well
cvbrandmem

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Mar 28, 2022

The memory leak in phetsims/scenery#1356 has been fixed, but that leak was first introdudced after this issue was first reported. There may still be a memory leak in this sim. The patch to fix the memory leak in AnimatedPanZoomListener is phetsims/scenery@6d4ecc6 for a center-and-variability-1.0 branch of scenery.

@amanda-phet amanda-phet added this to the 1.1 Publication milestone Apr 1, 2022
@samreid samreid removed their assignment Apr 2, 2022
@KatieWoe
Copy link
Author

KatieWoe commented Apr 6, 2022

The memory leak seems worse in phetsims/qa#795.
memleak

samreid added a commit that referenced this issue Apr 6, 2022
@samreid
Copy link
Member

samreid commented Apr 6, 2022

Thanks, I pushed a fix for the memory leak. There may be others but that was certainly one. Maybe we should cherry-pick it before release since running the sim for 30 minutes under normal usage seems like it might leak 100MB (if this rate keeps up).

@samreid samreid modified the milestones: 1.1 Publication, Classroom prototype Apr 6, 2022
samreid added a commit that referenced this issue Apr 12, 2022
samreid added a commit that referenced this issue Apr 12, 2022
@samreid
Copy link
Member

samreid commented Apr 12, 2022

The original fixes had erroneous parts. @chrisklus and I reverted that and applied the correct fix, which is ready for cherry-picking.

@chrisklus
Copy link
Contributor

The change above was cherry-picked for 1.0 publication. This issue may be ready to close, but for now I'm going to move to 1.1 milestone so a more careful delta comparison is completed before closing.

@chrisklus chrisklus modified the milestones: Publish 1.0 as a prototype, Publish 1.1 Apr 13, 2022
@chrisklus chrisklus removed their assignment Apr 13, 2022
@samreid
Copy link
Member

samreid commented Apr 13, 2022

There is another memory leak in

const map = new Map<CAVObject, CAVObjectNode>();
const createDotNode = ( casObject: CAVObject ) => {
const dotNode = dotNodeGroup.createCorrespondingGroupElement( casObject.tandem.name, casObject );
casObject.valueProperty.link( value => {
dotNode.setVisible( value !== null );
if ( value !== null && !this.dotLayer.hasChild( dotNode ) ) {
this.dotLayer.addChild( dotNode );
}
} );
map.set( casObject, dotNode );
};
model.objectGroup.forEach( createDotNode );
model.objectGroup.elementCreatedEmitter.addListener( createDotNode );
model.objectGroup.elementDisposedEmitter.addListener( casObject => {
const viewNode = map.get( casObject )!;
dotNodeGroup.disposeElement( viewNode );
} );

@samreid samreid removed this from the Publish 1.1 milestone Apr 13, 2022
@chrisklus
Copy link
Contributor

@samreid and I added in the missing delete for the dots in the commit above. We tested by printing the map size and it's working as expected, marking for cherry-pick.

After making the next for RC the maintenance release, we think it would be good to have QA do 5-10 min of spot check testing (normal sim use/testing) to sign off that this is good to go.

@chrisklus
Copy link
Contributor

@samreid and I deployed these changes for version 1.0.1, 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

5 participants