Use unique thread ID for each partial render to access Context #14182

sebmarkbage · 2018-11-09T22:44:58Z

Alternative to #13877

This first adds an allocator that keeps track of a unique ThreadID index for each currently executing partial renderer. IDs are not just growing but are reused as streams are destroyed.

This ensures that IDs are kept nice and compact.

One minor breakage is that it is no longer safe to just let streams be GC:ed for clean up. Typically, they you would never drop a stream on the floor. It's either exhausted or errors.

This lets us use an "array" for each Context object to store the current values. The look up for these are fast because they're just looking up an offset in a tightly packed "array".

I don't use an actual Array object to store the values. Instead, I rely on that VMs (notably V8) treat storage of numeric index property access as a separate "elements" allocation.

This lets us avoid an extra indirection.

However, we must ensure that these arrays are not holey to preserve this feature.

To do that I store the _threadCount on each context (effectively it takes the place of the .length property on an array, and also lets me use that pun). It's unclear whether a "real" .length would be faster if it could avoid the internal bounds check.

This lets us first validate that the context has enough slots before we access the slot. If not, we fill in the slots with the default value.

This should be a fast approach, in theory, but I haven't actually confirmed that the builds don't deopt somewhere yet.

cc @leebyron

The new context API stores the provided values on the shared context instance. When used in a synchronous context, this is not an issue. However when used in an concurrent context this can cause a "push provider" from one react render to have an effect on an unrelated concurrent react render. I've encountered this bug in production when using renderToNodeStream, which asks ReactPartialRenderer for bytes up to a high water mark before yielding. If two Node Streams are created and read from in parallel, the state of one can polute the other. I wrote a failing test to illustrate the conditions under which this happens. I'm also concerned that the experimental concurrent/async React rendering on the client could suffer from the same issue.

This first adds an allocator that keeps track of a unique ThreadID index for each currently executing partial renderer. IDs are not just growing but are reused as streams are destroyed. This ensures that IDs are kept nice and compact. This lets us use an "array" for each Context object to store the current values. The look up for these are fast because they're just looking up an offset in a tightly packed "array". I don't use an actual Array object to store the values. Instead, I rely on that VMs (notably V8) treat storage of numeric index property access as a separate "elements" allocation. This lets us avoid an extra indirection. However, we must ensure that these arrays are not holey to preserve this feature. To do that I store the _threadCount on each context (effectively it takes the place of the .length property on an array). This lets us first validate that the context has enough slots before we access the slot. If not, we fill in the slots with the default value.

sizebot · 2018-11-09T22:54:00Z

React: size: 🔺+0.1%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: d5e1bf0...e3c6b17

react

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react.development.js	+0.4%	+0.4%	95.56 KB	95.93 KB	25.12 KB	25.21 KB	UMD_DEV
react.production.min.js	🔺+0.1%	🔺+0.1%	11.52 KB	11.54 KB	4.58 KB	4.58 KB	UMD_PROD
react.profiling.min.js	+0.1%	+0.2%	13.68 KB	13.69 KB	5.1 KB	5.11 KB	UMD_PROFILING
react.development.js	+0.6%	+0.6%	59.52 KB	59.89 KB	16.12 KB	16.21 KB	NODE_DEV
react.production.min.js	🔺+0.2%	🔺+0.2%	6.08 KB	6.09 KB	2.59 KB	2.6 KB	NODE_PROD
React-dev.js	+0.6%	+0.6%	56.97 KB	57.34 KB	15.33 KB	15.43 KB	FB_WWW_DEV
React-prod.js	🔺+0.1%	🔺+0.2%	14.99 KB	15.01 KB	4.03 KB	4.04 KB	FB_WWW_PROD
React-profiling.js	+0.1%	+0.2%	14.99 KB	15.01 KB	4.03 KB	4.04 KB	FB_WWW_PROFILING

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom-server.browser.development.js	+2.7%	+3.1%	120.8 KB	124.11 KB	32.05 KB	33.03 KB	UMD_DEV
react-dom-server.browser.production.min.js	🔺+3.8%	🔺+4.7%	16.34 KB	16.96 KB	6.21 KB	6.51 KB	UMD_PROD
react-dom-server.browser.development.js	+2.8%	+3.2%	116.93 KB	120.24 KB	31.09 KB	32.09 KB	NODE_DEV
react-dom-server.browser.production.min.js	🔺+3.8%	🔺+4.8%	16.24 KB	16.87 KB	6.2 KB	6.5 KB	NODE_PROD
ReactDOMServer-dev.js	+2.8%	+3.2%	118.09 KB	121.43 KB	30.74 KB	31.73 KB	FB_WWW_DEV
ReactDOMServer-prod.js	🔺+5.5%	🔺+4.5%	42.08 KB	44.41 KB	9.83 KB	10.28 KB	FB_WWW_PROD
react-dom-server.node.development.js	+2.9%	+3.2%	118.86 KB	122.27 KB	31.62 KB	32.63 KB	NODE_DEV
react-dom-server.node.production.min.js	🔺+4.0%	🔺+4.7%	17.05 KB	17.73 KB	6.5 KB	6.81 KB	NODE_PROD

Generated by 🚫 dangerJS

sebmarkbage · 2018-11-09T23:16:04Z

packages/react-dom/src/server/ReactThreadIDAllocator.js

+
+// Allocates a new index for each request. Tries to stay as compact as possible so that these
+// indices can be used to reference a tightly packaged array. As opposed to being used in a Map.
+// The first allocated index is 1.


I think what we're going to do is use this same strategy on the client but primary renders like React DOM and React Native will use index 0 hard coded so they can skip any allocation and resizing logic. Then secondary renderers can use index 1 and above.

sophiebits

I think this looks correct

sophiebits · 2018-11-09T23:03:17Z

packages/react-dom/src/server/ReactPartialRendererContext.js

+  // If we don't have enough slots in this context to store this threadID,
+  // fill it in without leaving any holes to ensure that the VM optimizes
+  // this as non-holey index properties.
+  for (let i = context._threadCount; i <= threadID; i++) {


this is easier to read for me:

while (context._threadCount < threadID) { context[context._threadCount++] = context.currentValue2; } ``

sophiebits · 2018-11-09T23:03:54Z

packages/react/src/ReactContext.js

@@ -42,6 +42,9 @@ export function createContext<T>(
    // Secondary renderers store their context values on separate fields.
    _currentValue: defaultValue,
    _currentValue2: defaultValue,


why even keep these any more instead of using "threads" on the client too? only the lack of global coordination?

We'll probably just do this on the client too. See my other comment about primary renderers could skip the global coordination.

Interestingly, we do need the defaultValue somewhere, so at least one field will remain.

(originally set here)

sophiebits · 2018-11-09T23:05:44Z

packages/react-dom/src/server/ReactThreadIDAllocator.js

+  let oldArray = nextAvailableThreadIDs;
+  let oldSize = oldArray.length;
+  let newSize = oldSize * 2;
+  if (newSize > 0x10000) {


nit: (2 << 16) probably easier to read

sophiebits · 2018-11-09T23:12:36Z

packages/react-dom/src/server/ReactThreadIDAllocator.js

+    return growThreadCountAndReturnNextAvailable();
+  }
+  nextAvailableThreadIDs[0] = nextAvailableThreadIDs[nextID];
+  return nextID;


Could return nextID - 1 (then +1 in free) so they appear to start at 0.

I wonder if this will mess with SMI optimizations. SMI - 1 might no longer be a SMI maybe?

Also just doing a bit of extra math is annoying. I think I'll use the primary renderer optimization as an excuse for why we shouldn't. :P

sophiebits · 2018-11-09T23:19:13Z

packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js

  _read(size) {
    try {
      this.push(this.partialRenderer.read(size));
    } catch (err) {
-      this.emit('error', err);


is this breaking?

Maybe? Or is it a fix?

what is it fixing?

I submitted a PR to fix this: #14314

sophiebits · 2018-11-09T23:20:34Z

packages/react-dom/src/server/ReactThreadIDAllocator.js

+      'Maximum number of concurrent React renderers exceeded. ' +
+        'This can happen if you are not properly destroying the Readable provided by React. ' +
+        'Ensure that you call .destroy() on it if you no longer want to read from it.',
+    );


┏┓
┃┃╱╲ in this
┃╱╱╲╲ house
╱╱╭╮╲╲ we
▔▏┗┛▕▔ use
╱▔▔▔▔▔▔▔▔▔▔╲
invariant
╱╱┏┳┓╭╮┏┳┓ ╲╲
▔▏┗┻┛┃┃┗┻┛▕▔

also can we tweak:

Ensure that you call .destroy() on it when you are finished reading from it.

current wording sounds a bit like a special case

sophiebits · 2018-11-09T23:25:25Z

the fact that we're not using the storage for allocated IDs feels so wasteful 😂

sebmarkbage · 2018-11-09T23:35:18Z

Maybe we'll find something else to store in there. :D

I suspect that we'll want to use these IDs for more things in the future. E.g. we have similar concepts in the interaction tracking. If we start using actual threads, then all our globals needs to be thread local but adding many thread locals globally is bad news so maybe things like current owner and dispatcher will use this too.

sebmarkbage · 2018-11-09T23:38:34Z

We don't really have good coverage enough internally to really know if this is fast/safe in production environments. The best we can do at this point is probably just to release it in a patch.

sophiebits · 2018-11-09T23:45:52Z

packages/react-dom/src/server/ReactThreadIDAllocator.js

+    'Maximum number of concurrent React renderers exceeded. ' +
+      'This can happen if you are not properly destroying the Readable provided by React. ' +
+      'Ensure that you call .destroy() on it if you no longer want to read from it.' +
+      ', and did not read to the end. If you use .pipe() this should be automatic.',


sophiebits · 2018-11-10T00:03:56Z

Instead of denoting the terminal ID as nextIDs[id] === 0, you could note it as nextIDs[id] === id and then instead of using nextIDs[0] to track the next available, store a separate int pointing to the next index. Then your IDs would start at 0.

leebyron · 2018-11-12T21:18:40Z

packages/react-dom/src/server/ReactDOMStringRenderer.js

-  return markup;
+  try {
+    const markup = renderer.read(Infinity);
+    return markup;


Any reason why these aren't just return renderer.read(Infinity)?

leebyron · 2018-11-12T21:23:44Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -835,6 +799,7 @@ class ReactDOMServerRenderer {
      while (out[0].length < bytes) {
        if (this.stack.length === 0) {
          this.exhausted = true;
+          freeThreadID(this.threadID);


could also replace these two lines with this.destroy()

leebyron · 2018-11-12T21:38:38Z

I agree with @sophiebits that it seems like a useful optimization to have thread IDs start at 0 and grow larger rather than start at the high ID and shrink towards 0 - that avoids having to fill a bunch of holes on context objects for the common case of a single thread

leebyron · 2018-11-12T21:40:07Z

I'm also curious to see if there's a meaningful performance difference between this and using a Map per renderer (especially native Map) and instead swapping in an implementation of useContext while performing partial rendering to use the correct Map

sophiebits · 2018-11-12T23:39:47Z

@leebyron The IDs are increasing. They just start at 1.

…ook#14182) * BUG: ReactPartialRenderer / New Context polutes mutable global state The new context API stores the provided values on the shared context instance. When used in a synchronous context, this is not an issue. However when used in an concurrent context this can cause a "push provider" from one react render to have an effect on an unrelated concurrent react render. I've encountered this bug in production when using renderToNodeStream, which asks ReactPartialRenderer for bytes up to a high water mark before yielding. If two Node Streams are created and read from in parallel, the state of one can polute the other. I wrote a failing test to illustrate the conditions under which this happens. I'm also concerned that the experimental concurrent/async React rendering on the client could suffer from the same issue. * Use unique thread ID for each partial render to access Context This first adds an allocator that keeps track of a unique ThreadID index for each currently executing partial renderer. IDs are not just growing but are reused as streams are destroyed. This ensures that IDs are kept nice and compact. This lets us use an "array" for each Context object to store the current values. The look up for these are fast because they're just looking up an offset in a tightly packed "array". I don't use an actual Array object to store the values. Instead, I rely on that VMs (notably V8) treat storage of numeric index property access as a separate "elements" allocation. This lets us avoid an extra indirection. However, we must ensure that these arrays are not holey to preserve this feature. To do that I store the _threadCount on each context (effectively it takes the place of the .length property on an array). This lets us first validate that the context has enough slots before we access the slot. If not, we fill in the slots with the default value.

gaearon · 2018-11-18T20:13:55Z

Why didn’t tests catch this?

trueadm · 2018-11-18T20:14:47Z

@gaearon I don't know enough about this threaded SSR implementation. @sebmarkbage should be able to explain why?

sophiebits · 2018-11-18T20:17:33Z

packages/react-dom/src/server/ReactPartialRendererContext.js

+    // We assume that this is the same as the defaultValue which might not be
+    // true if we're rendering inside a secondary renderer but they are
+    // secondary because these use cases are very rare.
+    context[i] = context._currentValue2;


@trueadm This is meant to read the default value.

It doesn't hit the validateContextBounds code path when contextType is undefined.

I’m not sure how the proxing works with this. Is it DEV only or both?

What proxying?

sophiebits · 2018-11-18T20:17:59Z

packages/react/src/ReactContext.js

@@ -42,6 +42,9 @@ export function createContext<T>(
    // Secondary renderers store their context values on separate fields.
    _currentValue: defaultValue,
    _currentValue2: defaultValue,


(originally set here)

gaearon · 2018-11-19T20:33:12Z

@trueadm

I expect to get 10, but I instead get undefined.

Could you please explain the testing methodology? This test prints 10 if I add it to ReactServerRendering-test.js:

    fit('waat', () => {
const MyContext = React.createContext(10);

function Component() {
  const { Consumer, Provider } = MyContext;
  return (
    <React.Fragment>
      <Consumer>
          {(value: number) => <span>{value}</span>}
      </Consumer>
    </React.Fragment>
  )
}

  console.log(ReactDOMServer.renderToString(<Component />))
    });

I also checked that

const React = require('react')
const ReactDOMServer = require('react-dom/server')

const MyContext = React.createContext(10);

function Component() {
  const {
    Consumer,
    Provider
  } = MyContext;
  return React.createElement(React.Fragment, null, React.createElement(Consumer$
}

prints <span>10</span> with node index.js.

trueadm · 2018-11-19T20:42:38Z

@gaearon I'm doing this like in your second example, except I've copied over the production server bundle and pasted it into the node_modules/react-dom directory so it's using the master build.

gaearon · 2018-11-19T20:44:50Z

Are you sure you don't have two React copies?

sophiebits · 2018-11-19T20:45:30Z

packages/react/src/ReactContext.js

@@ -42,6 +42,9 @@ export function createContext<T>(
    // Secondary renderers store their context values on separate fields.
    _currentValue: defaultValue,
    _currentValue2: defaultValue,
+    // Used to track how many concurrent renderers this context currently
+    // supports within in a single renderer. Such as parallel server rendering.
+    _threadCount: 0,


@trueadm did you copy over the new react package? this line is important.

Oh. Does this mean react-dom/server is not compatible with react@16.0.0? We've tried to relax this requirement during 16.x.x release line. If this doesn't work we need to start bumping peer deps again.

This was it! I'm sorry, I feel stupid now :(

@gaearon We accidentally bumped peer deps anyway.

The fix is simple enough I did it anyway. We should relax peer deps back imo.

trueadm · 2018-11-19T20:49:23Z

Ignore me. I was using the wrong react package. Sorry for the waste of time.

gaearon · 2018-11-19T20:50:04Z

It still points out an issue though. We intended to support mismatching versions of react and react-dom within a major, and so far it worked in 16. So we should probably fix.

nomcopter · 2018-11-20T03:29:20Z

This broke me in a minor version update (16.6.1 -> 16.6.3). I was calling ReactDOMServer.renderToStaticMarkup(<Child />) in a Parent's render function in order to dangerouslySetInnerHTML in a contentEditable element. In 16.6.1 Child had access to the context that Parent had access to.
In 16.6.3 Child no longer has access to Parent's context.

This is probably desirable, as the previous behavior seems weird, but it is likely worth flagging as a potential breaking change in the release notes. Unfortunately, it is already out in a minor version, although this use case seems very rare.

sebmarkbage · 2018-11-20T05:30:07Z

@nomcopter Thanks for flagging. I think we’re going to consider that a bug fix. It is not intended or desirable that context is transferred. What is worse is that it could accidentally leak a context unintentionally in similar ways as the big this PR was meant to fix.

Maybe we should have some kind of Portal solution for that use case just like we did on the client.

Regression introduced in facebook#14182 resulted in errors no longer being emitted on streams, breaking many consumers. Co-authored-by: Elliot Jalgard <elliot.j@live.se>

gaearon · 2018-11-20T13:47:33Z

Filed #14292 to track it.

Regression introduced in facebook#14182 resulted in errors no longer being emitted on streams, breaking many consumers. Co-authored-by: Elliot Jalgard <elliot.j@live.se>

Regression introduced in #14182 resulted in errors no longer being emitted on streams, breaking many consumers. Co-authored-by: Elliot Jalgard <elliot.j@live.se>

…ook#14182) * BUG: ReactPartialRenderer / New Context polutes mutable global state The new context API stores the provided values on the shared context instance. When used in a synchronous context, this is not an issue. However when used in an concurrent context this can cause a "push provider" from one react render to have an effect on an unrelated concurrent react render. I've encountered this bug in production when using renderToNodeStream, which asks ReactPartialRenderer for bytes up to a high water mark before yielding. If two Node Streams are created and read from in parallel, the state of one can polute the other. I wrote a failing test to illustrate the conditions under which this happens. I'm also concerned that the experimental concurrent/async React rendering on the client could suffer from the same issue. * Use unique thread ID for each partial render to access Context This first adds an allocator that keeps track of a unique ThreadID index for each currently executing partial renderer. IDs are not just growing but are reused as streams are destroyed. This ensures that IDs are kept nice and compact. This lets us use an "array" for each Context object to store the current values. The look up for these are fast because they're just looking up an offset in a tightly packed "array". I don't use an actual Array object to store the values. Instead, I rely on that VMs (notably V8) treat storage of numeric index property access as a separate "elements" allocation. This lets us avoid an extra indirection. However, we must ensure that these arrays are not holey to preserve this feature. To do that I store the _threadCount on each context (effectively it takes the place of the .length property on an array). This lets us first validate that the context has enough slots before we access the slot. If not, we fill in the slots with the default value.

Regression introduced in facebook#14182 resulted in errors no longer being emitted on streams, breaking many consumers. Co-authored-by: Elliot Jalgard <elliot.j@live.se>

…ook#14182) * BUG: ReactPartialRenderer / New Context polutes mutable global state The new context API stores the provided values on the shared context instance. When used in a synchronous context, this is not an issue. However when used in an concurrent context this can cause a "push provider" from one react render to have an effect on an unrelated concurrent react render. I've encountered this bug in production when using renderToNodeStream, which asks ReactPartialRenderer for bytes up to a high water mark before yielding. If two Node Streams are created and read from in parallel, the state of one can polute the other. I wrote a failing test to illustrate the conditions under which this happens. I'm also concerned that the experimental concurrent/async React rendering on the client could suffer from the same issue. * Use unique thread ID for each partial render to access Context This first adds an allocator that keeps track of a unique ThreadID index for each currently executing partial renderer. IDs are not just growing but are reused as streams are destroyed. This ensures that IDs are kept nice and compact. This lets us use an "array" for each Context object to store the current values. The look up for these are fast because they're just looking up an offset in a tightly packed "array". I don't use an actual Array object to store the values. Instead, I rely on that VMs (notably V8) treat storage of numeric index property access as a separate "elements" allocation. This lets us avoid an extra indirection. However, we must ensure that these arrays are not holey to preserve this feature. To do that I store the _threadCount on each context (effectively it takes the place of the .length property on an array). This lets us first validate that the context has enough slots before we access the slot. If not, we fill in the slots with the default value.

Regression introduced in facebook#14182 resulted in errors no longer being emitted on streams, breaking many consumers. Co-authored-by: Elliot Jalgard <elliot.j@live.se>

leebyron and others added 2 commits November 9, 2018 14:41

sebmarkbage requested review from sophiebits and acdlite November 9, 2018 22:44

facebook-github-bot added the CLA Signed label Nov 9, 2018

sebmarkbage requested a review from gaearon November 9, 2018 22:47

Fix lint

4bb10fe

sebmarkbage force-pushed the contextbug branch from 32efa07 to 4bb10fe Compare November 9, 2018 23:07

sebmarkbage commented Nov 9, 2018

View reviewed changes

sophiebits approved these changes Nov 9, 2018

View reviewed changes

Use invariant and add a bit to the message

e3c6b17

sebmarkbage merged commit 961eb65 into facebook:master Nov 9, 2018

sebmarkbage mentioned this pull request Nov 9, 2018

FIX: Context pollutes mutable global state (with "thread local storage") #13877

Closed

sophiebits reviewed Nov 9, 2018

View reviewed changes

leebyron reviewed Nov 12, 2018

View reviewed changes

This was referenced Nov 13, 2018

React.createContext with SSR leads to concurrency problems in environments with co-routines #13854

Closed

Add 16.6.3 Changelog #14223

Merged

This comment has been minimized.

Sign in to view

sophiebits reviewed Nov 18, 2018

View reviewed changes

sophiebits reviewed Nov 19, 2018

View reviewed changes

aweary mentioned this pull request Nov 20, 2018

Context works strange in 16.6.3 in renderToStaticMarkup #14287

Closed

gaearon mentioned this pull request Nov 20, 2018

Provide a way to pass context to renderToStaticMarkup on the client #14292

Closed

voxpelli mentioned this pull request Nov 23, 2018

Fix regression: Errors not emitted in streams #14314

Merged

stevenbuccini mentioned this pull request Jan 19, 2019

Throwing an error from a component while server rendering changes the default value of a context if there is a provider in that tree. #14502

Closed

Use unique thread ID for each partial render to access Context #14182

Use unique thread ID for each partial render to access Context #14182

Conversation

sebmarkbage commented Nov 9, 2018 • edited

sizebot commented Nov 9, 2018 • edited

react

react-dom

Choose a reason for hiding this comment

sophiebits left a 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

Choose a reason for hiding this comment

sophiebits Nov 9, 2018 • edited

Choose a reason for hiding this comment

sophiebits commented Nov 9, 2018

sebmarkbage commented Nov 9, 2018

sebmarkbage commented Nov 9, 2018

Choose a reason for hiding this comment

sophiebits commented Nov 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented Nov 12, 2018

leebyron commented Nov 12, 2018

sophiebits commented Nov 12, 2018

This comment has been minimized.

gaearon commented Nov 18, 2018

trueadm commented Nov 18, 2018 • edited

Choose a reason for hiding this comment

trueadm Nov 18, 2018 • edited

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

gaearon commented Nov 19, 2018

trueadm commented Nov 19, 2018

gaearon commented Nov 19, 2018

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

trueadm commented Nov 19, 2018

gaearon commented Nov 19, 2018

nomcopter commented Nov 20, 2018

sebmarkbage commented Nov 20, 2018 • edited

gaearon commented Nov 20, 2018

sebmarkbage commented Nov 9, 2018 •

edited

sizebot commented Nov 9, 2018 •

edited

sophiebits Nov 9, 2018 •

edited

trueadm commented Nov 18, 2018 •

edited

trueadm Nov 18, 2018 •

edited

sebmarkbage commented Nov 20, 2018 •

edited