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

factor out constants for option default values #968

Closed
pixelzoom opened this issue Nov 9, 2018 · 5 comments
Closed

factor out constants for option default values #968

pixelzoom opened this issue Nov 9, 2018 · 5 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

Look for opportunities to factor out constants for option default values, especially in common code, but also in sim-specific code. This is not necessary for values of type {number}, {boolean}, or {string}. (JavaScript interns strings.)

phetsims/tandem#71 (the memory increase due to IO types) is an example of what can happen when default option values are created for each instance of an object. In that case, memory footprint ballooned by 50-100%.

So where possible, default values should be factored out into constants.

Example:

class Particle {

  constructor( options ) {
     options = _.extend( {
       location: new Vector2( 0, 0 ) // initial location
     } );
  }

  this.locationProperty = new Property( options.location );
} 

In the above, every instance of Particle will allocate a Vector2 for the default value of option location. If there are a large number of Particles, this could impact memory footprint, GC, and CPU usage. To avoid this:

class Particle {

  constructor( options ) {
     options = _.extend( {
       location:  Vector2.ZERO
     } );
  }

  this.locationProperty = new Property( options.location );
} 

Note that this is not strictly necessary for classes where you expect to have a small number of instances. But applying this pattern is good habit to develop.

@pixelzoom
Copy link
Contributor Author

We should also discuss ways to prevent such constants from being mutated. E.g. the pattern used for Vector2.ZERO, Object.freeze,...

@samreid
Copy link
Member

samreid commented Nov 14, 2018

It is unfortunate to move values further away from where they are used, but where a significant amount of memory would be saved, this pattern should be used.

@pixelzoom
Copy link
Contributor Author

This pattern should also be used in cases where the same font should be used. For example, SOM repeats this in both screens that have a thermometer:

    this.compositeThermometerNode = new CompositeThermometerNode( multipleParticleModel, modelViewTransform, {
      font: new PhetFont( 20 ),

So if I want to change the thermometer font, I have to do it in 2 places, and keep them in sync. THERMOMETER_FONT: new PhetFont( 20 ) should be in SOMConstants.

@pixelzoom
Copy link
Contributor Author

We discussed this in a recent dev meeting, but for some reason I don't see the conclusions here. My recollection is:

  • As a general practice, if a class has a default option value that is non-mutable and involves object allocation (e.g. new SomeClass or {...}) then factor out the default value as a constant, to be reused by instances.

  • Use your judgement about when this practice is important. If you're absolutely certain that there will only be a few instances of a class, then it's fine to put the default value directly in the options hash, since that may improve readability and maintainability. If you're uncertain about how many instances might exist, then apply the above practice.

If anyone objects, please comment here for 1/17/19 dev meeting. If there are no objects by that meeting, I'm going to close this issue.

@pixelzoom pixelzoom self-assigned this Jan 16, 2019
@pixelzoom
Copy link
Contributor Author

1/17/19 dev meeting: No objections to above. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants