openstreetmap / iD

🆔 The easy-to-use OpenStreetMap editor in JavaScript.
https://www.openstreetmap.org/edit?editor=id
ISC License
3.36k stars 1.21k forks source link

High CPU load because of breathe selection indicator #2911

Closed tyrasd closed 8 years ago

tyrasd commented 8 years ago

The new breathe selection indicator (#2879) produces constant high cpu load (close to or >100%) on my machine whenever an object is selected.

Browsers: Chrome 47.0.2526.106 / Firefox 43.0.4 OS: Ubuntu 14.04 CPU: Intel i5-4200U iD: master

bhousel commented 8 years ago

Yes, I noticed this on all browsers that I test with too. It could probably benefit from some more eyes on the code with performance in mind.

For what it's worth, I did also make an early version of the breathe behavior using CSS animation, and it did the same thing. In theory, the CSS animation implementation should be the most performant possible because it basically turns over control to the browser and uses no JavaScript. The code is here.

Obviously, the CPU load is more of an issue when you select a lot of things or a big 2000 node multipolygon, and just leave it selected and running for a while in an open tab.

johsin18 commented 8 years ago

Please improve this. My laptop fan starts rotating at full speed as soon as I select a single path (Chrome on Core i7), and the battery gets drained quickly. That's the last thing you want when you are on the road mapping.

If we cannot fix it by coding it differently (Do we at least use requestAnimationFrame somwhere under the hood?):

What about just blinking (on/off)? Or an option to switch if off? Or less steps for a less smooth animation, but better performance? Potentially make it depend on the battery status: https://developer.mozilla.org/en-US/docs/Web/API/Battery_Status_API

boothym commented 8 years ago

Interesting, I've heard my fan go on full speed for a few seconds but just put it down to the iD editor in general and not a specific action. Idling at 2-3%, if I click on a line it jumps up by 15-20% and stays there.

Firefox/Win10, on an i7 laptop.

bhousel commented 8 years ago

I looked into this a bit tonight and it's definitely caused by repaints. There is barely any JavaScript running. I'm not sure if there is much I can do to improve the performance. Maybe we can use a stepped interpolator instead of a smooth one? D3 is already doing a great job of scheduling the style tweens with requestAnimationFrame.

By default, Chrome seems to use CPU for rasterizing and compositing iD's content - this is because we use pretty complex SVG shapes everywhere..

screenshot 2016-09-24 00 05 27

It's possible to force GPU rasterization with about:flags - but that doesn't improve things as much as I had hoped. In fact, it seems to make things worse on my laptop:

screenshot 2016-09-23 23 47 56

tyrasd commented 8 years ago

Thanks for the analysis, Bryan!

The question is then probably: can we replace the "breathe" animation with something that's a bit lighter to compute and get the same or a very similar result? The intended "result" we want to get from this is to make it easier to find which element is currently selected, right? Which is most important to know when clicking on a feature that is next to some similar features (to know which one actually got selected, see #1814) or generally when reviewing changes from the save dialog (see https://github.com/openstreetmap/iD/issues/2645#issuecomment-102320798). In both of these cases the animation wouldn't need to run all the time (or not at the same speed all the time). I'm thinking at a faster (e.g. 3x repeated) pulsing right after the selection of the object, and after that either a pulse only every 5 seconds or none at all (where a new pulsed-selection-highlighting would need to be triggered by a dedicated keyboard shortcut, ui element or clicking the element again).

Another alternative would be to use a lighter to compute animation (I think blurred lines are relatively hard to do in CPU rendering?), maybe a dashed background that's tweened to run along the selected line.

bhousel commented 8 years ago

I added in a fix for this in b90c545: Add RatchetyInterpolator for breathe behavior. The interpolator now just runs a few discrete steps, avoiding most of the repainting that was causing high CPU and noisy laptop fans. And it still looks pretty good.

performance after: screenshot 2016-09-24 23 25 12