search

phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

data stream wrappers are slow and/or not responding #240

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

Originally reported by @KatieWoe in https://github.com/phetsims/QA/issues/551#issuecomment-697014587 for 1.2.0-rc.1 testing:

Noting a fair amount of slowdown between generations on ChromeOS when looking at a data stream (such as colorized).

I said:

@KatieWoe I don't know whether slowdown in the colorized data stream wrapper is an issue. I think it would be expected with NS, since it's doing a massive amount of writing to the console, and I don't know how it could be addressed. If you think it's significant, please create an issue.

@KatieWoe replied:

I don't know if it's significant. It is a massive amount of slow down, but I don't know how important performance is in that wrapper.

pixelzoom commented 3 years ago

@zepumph @samreid please comment on whether this slowdown is expected/unexpected, and whether this is blocking for the 10/1 deadline in https://github.com/phetsims/natural-selection/issues/208.

Labeling as blocking until I'm told otherwise.

KatieWoe commented 3 years ago

Also worth noting that this is the slower ChromeOS device that this was seen on.

pixelzoom commented 3 years ago

@KatieWoe which device is "the slower ChromeOS device"?

KatieWoe commented 3 years ago

Lovelace

samreid commented 3 years ago

This many console.log messages is expected to produce a slowdown, and does not block publication. @zepumph and I tested the console.log wrapper (not a wrapper) with and without dev tools open, and we tested the data-stream wrapper and it looked like the behavior was acceptable. We noticed the delayed response at Generation 5 or so, but that seems to be expected.

pixelzoom commented 3 years ago

... it looked like the behavior was acceptable.

There seems to be a bit of a disconnect between "acceptable" and "massive". @KatieWoe was testing with a Chromebook. What test device did you use?

samreid commented 3 years ago

The data stream is for a researcher to understand the nature of the data, it is not expected that a researcher should be able to run the colorized or data stream wrappers on Chromebook or iPad and have high performance.

@zepumph and I ran our testing on a Macbook Pro.

There seems to be a bit of a disconnect between "acceptable" and "massive".

In my opinion, a "massive" slowdown of the colorized or data stream wrappers is "acceptable" on iPad or Chromebook (in the same way that we don't support studio on iPad).

pixelzoom commented 3 years ago

Should QA even be testing data stream wrappers on Chromebook and iPad? (@KatieWoe asked recently about which wrappers should be tested on which platforms.)

pixelzoom commented 3 years ago

9/24/2020 phet-io meeting:

Does not need to be addressed for Natural Selection 1.2.

@zepumph will ask @KatieWoe to amend the QA Book section about testing data stream wrappers, then close this issue.

zepumph commented 3 years ago

From today's PhET-iO meeting, we feel like some changes should be made to how QA is testing data stream wrappers:

Assigning to @KatieWoe for questions, and to update the QA book as needed.

pixelzoom commented 3 years ago

Since this has morphed to a QA issue, I've moved https://github.com/phetsims/natural-selection/issues/240#issuecomment-698508288 to a new issue, https://github.com/phetsims/QA/issues/553.

Nothing to do here for Natural Selection, so closing this issue.

KatieWoe commented 3 years ago

In firefox on Win 10, I saw it slow down to the point where the window would say not responding and it wasn't really usable. In https://github.com/phetsims/QA/issues/557. Close again if this doesn't change anything

pixelzoom commented 3 years ago

Thanks for reporting @KatieWoe.

@samreid said in https://github.com/phetsims/natural-selection/issues/240#issuecomment-697934011 that a slowdown was "expected" and "acceptable". And in https://github.com/phetsims/natural-selection/issues/240#issuecomment-697979325, he said that "it is not expected that a researcher should be able to run the colorized or data stream wrappers on Chromebook or iPad and have high performance".

Not being able to run at all on Win + Firefox is different. Is that also "expected" and "acceptable"? I think not, so moving this to a general issue in https://github.com/phetsims/phet-io-wrappers/issues/381.

As for this issue, my understanding is that it's still not blocking for the #208 release on 10/1. So I'll re-close this issue.

pixelzoom commented 3 years ago

On second thought... I'm going to re-title this issue and leave it open, so that we can revisit for the 1.3 release.

zepumph commented 3 years ago

While working on https://github.com/phetsims/phet-io-wrappers/issues/381 with @samreid, we found that it was really helpful to mark Organism.positionProperty as high frequency. That in combination with the commit in https://github.com/phetsims/phet-io-wrappers/issues/381 seemed to fix this problem for us.

pixelzoom commented 3 years ago

The next milestones (#251) will be taking shas from master. I'll label this as "fixed", and leave open for verification by QA.

KatieWoe commented 3 years ago

Tested on 1.3.0-dev.1 on ChromeOS. When I was testing the colorized sim, it performed reasonably well, but after awhile I stopped seeing new events. @pixelzoom if you want to pair on this or want me to try and gather further info, let me know.

samreid commented 3 years ago

How did you see the messages on ChromeOS? Is there a way to open the dev tools? If that problem only occurs on one platform, it may be due to a buffer problem in that console (and not something we would address).

pixelzoom commented 3 years ago

Thanks for the offer to pair @KatieWoe. But I have no familiarity with the data-stream wrappers. So it would probably be more productive if you investigated with someone from the PhET-iO team. I'll assign them.

This is high priority, because we want to create a release branch immediately after dev test is completed.

KatieWoe commented 3 years ago

Yes, if you open the dev tools and look in the console you see the events. I've only seen on the ChromeOS device so far. I didn't see it (at least yet) on Win 10 Chrome so I think it might be some kind of performance issue.

KatieWoe commented 3 years ago

I did some more exploration, and it might be a significant delay until the console updates, rather than it not being sent at all. I'm not sure why that would happen though.

zepumph commented 3 years ago

If you are able to have good performance with the console closed, then I don't think this is a problem. The most-likely cause of the lag is the expensive nature of printing to the console. @KatieWoe can you notice any performance trouble without the dev tools open?

KatieWoe commented 3 years ago

Even with them open, the sim itself wasn't much delayed, just getting the events.

zepumph commented 3 years ago

This feels like a non issue to me, and expected. When we use a tool that logs thousands of lines per second to the console, this will be delayed by as much time as it takes to print all of that.

One of the original comments in this issue is:

I don't know if it's significant. It is a massive amount of slow down, but I don't know how important performance is in that wrapper.

I would say performance here is not important at all. I'm going to close, please reopen if there is more to discuss.