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

Dont recreate maked context unless unmasked context changes #8706

Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 7, 2017

The problem

Fiber memoizes the merged context object for context-providers but does not memoize the masked context object. This caused componentWillReceiveProps to be called tpo frequently for context consumers which can result in infinite loops (eg if componentWillReceiveProps() calls setState()).

A minimal reproduction of one such infinite loop has been added as a test. It looks a little silly but it is greatly reduced from a bug I was investigating in a larger, real-world app.

The proposed solution

Given the way context is currently implemented, I think the only solution to this issue is to store memoized copies of both the unmasked and masked context objects in order to avoid unnecessarily recreating the masked context. (Stack already does something like this.)

Concerns

The solution in this PR is kind of awkward due to the facts that:

  1. We probably want to cache this memoized context on a class instance rather than the Fiber to avoid adding 2 additional fields to all Fibers (because most won't consume context). Prior art is __reactInternalMemoizedMaskedChildContext.
  2. The class instance doesn't yet exist the first time the masked context is created so we have to initially set the cached object outside of ReactFiberContext.

This change is kind of flimsy, but so is context in general at the moment (see this comment for elaboration).

Let's talk about it. 😄

@gaearon
Copy link
Collaborator

gaearon commented Jan 7, 2017

Seems fine to me. We can just remove masking later if we want to.

@acdlite
Copy link
Collaborator

acdlite commented Jan 7, 2017

In general, we shouldn't be using the instance to store memoized values because it's shared between both copies of the fiber, as we discussed. But since context is already broken for this purpose, maybe we don't need to care for now.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 7, 2017

Agreed. That's why I mentioned the prior art. Seems like we need to clean up (or rethink) context at some point. This is just another band-aid.

Doing so can lead to infinite loops if componentWillReceiveProps() calls setState()
@bvaughn bvaughn force-pushed the dont-recreate-masked-context-unless-needed branch from af5c342 to a682059 Compare January 7, 2017 16:53
@bvaughn bvaughn merged commit 67c16e3 into facebook:master Jan 9, 2017
@bvaughn bvaughn deleted the dont-recreate-masked-context-unless-needed branch January 9, 2017 16:45
@sebmarkbage
Copy link
Collaborator

This is very different from the provider since this is for every recipient. We expect there to be many more recipients and that those will update much more frequently than the providers.

Also this is adding more to the already broken solution that compares props equality. We should fix that with a tag rather than piling on.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 9, 2017

This is very different from the provider since this is for every recipient. We expect there to be many more recipients and that those will update much more frequently than the providers.

I don't understand the first half of this comment. I'd love to talk about this with you when you're back in the office. Maybe we could just do that? Would be easier.

@acdlite
Copy link
Collaborator

acdlite commented Jan 9, 2017

@bvaughn __reactInternalMemoizedMaskedChildContext is on the provider (thing that provides context), not the consumer.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 9, 2017

What I meant was that I don't understand the implications of what @sebmarkbage is saying.

This PR added caching for unmasked and masked context on the provider in order to avoid triggering unnecessarily calls to componentWillReceiveProps on child consumers. That was intentional.

Edit: (Aside from the fact that using the instance to cache values is hack and error-prone) is the concern that I'm caching this value on the wrong instance? As in I'm caching a value on an instance that doesn't actually originate with that instance.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providers don't have masked contexts unless they're also receivers.

// Cache unmasked context so we can avoid recreating masked context unless necessary.
// ReactFiberContext usually updates this cache but can't for newly-created instances.
instance.__reactInternalMemoizedUnmaskedChildContext = unmaskedContext;
instance.__reactInternalMemoizedMaskedChildContext = context;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we doing this on every instance? That's probably not great.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed this. 😞

Copy link
Collaborator

@gaearon gaearon Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the context, before this PR we used to only set the hidden field on the context consumers, not on all instances. The rationale is that only components that actually use the context should pay the price for it, not intermediate components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help to only track this on context consumers?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. (But ideally we shouldn't check if something is a consumer twice either.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we also shouldn't use __reactInternal[...] fields outside of FiberContext file. Not that it matters a lot but it’s hacky and I want to keep it contained.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I mentioned that as a hack in the PR description.

I could move the setting of that hidden field into a helper function inside of ReactFiberContext to at least keep the naming of the field there. Briefly considered doing that initially.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is #8723 an improvement?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 9, 2017

Yeah, I'm kind of cheating by caching the masked and unmasked context values on the receiver not the provider. It's not necessary for the provider to also be a receiver.

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

Successfully merging this pull request may close these issues.

5 participants