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

Efficiently ignore irrelevant updates from context #12876

Closed
aldendaniels opened this issue May 22, 2018 · 9 comments
Closed

Efficiently ignore irrelevant updates from context #12876

aldendaniels opened this issue May 22, 2018 · 9 comments

Comments

@aldendaniels
Copy link

We're using context to represent where user focus is in a large app. In our app, we have many (hundreds) of "Focus Routes" that listen to the focus location in the app - much like routes in React Router.

Most focus transitions are irrelevant to most routes - some routes lose focus and some gain it, but most are unaffected. Today we have a context consumer at each route component that computes some derived state (e.g. isFocused, etc.) and provides that derived state as props to a wrapped component. The wrapped component has an optimized shouldComponentUpdate().

The problem we're hitting is that React appears to do a meaningful amount of work every time the context consumer is re-rendered, even though the wrapped component doesn't re-render. This adds up quickly across all the focus routes. Our next step is to do better profiling on in production mode, but on dev it appears to be ~0.5ms per route, which is problematic.

What we need is a way to efficiently prevent subscribers from re-rendering when the new value (focus location in our case) isn't relevant. The unstable_observedBits feature is almost right for us, but the the 32bit limitation is unsuitable for our scenario. If this were an unbounded list of booleans, for example, we could use this to only update invalidated components. Alternatively, if a ContextConsumder took a shouldComponentUpdate() hook we could use that.

Consider this a plug for a a non-32-bit restricted alternative to unstable_observedBits :)

If nothing like this is forthcoming, we'll likely revert to using an emitter/subscriber model in a batched update. This is non-ideal for us, however, because there's state that we need to propagate to new components in a single render pass and context is particularly good at this.

@gaearon
Copy link
Collaborator

gaearon commented May 22, 2018

Let’s profile in production first? I’d expect the difference to be significant.

@aldendaniels
Copy link
Author

Hi @gaearon - indeed. I expected production to be faster, but it's a LOT faster - ~ 10x. Happy to share numbers once we clean up our results. Likely we can make this performant enough.

There are also some optimizations we're looking at to hash routes and use dirty-bits, so that if you have 100 sibling routes only 100/31 ~ 3 get re-rendered. It feels rather arbitrary that our ability to optimize in this way is limited by the length of a 32 bit number, but this will probably be performant enough for our needs.

Without the dirty bit hack, the dev speeds are likely to be problematic - we'd waste time chasing phantom perf problems. However, with the dirty bits optimizations, we expect we can make this palatable as well.

It seems that as the community adopts the new context API for redux-like containers, routing, theming, etc. we're going to see huge numbers of context consumers in an app. A page that renders a couple thousand components might also render hundreds of consumers.

Our focus framework pushes this to a bit of an extreme with focus routes around most components that care about focus such as buttons, menus, modals, etc. In your opinion, are we outside the intended use-case of context, or does this wide-spread use seem within the intended use-case? If the latter, then I expect a better dirty bits alternative might still be widely useful.

I'll follow up with a more concrete outcome in a couple days once we get our approach and results cleaned up.

@aldendaniels
Copy link
Author

Sorry for the delay - to follow up. We did a bunch of profiling and (unsurprisingly) found several inefficiencies in our own code that was contributing to our performance issues. Having eliminated these and created a more scientific experiment, here's what we found.

Update times when rending 5,000 routes on a page

With Observed Bits Without Observed Bits
In Production 6 ms 20 ms
In Dev 26 ms 90 ms

This is testing the time to transition from route A being active to route B being active. 4,998 (5,000 - 2) other routes are being notified of the change, but have basically no work to do.

In the Without Observed Bits case, all context consumers receive the update and then all but 2 return identical props into the child component, which returns false in shouldComponentUpdate().

In the WithObservedBits case we do that same work to, but we make the results into a an observed bits hash. The hash makes it so that only 1 in 16 of the irrelevant routes hear the update.

It's not a clean test though, because we do a minimal amount of work in every handler to re-compute some derived data (props to inner component). So it's unclear if the savings is from avoiding this work, or avoiding work on React's end when rendering the inner component and running the shouldComponentUpdate() check. We ran out of time to run these profiles.

Regardless, the dirty bits API is a nice way to avoid spurious updates without bespoke caching logic at every consumer. OTOH, the dirty bits API is unstable and subject to the 31 bit limitation, so improvements to the API would still benefit us.

Take-aways for us are:

  1. React / context is quite fast - without the optimization we'd be fine for most uses
  2. The dirtyBits feature is useful for avoiding needless work
  3. Dev is a many times slower than prod

So the initial request (nicer API for dirty bits) is still relevant, but we're probably in a relatively small group of users that needs this.

Thanks for all the great work going into React 16+. Please leave open or close at your discretion.

@aweary aweary closed this as completed Jul 25, 2018
@markerikson
Copy link
Contributor

I see the issue is closed, but it seems fairly relevant to my own question. I'd like to experiment with exactly this same concept for React-Redux.

  • Just how "unstable" is unstable_observedBits ?
  • How likely is this feature to be changed or removed in the future?
  • Why was it marked "unstable" shortly before it was released in the first place?

@gaearon
Copy link
Collaborator

gaearon commented Aug 25, 2018

Mostly just because we didn’t know if it would be useful for libraries or too limited. Didn’t want to release something we’re not relying on ourselves yet.

@markerikson
Copy link
Contributor

The other thing I just found out last night is that the changedBits callback is apparently passed to createContext. I hadn't looked at things carefully, and originally assumed it was passed to the Context.Provider instead. That makes things potentially a lot more complicated.

My original train of thought was that React-Redux could have some kind of a default "any keys in this plain object changed?" bits calculation function, but allow the user to pass in their own callback to calculate changed bits for something like an Immutable.js Map. In our current experiments, we're using a singleton Context entry which is created off in its own file, so there's no way to let the end user customize its behavior.

Not an absolute immediate priority, but it's something I'd like to experiment with as a potential perf optimization approach in v6.

@gaearon
Copy link
Collaborator

gaearon commented Aug 25, 2018

If it's useful to decouple, mind filing an RFC with proposed behavior and justification?

@markerikson
Copy link
Contributor

Sure, I'll do that now. Not sure I'll be able to give a really detailed writeup, but I can at least explain my thinking.

I just skimmed the relevant chunks of code, and it doesn't seem like it would be too much of a change to implement at first glance. The _calculateChangedBits reference is currently stored on the Context fiber. updateContextProvider() calls calculateChangedBits(context, newValue, oldValue), which then references context._calculateChangedBits and uses it if available. I see const newProps = workInProgress.pendingProps; in updateContextProvider(), so presumably if calculateChangedBits was given as a prop to the Provider, it could be pulled from there instead and used.

@markerikson
Copy link
Contributor

markerikson commented Aug 25, 2018

Filed an RFC as reactjs/rfcs#60 .

It looks like a pretty simple addition - possibly just 4 changed and 2 added LOC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants