Skip to content

Commit cb2bc0d

Browse files
committed
Deprioritize setState within a deprioritized tree
Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority. This ensures that we don't let a deprioritized subtree get reconciled at a higher priority.
1 parent 6d8cd15 commit cb2bc0d

File tree

2 files changed

+127
-0
lines changed

2 files changed

+127
-0
lines changed

src/renderers/shared/fiber/ReactFiberBeginWork.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,26 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, s
309309
function bailoutOnAlreadyFinishedWork(current, workInProgress : Fiber) : ?Fiber {
310310
const priorityLevel = workInProgress.pendingWorkPriority;
311311

312+
if (workInProgress.tag === HostComponent &&
313+
workInProgress.memoizedProps.hidden &&
314+
workInProgress.pendingWorkPriority !== OffscreenPriority) {
315+
// This subtree still has work, but it should be deprioritized so we need
316+
// to bail out and not do any work yet.
317+
// TODO: It would be better if this tree got its correct priority set
318+
// during scheduleUpdate instead because otherwise we'll start a higher
319+
// priority reconciliation first before we can get down here. However,
320+
// that is a bit tricky since workInProgress and current can have
321+
// different "hidden" settings.
322+
let child = workInProgress.progressedChild;
323+
while (child) {
324+
// To ensure that this subtree gets its priority reset, the children
325+
// need to be reset.
326+
child.pendingWorkPriority = OffscreenPriority;
327+
child = child.sibling;
328+
}
329+
return null;
330+
}
331+
312332
// TODO: We should ideally be able to bail out early if the children have no
313333
// more work to do. However, since we don't have a separation of this
314334
// Fiber's priority and its children yet - we don't know without doing lots

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,113 @@ describe('ReactIncrementalSideEffects', () => {
601601
expect(ops).toEqual(['Baz', 'Bar', 'Baz', 'Bar', 'Bar']);
602602
});
603603

604+
it('deprioritizes setStates that happens within a deprioritized tree', () => {
605+
var ops = [];
606+
607+
var barInstances = [];
608+
609+
class Bar extends React.Component {
610+
constructor() {
611+
super();
612+
this.state = { active: false };
613+
barInstances.push(this);
614+
}
615+
activate() {
616+
this.setState({ active: true });
617+
}
618+
render() {
619+
ops.push('Bar');
620+
return <span prop={this.state.active ? 'X' : this.props.idx} />;
621+
}
622+
}
623+
function Foo(props) {
624+
ops.push('Foo');
625+
return (
626+
<div>
627+
<span prop={props.tick} />
628+
<div hidden={true}>
629+
<Bar idx={props.idx} />
630+
<Bar idx={props.idx} />
631+
<Bar idx={props.idx} />
632+
</div>
633+
</div>
634+
);
635+
}
636+
ReactNoop.render(<Foo tick={0} idx={0} />);
637+
ReactNoop.flush();
638+
expect(ReactNoop.root.children).toEqual([
639+
div(
640+
span(0),
641+
div(
642+
span(0),
643+
span(0),
644+
span(0)
645+
)
646+
),
647+
]);
648+
649+
expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']);
650+
651+
ops = [];
652+
653+
ReactNoop.render(<Foo tick={1} idx={0} />);
654+
ReactNoop.flushDeferredPri(70);
655+
expect(ReactNoop.root.children).toEqual([
656+
div(
657+
// Updated.
658+
span(1),
659+
div(
660+
// Still not updated.
661+
span(0),
662+
span(0),
663+
span(0)
664+
)
665+
),
666+
]);
667+
668+
expect(ops).toEqual(['Foo', 'Bar', 'Bar']);
669+
ops = [];
670+
671+
barInstances[0].activate();
672+
673+
// This should not be enough time to render the content of all the hidden
674+
// items. Including the set state since that is deprioritized.
675+
// TODO: The cycles it takes to do this could be lowered with further
676+
// optimizations.
677+
ReactNoop.flushDeferredPri(60);
678+
expect(ReactNoop.root.children).toEqual([
679+
div(
680+
// Updated.
681+
span(1),
682+
div(
683+
// Still not updated.
684+
span(0),
685+
span(0),
686+
span(0)
687+
)
688+
),
689+
]);
690+
691+
expect(ops).toEqual(['Bar']);
692+
ops = [];
693+
694+
// However, once we render fully, we will have enough time to finish it all
695+
// at once.
696+
ReactNoop.flush();
697+
expect(ReactNoop.root.children).toEqual([
698+
div(
699+
span(1),
700+
div(
701+
// Now we had enough time to finish the spans.
702+
span('X'),
703+
span(0),
704+
span(0),
705+
)
706+
),
707+
]);
708+
709+
expect(ops).toEqual(['Bar', 'Bar']);
710+
});
604711
// TODO: Test that side-effects are not cut off when a work in progress node
605712
// moves to "current" without flushing due to having lower priority. Does this
606713
// even happen? Maybe a child doesn't get processed because it is lower prio?

0 commit comments

Comments
 (0)