Skip to content
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.

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

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

Merged
merged 4 commits into from
Nov 9, 2018

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Nov 9, 2018

Fixes #13874

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

leebyron and others added 2 commits November 9, 2018 14:41
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
Copy link

sizebot commented Nov 9, 2018

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


// 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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

I think this looks correct

// 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++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is easier to read for me:

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

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

(originally set here)

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

Choose a reason for hiding this comment

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

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

return growThreadCountAndReturnNextAvailable();
}
nextAvailableThreadIDs[0] = nextAvailableThreadIDs[nextID];
return nextID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

is this breaking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe? Or is it a fix?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is it fixing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I submitted a PR to fix this: #14314

'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.',
);
Copy link
Collaborator

@sophiebits sophiebits Nov 9, 2018

Choose a reason for hiding this comment

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

┏┓
┃┃╱╲ 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
Copy link
Collaborator

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

@sebmarkbage
Copy link
Collaborator Author

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 sebmarkbage merged commit 961eb65 into facebook:master Nov 9, 2018
@sebmarkbage
Copy link
Collaborator Author

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.

'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.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

.,

@sophiebits
Copy link
Collaborator

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.

return markup;
try {
const markup = renderer.read(Infinity);
return markup;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@leebyron
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

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

acdlite pushed a commit to acdlite/react that referenced this pull request Nov 13, 2018
…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.
@trueadm

This comment has been minimized.

@gaearon
Copy link
Collaborator

gaearon commented Nov 18, 2018

Why didn’t tests catch this?

@trueadm
Copy link
Contributor

trueadm commented Nov 18, 2018

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

// 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@trueadm This is meant to read the default value.

Copy link
Contributor

@trueadm trueadm Nov 18, 2018

Choose a reason for hiding this comment

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

screen shot 2018-11-18 at 20 22 35

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

What proxying?

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

Choose a reason for hiding this comment

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

(originally set here)

@gaearon
Copy link
Collaborator

gaearon commented Nov 19, 2018

@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 />))
    });

screen shot 2018-11-19 at 8 29 55 pm

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
Copy link
Contributor

trueadm commented Nov 19, 2018

@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
Copy link
Collaborator

gaearon commented Nov 19, 2018

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

@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gaearon We accidentally bumped peer deps anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@trueadm
Copy link
Contributor

trueadm commented Nov 19, 2018

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

@gaearon
Copy link
Collaborator

gaearon commented Nov 19, 2018

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
Copy link

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
Copy link
Collaborator Author

sebmarkbage commented Nov 20, 2018

@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.

voxpelli added a commit to Sydsvenskan/react that referenced this pull request Nov 20, 2018
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
Copy link
Collaborator

gaearon commented Nov 20, 2018

Filed #14292 to track it.

voxpelli added a commit to Sydsvenskan/react that referenced this pull request Nov 23, 2018
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 pushed a commit that referenced this pull request Nov 27, 2018
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>
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…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.
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
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>
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
…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.
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants