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

State Wrapper performance problems #160

Closed
pixelzoom opened this issue Aug 5, 2020 · 9 comments
Closed

State Wrapper performance problems #160

pixelzoom opened this issue Aug 5, 2020 · 9 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 5, 2020

The performance of this sim is (and always has been) awful in the State Wrapper. As the number of bunnies increases, it becomes painful to interact with the upstream sim. And after bunnies have taken over the world, I often have to restart Chrome because the browser is locked up.

@pixelzoom
Copy link
Contributor Author

PhET-iO team: What should my expectations be for performance of the State Wrapper? And will any of the up-and-coming PhET-iO performance optimizations improve things?

@samreid
Copy link
Member

samreid commented Aug 5, 2020

The state wrapper is not used for high frequency operations (like visual playback). In the case of a fully populated world, @chrisklus and I observed that the file is 6MB and 144,000 lines of code. Perhaps it is to be expected that it takes many seconds to serialize/deserialize. We haven't put effort into performance optimizing this path in particular, but that's because we haven't needed to yet (because the state wrapper isn't used for high frequency operations).

The other issues mentioned at the tail of #140 seem unlikely to improve the performance of state set/get.

If we expect clients to create a customized state that already has >1000 bunnies, perhaps we should spend time optimizing the loading sequence so that it won't take too long to start up.

Thoughts?

@samreid samreid assigned pixelzoom and unassigned samreid Aug 5, 2020
@pixelzoom
Copy link
Contributor Author

The other issues mentioned at the tail of #140 seem unlikely to improve the performance of state set/get.

Good to hear!

If we expect clients to create a customized state that already has >1000 bunnies, ...

I don't know what kinds of initial state clients are going to create. But I would suspect that initial states will involve < 100 bunnies. I can't think of a good reason to start with an initial population near the max (750). @amanda-phet thoughts?

... perhaps we should spend time optimizing the loading sequence so that it won't take too long to start up.

I have no idea what I could do in sim-specific code to make the loading sequence any faster. Bunnies are created 1 at a time when restoring the PhetioGroup, so restoring N bunnies results in N sets of notifications and update. Are you proposing something in PhET-iO that would make this faster?

I do know that the State Wrapper has been difficult to use throughout implementation of this sim, and (as I mentioned) frequently locks up and requires a "force quit" of Chrome.

@samreid
Copy link
Member

samreid commented Aug 8, 2020

The other issues mentioned at the tail of #140 seem unlikely to improve the performance of state set/get.

Good to hear!

To clarify, the original statement said "unlikely".

Are you proposing something in PhET-iO that would make this faster?

If we need to support that use case, then we can profile both the simulation code and phet-io stack to look for performance optimization possibilities. Or we could look for other data structures of optimization strategies. Self-unassigning until we hear about whether we need to support this use case.

@samreid samreid removed their assignment Aug 8, 2020
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 11, 2020

@samreid said:

The state wrapper is not used for high frequency operations (like visual playback). ...

I'm still unclear on what the State Wrapper's purpose is. I've only found it to be useful for discovering and debugging problems with save/restore of state, and I've generally ignored performance. Does it have other uses? How is it intended to be used by the client?

@samreid
Copy link
Member

samreid commented Aug 11, 2020

The state wrapper is not intended for use by the client. It is to help the PhET team debug serialization issues.

@pixelzoom
Copy link
Contributor Author

I understand the motivation to use the same wrapper index for PhET internally. But it seems odd that the wrapper index that's delivered to clients has links to things that are not intended to be used by clients.

But to the point of this issue... The State Wrapper is for PhET use only, and I haven't heard any reasons to improve its performance for this sim. So closing this issue.

@pixelzoom pixelzoom added the type:wontfix This will not be worked on label Aug 11, 2020
@pixelzoom
Copy link
Contributor Author

Reopening.

For phetsims/qa#641, @brooklynlash reported via Slack:

Good afternoon, I found some lag in the State Wrapper of Natural Selection, but not as strong as #160. Should I make an issue to address it?

I replied:

Thanks for reporting! I’ll reopen #160, note in that issue, and see if anyone wants to revisit.

@amanda-phet and @zepumph - any reason to investigate this further, or should this issue just be re-closed?

@pixelzoom
Copy link
Contributor Author

Discussed at 4/22/21 phet-io meeting, notes:

  • This has already been highly optimized. According to @samreid, further optimization might not yield better performance ("might already be at the ceiling") and are likely to delay NS 1.3 publication.
  • It has never been a goal that the downstream sim needs to instantaneously reflect the changes in the upstream sim.
  • @kathy-phet is OK with closing this again as "will not fix".

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