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

IO types cause increase in memory footprint #71

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

IO types cause increase in memory footprint #71

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

Comments

@pixelzoom
Copy link
Contributor

From phetsims/unit-rates#207 (comment):

For clarity, @jonathanolson and I (and mainly @jonathanolson) worked out that the source of the memory leak came from the commit when I moved phetioInherit from the phet-io repo into tandem. This consequently made it so that phetioInherit was not just "stubbing" out TypeIOs and returning no-op functions like function(){}, but instead it was actually creating TypeIOs. This exposed the memory leak that @samreid explained above to all sims, even in phet brand.

Re his comment:
I think that there is likely other places where we can decrease memory consumption, seeing as 150% is still a large size increase to all sims, and likely that is from things like default parameters that @samreid was able to factor out some.

@samreid
Copy link
Member

samreid commented Nov 9, 2018

My summary from phetsims/unit-rates#207 (comment)

The changes I made were predominantly about factoring out instantiated parametric types. For instance, phetsims/axon@4a9762e factors out EmitterIO( [] ) instead of creating an equivalent but different one for each Emitter. I cannot say for certain that this was "the source of the memory increase", but it seemed like a safe and straightforward way to significantly reduce the memory footprint. We can probably reduce size further by eliminating closures in common code, or by other strategies.

@pixelzoom replied:

And yes, I've read #207 (comment) and #207 (comment), but I don't understand.

Perhaps a short call would be very helpful? I'll be on slack.

samreid added a commit to phetsims/scenery that referenced this issue Nov 9, 2018
samreid added a commit to phetsims/axon that referenced this issue Nov 9, 2018
samreid added a commit to phetsims/sun that referenced this issue Nov 9, 2018
samreid added a commit to phetsims/scenery-phet that referenced this issue Nov 9, 2018
samreid added a commit to phetsims/sun that referenced this issue Nov 9, 2018
@pixelzoom
Copy link
Contributor Author

Discussed with @samreid via Slack.

This has likely been a problem in brand=phet-io for a long time, and was not noticed due to insufficient memory testing. It didn't manifest previously in Unit Rates, because that sim has not been instrumented. After IO types were moved to tandem, they were no longer stubs, and therefore contributed to the memory footprint of all brands.

Example of the problem in BooleanProperty. Previously in the constructor, every new instance received a new instance of BooleanIO.

function BooleanProperty( value, options )
  ... 
  options.phetioType = BooleanIO();
  ...
}

This fix (which is what most of the above commits are doing) reuses 1 instance of BooleanIO for all instances of BooleanPropertyIO.

var BooleanPropertyIO = PropertyIO( BooleanIO )

function BooleanProperty( value, options )
  ... 
  options.phetioType = BooleanPropertyIO;
  ...
}

@samreid
Copy link
Member

samreid commented Nov 9, 2018

Another 0.5MB saved in the preceding commit.

@samreid
Copy link
Member

samreid commented Nov 9, 2018

We could reduce PhET-iO memory footprint by eliminating the wrapper instances (aka PhetioObject.phetioWrapper). This wouldn't help PhET brand though.

@samreid
Copy link
Member

samreid commented Nov 20, 2018

In phetsims/graphing-quadratics#50 (comment) @pixelzoom reported:

For comparison, the latest brand=phet memory tests are in phetsims/graphing-quadratics#106 (~41MB startup, ~52MB stable).

Test results for brand=phet-io:

  • 1.0.0-dev.46 with ?standalone&fuzz
  • Chrome 70.0.3538.102
  • macOS 10.11.6 on MacBook Pro
Minutes Heap Size (MB)
0 43.9
5 52.2
10 53.6
15 54.6
20 54.7
25 55.4
30 55.5
45 55.6

This looks fine. PhET-iO appears to add < 4MB of overhead to brand=phet, and that seems reasonable. Closing.

@pixelzoom anything else to do for this issue?

@pixelzoom
Copy link
Contributor Author

Seems like this issue is resolved. Closing.

jbphet pushed a commit to phetsims/scenery that referenced this issue Nov 20, 2018
jbphet pushed a commit that referenced this issue Nov 20, 2018
jbphet pushed a commit to phetsims/axon that referenced this issue Nov 20, 2018
jbphet pushed a commit to phetsims/sun that referenced this issue Nov 20, 2018
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