@samitouri / QOS-React / commits / c7f364d95b

Context providers and consumers should bailout on already finished work (#12254)

* Context providers and consumers should bail-out on already finished work Fixes bug where a consumer would re-render even if its props and context had not changed. * Encode output as JSON string * Add weights to random action generator * Add context to triangle fuzz tester * Move bailouts to as early as possible * Bailout if neither context value nor children haven't changed (sCU) * Change prop type invariant to a DEV-only warning

Andrew Clark committed Mar 12, 2018 at 13:39 UTC c7f364d95ba920657a10a71f23c0d43d71fa4845
3 files changed +367 -111
packages/react-reconciler/src/ReactFiberBeginWork.js
+82 -50
@@ -857,48 +857,70 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
857 pushProvider(workInProgress);
858 return bailoutOnAlreadyFinishedWork(current, workInProgress);
859 }
860 - workInProgress.memoizedProps = newProps;
860
861 const newValue = newProps.value;
862 + workInProgress.memoizedProps = newProps;
863
864 let changedBits: number;
865 if (oldProps === null) {
866 // Initial render
867 changedBits = MAX_SIGNED_31_BIT_INT;
868 } else {
869 - const oldValue = oldProps.value;
870 - // Use Object.is to compare the new context value to the old value.
871 - // Inlined Object.is polyfill.
872 - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
873 - if (
874 - (oldValue === newValue &&
875 - (oldValue !== 0 || 1 / oldValue === 1 / newValue)) ||
876 - (oldValue !== oldValue && newValue !== newValue) // eslint-disable-line no-self-compare
877 - ) {
878 - // No change.
869 + if (oldProps.value === newProps.value) {
870 + // No change. Bailout early if children are the same.
871 + if (oldProps.children === newProps.children) {
872 + workInProgress.stateNode = 0;
873 + pushProvider(workInProgress);
874 + return bailoutOnAlreadyFinishedWork(current, workInProgress);
875 + }
876 changedBits = 0;
877 } else {
881 - changedBits =
882 - typeof context.calculateChangedBits === 'function'
883 - ? context.calculateChangedBits(oldValue, newValue)
884 - : MAX_SIGNED_31_BIT_INT;
885 - if (__DEV__) {
886 - warning(
887 - (changedBits & MAX_SIGNED_31_BIT_INT) === changedBits,
888 - 'calculateChangedBits: Expected the return value to be a ' +
889 - '31-bit integer. Instead received: %s',
890 - changedBits,
891 - );
892 - }
893 - changedBits |= 0;
894 -
895 - if (changedBits !== 0) {
896 - propagateContextChange(
897 - workInProgress,
898 - context,
899 - changedBits,
900 - renderExpirationTime,
901 - );
878 + const oldValue = oldProps.value;
879 + // Use Object.is to compare the new context value to the old value.
880 + // Inlined Object.is polyfill.
881 + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
882 + if (
883 + (oldValue === newValue &&
884 + (oldValue !== 0 || 1 / oldValue === 1 / newValue)) ||
885 + (oldValue !== oldValue && newValue !== newValue) // eslint-disable-line no-self-compare
886 + ) {
887 + // No change. Bailout early if children are the same.
888 + if (oldProps.children === newProps.children) {
889 + workInProgress.stateNode = 0;
890 + pushProvider(workInProgress);
891 + return bailoutOnAlreadyFinishedWork(current, workInProgress);
892 + }
893 + changedBits = 0;
894 + } else {
895 + changedBits =
896 + typeof context.calculateChangedBits === 'function'
897 + ? context.calculateChangedBits(oldValue, newValue)
898 + : MAX_SIGNED_31_BIT_INT;
899 + if (__DEV__) {
900 + warning(
901 + (changedBits & MAX_SIGNED_31_BIT_INT) === changedBits,
902 + 'calculateChangedBits: Expected the return value to be a ' +
903 + '31-bit integer. Instead received: %s',
904 + changedBits,
905 + );
906 + }
907 + changedBits |= 0;
908 +
909 + if (changedBits === 0) {
910 + // No change. Bailout early if children are the same.
911 + if (oldProps.children === newProps.children) {
912 + workInProgress.stateNode = 0;
913 + pushProvider(workInProgress);
914 + return bailoutOnAlreadyFinishedWork(current, workInProgress);
915 + }
916 + } else {
917 + propagateContextChange(
918 + workInProgress,
919 + context,
920 + changedBits,
921 + renderExpirationTime,
922 + );
923 + }
924 }
925 }
926 }
@@ -906,9 +928,6 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
928 workInProgress.stateNode = changedBits;
929 pushProvider(workInProgress);
930
909 - if (oldProps !== null && oldProps.children === newProps.children) {
910 - return bailoutOnAlreadyFinishedWork(current, workInProgress);
911 - }
931 const newChildren = newProps.children;
932 reconcileChildren(current, workInProgress, newChildren);
933 return workInProgress.child;
@@ -921,11 +940,28 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
940 ) {
941 const context: ReactContext<any> = workInProgress.type;
942 const newProps = workInProgress.pendingProps;
943 + const oldProps = workInProgress.memoizedProps;
944
945 const newValue = context.currentValue;
946 const changedBits = context.changedBits;
947
928 - if (changedBits !== 0) {
948 + if (hasLegacyContextChanged()) {
949 + // Normally we can bail out on props equality but if context has changed
950 + // we don't do the bailout and we have to reuse existing props instead.
951 + } else if (changedBits === 0 && oldProps === newProps) {
952 + return bailoutOnAlreadyFinishedWork(current, workInProgress);
953 + }
954 + workInProgress.memoizedProps = newProps;
955 +
956 + let observedBits = newProps.observedBits;
957 + if (observedBits === undefined || observedBits === null) {
958 + // Subscribe to all changes by default
959 + observedBits = MAX_SIGNED_31_BIT_INT;
960 + }
961 + // Store the observedBits on the fiber's stateNode for quick access.
962 + workInProgress.stateNode = observedBits;
963 +
964 + if ((changedBits & observedBits) !== 0) {
965 // Context change propagation stops at matching consumers, for time-
966 // slicing. Continue the propagation here.
967 propagateContextChange(
@@ -934,24 +970,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
970 changedBits,
971 renderExpirationTime,
972 );
973 + } else if (oldProps !== null && oldProps.children === newProps.children) {
974 + // No change. Bailout early if children are the same.
975 + return bailoutOnAlreadyFinishedWork(current, workInProgress);
976 }
977
939 - // Store the observedBits on the fiber's stateNode for quick access.
940 - let observedBits = newProps.observedBits;
941 - if (observedBits === undefined || observedBits === null) {
942 - // Subscribe to all changes by default
943 - observedBits = MAX_SIGNED_31_BIT_INT;
944 - }
945 - workInProgress.stateNode = observedBits;
946 -
978 const render = newProps.children;
979
949 - if (typeof render !== 'function') {
950 - invariant(
951 - false,
952 - "A context consumer was rendered with multiple children, or a child that isn't a function. " +
953 - 'A context consumer expects a single child that is a function. ' +
954 - 'If you did pass a function, make sure there is no trailing or leading whitespace around it.',
980 + if (__DEV__) {
981 + warning(
982 + typeof render === 'function',
983 + 'A context consumer was rendered with multiple children, or a child ' +
984 + "that isn't a function. A context consumer expects a single child " +
985 + 'that is a function. If you did pass a function, make sure there ' +
986 + 'is no trailing or leading whitespace around it.',
987 );
988 }
989
packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js
+167 -61
@@ -35,6 +35,13 @@ describe('ReactIncrementalTriangle', () => {
35 };
36 }
37
38 + const FLUSH_ALL = 'FLUSH_ALL';
39 + function flushAll() {
40 + return {
41 + type: FLUSH_ALL,
42 + };
43 + }
44 +
45 const STEP = 'STEP';
46 function step(counter) {
47 return {
@@ -78,6 +85,8 @@ describe('ReactIncrementalTriangle', () => {
85 switch (action.type) {
86 case FLUSH:
87 return `flush(${action.unitsOfWork})`;
88 + case FLUSH_ALL:
89 + return 'flushAll()';
90 case STEP:
91 return `step(${action.counter})`;
92 case INTERRUPT:
@@ -112,16 +121,43 @@ describe('ReactIncrementalTriangle', () => {
121 }
122
123 function randomAction() {
115 - switch (randomInteger(0, 5)) {
116 - case 0:
124 + const weights = [
125 + [FLUSH, 1],
126 + [FLUSH_ALL, 1],
127 + [STEP, 1],
128 + [INTERRUPT, 1],
129 + [TOGGLE, 1],
130 + [EXPIRE, 1],
131 + ];
132 + let totalWeight = 0;
133 + for (let i = 0; i < weights.length; i++) {
134 + totalWeight += weights[i][1];
135 + }
136 +
137 + const randomNumber = Math.random() * totalWeight;
138 + let actionType;
139 + let remainingWeight = randomNumber;
140 + for (let i = 0; i < weights.length; i++) {
141 + const [option, weight] = weights[i];
142 + remainingWeight -= weight;
143 + if (remainingWeight <= 0) {
144 + actionType = option;
145 + break;
146 + }
147 + }
148 +
149 + switch (actionType) {
150 + case FLUSH:
151 return flush(randomInteger(0, TOTAL_TRIANGLES * 1.5));
118 - case 1:
152 + case FLUSH_ALL:
153 + return flushAll();
154 + case STEP:
155 return step(randomInteger(0, 10));
120 - case 2:
156 + case INTERRUPT:
157 return interrupt();
122 - case 3:
123 - return toggle(randomInteger(0, TOTAL_CHILDREN));
124 - case 4:
158 + case TOGGLE:
159 + return toggle(randomInteger(0, TOTAL_TRIANGLES));
160 + case EXPIRE:
161 return expire(randomInteger(0, 1500));
162 default:
163 throw new Error('Switch statement should be exhaustive');
@@ -137,6 +173,9 @@ describe('ReactIncrementalTriangle', () => {
173 }
174
175 function TriangleSimulator(rootID) {
176 + const CounterContext = React.createContext([]);
177 + const ActiveContext = React.createContext(0);
178 +
179 let triangles = [];
180 let leafTriangles = [];
181 let yieldAfterEachRender = false;
@@ -145,27 +184,22 @@ describe('ReactIncrementalTriangle', () => {
184 super();
185 this.index = triangles.length;
186 triangles.push(this);
148 - if (props.depth === 0) {
187 + if (props.remainingDepth === 0) {
188 this.leafIndex = leafTriangles.length;
189 leafTriangles.push(this);
190 }
191 this.state = {isActive: false};
192 }
193 activate() {
155 - if (this.props.depth !== 0) {
156 - throw new Error('Cannot activate non-leaf component');
157 - }
194 this.setState({isActive: true});
195 }
196 deactivate() {
161 - if (this.props.depth !== 0) {
162 - throw new Error('Cannot deactivate non-leaf component');
163 - }
197 this.setState({isActive: false});
198 }
199 shouldComponentUpdate(nextProps, nextState) {
200 return (
201 this.props.counter !== nextProps.counter ||
202 + this.props.activeDepth !== nextProps.activeDepth ||
203 this.state.isActive !== nextState.isActive
204 );
205 }
@@ -173,18 +207,56 @@ describe('ReactIncrementalTriangle', () => {
207 if (yieldAfterEachRender) {
208 ReactNoop.yield(this);
209 }
176 - const {counter, depth} = this.props;
177 - if (depth === 0) {
178 - if (this.state.isActive) {
179 - return <span prop={'*' + counter + '*'} />;
180 - }
181 - return <span prop={counter} />;
182 - }
183 - return [
184 - <Triangle key={1} counter={counter} depth={depth - 1} />,
185 - <Triangle key={2} counter={counter} depth={depth - 1} />,
186 - <Triangle key={3} counter={counter} depth={depth - 1} />,
187 - ];
210 + const {counter, remainingDepth} = this.props;
211 + return (
212 + <ActiveContext.Consumer>
213 + {activeContext => (
214 + <CounterContext.Consumer>
215 + {counterContext => {
216 + const activeDepthProp = this.state.isActive
217 + ? this.props.activeDepth + 1
218 + : this.props.activeDepth;
219 + const activeDepthContext = this.state.isActive
220 + ? activeContext + 1
221 + : activeContext;
222 + if (remainingDepth === 0) {
223 + // Leaf
224 + const output = JSON.stringify({
225 + prop: counter,
226 + isActive: this.state.isActive,
227 + counterContext: counterContext,
228 + activeDepthProp,
229 + activeDepthContext,
230 + });
231 + return <span prop={output} />;
232 + }
233 +
234 + return (
235 + <ActiveContext.Provider value={activeDepthContext}>
236 + <CounterContext.Provider value={counter}>
237 + <Triangle
238 + counter={counter}
239 + activeDepth={activeDepthProp}
240 + remainingDepth={remainingDepth - 1}
241 + />
242 + <Triangle
243 + counter={counter}
244 + activeDepth={activeDepthProp}
245 + remainingDepth={remainingDepth - 1}
246 + />
247 + <Triangle
248 + counter={counter}
249 + activeDepth={activeDepthProp}
250 + remainingDepth={remainingDepth - 1}
251 + />
252 + </CounterContext.Provider>
253 + </ActiveContext.Provider>
254 + );
255 + }}
256 + </CounterContext.Consumer>
257 + )}
258 + </ActiveContext.Consumer>
259 + );
260 }
261 }
262
@@ -202,7 +274,13 @@ describe('ReactIncrementalTriangle', () => {
274 }
275 render() {
276 appInstance = this;
205 - return <Triangle counter={this.state.counter} depth={3} />;
277 + return (
278 + <Triangle
279 + counter={this.state.counter}
280 + activeDepth={0}
281 + remainingDepth={this.props.remainingDepth}
282 + />
283 + );
284 }
285 }
286
@@ -213,11 +291,11 @@ describe('ReactIncrementalTriangle', () => {
291 // Remounts the whole tree by changing the key
292 if (rootID) {
293 ReactNoop.renderToRootWithID(
216 - <App depth={MAX_DEPTH} key={keyCounter++} />,
294 + <App remainingDepth={MAX_DEPTH} key={keyCounter++} />,
295 rootID,
296 );
297 } else {
220 - ReactNoop.render(<App depth={MAX_DEPTH} key={keyCounter++} />);
298 + ReactNoop.render(<App remainingDepth={MAX_DEPTH} key={keyCounter++} />);
299 }
300 ReactNoop.flush();
301 assertConsistentTree();
@@ -226,41 +304,59 @@ describe('ReactIncrementalTriangle', () => {
304
305 reset();
306
229 - function assertConsistentTree(activeTriangle, counter) {
230 - const activeIndex = activeTriangle ? activeTriangle.leafIndex : -1;
231 -
307 + function assertConsistentTree(activeTriangleIndices = new Set(), counter) {
308 const children = ReactNoop.getChildren(rootID);
309
310 if (children.length !== TOTAL_CHILDREN) {
311 throw new Error('Wrong number of children.');
312 }
313
314 + let expectedCounter = counter;
315 +
316 for (let i = 0; i < children.length; i++) {
317 let child = children[i];
240 - let num = child.prop;
318 +
319 + const output = JSON.parse(child.prop);
320 + const prop = output.prop;
321 + const isActive = output.isActive;
322 + const counterContext = output.counterContext;
323 + const activeDepthProp = output.activeDepthProp;
324 + const activeDepthContext = output.activeDepthContext;
325
326 // If an expected counter is not specified, use the value of the
327 // first child.
244 - if (counter === undefined) {
245 - if (typeof num === 'string') {
246 - counter = parseInt(num.substr(1, num.length - 2), 10);
247 - } else {
248 - counter = num;
249 - }
328 + if (expectedCounter === undefined) {
329 + expectedCounter = prop;
330 }
331 + const expectedIsActive = activeTriangleIndices.has(i);
332
252 - if (i === activeIndex) {
253 - if (num !== `*${counter}*`) {
254 - throw new Error(
255 - `Triangle ${i} is inconsistent: ${num} instead of *${counter}*.`,
256 - );
257 - }
258 - } else {
259 - if (num !== counter) {
260 - throw new Error(
261 - `Triangle ${i} is inconsistent: ${num} instead of ${counter}.`,
262 - );
263 - }
333 + if (prop !== expectedCounter) {
334 + throw new Error(
335 + `Triangle ${i} is inconsistent: prop ${prop} instead of ` +
336 + expectedCounter,
337 + );
338 + }
339 +
340 + if (isActive !== expectedIsActive) {
341 + throw new Error(
342 + `Triangle ${i} is inconsistent: isActive ${isActive} instead of ` +
343 + expectedIsActive,
344 + );
345 + }
346 +
347 + if (counterContext !== prop) {
348 + throw new Error(
349 + `Triangle ${i} is inconsistent: prop ${prop} does not match ` +
350 + `counterContext ${counterContext}`,
351 + );
352 + }
353 +
354 + if (activeDepthContext !== activeDepthProp) {
355 + throw new Error(
356 + `Triangle ${i} is inconsistent: activeDepthProp ` +
357 + `${activeDepthProp} does not match activeDepthContext ` +
358 + activeDepthContext,
359 + );
360 }
361 }
362 }
@@ -269,7 +365,8 @@ describe('ReactIncrementalTriangle', () => {
365 const app = reset();
366 let expectedCounterAtEnd = app.state.counter;
367
272 - let activeTriangle = null;
368 + let activeIndices = new Set();
369 + let activeLeafIndices = new Set();
370 let action;
371 while (true) {
372 action = yield;
@@ -281,6 +378,9 @@ describe('ReactIncrementalTriangle', () => {
378 case FLUSH:
379 ReactNoop.flushUnitsOfWork(action.unitsOfWork);
380 break;
381 + case FLUSH_ALL:
382 + ReactNoop.flush();
383 + break;
384 case STEP:
385 ReactNoop.deferredUpdates(() => {
386 app.setCounter(action.counter);
@@ -291,18 +391,23 @@ describe('ReactIncrementalTriangle', () => {
391 app.interrupt();
392 break;
393 case TOGGLE:
294 - const targetTriangle = leafTriangles[action.childIndex];
394 + const targetTriangle = triangles[action.childIndex];
395 if (targetTriangle === undefined) {
396 throw new Error('Target index is out of bounds');
397 }
298 - if (targetTriangle === activeTriangle) {
299 - activeTriangle = null;
398 + const index = targetTriangle.index;
399 + const leafIndex = targetTriangle.leafIndex;
400 + if (activeIndices.has(index)) {
401 + activeIndices.delete(index);
402 + if (leafIndex !== undefined) {
403 + activeLeafIndices.delete(leafIndex);
404 + }
405 targetTriangle.deactivate();
406 } else {
302 - if (activeTriangle !== null) {
303 - activeTriangle.deactivate();
407 + activeIndices.add(index);
408 + if (leafIndex !== undefined) {
409 + activeLeafIndices.add(leafIndex);
410 }
305 - activeTriangle = targetTriangle;
411 targetTriangle.activate();
412 }
413 break;
@@ -310,14 +415,14 @@ describe('ReactIncrementalTriangle', () => {
415 ReactNoop.expire(action.ms);
416 break;
417 default:
313 - break;
418 + throw new Error('Switch statement should be exhaustive');
419 }
420 });
316 - assertConsistentTree(activeTriangle);
421 + assertConsistentTree(activeLeafIndices);
422 }
423 // Flush remaining work
424 ReactNoop.flush();
320 - assertConsistentTree(activeTriangle, expectedCounterAtEnd);
425 + assertConsistentTree(activeLeafIndices, expectedCounterAtEnd);
426 }
427
428 function simulate(...actions) {
@@ -352,6 +457,7 @@ describe('ReactIncrementalTriangle', () => {
457 simulate(step(4), flush(52), expire(1476), flush(17), step(0));
458 simulate(interrupt(), toggle(10), step(2), expire(990), flush(46));
459 simulate(interrupt(), step(6), step(7), toggle(6), interrupt());
460 + simulate(interrupt(), toggle(31), toggle(31), toggle(13), step(1));
461 });
462
463 it('generative tests', () => {
packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
+118
@@ -644,6 +644,124 @@ describe('ReactNewContext', () => {
644 }
645 });
646
647 + it('warns if consumer child is not a function', () => {
648 + spyOnDev(console, 'error');
649 + const Context = React.createContext(0);
650 + ReactNoop.render(<Context.Consumer />);
651 + expect(ReactNoop.flush).toThrow('render is not a function');
652 + if (__DEV__) {
653 + expect(console.error.calls.argsFor(0)[0]).toContain(
654 + 'A context consumer was rendered with multiple children, or a child ' +
655 + "that isn't a function",
656 + );
657 + }
658 + });
659 +
660 + it("does not re-render if there's an update in a child", () => {
661 + const Context = React.createContext(0);
662 +
663 + let child;
664 + class Child extends React.Component {
665 + state = {step: 0};
666 + render() {
667 + ReactNoop.yield('Child');
668 + return (
669 + <span
670 + prop={`Context: ${this.props.context}, Step: ${this.state.step}`}
671 + />
672 + );
673 + }
674 + }
675 +
676 + function App(props) {
677 + return (
678 + <Context.Provider value={props.value}>
679 + <Context.Consumer>
680 + {value => {
681 + ReactNoop.yield('Consumer render prop');
682 + return <Child ref={inst => (child = inst)} context={value} />;
683 + }}
684 + </Context.Consumer>
685 + </Context.Provider>
686 + );
687 + }
688 +
689 + // Initial mount
690 + ReactNoop.render(<App value={1} />);
691 + expect(ReactNoop.flush()).toEqual(['Consumer render prop', 'Child']);
692 + expect(ReactNoop.getChildren()).toEqual([span('Context: 1, Step: 0')]);
693 +
694 + child.setState({step: 1});
695 + expect(ReactNoop.flush()).toEqual(['Child']);
696 + expect(ReactNoop.getChildren()).toEqual([span('Context: 1, Step: 1')]);
697 + });
698 +
699 + it('provider bails out if children and value are unchanged (like sCU)', () => {
700 + const Context = React.createContext(0);
701 +
702 + function Child() {
703 + ReactNoop.yield('Child');
704 + return <span prop="Child" />;
705 + }
706 +
707 + const children = <Child />;
708 +
709 + function App(props) {
710 + ReactNoop.yield('App');
711 + return (
712 + <Context.Provider value={props.value}>{children}</Context.Provider>
713 + );
714 + }
715 +
716 + // Initial mount
717 + ReactNoop.render(<App value={1} />);
718 + expect(ReactNoop.flush()).toEqual(['App', 'Child']);
719 + expect(ReactNoop.getChildren()).toEqual([span('Child')]);
720 +
721 + // Update
722 + ReactNoop.render(<App value={1} />);
723 + expect(ReactNoop.flush()).toEqual([
724 + 'App',
725 + // Child does not re-render
726 + ]);
727 + expect(ReactNoop.getChildren()).toEqual([span('Child')]);
728 + });
729 +
730 + it('consumer bails out if children and value are unchanged (like sCU)', () => {
731 + const Context = React.createContext(0);
732 +
733 + function Child() {
734 + ReactNoop.yield('Child');
735 + return <span prop="Child" />;
736 + }
737 +
738 + function renderConsumer(context) {
739 + return <Child context={context} />;
740 + }
741 +
742 + function App(props) {
743 + ReactNoop.yield('App');
744 + return (
745 + <Context.Provider value={props.value}>
746 + <Context.Consumer>{renderConsumer}</Context.Consumer>
747 + </Context.Provider>
748 + );
749 + }
750 +
751 + // Initial mount
752 + ReactNoop.render(<App value={1} />);
753 + expect(ReactNoop.flush()).toEqual(['App', 'Child']);
754 + expect(ReactNoop.getChildren()).toEqual([span('Child')]);
755 +
756 + // Update
757 + ReactNoop.render(<App value={1} />);
758 + expect(ReactNoop.flush()).toEqual([
759 + 'App',
760 + // Child does not re-render
761 + ]);
762 + expect(ReactNoop.getChildren()).toEqual([span('Child')]);
763 + });
764 +
765 describe('fuzz test', () => {
766 const Fragment = React.Fragment;
767 const contextKeys = ['A', 'B', 'C', 'D', 'E', 'F', 'G'];