Skip to content

Commit

Permalink
[Fizz] Improve text separator byte efficiency (#24630)
Browse files Browse the repository at this point in the history
* [Fizz] Improve text separator byte efficiency

Previously text separators were inserted following any Text node in Fizz. This increases bytes sent when streaming and in some cases such as title elements these separators are not interpreted as comment nodes and leak into the visual aspects of a page as escaped text.

The reason simple tracking on the last pushed type doesn't work is that Segments can be filled in asynchronously later and so you cannot know in a single pass whether the preceding content was a text node or not. This commit adds a concept of TextEmbedding which provides a best effort signal to Segments on whether they are embedded within text. This allows the later resolution of that Segment to add text separators when possibly necessary but avoid them when they are surely not.

The current implementation can only "peek" head if the segment is a the Root Segment or a Suspense Boundary Segment. In these cases we know there is no trailing text embedding and we can eliminate the separator at the end of the segment if the last emitted element was Text. In normal Segments we cannot peek and thus have to assume there might be a trailing text embedding and we issue a separator defensively. This should be rare in practice as it is assumed most components that will cause segment creation will also emit some markup at the edges.

* [Fizz] Improve separator efficiency when flushing delayed segments

The method by which we get segment markup into the DOM differs depending on when the Segment resolves.

If a Segment resolves before flushing begins for it's parent it will be emitted inline with the parent markup. In these cases separators may be necessary because they are how we clue the browser into breakup up text into distinct nodes that will later match up with what will be hydrated on the client.

If a Segment resolves after flushing has happened a script will be used to patch up the DOM in the client. when this happens if there are any text nodes on the boundary of the patch they won't be "merged" and thus will continue to have distinct representation as Nodes in the DOM. Thus we can avoid doing any separators at the boundaries in these cases.

After applying these changes the only time you will get text separators as follows

* in between serial text nodes that emit at the same time - these are necessary and cannot be eliminated unless we stop relying on the browser to automatically parse the correct text nodes when processing this HTML
* after a final text node in a non-boundary segment that resolves before it's parent has flushed - these are sometimes extraneous, like when the next emitted thing is a non-Text node.

In all other cases text separators should be omitted which means the general byte efficiency of this approach should be pretty good
  • Loading branch information
gnoff authored May 28, 2022
1 parent f786053 commit a276638
Show file tree
Hide file tree
Showing 11 changed files with 579 additions and 59 deletions.
422 changes: 422 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ describe('ReactDOMServerIntegration', () => {
) {
// For plain server markup result we have comments between.
// If we're able to hydrate, they remain.
expect(e.childNodes.length).toBe(render === streamRender ? 6 : 5);
expect(e.childNodes.length).toBe(5);
expectTextNode(e.childNodes[0], ' ');
expectTextNode(e.childNodes[2], ' ');
expectTextNode(e.childNodes[4], ' ');
Expand All @@ -119,8 +119,8 @@ describe('ReactDOMServerIntegration', () => {
Text<span>More Text</span>
</div>,
);
expect(e.childNodes.length).toBe(render === streamRender ? 3 : 2);
const spanNode = e.childNodes[render === streamRender ? 2 : 1];
expect(e.childNodes.length).toBe(2);
const spanNode = e.childNodes[1];
expectTextNode(e.childNodes[0], 'Text');
expect(spanNode.tagName).toBe('SPAN');
expect(spanNode.childNodes.length).toBe(1);
Expand All @@ -147,19 +147,19 @@ describe('ReactDOMServerIntegration', () => {
itRenders('a custom element with text', async render => {
const e = await render(<custom-element>Text</custom-element>);
expect(e.tagName).toBe('CUSTOM-ELEMENT');
expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
expect(e.childNodes.length).toBe(1);
expectNode(e.firstChild, TEXT_NODE_TYPE, 'Text');
});

itRenders('a leading blank child with a text sibling', async render => {
const e = await render(<div>{''}foo</div>);
expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

itRenders('a trailing blank child with a text sibling', async render => {
const e = await render(<div>foo{''}</div>);
expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

Expand All @@ -176,7 +176,7 @@ describe('ReactDOMServerIntegration', () => {
render === streamRender
) {
// In the server render output there's a comment between them.
expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
expect(e.childNodes.length).toBe(3);
expectTextNode(e.childNodes[0], 'foo');
expectTextNode(e.childNodes[2], 'bar');
} else {
Expand All @@ -203,7 +203,7 @@ describe('ReactDOMServerIntegration', () => {
render === streamRender
) {
// In the server render output there's a comment between them.
expect(e.childNodes.length).toBe(render === streamRender ? 6 : 5);
expect(e.childNodes.length).toBe(5);
expectTextNode(e.childNodes[0], 'a');
expectTextNode(e.childNodes[2], 'b');
expectTextNode(e.childNodes[4], 'c');
Expand Down Expand Up @@ -240,7 +240,11 @@ describe('ReactDOMServerIntegration', () => {
e
</div>,
);
if (render === serverRender || render === clientRenderOnServerString) {
if (
render === serverRender ||
render === streamRender ||
render === clientRenderOnServerString
) {
// In the server render output there's comments between text nodes.
expect(e.childNodes.length).toBe(5);
expectTextNode(e.childNodes[0], 'a');
Expand All @@ -249,15 +253,6 @@ describe('ReactDOMServerIntegration', () => {
expectTextNode(e.childNodes[3].childNodes[0], 'c');
expectTextNode(e.childNodes[3].childNodes[2], 'd');
expectTextNode(e.childNodes[4], 'e');
} else if (render === streamRender) {
// In the server render output there's comments after each text node.
expect(e.childNodes.length).toBe(7);
expectTextNode(e.childNodes[0], 'a');
expectTextNode(e.childNodes[2], 'b');
expect(e.childNodes[4].childNodes.length).toBe(4);
expectTextNode(e.childNodes[4].childNodes[0], 'c');
expectTextNode(e.childNodes[4].childNodes[2], 'd');
expectTextNode(e.childNodes[5], 'e');
} else {
expect(e.childNodes.length).toBe(4);
expectTextNode(e.childNodes[0], 'a');
Expand Down Expand Up @@ -296,7 +291,7 @@ describe('ReactDOMServerIntegration', () => {
render === streamRender
) {
// In the server markup there's a comment between.
expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
expect(e.childNodes.length).toBe(3);
expectTextNode(e.childNodes[0], 'foo');
expectTextNode(e.childNodes[2], '40');
} else {
Expand Down Expand Up @@ -335,13 +330,13 @@ describe('ReactDOMServerIntegration', () => {

itRenders('null children as blank', async render => {
const e = await render(<div>{null}foo</div>);
expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

itRenders('false children as blank', async render => {
const e = await render(<div>{false}foo</div>);
expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

Expand All @@ -353,7 +348,7 @@ describe('ReactDOMServerIntegration', () => {
{false}
</div>,
);
expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

Expand Down Expand Up @@ -740,10 +735,10 @@ describe('ReactDOMServerIntegration', () => {
</div>,
);
expect(e.id).toBe('parent');
expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
expect(e.childNodes.length).toBe(3);
const child1 = e.childNodes[0];
const textNode = e.childNodes[1];
const child2 = e.childNodes[render === streamRender ? 3 : 2];
const child2 = e.childNodes[2];
expect(child1.id).toBe('child1');
expect(child1.childNodes.length).toBe(0);
expectTextNode(textNode, ' ');
Expand All @@ -757,10 +752,10 @@ describe('ReactDOMServerIntegration', () => {
async render => {
// prettier-ignore
const e = await render(<div id="parent"> <div id="child" /> </div>); // eslint-disable-line no-multi-spaces
expect(e.childNodes.length).toBe(render === streamRender ? 5 : 3);
expect(e.childNodes.length).toBe(3);
const textNode1 = e.childNodes[0];
const child = e.childNodes[render === streamRender ? 2 : 1];
const textNode2 = e.childNodes[render === streamRender ? 3 : 2];
const child = e.childNodes[1];
const textNode2 = e.childNodes[2];
expect(e.id).toBe('parent');
expectTextNode(textNode1, ' ');
expect(child.id).toBe('child');
Expand All @@ -783,9 +778,7 @@ describe('ReactDOMServerIntegration', () => {
) {
// For plain server markup result we have comments between.
// If we're able to hydrate, they remain.
expect(parent.childNodes.length).toBe(
render === streamRender ? 6 : 5,
);
expect(parent.childNodes.length).toBe(5);
expectTextNode(parent.childNodes[0], 'a');
expectTextNode(parent.childNodes[2], 'b');
expectTextNode(parent.childNodes[4], 'c');
Expand Down Expand Up @@ -817,7 +810,7 @@ describe('ReactDOMServerIntegration', () => {
render === clientRenderOnServerString ||
render === streamRender
) {
expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
expect(e.childNodes.length).toBe(3);
expectTextNode(e.childNodes[0], '<span>Text1&quot;</span>');
expectTextNode(e.childNodes[2], '<span>Text2&quot;</span>');
} else {
Expand Down Expand Up @@ -868,7 +861,7 @@ describe('ReactDOMServerIntegration', () => {
);
if (render === serverRender || render === streamRender) {
// We have three nodes because there is a comment between them.
expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
expect(e.childNodes.length).toBe(3);
// Everything becomes LF when parsed from server HTML.
// Null character is ignored.
expectNode(e.childNodes[0], TEXT_NODE_TYPE, 'foo\nbar');
Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOMUseId-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,6 @@ describe('useId', () => {
id="container"
>
:R0:, :R0H1:, :R0H2:
<!-- -->
</div>
`);
});
Expand All @@ -369,7 +368,6 @@ describe('useId', () => {
id="container"
>
:R0:
<!-- -->
</div>
`);
});
Expand Down
11 changes: 0 additions & 11 deletions packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ export function flushBuffered(destination: Destination) {}

export function beginWriting(destination: Destination) {}

let prevWasCommentSegmenter = false;
export function writeChunk(
destination: Destination,
chunk: Chunk | PrecomputedChunk,
Expand All @@ -36,16 +35,6 @@ export function writeChunkAndReturn(
destination: Destination,
chunk: Chunk | PrecomputedChunk,
): boolean {
if (prevWasCommentSegmenter) {
prevWasCommentSegmenter = false;
if (chunk[0] !== '<') {
destination.push('<!-- -->');
}
}
if (chunk === '<!-- -->') {
prevWasCommentSegmenter = true;
return true;
}
return destination.push(chunk);
}

Expand Down
25 changes: 21 additions & 4 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,13 +284,30 @@ export function pushTextInstance(
target: Array<Chunk | PrecomputedChunk>,
text: string,
responseState: ResponseState,
): void {
textEmbedded: boolean,
): boolean {
if (text === '') {
// Empty text doesn't have a DOM node representation and the hydration is aware of this.
return;
return textEmbedded;
}
if (textEmbedded) {
target.push(textSeparator);
}
target.push(stringToChunk(encodeHTMLTextNode(text)));
return true;
}

// Called when Fizz is done with a Segment. Currently the only purpose is to conditionally
// emit a text separator when we don't know for sure it is safe to omit
export function pushSegmentFinale(
target: Array<Chunk | PrecomputedChunk>,
responseState: ResponseState,
lastPushedText: boolean,
textEmbedded: boolean,
): void {
if (lastPushedText && textEmbedded) {
target.push(textSeparator);
}
// TODO: Avoid adding a text separator in common cases.
target.push(stringToChunk(encodeHTMLTextNode(text)), textSeparator);
}

const styleNameCache: Map<string, PrecomputedChunk> = new Map();
Expand Down
25 changes: 23 additions & 2 deletions packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {FormatContext} from './ReactDOMServerFormatConfig';
import {
createResponseState as createResponseStateImpl,
pushTextInstance as pushTextInstanceImpl,
pushSegmentFinale as pushSegmentFinaleImpl,
writeStartCompletedSuspenseBoundary as writeStartCompletedSuspenseBoundaryImpl,
writeStartClientRenderedSuspenseBoundary as writeStartClientRenderedSuspenseBoundaryImpl,
writeEndCompletedSuspenseBoundary as writeEndCompletedSuspenseBoundaryImpl,
Expand Down Expand Up @@ -105,11 +106,31 @@ export function pushTextInstance(
target: Array<Chunk | PrecomputedChunk>,
text: string,
responseState: ResponseState,
): void {
textEmbedded: boolean,
): boolean {
if (responseState.generateStaticMarkup) {
target.push(stringToChunk(escapeTextForBrowser(text)));
return false;
} else {
return pushTextInstanceImpl(target, text, responseState, textEmbedded);
}
}

export function pushSegmentFinale(
target: Array<Chunk | PrecomputedChunk>,
responseState: ResponseState,
lastPushedText: boolean,
textEmbedded: boolean,
): void {
if (responseState.generateStaticMarkup) {
return;
} else {
pushTextInstanceImpl(target, text, responseState);
return pushSegmentFinaleImpl(
target,
responseState,
lastPushedText,
textEmbedded,
);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,17 @@ export function pushTextInstance(
target: Array<Chunk | PrecomputedChunk>,
text: string,
responseState: ResponseState,
): void {
// This Renderer does not use this argument
textEmbedded: boolean,
): boolean {
target.push(
INSTANCE,
RAW_TEXT, // Type
END, // Null terminated type string
// TODO: props { text: text }
END, // End of children
);
return false;
}

export function pushStartInstance(
Expand All @@ -156,6 +159,14 @@ export function pushEndInstance(
target.push(END);
}

// In this Renderer this is a noop
export function pushSegmentFinale(
target: Array<Chunk | PrecomputedChunk>,
responseState: ResponseState,
lastPushedText: boolean,
textEmbedded: boolean,
): void {}

export function writeCompletedRoot(
destination: Destination,
responseState: ResponseState,
Expand Down
16 changes: 15 additions & 1 deletion packages/react-noop-renderer/src/ReactNoopServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,18 @@ const ReactNoopServer = ReactFizzServer({
return null;
},

pushTextInstance(target: Array<Uint8Array>, text: string): void {
pushTextInstance(
target: Array<Uint8Array>,
text: string,
responseState: ResponseState,
textEmbedded: boolean,
): boolean {
const textInstance: TextInstance = {
text,
hidden: false,
};
target.push(Buffer.from(JSON.stringify(textInstance), 'utf8'), POP);
return false;
},
pushStartInstance(
target: Array<Uint8Array>,
Expand All @@ -128,6 +134,14 @@ const ReactNoopServer = ReactFizzServer({
target.push(POP);
},

// This is a noop in ReactNoop
pushSegmentFinale(
target: Array<Uint8Array>,
responseState: ResponseState,
lastPushedText: boolean,
textEmbedded: boolean,
): void {},

writeCompletedRoot(
destination: Destination,
responseState: ResponseState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,7 @@ describe('ReactDOMServerFB', () => {
await jest.runAllTimers();

const result = readResult(stream);
expect(result).toMatchInlineSnapshot(
`"<div><!--$-->Done<!-- --><!--/$--></div>"`,
);
expect(result).toMatchInlineSnapshot(`"<div><!--$-->Done<!--/$--></div>"`);
});

it('should throw an error when an error is thrown at the root', () => {
Expand Down
Loading

0 comments on commit a276638

Please sign in to comment.