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

does this sim do any memory management? #51

Open
3 tasks
pixelzoom opened this issue Feb 10, 2020 · 2 comments
Open
3 tasks

does this sim do any memory management? #51

pixelzoom opened this issue Feb 10, 2020 · 2 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 10, 2020

Related to #2 (code review), there are 3 items related to memory management:

  • For each common-code component (sun, scenery-phet, vegas, …) that opaquely registers observers or listeners, is there a call to that component’s dispose function, or is it obvious why it isn't necessary, or is there documentation about why dispose isn't called? An example of why no call to dispose is needed is if the component is used in a ScreenView that would never be removed from the scene graph.
  • Are there leaks due to registering observers or listeners? The following guidelines should be followed unless there it is obviously no need to unlink, or documentation (in-line or in the implementation nodes)added about why following them is not necessary. Unlink is not needed for properties contained in classes that are never disposed of,
    such as primary model and view classes that exist for the duration of the sim.
    - [ ] AXON: Property.link is accompanied by Property.unlink.
    - [ ] AXON: Creation of DerivedProperty is accompanied by dispose.
    - [ ] AXON: Creation of Multilink is accompanied by dispose.
    - [ ] AXON: Events.on is accompanied by Events.off.
    - [ ] AXON: Emitter.addListener is accompanied by Emitter.removeListener.
    - [ ] SCENERY: Node.on is accompanied by Node.off
    - [ ] TANDEM: PhET-iO instrumented PhetioObject instances should be disposed.
  • Do all types that require a dispose function have one? This should expose a public dispose function that calls this.disposeMyType(), where disposeMyType is a private function declared in the constructor. MyType should exactly match the filename.

There are zero definitions of dispose in sim-specific code, and zero calls to dispose of common-code. That may be OK, or it may point to memory leaks.

Things to do:

@tmildemberger
Copy link
Contributor

The above commits add a comment for each Property.link, DerivedProperty or Multilink explaining why they don't need to be disposed/unlinked.

In our case, most of the objects are created at the start of the simulation and are not meant to be disposed. There is only one place where a new VStrut needs to be created sometimes, and the old one was not disposed until bd6e9a6.

@pixelzoom
Copy link
Contributor Author

... most of the objects are created at the start of the simulation and are not meant to be disposed. There is only one place where a new VStrut needs to be created sometimes ...

When you get to #8... This is the type of information that we typically put in implementation-notes.md, under a "Memory Management" heading.

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

2 participants