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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Mar 7, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e302f7
write chunks to a buffer with no re-use
gnoff Mar 5, 2022
3ae6dea
refactor writeChunk to be more defensive and efficient
gnoff Mar 5, 2022
2d7b743
add tests to exercise codepaths dealing with buffer overlows
gnoff Mar 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 44 additions & 4 deletions packages/react-server/src/ReactServerStreamConfigBrowser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,64 @@ export function flushBuffered(destination: Destination) {
// transform streams. https://github.com/whatwg/streams/issues/960
}

let currentView = null;
let writtenBytes = 0;

export function beginWriting(destination: Destination) {}

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

Choose a reason for hiding this comment

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

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.

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 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)

currentView = new Uint8Array(512);
writtenBytes = 0;
}

if (chunk.length > currentView.length) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yup that makes sense

// this chunk is larger than our view which implies it was not
// one that is cached by the streaming renderer. We will enqueu
// it directly and expect it is not re-used
if (writtenBytes > 0) {
destination.enqueue(new Uint8Array(currentView.buffer, 0, writtenBytes));
currentView = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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?

writtenBytes = 0;
}
destination.enqueue(chunk);
return;
}

const allowableBytes = currentView.length - writtenBytes;
if (allowableBytes < chunk.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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?

// this chunk would overflow the current view. We enqueu a full view
// and start a new view with the remaining chunk
currentView.set(chunk.subarray(0, allowableBytes), writtenBytes);
destination.enqueue(currentView);
currentView = new Uint8Array(512);
currentView.set(chunk.subarray(allowableBytes));
writtenBytes = chunk.length - allowableBytes;
} else {
currentView.set(chunk, writtenBytes);
writtenBytes += chunk.length;
}
}

export function writeChunkAndReturn(
destination: Destination,
chunk: PrecomputedChunk | Chunk,
): boolean {
destination.enqueue(chunk);
return destination.desiredSize > 0;
writeChunk(destination, chunk);
// in web streams there is no backpressure so we can alwas write more
return true;
}

export function completeWriting(destination: Destination) {}
export function completeWriting(destination: Destination) {
if (currentView && writtenBytes > 0) {
destination.enqueue(new Uint8Array(currentView.buffer, 0, writtenBytes));
currentView = null;
writtenBytes = 0;
}
}

export function close(destination: Destination) {
destination.close();
Expand Down