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

Jump to bottom

[Fizz] Improve text separator byte efficiency #24630

Merged
merged 10 commits into from May 28, 2022
Merged

[Fizz] Improve text separator byte efficiency #24630

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
96fad9f
[Fizz] Improve text separator byte efficiency
gnoff May 27, 2022
6393401
update tests that relied on older textSeparator behavior
gnoff May 27, 2022
a9e72bf
[Fizz] Improve separator efficiency when flushing delayed segments
gnoff May 27, 2022
d0660fe
Remove renderToString hack for supressing textSeparators
gnoff May 27, 2022
3030aec
fixup! Remove renderToString hack for supressing textSeparators
gnoff May 27, 2022
3f61057
refactor to move embed status to the segment itself
gnoff May 27, 2022
f7c4b86
flow and lint
gnoff May 27, 2022
19c76fa
cleanup flow some more
gnoff May 27, 2022
fc76c7f
more flow refinement
gnoff May 27, 2022
277beb8
cleanup ReactNoopServer
gnoff May 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
211 changes: 202 additions & 9 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Expand Up @@ -235,7 +235,7 @@ describe('ReactDOMFizzServer', () => {
}

function AsyncTextWrapped({as, text}) {
let As = as;
const As = as;
return <As>{readText(text)}</As>;
}

Expand Down Expand Up @@ -3584,6 +3584,14 @@ describe('ReactDOMFizzServer', () => {
});

describe('text separators', () => {
// To force performWork to start before resolving AsyncText but before piping we need to wait until
// after scheduleWork which currently uses setImmediate to delay performWork
function afterImmediate() {
return new Promise(resolve => {
setImmediate(resolve);
});
}

it('it only includes separators between adjacent text nodes', async () => {
function App({name}) {
return (
Expand All @@ -3603,9 +3611,22 @@ describe('ReactDOMFizzServer', () => {
expect(container.innerHTML).toEqual(
'<div>hello<b>world, <!-- -->Foo</b>!</div>',
);
const errors = [];
ReactDOMClient.hydrateRoot(container, <App name="Foo" />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div>
hello<b>world, {'Foo'}</b>!
</div>,
);
});

it('it inserts text separators even when adjacent text is in a delayed segment', async () => {
it('it does not insert text separators even when adjacent text is in a delayed segment', async () => {
function App({name}) {
return (
<Suspense fallback={'loading...'}>
Expand Down Expand Up @@ -3634,7 +3655,29 @@ describe('ReactDOMFizzServer', () => {
await act(() => resolveText('Foo'));

expect(container.firstElementChild.outerHTML).toEqual(
'<div id="app-div">hello<b>world, <!-- -->Foo<!-- --></b>!</div>',
'<div id="app-div">hello<b>world, Foo</b>!</div>',
);
// there are extra script nodes at the end of container
expect(container.childNodes.length).toBe(5);
const div = container.childNodes[1];
expect(div.childNodes.length).toBe(3);
const b = div.childNodes[1];
expect(b.childNodes.length).toBe(2);
expect(b.childNodes[0]).toMatchInlineSnapshot('world, ');
expect(b.childNodes[1]).toMatchInlineSnapshot('Foo');

const errors = [];
ReactDOMClient.hydrateRoot(container, <App name="Foo" />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div id="app-div">
hello<b>world, {'Foo'}</b>!
</div>,
);
});

Expand Down Expand Up @@ -3662,12 +3705,24 @@ describe('ReactDOMFizzServer', () => {
await act(() => resolveText('orld'));

expect(document.getElementById('app-div').outerHTML).toEqual(
'<div id="app-div">h<template id="P:1"></template>w<!-- -->orld<!-- --></div>',
'<div id="app-div">h<template id="P:1"></template>world</div>',
);

await act(() => resolveText('ello'));
expect(container.firstElementChild.outerHTML).toEqual(
'<div id="app-div">h<!-- -->ello<!-- -->w<!-- -->orld<!-- --></div>',
'<div id="app-div">helloworld</div>',
);

const errors = [];
ReactDOMClient.hydrateRoot(container, <App name="Foo" />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div id="app-div">{['h', 'ello', 'w', 'orld']}</div>,
);
});

Expand All @@ -3685,6 +3740,7 @@ describe('ReactDOMFizzServer', () => {

await act(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
await afterImmediate();
await act(() => resolveText('ello'));
pipe(writable);
});
Expand All @@ -3696,7 +3752,19 @@ describe('ReactDOMFizzServer', () => {
await act(() => resolveText('orld'));

expect(container.firstElementChild.outerHTML).toEqual(
'<div id="app-div">h<!-- -->ello<!-- -->w<!-- -->orld<!-- --></div>',
'<div id="app-div">h<!-- -->ello<!-- -->world</div>',
);

const errors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div id="app-div">{['h', 'ello', 'w', 'orld']}</div>,
);
});

Expand All @@ -3716,12 +3784,27 @@ describe('ReactDOMFizzServer', () => {

await act(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
await afterImmediate();
await act(() => resolveText('world'));
pipe(writable);
});

expect(container.firstElementChild.outerHTML).toEqual(
'<div>hello<b>world</b></div>',
'<div>hello<b>world<!-- --></b></div>',
);

const errors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div>
hello<b>world</b>
</div>,
);
});

Expand All @@ -3739,13 +3822,28 @@ describe('ReactDOMFizzServer', () => {

await act(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
await afterImmediate();
await act(() => resolveText('world'));
pipe(writable);
});

expect(container.firstElementChild.outerHTML).toEqual(
'<div>hello<b>world</b></div>',
);

const errors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div>
hello<b>world</b>
</div>,
);
});

it('should not insert separators for text inside Suspense boundaries even if they would otherwise be considered text-embedded', async () => {
Expand Down Expand Up @@ -3773,6 +3871,7 @@ describe('ReactDOMFizzServer', () => {

await act(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
await afterImmediate();
await act(() => resolveText('world'));
pipe(writable);
});
Expand All @@ -3786,15 +3885,109 @@ describe('ReactDOMFizzServer', () => {
});

expect(document.getElementById('app-div').outerHTML).toEqual(
'<div id="app-div">start<!--$-->firststart<!-- -->first suspended<!-- -->firstend<!--/$--><!--$?--><template id="B:1"></template>[loading second]<!--/$-->end</div>',
'<div id="app-div">start<!--$-->firststartfirst suspendedfirstend<!--/$--><!--$?--><template id="B:1"></template>[loading second]<!--/$-->end</div>',
);

const errors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div id="app-div">
{'start'}
{'firststart'}
{'first suspended'}
{'firstend'}
{'[loading second]'}
{'end'}
</div>,
);

await act(async () => {
resolveText('second suspended');
});

expect(container.firstElementChild.outerHTML).toEqual(
'<div id="app-div">start<!--$-->firststart<!-- -->first suspended<!-- -->firstend<!--/$--><!--$-->secondstart<b>second suspended<!-- --></b><!--/$-->end</div>',
'<div id="app-div">start<!--$-->firststartfirst suspendedfirstend<!--/$--><!--$-->secondstart<b>second suspended</b><!--/$-->end</div>',
);

expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div id="app-div">
{'start'}
{'firststart'}
{'first suspended'}
{'firstend'}
{'secondstart'}
<b>second suspended</b>
{'end'}
</div>,
);
});

it('(only) includes extraneous text separators in segments that complete before flushing, followed by nothing or a non-Text node', async () => {
function App() {
return (
<div>
<Suspense fallback={'text before, nothing after...'}>
hello
<AsyncText text="world" />
</Suspense>
<Suspense fallback={'nothing before or after...'}>
<AsyncText text="world" />
</Suspense>
<Suspense fallback={'text before, element after...'}>
hello
<AsyncText text="world" />
<br />
</Suspense>
<Suspense fallback={'nothing before, element after...'}>
<AsyncText text="world" />
<br />
</Suspense>
</div>
);
}

await act(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
await afterImmediate();
await act(() => resolveText('world'));
pipe(writable);
});

expect(container.innerHTML).toEqual(
'<div><!--$-->hello<!-- -->world<!-- --><!--/$--><!--$-->world<!-- --><!--/$--><!--$-->hello<!-- -->world<!-- --><br><!--/$--><!--$-->world<!-- --><br><!--/$--></div>',
);

const errors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
errors.push(error.message);
},
});
expect(Scheduler).toFlushAndYield([]);
expect(errors).toEqual([]);
expect(getVisibleChildren(container)).toEqual(
<div>
{/* first boundary */}
{'hello'}
{'world'}
{/* second boundary */}
{'world'}
{/* third boundary */}
{'hello'}
{'world'}
<br />
{/* fourth boundary */}
{'world'}
<br />
</div>,
);
});
});
Expand Down
19 changes: 13 additions & 6 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
View file
Open in desktop
Expand Up @@ -307,17 +307,16 @@ export function pushTextInstance(
// advanced with the tracking and drop the trailing edge in some cases when we know a segment
// is followed by a Text or non-Text Node explicitly vs followed by another Segment
export opaque type TextEmbedding = number;
type Embeddable = {textEmbedding: TextEmbedding, ...};

const NO_TEXT_EMBED = /* */ 0b0000;
const NO_TEXT_EMBED = /* */ 0b0000;

const LEADING_SEPARATOR_NEEDED = /* */ 0b0011;
const LEADING_TEXT_EMBED_KNOWN = /* */ 0b0001;
const LEADING_SEPARATOR_NEEDED = /* */ 0b0011;
const LEADING_TEXT_EMBED_KNOWN = /* */ 0b0001;
// const LEADING_TEXT_EMBED_POSSIBLE = /* */ 0b0010;

const TRAILING_SEPARATOR_NEEDED = /* */ 0b1100;
const TRAILING_SEPARATOR_NEEDED = /* */ 0b1100;
// const TRAILING_TEXT_EMBED_KNOWN = /* */ 0b0100;
const TRAILING_TEXT_EMBED_POSSIBLE = /* */ 0b1000;
const TRAILING_TEXT_EMBED_POSSIBLE = /* */ 0b1000;

// Suspense boundaries always emit a comment node at the leading and trailing edge and thus need
// no additional separators. we also use this for the root segment even though it isn't a Boundary
Expand All @@ -327,6 +326,8 @@ export function textEmbeddingForBoundarySegment(): TextEmbedding {
return NO_TEXT_EMBED;
}

// Segments that are not the boundary segment can be truly embedded in Text. we determine the leading edge
// based on what was last pushed. We cannot know the trailing edge so we conservatively assume we are embedded there
export function textEmbeddingForSegment(): TextEmbedding {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this whole feature is DOM specific and ideally we wouldn't punish other environments - it's also really DOM that's our main (and currently only target). I wouldn't be too hurt if this feature was just built into ReactFizzServer instead. Including the lastPushedText part.

Since otherwise we leak a lot of concepts and have to make up flags etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I had this thought as I kept adding more functions to the format config. glad you see it the same way and makes sense. I'll move things over so we can clean up the interface

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you able to just use the segment.textEmbedding flag as the running tally for lastPushedText?

Copy link
Collaborator Author

@gnoff gnoff May 27, 2022 •
edited

Choose a reason for hiding this comment

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

not totally, for boundaries, we need to track lastPushedText internally but we never need to push a separator at the end b/c we always have the suspense comment node. I suppose if I could know I was in a segment that was a boundary or the root I could use that as an alternative way to discriminating those cases

and actually for delayed segments we don't want to emit a final separator but they aren't boundaries so I need a way to know if it is delayed or not when I get to the final push

but I can just track that all as another property on the Segment

let embedding = TRAILING_TEXT_EMBED_POSSIBLE;
embedding |= lastPushedText ? LEADING_TEXT_EMBED_KNOWN : NO_TEXT_EMBED;
Expand All @@ -335,6 +336,12 @@ export function textEmbeddingForSegment(): TextEmbedding {
return embedding;
}

// If a Segment is going to be flushed later than it's parent text separators arent needed
// because the DOM patch will leavee the adjacent text as separate nodes
export function textEmbeddingForDelayedSegment(): TextEmbedding {
return NO_TEXT_EMBED;
}

// Called when a segment is about to be rendered by a Task
export function prepareForSegment(textEmbedding: TextEmbedding) {
lastPushedText = textEmbedding & LEADING_SEPARATOR_NEEDED;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js
View file
Open in desktop
Expand Up @@ -75,6 +75,7 @@ export function createRootFormatContext(): FormatContext {
export type {
FormatContext,
SuspenseBoundaryID,
TextEmbedding,
} from './ReactDOMServerFormatConfig';

export {
Expand All @@ -97,6 +98,7 @@ export {
writeCompletedRoot,
textEmbeddingForBoundarySegment,
textEmbeddingForSegment,
textEmbeddingForDelayedSegment,
prepareForSegment,
finalizeForSegment,
} from './ReactDOMServerFormatConfig';
Expand Down
11 changes: 4 additions & 7 deletions packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js
View file
Open in desktop
Expand Up @@ -155,13 +155,10 @@ export function pushEndInstance(
): void {
target.push(END);
}

export function textEmbeddingForBoundarySegment() {
return null;
}
export function textEmbeddingForSegment() {
return null;
}
export opaque type TextEmbedding = void;
export function textEmbeddingForBoundarySegment() {}
export function textEmbeddingForSegment() {}
export function textEmbeddingForDelayedSegment() {}
export function prepareForSegment() {}
export function finalizeForSegment() {}

Expand Down