Improve unmasked context caching

[bvaughn]

• Only store cached masked/unmasked contexts on context consumers.
• Move all references to __reactInternal* cached attributes inside of ReactFiberContext.

Related to this discussion thread.

[gaearon]

We already doing an equivalent check in getMaskedContext which is called just before cacheContext.
Can we avoid doing it twice?

[bvaughn]

The thing that makes this tricky is that instance won't always exist yet when getMaskedContext is called. That's the whole reason the separate function exists at all.

What would you suggest?

[gaearon]

Right, but we don’t need the instance to tell if it’s a context consumer or not.
Or do we?

I thought that maybe we can make getMaskedContext return null for non-context consumers. Then we’d know for sure that we don’t need to call cacheContext.

Not saying it’s the best way, just a proposal.

[bvaughn]

You are correct. We do not need instance for that. I just mentioned that we need to do some kind of duplicate work since instance won't always exist.

getMaskedContext returns emptyObject for non-consumers. We could compare it? eg

function cacheContext(workInProgress : Fiber, unmaskedContext : Object, maskedContext : Object) {
  if (maskedContext !== emptyObject) {
    const instance = workInProgress.stateNode;

    if (instance) {
      instance.__reactInternalMemoizedUnmaskedChildContext = unmaskedContext;
      instance.__reactInternalMemoizedMaskedChildContext = maskedContext;
    }
  }
}

[gaearon]

We could but it seems a bit fishy to make one perf optimization to rely on another perf optimization being present in another file. Returning null since more explicit if we want to "signal" that it's not a context consumer.

[gaearon]

Alternatively we could call isContextConsumer from outside in the first place.

[bvaughn]

I assume that getMaskedContext returns emptyObject instead of null for a reason.

We could but it seems a bit fishy to make one perf optimization to rely on another perf optimization being present in another file.

This all inside of ReactFiberContext. What's the other file?

[bvaughn]

Ah! I think I understand.

[gaearon]

I assume that getMaskedContext returns emptyObject instead of null for a reason.

Because we want to pass an empty object to components. But we could do || emptyObject this here (and similar places) instead.

This all inside of ReactFiberContext. What's the other file?

I am talking about these lines in ClassComponent specifically:

// This will check if it's a context consumer:
const context = getMaskedContext(workInProgress, unmaskedContext);
// ...
// This will do the same check:
cacheContext(workInProgress, unmaskedContext, context);

I was proposing to do something like

// Check once:
const needsContext = isContextConsumer(workInProgress);

const context = needsContext ? getMaskedContext(workInProgress, unmaskedContext) : emptyObject;
// ...
if (needsContext) {
  cacheContext(workInProgress, unmaskedContext, context);
}

Then you could remove checks in both getMaskedContext an cacheContext.

[bvaughn]

Yup. Thanks for clarifying. 😄 Updated.

[gaearon]

Can we move this check in getMaskedContext instead?
The other caller of cacheContext always has the instance.

[bvaughn]

Sure, we could do that. I didn't do it initially because it seemed unsafe. Fiber may not have an instance and I didn't wan to rely on external callers of this function having to know only to call this method when an instance existed.

[bvaughn]

PS. I'm happy to defer to your judgement here. You're more familiar with the project and the way things are done. Just sharing my thought process. 😄

[gaearon]

Generally I'm of the opinion that methods should be strict and either have a safety check inside (and we never do it outside) or have no safety check, and we must always do it outside.

Doing it both ways means somebody will see the "early check" pattern that was made for performance reasons and scratch their head why other calls aren't behind the same check. This will look "surprising" if one call is different from other calls in the same file. And every difference like this is mental overhead when you have to move those pieces around. Strict contracts help avoid that, and we can always fix safety concerns with tests for regressions.

[bvaughn]

Okay. I understand your thoughts now. Thank you for elaborating.

[gaearon]

Can we remove this check here now, and always do it externally?

[bvaughn]

We could. Same safety-related concern as above.

Edit: I think I'm going to leave this as-is since we call getMaskedContext in half a dozen places and it seems to easy to mess up if we require all callers to check isContextConsumer before calling.

[gaearon]

👍

[gaearon]

Looks good to me. I don't have a strong preference for the above comments, please do as it makes more sense to you.