Added ReactFiberStack shared by ReactFiberContext and ReactFiberHostContext #8611

bvaughn · 2016-12-21T01:56:30Z

Overview

ReactFiberStack is the new underlying stack construct used by ReactFiberContext and ReactFiberHostContext. The goal is to simplify the 2 context managers and to add earlier and better dev-warnings when we pop unexpectedly.

This change set causes a lot of tests to fail as we are currently popping too many times and/or in the wrong order. I'm pushing this branch now to share with Sebastian as he is working on a related cleanup in beginWork.

Work needing to be done for this branch

Track Fibers with unique host contexts to avoid over-popping; (see this)
Tighten up the Fiber | null type in ReactFiberStack by making sure that we always pass a Fiber down; currently this is not the case for pushHostContainer and popHostContainer.
Verify that no new test failures are caused by this refactor.

sebmarkbage · 2016-12-21T03:54:11Z

src/renderers/shared/fiber/ReactFiberStack.js

+    valueStack[index] = emptyObject;
+
+    if (__DEV__) {
+      fiberStack[index] = emptyObject;


Why emptyObject instead of null on both of these? We could use ES2015 .fill() here if available.

Another thing we could do if we're worried about leakage is to store the max index of the previous run and then only clear the delta between the previous two runs. That way if we end up refilling the stack every time we rerender we don't have to fill it twice.

My initial thought was to go with null, but I noticed that ReactFiberContext returned emptyObject in a few cases so I followed that pattern. I'll change it to null. Probably best to have the emptyObject fallback happening in ReactFiberContext rather than ReactFiberStack anyway.

sebmarkbage · 2016-12-21T03:58:42Z

src/renderers/shared/fiber/ReactFiberStack.js

+
+import type { Fiber } from 'ReactFiber';
+
+export type StackCursor = {|


We should make this generic type StackCursor<T> = {| current: T |}.

You will have to make some unsound things when you pop here but then everywhere else is fine.

sebmarkbage · 2016-12-21T04:00:39Z

src/renderers/shared/fiber/ReactFiberStack.js

+
+let index = -1;
+
+exports.createCursor = function() : StackCursor {


If you add an initial default value, Flow will let you make this generic.

function<T>(defaultValue : T) : StackCursor<T> {

When you can invoke it with createCursor((null : ?string)) to get a typed version of the stack cursor.

Passing a default value feels a bit awkward since (at least for now) none of the cursors have meaningful defaults. But the generics benefit is nice! So sure! 😄

sebmarkbage · 2016-12-21T05:46:55Z

In ReactFiberScheduler we call unwindContext followed by unwindHostContext. The point of these is to reset the context of both to where the error boundary is. However, this will break with your change because the popping needs to be interleaved not sequential.

I.e. you need to pop both (or all three I guess) until we reach the desired error boundary. I think we need to have an unwindWork that does only the normal popping that would happen in completeWork based on the tag. This is effectively what goes into a finally block in a programming language.

EDIT: Basically what unwindHostContext is already doing. Just break that out into a separate module and have it unwind the ReactFiberContext too.

The functional component branch is a bit special since it has sCU in it. The class component branch actually already covers this in a bit convoluted way. I will next replicate this check in each other branch.

bvaughn · 2016-12-21T17:43:17Z

Regarding unwindContext and unwindHostContext, it seems like the simplest solution would be to rename unwindHostContext to something like unwindContexts and add a case for ClassComponent that pop context providers. Not sure what the benefit of it being a separate module is at the moment since the scheduler is the only thing that unwinds. What's the motivation there?

bvaughn · 2016-12-21T18:03:48Z

Another TODO item for me: In pushHostContainer, we don't push if the current host context and the next/child host context are equal.

In order to avoid over-popping, I need to add another cursor to track the fibers we push for. And to do that, I think we'll need to address the other TODO to pass the host container Fiber down.

I have a meeting shortly but I'll work on both of these afterward.

sebmarkbage · 2016-12-21T20:47:37Z

For some reason I thought that unwindHostContext wasn't in the scheduler. Ignore the comment about separate module.

sebmarkbage · 2016-12-21T20:53:16Z

I was thinking that maybe we should tag the host components with a different tag if they don't need to change context. That way we know when we complete that we don't need to pop it. Similarly, the next time we update and beginWork, we can pick the fast path of skipping checking for context changes.

bvaughn · 2016-12-21T20:55:42Z

Where would this tagging be done? Seems inappropriate for ReactFiberHostContext to modify a Fiber in any way. I was planning on tracking which host Fibers provided unique context with another StackCursor. In theory this should work but my first attempt causes 3 Jest tests to hang. Still investigating.

If you'd like to chat about ideas here ping me on Messenger. 😄

sebmarkbage · 2016-12-21T21:02:05Z

The thing calling pushHostContext can mutate the tag from beginWork. Indeterminate and Coroutine components already do this. I'm not sure the ReactFiberContext and ReactFiberHostContext modules need to exist as abstractions at all. Their can also just be inlined at their callsites. Don't let the abstraction get in your way.

gaearon · 2016-12-21T21:57:35Z

src/renderers/shared/fiber/ReactFiberHostContext.js


  function getRootHostContainer() : C {
-    if (rootInstance == null) {
+    if (rootInstanceStackCursor.current == null) {


Minor nit: could you read .current once? I don't know if compiler is smart enough to know it hasn't changed.

Absolutely.

gaearon · 2016-12-21T22:19:18Z

The issue I'm noticing is that adding

      static childContextTypes = {
        foo: React.PropTypes.number
      };
      getChildContext() {
        return {foo: 42};
      }

to some of the "broken" components in ReactErrorBoundaries-test breaks some tests with unexpected pops. It's not just BrokenRender that fails this way but BrokenComponentWillMount, BrokenComponentWillReceiveProps and a few more.

It's likely the same issue as #8604 but worth noting it doesn't only happen for render method.

bvaughn · 2016-12-21T22:35:35Z

There's a kind of obvious design flaw with this single-stack-with-shared-cursors approach that I managed to overlook until a half hour ago.

Action      Cursor A    Cursor b    Stack
------      --------    --------    -----
(init)      null        null        []
push(a,1)   1           null        [1]
push(b,2)   1           2           [1,2]
pop(b)      1           1           [1]     <- b is invalid
pop(a)      null        1           []      <- b is still invalid

Irrespective of mismatched pushes/pops, this breaks a lot of things. I'm reevaluating the best way to proceed at the moment. I think Sebastian was a big fan of a single stack but I'm not currently able to visualize a way for it to work at the moment.

sebmarkbage · 2016-12-21T22:41:52Z

The thing that is suppose to go on the stack is the previous value of the cursor. Not the value you push.

Action      Cursor A    Cursor b    Stack
------      --------    --------    -----
(init)      null        null        []
push(a,1)   1           null        [null]
push(b,2)   1           2           [null,null]
pop(b)      1           null        [null]     <- b is valid
pop(a)      null        null        []      <- a and b is valid

bvaughn · 2016-12-21T22:43:36Z

Ahhh, that makes sense. Thanks @sebmarkbage.

sebmarkbage · 2016-12-21T22:44:07Z

src/renderers/shared/fiber/ReactFiberStack.js

+
+  index++;
+
+  valueStack[index] = value;


index++; valueStack[index] = cursor.current; cursor.current = value;

sebmarkbage · 2016-12-21T22:47:11Z

The mental model is:

var current = null;
function work(foo) {
  // Begin Work
  var prevOnTheStack = current;
  current = getContext(foo);
  try {
    processChildren(foo);
    // Complete Work
    completeSomething();
  } finally {
    // Complete Work and Unwind Context 
    current = prevOnTheStack;
  }
}

bvaughn · 2016-12-21T23:10:53Z

As of my most recent push, all tests that were previously passing (in master) are now once again passing on this branch.

…ontext ReactFiberStack is the new underlying stack used by ReactFiberContext and ReactFiberHostContext. The goal is to simplify the 2 context managers and to add more errors/dev-warnings when we pop unexpected. This changeset currently causes a lot of tests to fail (as we are currently popping too many times and/or in the wrong order). I'm pushing this branch now to share with Sebastian as he is working on a related cleanup pass at beginWork.

…null.

…iber

sebmarkbage · 2016-12-21T23:13:21Z

CI is failing for some reason. Try running grunt build locally?

bvaughn · 2016-12-21T23:21:52Z

Yeah, I noticed that. Running locally I see a lot of garbage in the console like

Error message "Missing owner for string ref %s" cannot be found. The current React version and the error map are probably out of sync. Please run gulp react:extract-errors before building React.

...but then it ultimately finishes with

Done, without errors.

bvaughn · 2016-12-21T23:40:49Z

I am able to reproduce the error locally by manually running the gulp task ./node_modules/.bin/gulp react:extract-errors. After looking at the node failing, I've determined that it's caused by my use of an exact object type for StackCursor:

// This works
export type StackCursor<T> = { current: T };

// This causes the error even though it's a valid Flow type
export type StackCursor<T> = {| current: T |};

For now I will relax that type so that Gulp works. I assume I should also be committing the updated scripts/error-codes/codes.json file but please let me know if that is not the case.

bvaughn · 2016-12-21T23:59:53Z

CI gets further now but ultimately fails at a different stage- ./scripts/circleci/test_coverage.sh. Running that task locally fails for me against both my branch and master.

This was causing the 'react:extract-errors' Gulp taks to fail.

bvaughn · 2016-12-22T01:14:11Z

Had an informative chat with Ben about this. The failing Grunt task was caused by the fact that I've added new invariants/warnings in this PR and updated my local error codes JSON. This in turn caused our ReactPropTypesProduction test to fail but in a non-obvious way. Rather than showing the error message from ReactPropTypesProduction, another error- TypeError: process.cwd is not a function- was shown. This is because we're mocking out the process object that Jest is trying to use. Commenting out the call to process.cwd in Jest revealed the underlying error code message.

This further confused me for a while because Grunt failed for me locally on master as well. That turned out to be caused by a caching issue which was fixed with PR #8622.

In general we have a test flakiness problem that will bite us when we next update the error codes (before the next release). That flakiness issue will need to be addressed in a subsequent PR. I don't mind taking that task on once these context PRs have been wrapped up. 😄

For now, CI passes again for this branch. Yay.

sebmarkbage · 2016-12-22T01:17:26Z

src/renderers/shared/fiber/ReactFiberContext.js

-const contextStack : Array<Object> = [];
-const didPerformWorkStack : Array<boolean> = [];
+let contextStackCursor : StackCursor<?Object> = createCursor((null: ?Object));
+let didPerformWorkStackCursor : StackCursor<?boolean> = createCursor((null: ?boolean));


If you set the default here to (false : boolean) you can get rid of the nullable type.

I thought about doing that, but then I'd lose the ability to differentiate between "empty" and default-state. Which isn't really important in this case but... slightly more compelling is that it would also mean the cursor would have asymmetric behavior- false by default, and then null after the stack gets reset (since we don't track and fall back to the default value). It seemed worth doing the cast to avoid that, but I don't feel strongly about it.

It won't have asymmetric behavior if you fix the reset below. :)

You are correct of course! 😁 That turnery was a holdover from when I was pushing/popping incorrectly. D'oh.

sebmarkbage · 2016-12-22T01:17:58Z

src/renderers/shared/fiber/ReactFiberContext.js

@@ -62,7 +66,7 @@ exports.getMaskedContext = function(workInProgress : Fiber) {
 };

 exports.hasContextChanged = function() : boolean {
-  return index > -1 && didPerformWorkStack[index];
+  return Boolean(didPerformWorkStackCursor.current);


By avoiding the nullable type above, you don't need coerce this. You can just return didPerformWorkStackCursor.current.

Ok. :) I'll go ahead and make that change then. I don't feel strongly about it.

sebmarkbage · 2016-12-22T01:23:07Z

src/renderers/shared/fiber/ReactFiberStack.js

+};
+
+exports.reset = function<T>(
+  cursor : StackCursor<T>,


Passing a cursor here seems like an odd API since it's invalid to only reset one cursor. You have to reset all at once. It also causes the unsound assignment below.

Currently this only works because index is already at -1 at the second call to reset so the loop is unnecessary.

I'd recommend just dropping the cursor argument and reset them separately if you think it's necessary to reset them.

Sure. It's a little weird either way I guess. I'll drop the cursor from reset and just manually clear the current pointer :)

sebmarkbage · 2016-12-22T01:25:05Z

src/renderers/shared/fiber/ReactFiberStack.js

+
+exports.push = function<T>(
+  cursor : StackCursor<T>,
+  value : any,


This can be value : T for a bit stronger guarantees that ensures that we don't push unexpected values.

Good catch.

sebmarkbage · 2016-12-22T01:30:10Z

src/renderers/shared/fiber/ReactFiberStack.js

+
+  cursor.current = index > 0
+    ? valueStack[index]
+    : (null : any);


This is unsound since it doesn't reset to the defaultValue. However, I don't think this should ever be possible since the first value that goes into the value stack is the defaultValue. So you shouldn't be able to pop further than that.

This should just be:

cursor.current = valueStack[index];

Otherwise you're never reading index 0.

Good suggestion.

sebmarkbage · 2016-12-22T01:35:54Z

src/renderers/shared/fiber/ReactFiberContext.js

 };

 exports.resetContext = function() : void {
-  index = -1;
+  reset();


We now have two calls to reset in a row because the only thing that calls this is:

https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberScheduler.js#L157-L160

So we could just call reset(); at the top of that function instead.

Seems slightly weird to bypass the contexts and talk directly to the stack they use, no?

But sure. I can change that.

sebmarkbage

Awesome! Let's go!

bvaughn · 2016-12-22T02:50:27Z

Thanks for the review Sebastian!!

facebook-github-bot added the GH Review: review-needed label Dec 21, 2016

bvaughn force-pushed the shared-context-stack branch from 6941810 to badc65a Compare December 21, 2016 02:04

sebmarkbage reviewed Dec 21, 2016

View reviewed changes

sebmarkbage requested a review from gaearon December 21, 2016 06:06

sebmarkbage mentioned this pull request Dec 21, 2016

[Fiber] Refactor bailoutOnFinishedWork #8613

Merged

bvaughn force-pushed the shared-context-stack branch from 63c8cc5 to 720035b Compare December 21, 2016 21:39

gaearon reviewed Dec 21, 2016

View reviewed changes

bvaughn force-pushed the shared-context-stack branch from 4a39d39 to 2bea981 Compare December 21, 2016 22:06

sebmarkbage reviewed Dec 21, 2016

View reviewed changes

Brian Vaughn added 5 commits December 21, 2016 15:11

Generic type added to StackCursor. Default emptyObject replaced with …

a0902fc

…null.

Combined unwindContext and unwindHostContext into unwindContexts

0c1fdfd

Tightened up Fiber | null type in ReactFiberStack to always require F…

0b7c5ff

…iber

Avoid popping hosts that do not provide unique contexts

6b51e03

Brian Vaughn added 2 commits December 21, 2016 15:11

Avoid reading the same StackCursor current multiple times

1148e82

ReactFiberStack cursor values are no longer corrupted on pop

e7a591e

bvaughn mentioned this pull request Dec 21, 2016

Fix context handling for errors thrown in ClassComponent render #8604

Closed

2 tasks

bvaughn force-pushed the shared-context-stack branch from 2c684f0 to e7a591e Compare December 21, 2016 23:17

Removed exact object type for StackCursor Flow type

ea6530d

This was causing the 'react:extract-errors' Gulp taks to fail.

bvaughn mentioned this pull request Dec 22, 2016

Jest preprocessor better aware of error-codes/codes.json WRT caching #8622

Merged

bvaughn force-pushed the shared-context-stack branch from 58a7d56 to ea6530d Compare December 22, 2016 01:05

sebmarkbage reviewed Dec 22, 2016

View reviewed changes

Improved Flowtypes in response to code review feedback

2f2a440

bvaughn force-pushed the shared-context-stack branch from 52f7d00 to 2f2a440 Compare December 22, 2016 01:33

sebmarkbage reviewed Dec 22, 2016

View reviewed changes

Removed redundant calls to ReactFiberStack.reset()

d1e6040

sebmarkbage approved these changes Dec 22, 2016

View reviewed changes

bvaughn merged commit f1e193b into facebook:master Dec 22, 2016

bvaughn deleted the shared-context-stack branch December 22, 2016 02:50


		import type { Fiber } from 'ReactFiber';

		export type StackCursor = {\|


		let index = -1;

		exports.createCursor = function() : StackCursor {

Added ReactFiberStack shared by ReactFiberContext and ReactFiberHostContext #8611

Added ReactFiberStack shared by ReactFiberContext and ReactFiberHostContext #8611

Conversation

bvaughn commented Dec 21, 2016 • edited Loading

Overview

Work needing to be done for this branch

Choose a reason for hiding this comment

bvaughn Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Dec 21, 2016 • edited Loading

bvaughn commented Dec 21, 2016

bvaughn commented Dec 21, 2016 • edited Loading

sebmarkbage commented Dec 21, 2016

sebmarkbage commented Dec 21, 2016 • edited Loading

bvaughn commented Dec 21, 2016

sebmarkbage commented Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Dec 21, 2016

bvaughn commented Dec 21, 2016

sebmarkbage commented Dec 21, 2016

bvaughn commented Dec 21, 2016

Choose a reason for hiding this comment

sebmarkbage commented Dec 21, 2016

bvaughn commented Dec 21, 2016 • edited Loading

sebmarkbage commented Dec 21, 2016

bvaughn commented Dec 21, 2016 • edited Loading

bvaughn commented Dec 21, 2016 • edited Loading

bvaughn commented Dec 21, 2016 • edited Loading

bvaughn commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

bvaughn commented Dec 22, 2016

bvaughn commented Dec 21, 2016 •

edited

Loading

bvaughn Dec 21, 2016 •

edited

Loading

sebmarkbage commented Dec 21, 2016 •

edited

Loading

bvaughn commented Dec 21, 2016 •

edited

Loading

sebmarkbage commented Dec 21, 2016 •

edited

Loading

sebmarkbage commented Dec 21, 2016 •

edited

Loading

bvaughn commented Dec 21, 2016 •

edited

Loading

bvaughn commented Dec 21, 2016 •

edited

Loading

bvaughn commented Dec 21, 2016 •

edited

Loading

bvaughn commented Dec 21, 2016 •

edited

Loading

sebmarkbage Dec 22, 2016 •

edited

Loading