[Fizz] write chunks to a buffer with no re-use #24034

gnoff · 2022-03-05T19:40:10Z

chunks were previously enqueued to a ReadableStream as they were written. We now write them to a view over an ArrayBuffer and enqueue them only when writing has completed or the buffer's size is exceeded. In addition this copy now ensures we don't attempt to re-send buffers that have already been transferred.

this pr replaces #24033

~~this pr should close #24032 however we have yet to repro in jsdom~~
was able to repro original issue and confirm this change resolves it

chunks were previously enqueued to a ReadableStream as they were written. We now write them to a view over an ArrayBuffer and enqueue them only when writing has completed or the buffer's size is exceeded. In addition this copy now ensures we don't attempt to re-send buffers that have already been transferred.

sebmarkbage · 2022-03-05T21:38:48Z

packages/react-server/src/ReactServerStreamConfigBrowser.js

 export function beginWriting(destination: Destination) {}

 export function writeChunk(
  destination: Destination,
  chunk: PrecomputedChunk | Chunk,
 ): void {
-  destination.enqueue(chunk);
+  if (currentView === null) {


Why do this lazily here rather than eagerly in beginWriting? You can cheat flow and assume that it has already been initialized at this point since we know it has. That way you don't need a runtime check for every call.

I forget the original intent but I changed it to match your suggestion and also figured it was more defensive in case endWriting was ever not called (may be impossible but figured a wayward throw might leave this as a possibility)

sebmarkbage · 2022-03-05T21:45:13Z

packages/react-server/src/ReactServerStreamConfigBrowser.js

+    // it directly and expect it is not re-used
+    if (writtenBytes > 0) {
+      destination.enqueue(new Uint8Array(currentView.buffer, 0, writtenBytes));
+      currentView = null;


Ah, maybe because of this case. It is very likely that we're going to have at least one more call after this. Like we have to write </html> for example. So you can eagerly allocate the next buffer.

One case where this might not hold is if we start using byobRequest.view to get the buffer in beginWriting. Then we wouldn't need to allocate one for the future.

I’m confused, why would using byobRequest make it so we wouldn’t need to allocate a new buffer? What about byob is distinct from our self created buffers that would change this logic?

sebmarkbage · 2022-03-05T21:53:00Z

packages/react-server/src/ReactServerStreamConfigBrowser.js

+    writtenBytes = 0;
+  }
+
+  if (chunk.length > currentView.length) {


You can hard code this to 512. This gets important if we start using byobRequest since the current view could be smaller in that case. However, what really matters is whether it'll overflow the next view which you allocate below.

Ah yup that makes sense

sebmarkbage · 2022-03-05T21:54:49Z

packages/react-server/src/ReactServerStreamConfigBrowser.js

+  }
+
+  const allowableBytes = currentView.length - writtenBytes;
+  if (allowableBytes < chunk.length) {


allowableBytes could be zero if the previous write ended up filling the whole view. You could special case that to just enqueue the whole view.

I’m curious about the performance of subarrays given they are fixed address ranges with no copying. Is there still significant overhead to subarray(0) that is worth avoiding with special cases?

We now defend against overflows using the next views length instead of the current one. this protects us against a future where we use byobRequest and we get longer initial views than we might create after overflowing the first time. Additionally we add in an optimization when we have completely filled up the currentView where we avoid creating subarrays of the chunk to write since it lands exactly on a view boundary. Finally we move the view creation to beginWriting to avoid a runtime check on each write and because we want to reset the view on each beginWriting call in case a throw elsewhere in the program leaves the currentView in an unfinished state

gnoff · 2022-03-07T18:33:59Z

I was able to reproduce using miniflare both the original issue and that this PR fixes it

Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb

* write chunks to a buffer with no re-use chunks were previously enqueued to a ReadableStream as they were written. We now write them to a view over an ArrayBuffer and enqueue them only when writing has completed or the buffer's size is exceeded. In addition this copy now ensures we don't attempt to re-send buffers that have already been transferred. * refactor writeChunk to be more defensive and efficient We now defend against overflows using the next views length instead of the current one. this protects us against a future where we use byobRequest and we get longer initial views than we might create after overflowing the first time. Additionally we add in an optimization when we have completely filled up the currentView where we avoid creating subarrays of the chunk to write since it lands exactly on a view boundary. Finally we move the view creation to beginWriting to avoid a runtime check on each write and because we want to reset the view on each beginWriting call in case a throw elsewhere in the program leaves the currentView in an unfinished state * add tests to exercise codepaths dealing with buffer overlows

Summary: This sync includes the following changes: - **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>// - **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>// - **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>// - **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>// - **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>// - **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>// - **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>// - **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>// - **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>// - **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>// - **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>// - **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>// - **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>// - **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>// - **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>// - **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>// - **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>// - **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>// - **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>// - **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>// - **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>// - **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>// - **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>// - **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>// - **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>// - **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 1780659...1159ff6 jest_e2e[run_all_tests] Reviewed By: lunaleaps Differential Revision: D34928167 fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb

facebook-github-bot added the CLA Signed label Mar 5, 2022

gnoff requested a review from sebmarkbage March 5, 2022 19:40

sebmarkbage reviewed Mar 5, 2022

View reviewed changes

gnoff force-pushed the bugfix-readablestream-buffering branch from affda4f to 3ae6dea Compare March 5, 2022 23:35

sebmarkbage approved these changes Mar 6, 2022

View reviewed changes

gnoff force-pushed the bugfix-readablestream-buffering branch from 5f1313b to 054aba3 Compare March 6, 2022 18:53

add tests to exercise codepaths dealing with buffer overlows

2d7b743

gnoff force-pushed the bugfix-readablestream-buffering branch 2 times, most recently from 41f888b to 2d7b743 Compare March 7, 2022 17:00

gnoff merged commit e09518e into facebook:main Mar 7, 2022

sebmarkbage mentioned this pull request Mar 31, 2022

Server-side rendering performance degradation with renderToPipeableStream #24232

Open

gnoff mentioned this pull request Apr 7, 2022

[Fizz] Pipeable Stream Perf #24291

Merged

gnoff deleted the bugfix-readablestream-buffering branch January 11, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fizz] write chunks to a buffer with no re-use #24034

[Fizz] write chunks to a buffer with no re-use #24034

gnoff commented Mar 5, 2022 •

edited

Loading

sebmarkbage Mar 5, 2022

gnoff Mar 6, 2022

sebmarkbage Mar 5, 2022

sebmarkbage Mar 5, 2022

gnoff Mar 5, 2022

sebmarkbage Mar 5, 2022 •

edited

Loading

gnoff Mar 5, 2022

sebmarkbage Mar 5, 2022

gnoff Mar 5, 2022

gnoff commented Mar 7, 2022

[Fizz] write chunks to a buffer with no re-use #24034

[Fizz] write chunks to a buffer with no re-use #24034

Conversation

gnoff commented Mar 5, 2022 • 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

Choose a reason for hiding this comment

sebmarkbage Mar 5, 2022 • 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

gnoff commented Mar 7, 2022

gnoff commented Mar 5, 2022 •

edited

Loading

sebmarkbage Mar 5, 2022 •

edited

Loading