Particles drop performance

[samreid]

From https://github.com/phetsims/phet-io-wrappers/issues/23

Carried from https://github.com/phetsims/phet-io/issues/414 into this repo

@phet-steele said:

Precipitate and shaker particle data streams cause significant loses in performance. I can reliably cause an unresponsive web page on Win 10 browsers after reaching saturation. This is a pretty small amount of precip. being made, mind you....I can do much worse things 😈

https://drive.google.com/file/d/0B3HJopSo_QqLMndYQkdxcFoxelk/view?usp=sharing

For phetsims/phet-io#408.

and then:

@samreid particles are significantly affecting this sim's performance in instance proxies. Can anything at all be done about this? It hardly takes the addition of any particles to kill the browser.

[samreid]

In my opinion, each solute particle doesn't need to have a decorator in instance proxies. It may still need to be instrumented for state though (using our current save/load paradigm).

[pixelzoom]

Agreed, representing each particle in instance proxies may be overkill.

As for saving state... I'm not convinced that saving/restoring individual particles is the way to go, or that the current implementation supports save/load well. There are lots of other way that "precipitate" could have been implemented. For example: have 1 model object (Precipitate) that has the state of where its particles are, and the view would be responsible for rendering Precipitate as multiple instances of ParticleNode, or perhaps even as PrecipitateNode that draws all particles using 1 path.

The more general point here is that PhET-iO requirements (like what aspects of the precipitate need to be customizable, serializable, etc.) need to be considered during design, because they impact how the sim is implemented.

[samreid]

I'm having trouble killing performance on my Mac browsers or in my VirtualBox+IE by shaking out particles, but it would be straightforward to omit TShakerParticle, TPrecipitateParticle from instance proxies. The original bug report was from May 5, 2016 and it would be good to confirm it is still a problem before working on it more. @phet-steele can you replicate this problem on master?

[phet-steele]

Performance suffers pretty badly on an iPad 2 right out of the gate...so not really related to high amounts of precipitate in that case.

I also cannot ruin performance on my Mac with current master (yay!). Precip instances look to be properly disposed on reset, removal, etc., so that makes it a bit hard to create the thousands of particles I used to be able to.

That being said, I do lose performance quite handily after doing this:

1. master BLL + Instance Proxies, Concentration screen
2. Drain the beaker.
3. Shake particles, and keep shaking until some hit the bottom of the beaker.
4. Quickly, while some particles are still falling (and some are on the bottom too), start adding water to the beaker.
5. Drain the beaker and repeat if no errors were thrown.

I get hundreds of these:

Uncaught Error: Assertion failed: mismatched messageIndex, possible start/end mismatch
    at window.assertions.assertFunction (assert.js:22)
    at Object.end (phetioEvents.js?bust=1506540405566:175)
    at Array.endedCallback (toEventOnEmit.js?bust=1506540405566:29)
    at Emitter.emit (Emitter.js?bust=1506540405566:169)
    at DerivedProperty._notifyListeners (Property.js?bust=1506540405566:206)
    at DerivedProperty._setAndNotifyListeners (Property.js?bust=1506540405566:193)
    at DerivedProperty.set (Property.js?bust=1506540405566:140)
    at Array.listener (DerivedProperty.js?bust=1506540405566:67)
    at Emitter.emit2 (Emitter.js?bust=1506540405566:206)
    at NumberProperty._notifyListeners (Property.js?bust=1506540405566:204)

@samreid The performance drop is likely due to the massive cascade of errors this creates, and not due to the number of precip particles present. So feel free to close this and create a new issue for this error.

[samreid]

There is a new log query parameter to help diagnose mismatched messageIndex, possible start/end mismatch we might be able to use. But several emitters changed recently, we should see if we can replicate this problem in master.

[samreid]

I tried the instructions from #209 (comment) on mac chrome and 4/5 times it was fine and on the 5th time it threw an assertion error that the percentConcentration was -2000 or so.

@phet-steele can you double check on master at the moment and if this is replicable?

[phet-steele]

@samreid, broke it first try with steps in #209 (comment). My percentConcentration was 212...I think you beat me! Throws this assertion:

Uncaught Error: Assertion failed
    at window.assertions.assertFunction (/assert/js/assert.js:22)
    at ConcentrationSolution.js?bust=1507652942716:112
    at Array.listener (DerivedProperty.js?bust=1507652942716:67)
    at Emitter.emit2 (Emitter.js?bust=1507652942716:205)
    at DerivedProperty._notifyListeners (Property.js?bust=1507652942716:207)
    at DerivedProperty._setAndNotifyListeners (Property.js?bust=1507652942716:195)
    at DerivedProperty.set (Property.js?bust=1507652942716:142)
    at Array.listener (DerivedProperty.js?bust=1507652942716:67)
    at Emitter.emit2 (Emitter.js?bust=1507652942716:205)
    at DerivedProperty._notifyListeners (Property.js?bust=1507652942716:207)

[samreid]

It would be good for @pixelzoom to look into the transient out of bounds concentration issue. @pixelzoom after investigating that part, please reassign to me to look into whether the mismatched messageIndex is still happening.

[pixelzoom]

@samreid said:

It would be good for @pixelzoom to look into the transient out of bounds concentration issue.

Before I do anything here.... Step 1 of #209 (comment) says:

1. master BLL + Instance Proxies, Concentration screen

Has this been reproduced when not running with Instance Proxies? If so, then let's break this out into a separate issue.

[pixelzoom]

Answered my own question. Yes, this can be reproduced without instance proxies. Moving that issue to #210.

[pixelzoom]

With the conversion to PhetioGroup in #244, there's a possibility that performance may be impacted further. The previous implementation resulted in at most 1 call to invalidatePaint on each call to step. The PhetioGroup implementation results in a call to invalidatePaint for each particle added or removed, as well as 1 call to invalidatePaint per step if any particle has moved.

[kathy-phet]

Does each particle need to be instrumented? What if that wasn't part of state, but just populated based on how saturated the solution was?

[pixelzoom]

We just did a lot of work in #244 to instrument every particle (mainly because it was causing the sim to fail CT), so it will be a shame if we chuck all of this. But we can certainly revisit when we get to redesign in #230. Populating based on how saturated the solution is only works for precipitate, and precipitate isn't the main performance issue (it just sits there on the bottom of the beaker). Shaker particles are the performance issue, because they animate - and don't we also need to address them?

[kathy-phet]

Those particles dropping transient, so I would be OK with not instrumenting them. I don't think we need to support launching a saved simulation with particles that are actually dropping. @arouinfar - what do you think?

[samreid]

Please confirm that multiple calls to invalidatePaint are actually a performance problem before making any changes based on that assumption.

[arouinfar]

Those particles dropping transient, so I would be OK with not instrumenting them. I don't think we need to support launching a saved simulation with particles that are actually dropping. @arouinfar - what do you think?

@kathy-phet agreed.

[zepumph]

I'm not sure who should be assigned here, perhaps the lead dev to determine if invalidatePaint is the cause of the issue?

[pixelzoom]

This issue (and all PhET-iO related issues for this sim) is unassigned and deferred until work resumes on #230 (redesign PhET-iO API).

[pixelzoom]

Two related issues that will affect performance:

#279 - determine whether individual particles need to be instrumented (as they are now)
#276 - consider using Sprites.js to render particles

[pixelzoom]

In #279 (comment), @arouinfar said:

@pixelzoom leaving the particles instrumented sounds reasonable. They look fine in the tree, but I also don't plan to feature them. Performance has been fine, even on my phone.

This is further evidence that we may no longer have a performance problem, and that the Canvas implementation of particles may be just fine -- especially after the optimizations that I've made recently. So I decided to do some more rigourous testing.

Test procedure:

1. Run 1.7.0-dev.7
2. Go to the Concentration screen
3. Change the Solute combo box to "Potassium permanganate"
4. Use the shaker (radio button set to "Solid")
5. Shake the shaker until it is empty. This will result in 1400+ precipiate particles.
6. Observe the responsiveness while shaking particles. Observe the responsiveness after there's a lot of precipitate particles in the beaker.

Results:

There was no observable performance degradation on the following platforms. All interactions remained smooth.

• iPad 6 + iPadOS 16.1.1 + Safari
• iPhone 11 + iOS 16.1.2 + Safari
• MacBookPro16,1 + macOS 12.6.1 + Safari 16.1
• MacBookPro16,1 + macOS 12.6.1 + Chrome 108.0.5359.98

I then tested PhET-iO performance using the same steps, using https://phet-dev.colorado.edu/html/beers-law-lab/1.7.0-dev.7/phet-io/. Performance in Standalone was great for all of the above platforms. Performance on Studio was tested only for macOS (since iPad and iPhone are not instructional-design platforms) and performance was great.

[pixelzoom]

While I'm inclined to close this issue now, it's probably worth testing on a few "performance challenged" platforms, like older Chromebooks.

@KatieWoe could you please follow the steps in #209 (comment), and report back for 1 or 2 of PhET's performance-challenged devices? You can skip Studio, but please test both phet and phet-io brands, using these links:

• https://phet-dev.colorado.edu/html/beers-law-lab/1.7.0-dev.7/phet/beers-law-lab_en_phet.html
• https://phet-dev.colorado.edu/html/beers-law-lab/1.7.0-dev.7/phet-io/beers-law-lab_all_phet-io.html?postMessageOnError&phetioStandalone

[KatieWoe]

I tested this on a chrome book and didn't see performance issues.

[pixelzoom]

Thanks @KatieWoe.

Since there is no evidence of a performance problem, there is nothing to do, and I'm closing this issue.