Skip to content

[Fiber] Introduce API to opt-out of batching #8661

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
acdlite merged 2 commits into from
Jan 3, 2017
Merged

[Fiber] Introduce API to opt-out of batching #8661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b79531e
No nested updates
acdlite Dec 30, 2016
ad7bcdf
Use unbatchedUpdates to opt-out of batching
acdlite Dec 30, 2016
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
3 changes: 2 additions & 1 deletion scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,7 +1207,8 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
* can opt-in to deferred/animation scheduling inside componentDidMount/Update
* performs Task work even after time runs out
* does not perform animation work after time runs out
* can force synchronous updates with syncUpdates, even inside batchedUpdates
* can opt-out of batching using unbatchedUpdates
* nested updates are always deferred, even inside unbatchedUpdates

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
* can update child nodes of a host instance
Expand Down
8 changes: 4 additions & 4 deletions src/renderers/dom/fiber/ReactDOMFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,8 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any,
}
const newRoot = DOMRenderer.createContainer(container);
root = container._reactRootContainer = newRoot;
// Initial mount is always sync, even if we're in a batch.
DOMRenderer.syncUpdates(() => {
// Initial mount should not be batched.
DOMRenderer.unbatchedUpdates(() => {
DOMRenderer.updateContainer(children, newRoot, parentComponent, callback);
});
} else {
Expand All @@ -354,8 +354,8 @@ var ReactDOM = {
unmountComponentAtNode(container : DOMContainerElement) {
warnAboutUnstableUse();
if (container._reactRootContainer) {
// Unmount is always sync, even if we're in a batch.
return DOMRenderer.syncUpdates(() => {
// Unmount should not be batched.
return DOMRenderer.unbatchedUpdates(() => {
return renderSubtreeIntoContainer(null, null, container, () => {
container._reactRootContainer = null;
});
Expand Down
2 changes: 2 additions & 0 deletions src/renderers/noop/ReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ var ReactNoop = {

batchedUpdates: NoopRenderer.batchedUpdates,

unbatchedUpdates: NoopRenderer.unbatchedUpdates,

syncUpdates: NoopRenderer.syncUpdates,

// Logs the current state of the tree.
Expand Down
15 changes: 0 additions & 15 deletions src/renderers/shared/__tests__/ReactPerf-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ describe('ReactPerf', () => {
var ReactDOM;
var ReactPerf;
var ReactTestUtils;
var ReactDOMFeatureFlags;
var emptyFunction;

var App;
Expand All @@ -39,7 +38,6 @@ describe('ReactPerf', () => {
ReactDOM = require('ReactDOM');
ReactPerf = require('ReactPerf');
ReactTestUtils = require('ReactTestUtils');
ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
emptyFunction = require('emptyFunction');

App = class extends React.Component {
Expand Down Expand Up @@ -647,10 +645,6 @@ describe('ReactPerf', () => {
var container = document.createElement('div');
var thrownErr = new Error('Muhaha!');

if (ReactDOMFeatureFlags.useFiber) {
spyOn(console, 'error');
}

class Evil extends React.Component {
componentWillMount() {
throw thrownErr;
Expand Down Expand Up @@ -685,15 +679,6 @@ describe('ReactPerf', () => {
}
ReactDOM.unmountComponentAtNode(container);
ReactPerf.stop();

if (ReactDOMFeatureFlags.useFiber) {
// A sync `render` inside cWM will print a warning. That should be the
// only warning.
expect(console.error.calls.count()).toEqual(1);
expect(console.error.calls.argsFor(0)[0]).toMatch(
/Render methods should be a pure function of props and state/
);
}
});

it('should not print errant warnings if portal throws in componentDidMount()', () => {
Expand Down
4 changes: 4 additions & 0 deletions src/renderers/shared/fiber/ReactFiberReconciler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export type Reconciler<C, I, TI> = {
/* eslint-disable no-undef */
// FIXME: ESLint complains about type parameter
batchedUpdates<A>(fn : () => A) : A,
unbatchedUpdates<A>(fn : () => A) : A,
syncUpdates<A>(fn : () => A) : A,
deferredUpdates<A>(fn : () => A) : A,
/* eslint-enable no-undef */
Expand All @@ -107,6 +108,7 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
getPriorityContext,
performWithPriority,
batchedUpdates,
unbatchedUpdates,
syncUpdates,
deferredUpdates,
} = ReactFiberScheduler(config);
Expand Down Expand Up @@ -161,6 +163,8 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T

batchedUpdates,

unbatchedUpdates,

syncUpdates,

deferredUpdates,
Expand Down
90 changes: 29 additions & 61 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ var {
if (__DEV__) {
var ReactFiberInstrumentation = require('ReactFiberInstrumentation');
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
var warning = require('warning');
}

var timeHeuristicForUnitOfWork = 1;
Expand Down Expand Up @@ -111,8 +110,8 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
// Keeps track of whether we're currently in a work loop.
let isPerformingWork : boolean = false;

// Keeps track of whether sync updates should be downgraded to task updates.
let shouldDeferSyncUpdates : boolean = false;
// Keeps track of whether we should should batch sync updates.
let isBatchingUpdates : boolean = false;

// The next work in progress fiber that we're currently working on.
let nextUnitOfWork : ?Fiber = null;
Expand Down Expand Up @@ -675,31 +674,10 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
}

function performWork(priorityLevel : PriorityLevel, deadline : Deadline | null) {
// If performWork is called recursively, we need to save the previous state
// of the scheduler so it can be restored before the function exits.
// Recursion is only possible when using syncUpdates.
const previousPriorityContext = priorityContext;
const previousPriorityContextBeforeReconciliation = priorityContextBeforeReconciliation;
const previousIsPerformingWork = isPerformingWork;
const previousShouldDeferSyncUpdates = shouldDeferSyncUpdates;
const previousNextEffect = nextEffect;
const previousCommitPhaseBoundaries = commitPhaseBoundaries;
const previousFirstUncaughtError = firstUncaughtError;
const previousFatalError = fatalError;
const previousIsCommitting = isCommitting;
const previousIsUnmounting = isUnmounting;

priorityContext = NoWork;
priorityContextBeforeReconciliation = NoWork;
if (isPerformingWork) {
throw new Error('performWork was called recursively.');
}
isPerformingWork = true;
shouldDeferSyncUpdates = true;
nextEffect = null;
commitPhaseBoundaries = null;
firstUncaughtError = null;
fatalError = null;
isCommitting = false;
isUnmounting = false;

const isPerformingDeferredWork = Boolean(deadline);
let deadlineHasExpired = false;

Expand Down Expand Up @@ -805,17 +783,12 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T

const errorToThrow = fatalError || firstUncaughtError;

// We're done performing work. Restore the previous state of the scheduler.
priorityContext = previousPriorityContext;
priorityContextBeforeReconciliation = previousPriorityContextBeforeReconciliation;
isPerformingWork = previousIsPerformingWork;
shouldDeferSyncUpdates = previousShouldDeferSyncUpdates;
nextEffect = previousNextEffect;
commitPhaseBoundaries = previousCommitPhaseBoundaries;
firstUncaughtError = previousFirstUncaughtError;
fatalError = previousFatalError;
isCommitting = previousIsCommitting;
isUnmounting = previousIsUnmounting;
// We're done performing work. Time to clean up.
isPerformingWork = false;
fatalError = null;
firstUncaughtError = null;
capturedErrors = null;
failedBoundaries = null;

// It's safe to throw any unhandled errors.
if (errorToThrow) {
Expand Down Expand Up @@ -1030,20 +1003,6 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
}

function scheduleUpdate(fiber : Fiber, priorityLevel : PriorityLevel) {
// Detect if a synchronous update is made during render (or begin phase).
if (priorityLevel === SynchronousPriority && isPerformingWork && !isCommitting) {
if (__DEV__) {
warning(
false,
'Render methods should be a pure function of props and state; ' +
'triggering nested component updates from render is not allowed. ' +
'If necessary, trigger nested updates in componentDidUpdate.'
);
}
// Downgrade to Task priority to prevent an infinite loop.
priorityLevel = TaskPriority;
}

let node = fiber;
let shouldContinue = true;
while (node && shouldContinue) {
Expand Down Expand Up @@ -1098,8 +1057,9 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
}

function getPriorityContext() : PriorityLevel {
// If we're in a batch, downgrade sync priority to task priority
if (priorityContext === SynchronousPriority && shouldDeferSyncUpdates) {
// If we're in a batch, or if we're already performing work, downgrade sync
// priority to task priority
if (priorityContext === SynchronousPriority && (isPerformingWork || isBatchingUpdates)) {
return TaskPriority;
}
return priorityContext;
Expand All @@ -1120,30 +1080,37 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
}

function batchedUpdates<A, R>(fn : (a: A) => R, a : A) : R {
const previousShouldDeferSyncUpdates = shouldDeferSyncUpdates;
shouldDeferSyncUpdates = true;
const previousIsBatchingUpdates = isBatchingUpdates;
isBatchingUpdates = true;
try {
return fn(a);
} finally {
shouldDeferSyncUpdates = previousShouldDeferSyncUpdates;
isBatchingUpdates = previousIsBatchingUpdates;
// If we're not already inside a batch, we need to flush any task work
// that was created by the user-provided function.
if (!shouldDeferSyncUpdates) {
if (!isPerformingWork && !isBatchingUpdates) {
performWork(TaskPriority);
}
}
}

function unbatchedUpdates<A>(fn : () => A) : A {
const previousIsBatchingUpdates = isBatchingUpdates;
isBatchingUpdates = false;
try {
return fn();
} finally {
isBatchingUpdates = previousIsBatchingUpdates;
}
}

function syncUpdates<A>(fn : () => A) : A {
const previousPriorityContext = priorityContext;
const previousShouldDeferSyncUpdates = shouldDeferSyncUpdates;
priorityContext = SynchronousPriority;
shouldDeferSyncUpdates = false;
try {
return fn();
} finally {
priorityContext = previousPriorityContext;
shouldDeferSyncUpdates = previousShouldDeferSyncUpdates;
}
}

Expand All @@ -1162,6 +1129,7 @@ module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, T
getPriorityContext: getPriorityContext,
performWithPriority: performWithPriority,
batchedUpdates: batchedUpdates,
unbatchedUpdates: unbatchedUpdates,
syncUpdates: syncUpdates,
deferredUpdates: deferredUpdates,
};
Expand Down
69 changes: 63 additions & 6 deletions src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,13 +341,70 @@ describe('ReactIncrementalScheduling', () => {
expect(ReactNoop.getChildren()).toEqual([span(1)]);
});

it('can force synchronous updates with syncUpdates, even inside batchedUpdates', done => {
ReactNoop.batchedUpdates(() => {
ReactNoop.syncUpdates(() => {
ReactNoop.render(<span />);
expect(ReactNoop.getChildren()).toEqual([span()]);
done();
it('can opt-out of batching using unbatchedUpdates', () => {
// syncUpdates gives synchronous priority to updates
ReactNoop.syncUpdates(() => {
// batchedUpdates downgrades sync updates to task priority
ReactNoop.batchedUpdates(() => {
ReactNoop.render(<span prop={0} />);
expect(ReactNoop.getChildren()).toEqual([]);
// Should not have flushed yet because we're still batching

// unbatchedUpdates reverses the effect of batchedUpdates, so sync
// updates are not batched
ReactNoop.unbatchedUpdates(() => {
ReactNoop.render(<span prop={1} />);
expect(ReactNoop.getChildren()).toEqual([span(1)]);
ReactNoop.render(<span prop={2} />);
expect(ReactNoop.getChildren()).toEqual([span(2)]);
});

ReactNoop.render(<span prop={3} />);
expect(ReactNoop.getChildren()).toEqual([span(2)]);
});
// Remaining update is now flushed
expect(ReactNoop.getChildren()).toEqual([span(3)]);
});
});

it('nested updates are always deferred, even inside unbatchedUpdates', () => {
let instance;
let ops = [];
class Foo extends React.Component {
state = { step: 0 };
componentDidUpdate() {
ops.push('componentDidUpdate: ' + this.state.step);
if (this.state.step === 1) {
ReactNoop.unbatchedUpdates(() => {
// This is a nested state update, so it should not be
// flushed synchronously, even though we wrapped it
// in unbatchedUpdates.
this.setState({ step: 2 });
});
expect(ReactNoop.getChildren()).toEqual([span(1)]);
}
}
render() {
ops.push('render: ' + this.state.step);
instance = this;
return <span prop={this.state.step} />;
}
}
ReactNoop.render(<Foo />);
ReactNoop.flush();
expect(ReactNoop.getChildren()).toEqual([span(0)]);

ReactNoop.syncUpdates(() => {
instance.setState({ step: 1 });
expect(ReactNoop.getChildren()).toEqual([span(2)]);
Copy link
Contributor

@bvaughn bvaughn Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expectation won't be evaluated. It could fail and Jest won't catch it because you've already called done in componentDidUpdate above. To resolve this you could set a bool (eg componentDidUpdateCalled = true) in componentDidUpdate above, verify it here, and then call done() at the end of the sync update.

it('nested updates are always deferred, even inside unbatchedUpdates', done => {
  let instance;
  let componentDidUpdateCalled = false;
  class Foo extends React.Component {
    state = { step: 0 };
    componentDidUpdate() {
      if (this.state.step === 1) {
        ReactNoop.unbatchedUpdates(() => {
          // This is a nested state update, so it should not be
          // flushed synchronously, even though we wrapped it
          // in unbatchedUpdates.
          this.setState({ step: 2 });
        });
        expect(ReactNoop.getChildren()).toEqual([span(1)]);
        componentDidUpdateCalled = true;
      }
    }
    render() {
      instance = this;
      return <span prop={this.state.step} />;
    }
  }
  ReactNoop.render(<Foo />);
  ReactNoop.flush();
  expect(ReactNoop.getChildren()).toEqual([span(0)]);

  ReactNoop.syncUpdates(() => {
    instance.setState({ step: 1 });
    expect(componentDidUpdateCalled).toBe(true);
    expect(ReactNoop.getChildren()).toEqual([span(2)]);
    done();
  });
});

Copy link
Collaborator Author

@acdlite acdlite Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch!

Copy link
Collaborator Author

@acdlite acdlite Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by pushing to an array like we usually do

Copy link
Contributor

@bvaughn bvaughn Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

});

expect(ops).toEqual([
'render: 0',
'render: 1',
'componentDidUpdate: 1',
'render: 2',
'componentDidUpdate: 2',
]);
});
});
13 changes: 0 additions & 13 deletions src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ describe('ReactComponentTreeHook', () => {
var ReactInstanceMap;
var ReactComponentTreeHook;
var ReactComponentTreeTestUtils;
var ReactDOMFeatureFlags;

beforeEach(() => {
jest.resetModules();
Expand All @@ -29,7 +28,6 @@ describe('ReactComponentTreeHook', () => {
ReactInstanceMap = require('ReactInstanceMap');
ReactComponentTreeHook = require('ReactComponentTreeHook');
ReactComponentTreeTestUtils = require('ReactComponentTreeTestUtils');
ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
});

function assertTreeMatches(pairs) {
Expand Down Expand Up @@ -1843,9 +1841,6 @@ describe('ReactComponentTreeHook', () => {
// https://github.com/facebook/react/issues/7187
var el = document.createElement('div');
var portalEl = document.createElement('div');
if (ReactDOMFeatureFlags.useFiber) {
spyOn(console, 'error');
}
class Foo extends React.Component {
componentWillMount() {
ReactDOM.render(<div />, portalEl);
Expand All @@ -1855,14 +1850,6 @@ describe('ReactComponentTreeHook', () => {
}
}
ReactDOM.render(<Foo />, el);
if (ReactDOMFeatureFlags.useFiber) {
// A sync `render` inside cWM will print a warning. That should be the
// only warning.
expect(console.error.calls.count()).toEqual(1);
expect(console.error.calls.argsFor(0)[0]).toMatch(
/Render methods should be a pure function of props and state/
);
}
});

it('is created when calling renderToString during render', () => {
Expand Down
Loading