preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.53k stars 1.94k forks source link

Too many DOM updates / not enough display updates #1137

Closed mindplay-dk closed 6 years ago

mindplay-dk commented 6 years ago

I'm porting a Photoshop-style cropping widget from plain JS to Preact, and ran into some serious performance problems.

During drag-and-drop, the display updates (which are triggered by setState() internally in the component) were extremely sluggish, with the rectangle clearly and visibly lagging behind the mouse pointer.

Dumbfounded, I launched a profiler and CPU monitor, and eventually started digging around in the source-code, where I found this:

export const defer = typeof Promise=='function' ? Promise.resolve().then.bind(Promise.resolve()) : setTimeout;

On a modern browser, this would resolve to a Promise, which, as far as I can figure, means that this defer function will defer for the shortest possible time - e.g. until the moment where the JS thread is idle.

For animation scenarios, this seems to mean it's doing a lot more DOM updates than the browser (Chrome) can actually do repaints.

In other words, it's burning CPU on calls to render() and DOM updates, leaving not enough time for the browser to keep up with smooth display updates.

I eventually found the solution in this comment:

options.debounceRendering = requestAnimationFrame

Boom! Buttery smooth display updates - at about 20-30% CPU load, whereas before I was getting choppy animation and serious lag at 90-something % 😮

Is there any practical reason why requestAnimationFrame isn't the default?

marvinhagemeister commented 6 years ago

/cc @developit

wen911119 commented 6 years ago

I'm also puzzled about this. The creator of Vue.js also said that "requestAnimationFrame has too long a delay and is quite indeterministic" when deal with similar problem. Could you please explain for us the consideration about this situation ? @developit

yaodingyd commented 6 years ago

requestAnimationFrame has too long a delay (compared to microtask)

rAF is only called at the beginning of a frame. Microtask could happen anytime during one frame, as long as no other JavaScript is mid-execution, so it could get called after a callback, or at the end of each task.

requestAnimationFrame is quite indeterministic

According to https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model, any queued requestAnimationFrame callbacks should be executed before the next style recalculation/layout/paint. Currently Chrome and Firebox follow this spec, Safari and Edge do not.

There is a lengthy thread on why React doesn't use rAF to do render. The gist is:

  1. For interactive events (such as clicks), using rAF batching is not suitable. Because these events are usually intentional and can happen multiple times in a single frame, delaying all flushing until a rAF breaks React rendering model and developer expectations in cases where the value of event handler depends on the state, or when event handlers read the state. Thus we need to flush each of them at the end of each event.

  2. Other effects can be deferred until later than next frame is suitable for a task to handle. React uses requestIdleCallback.

  3. The above two strategy would satisfy most use cases. For animation-specific usage, provide a migration path to allow user to use rAf for rendering. So in preact we have options.debounceRendering.

mindplay-dk commented 6 years ago

This goes over my head, but my main point is that the current default doesn't work well.

I didn't know about requestIdleCallback, and just tested my cropper with that assigned to options.debounceRendering, and it's just as smooth as with requestAnimationFrame - and with somewhat lower CPU usage, so that's good.

Currently Chrome and Firebox follow this spec, Safari and Edge do not.

Yeah, so if that was the default, it would need at least a minimal polyfill?

React internally uses a polyfill that does use requestAnimationFrame:

https://github.com/facebook/react/blob/master/packages/react-scheduler/src/ReactScheduler.js

Maybe we could reference that or one of the other available polyfills. (This is Preact, so if we could minimally polyfill around requestAnimationFrame internally to make this work well enough, that would be good - we have complexity and filesize constraints.)

The above two strategy would satisfy most use cases. For animation-specific usage, provide a migration path to allow user to use rAf for rendering. So in preact we have options.debounceRendering

Honestly, that's not really good enough - the global override is a really bad work-around, since you can't know what will work well for which components. Really, only the components can know that. Leaving it to the app/project to globally override this means you may have to make trade-offs - you may have to choose a setting that works well for one component but works poorly for another.

I don't plan on shipping this cropper as a library, but if I did, it would be awkward to have to explain in a README how to override the default to get acceptable performance and a good experience - basically anyone who tries it at first will have a poor experience by default.

So, in my opinion, ideally, this option shouldn't even exist - global options are always a problem, and if the default setting worked for the large majority of use-cases, you wouldn't really need it.

If a global option is useful for very particular, very technical use-cases, I think it's fine that it exists - but it really shouldn't be something the average user needs to worry or know about at all.

It certainly should be possible to ship something like a cropper component that will work well for everyone out of the box, right?

developit commented 6 years ago

What about debouncing your setState() calls with rAF? That's the core of the issue here - you're changing state in response to mouse movement, but inevitably you'll end up with a lot of useless intermediary states along the way.

Think about the options we have: everyone wants setState() to be synchronous because that defines how long it takes to get a frame out (if it were rAF-based, it wouldn't be possible to have setState() update the current frame). At the same time, when using setState() from a mouse event there's a use-case for state updates to be batched across entire frames.

Personally, I actually did prefer the old rAF-based default. Generally my renders produce UI, which means I have no interest in rendering faster than the browser can paint. However, for folks using a lot of async nested components, maybe sequential setStates via rAF could slow things down. Imagine a case where you have something like setState({ showDialog: true }, () => setState({ showDialog: false })) - now the render produced by showDialog=true will be painted, whereas with Promise-based render debouncing it actually gets completely dropped.

class Thing extends Component {
  onmove = e => {
    if (!this.lastMove) (requestAnimationFrame || setTimeout)(this.update);
    this.lastMove = e;
  };
  update() {
    let { clientX, clientY } = this.lastMove;
    this.lastMove = null;
    this.setState({ clientX, clientY });
  }
}

I don't think there is one general-purpose solution here. Scheduling is an incredibly complex problem, and it's unlikely that a generalized solution would have enough knowledge of render intent to be able to accurately predict how a given state update should be batched/applied.

I do agree that global options are problematic. We use them as a last resort, and they are really only present in Preact because they're needed by preact-compat. Perhaps there's potential here to replace the current options.debounceRendering global with something per-component, but that'll make the queue implementation more complex and possibly more expensive.

mindplay-dk commented 6 years ago

Personally, I actually did prefer the old rAF-based default. Generally my renders produce UI, which means I have no interest in rendering faster than the browser can paint.

Exactly, but... "Generally"?

What else would your renders be doing besides producing UI?

Is there any use-case for more renders than the browser can paint?

developit commented 6 years ago

Yes - I outlined it above, but specifically: if you have a tree of components, each that invokes setState() in order to render child components (perhaps from a Promise resolution, which cannot be synchronous), that tree will now take N frames to paint instead of 1 (N being the depth of the tree).

mindplay-dk commented 6 years ago

@developit ohh, now I get it... yeah, so state updates will propagate through layers of components at a rate of one setState() call per frame, basically. Yeah, that's not great.

In my particular use-case, the whole app is a single component, so not a deal-breaker for this project.

But so how about requestIdleCallback, would that work better? Or would that basically just allow up to around full CPU usage, e.g. doing as many DOM updates as there's CPU for? That's still not great.

I guess the core of the problem is that the render-queue really doesn't completely govern rendering - it's partially governed by components. I wonder if there's any way around that at all....?

mindplay-dk commented 6 years ago

if you have a tree of components, each that invokes setState() in order to render child components (perhaps from a Promise resolution, which cannot be synchronous), that tree will now take N frames to paint instead of 1 (N being the depth of the tree).

To better understand this problems and visualize it for myself, I created this example:

https://codesandbox.io/s/lrj98q081l

You can see the line tearing when you move the mouse quickly, and the problem disappears when you don't use requestAnimationFrame - and to my surprise, requestIdleCallback has the same problem.

mindplay-dk commented 6 years ago

Imagine a case where you have something like setState({ showDialog: true }, () => setState({ showDialog: false })) - now the render produced by showDialog=true will be painted, whereas with Promise-based render debouncing it actually gets completely dropped.

Maybe part of the problem here is it wouldn't be enough to simply override options.debounceRendering, because the render callbacks will still fire aggressively, e.g. before the DOM updates generated by Preact have actually been rendered by the browser.

Perhaps what might work better is don't process _renderCallbacks immediately, but rather defer that until requestAnimationFrame, e.g. until the updates have actually been painted?

The render-callbacks, at the moment, don't strictly get called when the updates have been rendered - they get called when the DOM updates have been performed, but we don't actually know that the browser has rendered the updates until requestAnimationFrame, right?

I guess maybe it's pointless to discuss that, since it's compatible with React, where these callbacks actually mean "the state change has been applied", and that's probably how people use them...

Kanaye commented 6 years ago

Normaly I don't need my whole App to be rendered in rAF I normally throttle updates within the animating components with a combination of rAF, setState and forceUpdate. Not in all but in some cases forceUpdate is needed (I haven't had time to debug those cases yet, so I can't explain them. But it seems like there are some edge-cases ).

developit commented 6 years ago

@mindplay-dk I think you touched on the core of the issue: the async rendering semantics weren't designed with the intention of being used as an animation loop or for true throttling. They're really just there to coalesce adjacent state updates in the tree since doing so avoids the most common cause of unnecessary rendering (duplicate/subsequent renders).

Your explanation is super interesting though - when the whole app is one component, there isn't really a meaningful difference between local and global render throttling. The solution here is probably to do the rAF throttle as part of input handling as you alluded to, but it makes me wonder if there's a valuable primitive for input-bound render response missing here. I guess this kind of stuff that can be implemented just as well in userland though, maybe that's the next best thing.

mindplay-dk commented 6 years ago

So, I have no idea what changed, but... the problem is gone - I can't reproduce it anymore, not even with an older version from source-control from around the time when I posted this.

Even with a 6 x CPU slow-down enabled in dev-tools, without the rAF work-around, it's totally fine now - it simply appears to get fewer mouse-events and as a result does fewer updates.

Best guess, something must have changed in Chrome since I opened this issue?

Either way, I think we can close this issue.

@developit thank you for taking the time to discuss it :-)