[Fiber] Simplify coroutines by making yields stateless #8840

sebmarkbage · 2017-01-21T07:21:42Z

Builds on top of #8414 and #8413

Coroutines was kind of broken because it tried to do reparenting and enabling state preservation to be passed along the coroutine. However, since we couldn't determine which Fiber was "current" on a reified yield this was kind of broken.

This removes the "continuation" part of yields so they're basically just return values. It is still possible to do continuations by just passing simple functions or classes as part of the return value but they're not stateful.

This means that we won't have reparenting, but I actually don't think we need it. There's another way to structure this by doing all the state in the first phase and then yielding a stateless representation of the result. This stateless representation of the tree can then be rendered in different (or even multiple) locations.

Because we no longer have a stateful continuation, you may have noticed that this really no longer represent the "coroutine" concept. I will rename it in a follow up PR. I'm thinking createWait and createReturn maybe?

gaearon · 2017-01-21T16:16:09Z

I don't understand what "stateful" meant in this context even before the change.

Can you write a small example that would've been possible with the old implementation but is not possible now?

sebmarkbage · 2017-01-21T16:43:55Z

class Continuation extends React.Component {
  state = { renders: 0 };
  componentWillRecieveProps() {
    this.setState({ renders: this.state.renders + 1 });
  }
  render() {
    return <div>{this.context.parentName}{renders}</div>;
  }
}

class Child extends React.Component {
  getChildContext() {
    return { parentName: 'Child '};
  }
  render() {
    return ReactCoroutine.createYield(Continuation);
  }
}

function HandleYields(props, yields) {
  return yields.map(ContinuationComponent => props.wrap ? 
    <div><ContinuationComponent /></div> : <ContinuationComponent />);
}

class Parent extends React.Component {
  getChildContext() {
    return { parentName: 'Parent' };
  }
  render() {
    return ReactCoroutine.createCoroutine(
      this.props.children,
      HandleYields,
      this.props
    );
  }
}

render(<Parent><Child /><Parent>);

render(<Parent wrap><Child /><Parent>);

In this scenario, it used to be that the "ContinuationComponent" would get its state from "Child".

Therefore, it would keep its state when wrap is toggled. I.e. when it is reparented. That is no longer the case, it will get unmounted and remounted with this PR.

It used to be that the context passed to the "ContinuationComponent" should be "Child" because the Fiber hangs off of that tree (although that was already broken). With this PR the context is "Parent" because the Fiber now materializes as a child of HandleYields whose parent is "Parent".

gaearon · 2017-01-25T17:19:04Z

I played with it a little, and this example seems to break it:

      function increment(prevState) {
        return {
          value: prevState.value + 1,
        };
      }

      class Counter extends React.Component {
        constructor(props) {
          super(props);
          this.state = {value: 0};
          this.tick = this.tick.bind(this);
        }
        componentDidMount() {
          this.interval = setInterval(this.tick, 1000);
        }
        tick() {
          this.setState(increment);
        };
        componentWillUnmount() {
          clearInterval(this.interval);
        }
        render() {
          return createYield(<h1>{this.state.value}</h1>)
        }
      }

      function App() {
        return (
          <div>
            {createCoroutine(
              [<Counter />],
              (props, yields) => yields.map(y => y)
            )}
            <h1 />
          </div>
        )
      }

      ReactDOMFiber.render(
        <App />,
        document.getElementById('container')
      );

In particular the first mount renders, but the updates fail with "A coroutine cannot have host component children.".

Is this a bug?

gaearon

Updates seem still broken.

gaearon · 2017-01-25T18:57:24Z

I tried to write a failing test for this but it just hangs:

  fit('should handle deep updates in coroutine', () => {
    let instances = {};

    class Counter extends React.Component {
      state = {value: 5};
      render() {
        instances[this.props.id] = this;
        return ReactCoroutine.createYield(this.state.value);
      }
    }

    function App(props) {
      return ReactCoroutine.createCoroutine(
        [
          <Counter id="a" />,
          <Counter id="b" />,
          <Counter id="c" />,
        ],
        (props, yields) => yields.map(y => <span prop={y * 100} />),
        {}
      );
    }

    ReactNoop.render(<App />);
    ReactNoop.flush();
    expect(ReactNoop.getChildren()).toEqual([
      span(500),
      span(500),
      span(500),
    ]);

    instances.a.setState({value: 1});
    instances.b.setState({value: 2});
    ReactNoop.flush();
    expect(ReactNoop.getChildren()).toEqual([
      span(100),
      span(200),
      span(500),
    ]);
  });

gaearon · 2017-01-25T21:51:11Z

src/renderers/shared/fiber/ReactChildFiber.js

      const created = createFiberFromYield(yieldNode, priority);
-      created.type = reifiedYield;
+      created.type = yieldNode.value;


Can this break hidden classes if it's a number?

Yes but only in V8. Others use NaN tagging. It's likely already unlike that you'd use it this way. We can warn if it becomes an issue.

It is slightly more useful this way because when we want to find host nodes we typically want to do so in the second phase. That's the real tree where as the first phase is more of a virtual part of the tree.

Coroutines was kind of broken because it tried to do reparenting and enabling state preservation to be passed along the coroutine. However, since we couldn't determine which Fiber was "current" on a reified yield this was kind of broken. This removes the "continuation" part of yields so they're basically just return values. It is still possible to do continuations by just passing simple functions or classes as part of the return value but they're not stateful. This means that we won't have reparenting, but I actually don't think we need it. There's another way to structure this by doing all the state in the first phase and then yielding a stateless representation of the result. This stateless representation of the tree can then be rendered in different (or even multiple) locations. Because we no longer have a stateful continuation, you may have noticed that this really no longer represent the "coroutine" concept. I will rename it in a follow up commit.

There are a few different issues: * Updates result in unnecessary duplicate placements because it can't find the current fiber for continuations. * When run together, coroutine update and unmounting tests appear to lock down in an infinite loop. They don't freeze in isolation. I don't have a solution for this but just leaving it for future fixes.

sebmarkbage · 2017-01-26T01:12:38Z

So I fixed the "A coroutine cannot have host component children." issue. That was a bug introduced by this PR. a44f19b

The other issues has to do with the scheduler forgetting to look in the stateNode. There's a few TODO around but unrelated to this.

sebmarkbage · 2017-01-26T01:29:03Z

Actually, that's not right. The fix is not right and the bug is not the remaining TODO but that memoization bail out is wrong.

gaearon

I don't fully understand this yet so I may have missed something. I'll be happy to review more when we fix the scheduler stuff and I can throw more corner cases at it.

gaearon · 2017-01-26T01:25:15Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+
+      transferDeletions(workInProgress);
+    }
+
    memoizeProps(workInProgress, nextCoroutine);
    // This doesn't take arbitrary time so we could synchronously just begin
    // eagerly do the work of workInProgress.child as an optimization.


Nit: probably need to change workInProgress.child to workInProgress.stateNode in the comment (or drop it)

gaearon · 2017-01-26T01:29:13Z

src/renderers/shared/fiber/ReactFiberCompleteWork.js

@@ -66,31 +65,43 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
    popHostContainer,
  } = hostContext;

+  function markChildAsProgressed(current, workInProgress, priorityLevel) {


Maybe move to child fiber since it's duplicated between phases?

gaearon · 2017-01-26T01:29:53Z

src/renderers/shared/fiber/ReactChildFiber.js

@@ -474,7 +462,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
        }

        case REACT_YIELD_TYPE: {
-          if (newChild.key === key) {
+          // Yields doesn't have keys. If the previous node is implicitly keyed


Nit: don't have keys

gaearon · 2017-01-26T01:35:53Z

This test now breaks with the last commit:

  fit('whatever', () => {
    let instances = {};

    class Counter extends React.Component {
      state = {value: 5};
      render() {
        instances[this.props.id] = this;
        return ReactCoroutine.createYield(this.state.value);
      }
    }

    function App(props) {
      return ReactCoroutine.createCoroutine(
        [
          <Counter id="a" />,
          <Counter id="b" />,
          <Counter id="c" />,
        ],
        (props, yields) => yields.map(y => <span prop={y * 100} />),
        {}
      );
    }

    ReactNoop.render(<App />);
    ReactNoop.flush();
    expect(ReactNoop.getChildren()).toEqual([
      span(500),
      span(500),
      span(500),
    ]);
  });

Note I'm not actually testing updates, this is a subset of the previous (hanging) test. This part so far used to pass, but now I only see a single span.

When visiting the yields, the root is the stateNode of the coroutine. If its return value is wrong we'll end up at the alternate of the workInProgress which will start scanning its children which is wrong.

gaearon

sebmarkbage · 2017-01-26T01:38:39Z

Yea, but not with this: eb0bae3

@gaearon

Passes @gaearon's stress testing. New unit test added. Tested in stand alone demo using above ticks.

gaearon

I think this works.
(You'll want to update Fiber tests before merging)

This should bail to stateNode instead.

sebmarkbage requested review from gaearon and acdlite January 21, 2017 07:21

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 21, 2017

sebmarkbage mentioned this pull request Jan 21, 2017

[Fiber] Umbrella for remaining features / bugs #7925

Closed

sebmarkbage force-pushed the statelesscoroutines branch from 6ba962d to ca0d551 Compare January 21, 2017 07:27

sebmarkbage changed the title ~~Simplify coroutines by making yields stateless~~ [Fiber] Simplify coroutines by making yields stateless Jan 21, 2017

sebmarkbage force-pushed the statelesscoroutines branch from ca0d551 to 9ae9f5c Compare January 21, 2017 07:57

gaearon requested changes Jan 25, 2017

View reviewed changes

gaearon reviewed Jan 25, 2017

View reviewed changes

sebmarkbage added 3 commits January 25, 2017 15:41

Swap .child and .stateNode in coroutines

648d6e1

It is slightly more useful this way because when we want to find host nodes we typically want to do so in the second phase. That's the real tree where as the first phase is more of a virtual part of the tree.

sebmarkbage force-pushed the statelesscoroutines branch from 9ae9f5c to a44f19b Compare January 26, 2017 01:11

gaearon approved these changes Jan 26, 2017

View reviewed changes

Don't visit siblings of the coroutine when looking for yields

754f72a

When visiting the yields, the root is the stateNode of the coroutine. If its return value is wrong we'll end up at the alternate of the workInProgress which will start scanning its children which is wrong.

gaearon previously requested changes Jan 26, 2017

View reviewed changes

sebmarkbage force-pushed the statelesscoroutines branch from a44f19b to eb0bae3 Compare January 26, 2017 01:37

sebmarkbage force-pushed the statelesscoroutines branch from eb0bae3 to 156eb86 Compare January 26, 2017 01:39

gaearon approved these changes Jan 26, 2017

View reviewed changes

Don't bail to .child of coroutines

975ad92

This should bail to stateNode instead.

sebmarkbage force-pushed the statelesscoroutines branch from 156eb86 to 975ad92 Compare January 26, 2017 01:50

sebmarkbage merged commit b262657 into facebook:master Jan 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Simplify coroutines by making yields stateless #8840

[Fiber] Simplify coroutines by making yields stateless #8840

sebmarkbage commented Jan 21, 2017 •

edited

Loading

gaearon commented Jan 21, 2017

sebmarkbage commented Jan 21, 2017

gaearon commented Jan 25, 2017

gaearon left a comment

gaearon commented Jan 25, 2017

gaearon Jan 25, 2017

sebmarkbage Jan 25, 2017

sebmarkbage commented Jan 26, 2017

sebmarkbage commented Jan 26, 2017

gaearon left a comment

gaearon Jan 26, 2017

gaearon Jan 26, 2017

gaearon Jan 26, 2017

gaearon commented Jan 26, 2017

gaearon left a comment

sebmarkbage commented Jan 26, 2017

gaearon left a comment

[Fiber] Simplify coroutines by making yields stateless #8840

[Fiber] Simplify coroutines by making yields stateless #8840

Conversation

sebmarkbage commented Jan 21, 2017 • edited Loading

gaearon commented Jan 21, 2017

sebmarkbage commented Jan 21, 2017

gaearon commented Jan 25, 2017

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented Jan 25, 2017

gaearon Jan 25, 2017

Choose a reason for hiding this comment

sebmarkbage Jan 25, 2017

Choose a reason for hiding this comment

sebmarkbage commented Jan 26, 2017

sebmarkbage commented Jan 26, 2017

gaearon left a comment

Choose a reason for hiding this comment

gaearon Jan 26, 2017

Choose a reason for hiding this comment

gaearon Jan 26, 2017

Choose a reason for hiding this comment

gaearon Jan 26, 2017

Choose a reason for hiding this comment

gaearon commented Jan 26, 2017

gaearon left a comment

Choose a reason for hiding this comment

sebmarkbage commented Jan 26, 2017

gaearon left a comment

Choose a reason for hiding this comment

sebmarkbage commented Jan 21, 2017 •

edited

Loading