search

openseattle / seattlespeeds

Communal Broadband Situational Awareness Utility
MIT License
10 stars 5 forks source link

NDT-JS: post test (should be idle) it can be a CPU hog. New code fixes it. #21

Open JohnTigue opened 9 years ago

JohnTigue commented 9 years ago

See also #11 as that may be the same code being discussed there.

The following is an email thread between @JohnTigue and @collina on 2015-05-05:

On 05/01/2015 03:03 PM, John Tigue wrote: Chris and Will,

On May 1, 2015, at 6:15 AM, Chris Ritzo critzo@opentechinstitute.org wrote:

Chirs: Please tell me about this new functionality?

I'll have to defer to Nathan and Collin on this (cc'd).

I think Collin ran into some performance issues with D3.js in another project he was working on, but this will only be an issue for your project if you want to stay updated with the current code in our repo.

Hello Nathan and Collin,

I am one of the folks working as part of Code for Seattle in order to get a M-Lab clienting portal going for the city of Seattle.

I am writing because of the performance issues that Chris mentioned above. I too have seen something. Perhaps it is the same.

I can repo the test via the ndt-javascript-client as found at:

http://files.opentechinstitute.org/~critzo/mlab/examples/ndt-javascript-client/examples/d3_circle/

If I simply load that page and wait for things to settle down, my CPU usage drops quickly to idle. If I then I run the test, the CPU meter immediately goes to about 50% (that's OK; it's working hard). After a few second the NDT test ends but the CPU stays the same, about 50%. This is a 2011 MacBook Air I am testing on.

I was going to dig into and report via GitHub but Chris' comments made me think that perhaps you are already on it.

Let me know if I can help with test, or I can get down to trying to debug it as well if this is not actually the same performance issue that Chris mentioned. -John

Nice catch.

The D3 progress meter was designed when there was no onprogress callback for NDTjs, and without performance in mind. As a result, it's updated based on the d3.timer function with an interval of 0, which means that it's constantly triggering and consequently takes a large amount of resources.

The alternative that we are transitioning to is using the onprogress callback, which also has the benefit of passing the current test performance information, and then using a Javascript progress meter. We determine the progress through retaining the timestamp that the test changed state (to 'running_s2c' or 'running_c2s'). In this case, we set the call back to trigger every few hundred milliseconds, and then increment the progress meter based on the amount of lapsed since the state changed.

Since NDT runs for 10 secs, so progress = (currentTime - timeStateChanged)/10.

As a result, you won't be redrawing D3 at every moment of the test.

Does that make sense?

Cordially, Collin

Yup, got it.

There's no way that our site is going to be going public in the next week or so.

I'll put this on hold and wait for the new onprogress callback mechanism.

I've got tons of status==="does not exist" keeping me busy before I need to worry about CPU piggies in the client.

Thanks,

-John

It exists currently, we are now developing out in the official NDT repository, so you can grab it here: https://github.com/ndt-project/ndt/blob/master/HTML5-frontend/ndt-browser-client.js

JohnTigue commented 9 years ago

crosslink: #26

brett-miller commented 9 years ago

d3.js shouldn't be considered as a likely cause for performance issues - however d3 makes it easy to shoot yourself in the foot by overloading the DOM with changes.

JohnTigue commented 9 years ago

Yup. @nkinkade was saying in the last meeting that it has to do with an overactive loop doing something every millisecond. This should go away with new code that should be pulled in.

nkinkade commented 9 years ago

I believe this is correct. d3.js isn't the problem, only that it was being asked to do something on essentially every cycle of the event loop, which was unnecessary. This was correct in the NDTjs code by adding an updateInterveral parameter:

https://github.com/ndt-project/ndt/search?utf8=%E2%9C%93&q=updateinterval

This parameter tells NDTjs how often it should call the "onprogress" callback. Setting this to something reasonable should quiet down D3.js, or anything else associated with the callback for that matter.

JohnTigue commented 9 years ago

I think it is time to start bringing in client side dependencies via bower. The project needs to level-up to a more mature process of this front. Currently the code, say for NDT-JS, is just copied manually into the repo. We should really be pulling it in as a dependency, not a code inside SEANetMap's repo.

Same goes for server deps. We have bq2geojson in the codebase:
https://github.com/codeforseattle/seanetmap/tree/master/server/bq2geojson

That too should be pulled in (via npm, not bower)