phetsims / normal-modes

"Normal Modes" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 2 forks source link

does this sim do any memory management? #51

Open pixelzoom opened 4 years ago

pixelzoom commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

... 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.