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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1165,6 +1165,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* reads context when setState is above the provider
* maintains the correct context when providers bail out due to low priority
* maintains the correct context when unwinding due to an error in render
* should not recreate masked context unless inputs have changed

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
* catches render error in a boundary during full deferred mounting
Expand Down
7 changes: 5 additions & 2 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var {
var ReactTypeOfWork = require('ReactTypeOfWork');
var {
getMaskedContext,
getUnmaskedContext,
hasContextChanged,
pushContextProvider,
pushTopLevelContextObject,
Expand Down Expand Up @@ -210,7 +211,8 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
return bailoutOnAlreadyFinishedWork(current, workInProgress);
}

var context = getMaskedContext(workInProgress);
var unmaskedContext = getUnmaskedContext(workInProgress);
var context = getMaskedContext(workInProgress, unmaskedContext);

var nextChildren;

Expand Down Expand Up @@ -427,7 +429,8 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
}
var fn = workInProgress.type;
var props = workInProgress.pendingProps;
var context = getMaskedContext(workInProgress);
var unmaskedContext = getUnmaskedContext(workInProgress);
var context = getMaskedContext(workInProgress, unmaskedContext);

var value;

Expand Down
22 changes: 17 additions & 5 deletions src/renderers/shared/fiber/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type { PriorityLevel } from 'ReactPriorityLevel';

var {
getMaskedContext,
getUnmaskedContext,
} = require('ReactFiberContext');
var {
addUpdate,
Expand Down Expand Up @@ -204,10 +205,17 @@ module.exports = function(
function constructClassInstance(workInProgress : Fiber) : any {
const ctor = workInProgress.type;
const props = workInProgress.pendingProps;
const context = getMaskedContext(workInProgress);
const unmaskedContext = getUnmaskedContext(workInProgress);
const context = getMaskedContext(workInProgress, unmaskedContext);
const instance = new ctor(props, context);
adoptClassInstance(workInProgress, instance);
checkClassInstance(workInProgress);

// 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?


return instance;
}

Expand All @@ -221,9 +229,11 @@ module.exports = function(
throw new Error('There must be pending props for an initial mount.');
}

const unmaskedContext = getUnmaskedContext(workInProgress);

instance.props = props;
instance.state = state;
instance.context = getMaskedContext(workInProgress);
instance.context = getMaskedContext(workInProgress, unmaskedContext);

if (typeof instance.componentWillMount === 'function') {
instance.componentWillMount();
Expand Down Expand Up @@ -256,7 +266,8 @@ module.exports = function(
throw new Error('There should always be pending or memoized props.');
}
}
const newContext = getMaskedContext(workInProgress);
const newUnmaskedContext = getUnmaskedContext(workInProgress);
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);

// TODO: Should we deal with a setState that happened after the last
// componentWillMount and before this componentWillMount? Probably
Expand All @@ -277,7 +288,7 @@ module.exports = function(
const newInstance = constructClassInstance(workInProgress);
newInstance.props = newProps;
newInstance.state = newState = newInstance.state || null;
newInstance.context = getMaskedContext(workInProgress);
newInstance.context = newContext;

if (typeof newInstance.componentWillMount === 'function') {
newInstance.componentWillMount();
Expand Down Expand Up @@ -314,7 +325,8 @@ module.exports = function(
}
}
const oldContext = instance.context;
const newContext = getMaskedContext(workInProgress);
const newUnmaskedContext = getUnmaskedContext(workInProgress);
const newContext = getMaskedContext(workInProgress, newUnmaskedContext);

// Note: During these life-cycles, instance.props/instance.state are what
// ever the previously attempted to render - not the "current". However,
Expand Down
21 changes: 19 additions & 2 deletions src/renderers/shared/fiber/ReactFiberContext.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,26 @@ function getUnmaskedContext(workInProgress : Fiber) : Object {
}
return contextStackCursor.current;
}
exports.getUnmaskedContext = getUnmaskedContext;

exports.getMaskedContext = function(workInProgress : Fiber) {
exports.getMaskedContext = function(workInProgress : Fiber, unmaskedContext : Object) {
const type = workInProgress.type;
const contextTypes = type.contextTypes;
if (!contextTypes) {
return emptyObject;
}

const unmaskedContext = getUnmaskedContext(workInProgress);
// Avoid recreating masked context unless unmasked context has changed.
// Failing to do this will result in unnecessary calls to componentWillReceiveProps.
// This may trigger infinite loops if componentWillReceiveProps calls setState.
const instance = workInProgress.stateNode;
if (
instance &&
instance.__reactInternalMemoizedUnmaskedChildContext === unmaskedContext
) {
return instance.__reactInternalMemoizedMaskedChildContext;
}

const context = {};
for (let key in contextTypes) {
context[key] = unmaskedContext[key];
Expand All @@ -74,6 +85,12 @@ exports.getMaskedContext = function(workInProgress : Fiber) {
checkReactTypeSpec(contextTypes, context, 'context', name, null, workInProgress);
}

// Cache unmasked context so we can avoid recreating masked context unless necessary.
if (instance) {
instance.__reactInternalMemoizedUnmaskedChildContext = unmaskedContext;
instance.__reactInternalMemoizedMaskedChildContext = context;
}

return context;
};

Expand Down
46 changes: 46 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2021,4 +2021,50 @@ describe('ReactIncremental', () => {
});
ReactNoop.flush();
});

it('should not recreate masked context unless inputs have changed', () => {
const ops = [];

let scuCounter = 0;

class MyComponent extends React.Component {
static contextTypes = {};
componentDidMount(prevProps, prevState) {
ops.push('componentDidMount');
this.setState({ setStateInCDU: true });
}
componentDidUpdate(prevProps, prevState) {
ops.push('componentDidUpdate');
if (this.state.setStateInCDU) {
this.setState({ setStateInCDU: false });
}
}
componentWillReceiveProps(nextProps) {
ops.push('componentWillReceiveProps');
this.setState({ setStateInCDU: true });
}
render() {
ops.push('render');
return null;
}
shouldComponentUpdate(nextProps, nextState) {
ops.push('shouldComponentUpdate');
return scuCounter++ < 5; // Don't let test hang
}
}

ReactNoop.render(<MyComponent />);
ReactNoop.flush();

expect(ops).toEqual([
'render',
'componentDidMount',
'shouldComponentUpdate',
'render',
'componentDidUpdate',
'shouldComponentUpdate',
'render',
'componentDidUpdate',
]);
});
});