[Fizz] Fork Fizz instruction set for inline script and external runtime

[mofeiZ]

[Fizz] Duplicate completeBoundaryWithStyles to not reference globals

Summary

Follow-up / cleanup PR to #25437

• completeBoundaryWithStylesInlineLocals is used by the Fizz external runtime, which bundles together all Fizz instruction functions (and is able to reference / rename completeBoundary and resourceMap as locals).
• completeBoundaryWithStylesInlineGlobals is used by the Fizz inline script writer, which sends Fizz instruction functions on an as-needed basis. This version needs to reference completeBoundary($RC) and resourceMap($RM) as globals.

Ideally, Closure would take care of inlining a shared implementation, but I couldn't figure out a zero-overhead inline due to lack of an @inline compiler directive. It seems that Closure thinks that a shared completeBoundaryWithStyles is too large and will always keep it as a separate function. I've also tried currying / writing a higher order function (getCompleteBoundaryWithStyles) with no luck

How did you test this change?

• generated Fizz inline instructions should be unchanged
• bundle size for unstable_external_runtime should be slightly smaller (due to lack of globals)
• ReactDOMFizzServer-test.js and ReactDOMFloat-test.js should be unaffected

[sizebot]

Comparing: b14d7fa...00e5dad

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	154.37 kB	154.37 kB	=	48.97 kB	48.97 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	156.29 kB	156.29 kB	=	49.63 kB	49.63 kB
facebook-www/ReactDOM-prod.classic.js	=	533.12 kB	533.12 kB	=	94.96 kB	94.96 kB
facebook-www/ReactDOM-prod.modern.js	=	518.22 kB	518.22 kB	=	92.76 kB	92.76 kB
facebook-www/ReactDOMForked-prod.classic.js	=	533.12 kB	533.12 kB	=	94.96 kB	94.96 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/react-dom/unstable_server-external-runtime.js	=	2.51 kB	2.46 kB	=	1.14 kB	1.12 kB
oss-stable-semver/react-dom/unstable_server-external-runtime.js	=	2.51 kB	2.46 kB	=	1.14 kB	1.12 kB
oss-stable/react-dom/unstable_server-external-runtime.js	=	2.51 kB	2.46 kB	=	1.14 kB	1.12 kB

Generated by 🚫 dangerJS against 00e5dad

[acdlite]

Did you try extracting smaller parts of the function out into a shared function? My guess is if you extract everything between lines 67-140 into a separate function, Closure would be able to inline it.

[mofeiZ]

Did you try extracting smaller parts of the function out into a shared function? My guess is if you extract everything between lines 67-140 into a separate function, Closure would be able to inline it.

@acdlite Yep I tried extracting just those lines (with no luck). I played around with compiling some much smaller functions, and it looks like Closure does inline small ones (so it's not that Closure decides to not inline accesses to globals or something).

function completeBoundaryWithStylesShared(
  resourceMap,
  dependencies,
  styles,
) {
  // ...
}

export function completeBoundaryWithStylesInlineGlobals(
  suspenseBoundaryID,
  contentID,
  styles,
) {
  const completeBoundaryImpl = window['$RC'];
  const resourceMap = window['$RM'];
  const dependencies = [];

  completeBoundaryWithStylesShared(resourceMap, dependencies, styles);

  Promise.all(dependencies).then(
    completeBoundaryImpl.bind(null, suspenseBoundaryID, contentID, ''),
    completeBoundaryImpl.bind(
      null,
      suspenseBoundaryID,
      contentID,
      'Resource failed to load',
    ),
  );
}

I'm not sure how maintainable it is to try to find "at which point" Closure stops inlining functions. The code duplication does feel silly -- an alternative would be to use rollup's replace, or even import from a forked module. However these feel quite heavy handed (and fragile, since this would need to be added in build.js, generate-inline-fizz-runtime.js, and fizzTestUtils.js.)
e.g.

let completeBoundaryImpl;
if (COMPILING_INLINE_JS) {
  completeBoundaryImpl = window['$RC'];
} else {
  completeBoundaryImpl = completeBoundary;
}

[mofeiZ]

It feels extra silly, since Closure will actually inline the variables correctly! 😅

function completeBoundaryWithStylesShared(
  completeBoundaryImpl,
  resourceMaps,
  suspenseBoundaryID,
  contentID,
  styles,
) {
  ...
}
window['$RR'] = function (
  suspenseBoundaryID,
  contentID,
  styles,
) {
  completeBoundaryWithStylesShared(
    window['$RC'],
    window['$RM'],
    suspenseBoundaryID,
    contentID,
    styles,
  );
}

gets compiled to this. Notice that the first two parameters (completeBoundaryImpl and resourceMaps) get inlined, but this useless wrapper function is still kept around.

function w(g,h,n){...} $RR=function(g,h,n){w(g,h,n)};

[acdlite]

Hmm my guess is that its not size alone, because there are some really large functions in the reconciler that Closure manages to inline.

How about if you move the non-shared function wrappers out of ReactDOMFizzInstructionSet.js and into the runtime module that imports it? Then, from Closure's perspective, it will only see a single call of the shared function. Because the other call is in the other runtime.

[mofeiZ]

I've tried this as well (and more drastically, only exporting the version referencing globals + only defining the shared function), with no luck. 😟

Also tried running this through rollup + closure

[acdlite]

That's surprising, huh

[acdlite]

This seems fine to me, unless @sebmarkbage or @gnoff have an alternate approach to suggest. I'm surprised running Closure on it doesn't just work if you extract out a shared function, but don't have any ideas (tried in the online playground and it doesn't work there, either).

[acdlite]

Thought of an idea. Maybe you could import the dependencies from a separate file, like this:

export const completeBoundaryImpl = window['$RC'];
export const resourceMap = window['$RM'];

import {completeBoundaryImpl, resourceMap} from './FizzRuntimeConfig';

Then in the Rollup config, you can override what the FizzRuntimeConfig module resolves to. That's the approach we use for the reconciler's host config — the host specific functions (i.e. DOM versus RN) get statically "injected" at build time.

Basically the same as your idea to use the replace plugin, but uses the module system instead of a string replace.

[gnoff]

I think the rollup option or forking makes sense. I'd prefer we not duplicate each function if we can avoid it

[gnoff]

It's too bad closure's @define syntax doesn't work with modules since they almost just support this out of the box but alas I don't think it will work for our situation

[gnoff]

@mofeiZ I'm working on some changes to renderIntoContainer that I think will require more substantial differences between the external and inline fizz runtimes.

https://github.com/facebook/react/pull/25703/files#diff-4715de966d011bb2b3e8982838a4ff0eaf93a8d877e6214102ad8bb24250fbf4R2627-R2636

If you take a look at these lines from this commit I am embedding the bootstrap scripts in a template element when using inline mode when there are also stylesheets to inject.

For external mode I was hoping to just embed the scripts directly as a child of the template that encodes the completion instruction.

The challenge is the completeContainer method that eventually is called after styles have either loaded or errored needs to relocate the bootstrap scripts from their template parent and into the body. However the way we locate the template in the DOM will be different depending on the mode you are in.

If you were able to get an implementation working that allowed us to have divergent behavior between each runtime I think that would be valuable

[mofeiZ]

Inline scripts and external runtime may require divergent implementations (or hand optimizations for code size). Since we do not need a bundle time fork, I split this to two differently named modules.

• ReactDOMFizzInstructionSetInlineSource is used only by yarn generate-fizz-runtime
• ReactDOMFizzInstructionSetExternalRuntime is used only by ReactDOMServerExternalRuntime

[mofeiZ]

@gnoff Would love any feedback on this - happy to make changes as needed

(I think ci/circleci: sizebot is failing due to credentials and yarn flow dom is failing to upstream/main issues)

[gnoff]

Works for me! I do think in the future we may want feature flags to be usable in these and perhaps then a rollup option will make sense. That said given the fact that the implementations can now be completely distinct we can potentially optimize them independently in a way that is nicer. More and more I suspect there will be divergence so seems reasonable we would just fork the entire thing like you did here

[gnoff]

a rebase will probably take care of the flow issue. seems fixed on main

[mofeiZ]

Manually ran yarn build to confirm other bundles did not change + external runtime became smaller