Skip to content

Fix host bailout for the persistent mode #13611

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 10, 2018
Merged

Fix host bailout for the persistent mode #13611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe677c5
Add regression test for persistent bailout bug
gaearon Sep 10, 2018
4b897d7
Fork more logic into updateHostComponent
gaearon Sep 10, 2018
cfadb93
Add failing test for persistence host minimalism
gaearon Sep 10, 2018
ec384d0
Add bailouts to the persistent host updates
gaearon Sep 10, 2018
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
39 changes: 39 additions & 0 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,45 @@ describe('ReactFabric', () => {
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
});

it('recreates host parents even if only children changed', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {title: true},
uiViewClassName: 'RCTView',
}));

const before = 'abcdefghijklmnopqrst';
const after = 'mxhpgwfralkeoivcstzy';

class Component extends React.Component {
state = {
chars: before,
};
render() {
const chars = this.state.chars.split('');
return (
<View>{chars.map(text => <View key={text} title={text} />)}</View>
);
}
}

const ref = React.createRef();
// Wrap in a host node.
ReactFabric.render(
<View>
<Component ref={ref} />
</View>,
11,
);
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();

// Call setState() so that we skip over the top-level host node.
// It should still get recreated despite a bailout.
ref.current.setState({
chars: after,
});
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
});

it('calls setState with no arguments', () => {
let mockArgs;
class Component extends React.Component {
Expand Down
52 changes: 52 additions & 0 deletions packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,57 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`ReactFabric recreates host parents even if only children changed 1`] = `
"11
RCTView null
RCTView null
RCTView {\\"title\\":\\"a\\"}
RCTView {\\"title\\":\\"b\\"}
RCTView {\\"title\\":\\"c\\"}
RCTView {\\"title\\":\\"d\\"}
RCTView {\\"title\\":\\"e\\"}
RCTView {\\"title\\":\\"f\\"}
RCTView {\\"title\\":\\"g\\"}
RCTView {\\"title\\":\\"h\\"}
RCTView {\\"title\\":\\"i\\"}
RCTView {\\"title\\":\\"j\\"}
RCTView {\\"title\\":\\"k\\"}
RCTView {\\"title\\":\\"l\\"}
RCTView {\\"title\\":\\"m\\"}
RCTView {\\"title\\":\\"n\\"}
RCTView {\\"title\\":\\"o\\"}
RCTView {\\"title\\":\\"p\\"}
RCTView {\\"title\\":\\"q\\"}
RCTView {\\"title\\":\\"r\\"}
RCTView {\\"title\\":\\"s\\"}
RCTView {\\"title\\":\\"t\\"}"
`;

exports[`ReactFabric recreates host parents even if only children changed 2`] = `
"11
RCTView null
RCTView null
RCTView {\\"title\\":\\"m\\"}
RCTView {\\"title\\":\\"x\\"}
RCTView {\\"title\\":\\"h\\"}
RCTView {\\"title\\":\\"p\\"}
RCTView {\\"title\\":\\"g\\"}
RCTView {\\"title\\":\\"w\\"}
RCTView {\\"title\\":\\"f\\"}
RCTView {\\"title\\":\\"r\\"}
RCTView {\\"title\\":\\"a\\"}
RCTView {\\"title\\":\\"l\\"}
RCTView {\\"title\\":\\"k\\"}
RCTView {\\"title\\":\\"e\\"}
RCTView {\\"title\\":\\"o\\"}
RCTView {\\"title\\":\\"i\\"}
RCTView {\\"title\\":\\"v\\"}
RCTView {\\"title\\":\\"c\\"}
RCTView {\\"title\\":\\"s\\"}
RCTView {\\"title\\":\\"t\\"}
RCTView {\\"title\\":\\"z\\"}
RCTView {\\"title\\":\\"y\\"}"
`;

exports[`ReactFabric renders and reorders children 1`] = `
"11
RCTView null
Expand Down
30 changes: 22 additions & 8 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
let instanceCounter = 0;
let hostDiffCounter = 0;
let hostUpdateCounter = 0;
let hostCloneCounter = 0;

function appendChildToContainerOrInstance(
parentInstance: Container | Instance,
Expand Down Expand Up @@ -370,6 +371,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
value: clone.id,
enumerable: false,
});
hostCloneCounter++;
return clone;
},

Expand Down Expand Up @@ -579,21 +581,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

flushWithHostCounters(
fn: () => void,
): {|
hostDiffCounter: number,
hostUpdateCounter: number,
|} {
):
| {|
hostDiffCounter: number,
hostUpdateCounter: number,
|}
| {|
hostDiffCounter: number,
hostCloneCounter: number,
|} {
hostDiffCounter = 0;
hostUpdateCounter = 0;
hostCloneCounter = 0;
try {
ReactNoop.flush();
return {
hostDiffCounter,
hostUpdateCounter,
};
return useMutation
? {
hostDiffCounter,
hostUpdateCounter,
}
: {
hostDiffCounter,
hostCloneCounter,
};
} finally {
hostDiffCounter = 0;
hostUpdateCounter = 0;
hostCloneCounter = 0;
}
},

Expand Down
158 changes: 83 additions & 75 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@ import type {
Instance,
Type,
Props,
UpdatePayload,
Container,
ChildSet,
HostContext,
} from './ReactFiberHostConfig';

import {
Expand Down Expand Up @@ -126,13 +124,36 @@ if (supportsMutation) {
updateHostComponent = function(
current: Fiber,
workInProgress: Fiber,
updatePayload: null | UpdatePayload,
type: Type,
oldProps: Props,
newProps: Props,
rootContainerInstance: Container,
currentHostContext: HostContext,
) {
// If we have an alternate, that means this is an update and we need to
// schedule a side-effect to do the updates.
const oldProps = current.memoizedProps;
if (oldProps === newProps) {
// In mutation mode, this is sufficient for a bailout because
// we won't touch this node even if children changed.
return;
}

// If we get updated because one of our children updated, we don't
// have newProps so we'll have to reuse them.
// TODO: Split the update API as separate for the props vs. children.
// Even better would be if children weren't special cased at all tho.
const instance: Instance = workInProgress.stateNode;
const currentHostContext = getHostContext();
// TODO: Experiencing an error where oldProps is null. Suggests a host
// component is hitting the resume path. Figure out why. Possibly
// related to `hidden`.
const updatePayload = prepareUpdate(
instance,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);
// TODO: Type this specific to this type of component.
workInProgress.updateQueue = (updatePayload: any);
// If the update payload indicates that there is a change or if there
Expand Down Expand Up @@ -211,54 +232,70 @@ if (supportsMutation) {
updateHostComponent = function(
current: Fiber,
workInProgress: Fiber,
updatePayload: null | UpdatePayload,
type: Type,
oldProps: Props,
newProps: Props,
rootContainerInstance: Container,
currentHostContext: HostContext,
) {
const currentInstance = current.stateNode;
const oldProps = current.memoizedProps;
// If there are no effects associated with this node, then none of our children had any updates.
// This guarantees that we can reuse all of them.
const childrenUnchanged = workInProgress.firstEffect === null;
const currentInstance = current.stateNode;
if (childrenUnchanged && updatePayload === null) {
if (childrenUnchanged && oldProps === newProps) {
// No changes, just reuse the existing instance.
// Note that this might release a previous clone.
workInProgress.stateNode = currentInstance;
} else {
let recyclableInstance = workInProgress.stateNode;
let newInstance = cloneInstance(
currentInstance,
updatePayload,
return;
}
const recyclableInstance: Instance = workInProgress.stateNode;
const currentHostContext = getHostContext();
let updatePayload = null;
if (oldProps !== newProps) {
updatePayload = prepareUpdate(
recyclableInstance,
type,
oldProps,
newProps,
workInProgress,
childrenUnchanged,
recyclableInstance,
rootContainerInstance,
currentHostContext,
);
if (
finalizeInitialChildren(
newInstance,
type,
newProps,
rootContainerInstance,
currentHostContext,
)
) {
markUpdate(workInProgress);
}
workInProgress.stateNode = newInstance;
if (childrenUnchanged) {
// If there are no other effects in this tree, we need to flag this node as having one.
// Even though we're not going to use it for anything.
// Otherwise parents won't know that there are new children to propagate upwards.
markUpdate(workInProgress);
} else {
// If children might have changed, we have to add them all to the set.
appendAllChildren(newInstance, workInProgress);
}
}
if (childrenUnchanged && updatePayload === null) {
// No changes, just reuse the existing instance.
// Note that this might release a previous clone.
workInProgress.stateNode = currentInstance;
return;
}
let newInstance = cloneInstance(
currentInstance,
updatePayload,
type,
oldProps,
newProps,
workInProgress,
childrenUnchanged,
recyclableInstance,
);
if (
finalizeInitialChildren(
newInstance,
type,
newProps,
rootContainerInstance,
currentHostContext,
)
) {
markUpdate(workInProgress);
}
workInProgress.stateNode = newInstance;
if (childrenUnchanged) {
// If there are no other effects in this tree, we need to flag this node as having one.
// Even though we're not going to use it for anything.
// Otherwise parents won't know that there are new children to propagate upwards.
markUpdate(workInProgress);
} else {
// If children might have changed, we have to add them all to the set.
appendAllChildren(newInstance, workInProgress);
}
};
updateHostText = function(
Expand Down Expand Up @@ -290,12 +327,9 @@ if (supportsMutation) {
updateHostComponent = function(
current: Fiber,
workInProgress: Fiber,
updatePayload: null | UpdatePayload,
type: Type,
oldProps: Props,
newProps: Props,
rootContainerInstance: Container,
currentHostContext: HostContext,
) {
// Noop
};
Expand Down Expand Up @@ -358,39 +392,13 @@ function completeWork(
const rootContainerInstance = getRootHostContainer();
const type = workInProgress.type;
if (current !== null && workInProgress.stateNode != null) {
// If we have an alternate, that means this is an update and we need to
// schedule a side-effect to do the updates.
const oldProps = current.memoizedProps;
if (oldProps !== newProps) {
// If we get updated because one of our children updated, we don't
// have newProps so we'll have to reuse them.
// TODO: Split the update API as separate for the props vs. children.
// Even better would be if children weren't special cased at all tho.
const instance: Instance = workInProgress.stateNode;
const currentHostContext = getHostContext();
// TODO: Experiencing an error where oldProps is null. Suggests a host
// component is hitting the resume path. Figure out why. Possibly
// related to `hidden`.
const updatePayload = prepareUpdate(
instance,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);

updateHostComponent(
current,
workInProgress,
updatePayload,
type,
oldProps,
newProps,
rootContainerInstance,
currentHostContext,
);
}
updateHostComponent(
current,
workInProgress,
type,
newProps,
rootContainerInstance,
);

if (current.ref !== workInProgress.ref) {
markRef(workInProgress);
Expand Down
Loading