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

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

Closed
Tracked by #573
pixelzoom opened this issue Sep 23, 2020 · 24 comments
Closed
Tracked by #573

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

pixelzoom opened this issue Sep 23, 2020 · 24 comments

Comments

@pixelzoom
Copy link
Contributor

Originally reported by @KatieWoe in phetsims/qa#551 (comment) 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
Copy link
Contributor Author

pixelzoom commented Sep 23, 2020

@zepumph @samreid please comment on whether this slowdown is expected/unexpected, and whether this is blocking for the 10/1 deadline in #208.

Labeling as blocking until I'm told otherwise.

@pixelzoom pixelzoom changed the title "massive" slow down with data stream wrappers "massive" slowdown with data stream wrappers Sep 23, 2020
@KatieWoe
Copy link
Contributor

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

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 23, 2020

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

@KatieWoe
Copy link
Contributor

Lovelace

@samreid
Copy link
Member

samreid commented Sep 23, 2020

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
Copy link
Contributor Author

... 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
Copy link
Member

samreid commented Sep 23, 2020

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).

@samreid samreid assigned pixelzoom and unassigned samreid Sep 23, 2020
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 23, 2020

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 pixelzoom assigned samreid and unassigned pixelzoom Sep 23, 2020
@pixelzoom
Copy link
Contributor Author

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
Copy link
Member

zepumph commented Sep 24, 2020

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

  • Please do not test iPad or Chromebook on the Data: colorized or Data: JSON wrappers.
  • Please test Data: colorized and Data: JSON very lightly, only confirming that the data stream prints as expected to the console on a couple device/browser combos.
  • Please test thoroughly the data: textarea wrapper on all devices to make sure that the cross frame messages for data stream support are running correctly. This shouldn't have performance trouble on chromebook or iPad.

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

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 24, 2020

Since this has morphed to a QA issue, I've moved #240 (comment) to a new issue, phetsims/qa#553.

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

@KatieWoe
Copy link
Contributor

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 phetsims/qa#557. Close again if this doesn't change anything

@KatieWoe KatieWoe reopened this Sep 26, 2020
@pixelzoom pixelzoom reopened this Sep 27, 2020
@pixelzoom pixelzoom changed the title "massive" slowdown with data stream wrappers data stream wrappers are slow and/or not responding Sep 27, 2020
@zepumph
Copy link
Member

zepumph commented Sep 30, 2020

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
Copy link
Contributor Author

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

@KatieWoe
Copy link
Contributor

KatieWoe commented Apr 6, 2021

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
Copy link
Member

samreid commented Apr 6, 2021

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
Copy link
Contributor Author

pixelzoom commented Apr 6, 2021

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
Copy link
Contributor

KatieWoe commented Apr 6, 2021

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
Copy link
Contributor

KatieWoe commented Apr 6, 2021

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
Copy link
Member

zepumph commented Apr 7, 2021

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
Copy link
Contributor

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

@zepumph
Copy link
Member

zepumph commented Apr 12, 2021

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.

@zepumph zepumph closed this as completed Apr 12, 2021
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

4 participants