search

phetsims / beers-law-lab

"Beer's Law Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/beers-law-lab
GNU General Public License v3.0
2 stars 9 forks source link

Particles drop performance #209

Closed samreid closed 1 year ago

samreid commented 7 years ago

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

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

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

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

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

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

I tried the instructions from https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-332631768 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 commented 7 years ago

@samreid, broke it first try with steps in https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-332631768. 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 commented 7 years ago

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

@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 https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-332631768 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 commented 7 years ago

Answered my own question. Yes, this can be reproduced without instance proxies. Moving that issue to https://github.com/phetsims/beers-law-lab/issues/210.

samreid commented 6 years ago

On hold until #210 is complete.

pixelzoom commented 6 years ago

@samreid #210 is completed (pending verification by @phet-steele), so you may proceed with this issue.

phet-steele commented 6 years ago

@samreid go ahead.

pixelzoom commented 6 years ago

Btw... #210 should be totally unrelated to the "mismatched messageIndex" failures reported in https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-332631768.

samreid commented 6 years ago

OK It seems the next step for this issue is to see if the mismatched messageIndex is still occurring now that #210 is fixed. I tried the instructions in https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-332631768 5 times on my Mac/Chrome and didn't see any errors in the console or performance degradation. @phet-steele can you please take a look?

phet-steele commented 6 years ago

Also no errors or performance concerns here.

phet-steele commented 6 years ago

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.

@samreid @pixelzoom the quote I said above looks to not be true anymore. I tried BLL studio on master (and 1.5.6, and 1.6.4) and precip particles are not being removed from the list anymore. Kills performance. What happened?

pixelzoom commented 6 years ago

No idea, I haven't changed anything related. But phet-io has been undergoing big changes. @samreid ?

phet-steele commented 6 years ago

@samreid, when "checking against the prior published version", which is 1.5.5-phetio, the problem did not exist. So newly introduced again in 1.5.6-phetio.

Oh and this is for phetsims/QA/issues/128.

samreid commented 6 years ago

Testing 1.5.6-phetio, I see 145 instrumented instances on startup. Then shaking in some particles, it goes to 182 while they are in the air. Then when they dissolve, it goes back to 145. I also tried resetting while the particles were in the air, and it correctly went back to 145. Am I doing this wrong?

phet-steele commented 6 years ago

@samreid I don't see it in the 1.5.6 branch (don't know why I did earlier), but I do see it in 1.6.4 (and probably master, didn't double check).

samreid commented 6 years ago

I confirmed the particles are leaking in studio/master.

samreid commented 6 years ago

@zepumph is this something you can address?

zepumph commented 6 years ago

On second thought, dropping priority because I should work on FL thoughtful api first.

pixelzoom commented 4 years ago

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

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

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

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

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

arouinfar commented 4 years ago

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

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

pixelzoom commented 4 years ago

This issue (and all PhET-iO related issues for this sim) is unassigned and deferred until work resumes on https://github.com/phetsims/beers-law-lab/issues/230 (redesign PhET-iO API).

pixelzoom commented 3 years ago

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 commented 1 year ago

In https://github.com/phetsims/beers-law-lab/issues/279#issuecomment-1346798048, @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.

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 commented 1 year ago

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 https://github.com/phetsims/beers-law-lab/issues/209#issuecomment-1347245970, 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:

KatieWoe commented 1 year ago

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

pixelzoom commented 1 year ago

Thanks @KatieWoe.

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