Inline host config #30

ManasJayanth · 2018-06-16T03:24:43Z

Attempts to close #27

ManasJayanth · 2018-06-16T03:28:19Z

Moving all deps to devDeps since they are all resolved during compile time.

jquense · 2018-06-16T13:07:36Z

Nice! I'm not sure about about the dependency move, what about all the runtime deps like dom helpers and react. Those are still needed at runtime and probably shouldn't be inlined right?

ManasJayanth · 2018-06-16T13:17:46Z

As much as I understand, not all (especially the large ones) functions are inline

…

On Sat 16 Jun, 2018, 6:37 PM Jason Quense, ***@***.***> wrote: Nice! I'm not sure about about the dependency move, what about all the runtime deps like dom helpers and react. Those are still needed at runtime and probably shouldn't be inlined right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC9BukH7CRr2Zx2qWY2vGmK_VLRki0_gks5t9QMZgaJpZM4UqVWL> .

jquense · 2018-06-16T13:25:48Z

I think we only want to inline react-reconciler, otherwise the bundle will include React and dom-helpers which will be a big bundle increase for folks since they will have duplicated copies of react, and anyone using dom-helpers will also be duplicated right?

ManasJayanth · 2018-06-16T13:47:28Z

I think the closure compiler takes a call on what to inline or not. And only small functions, where perf gains from avoiding extra stack frames are significant, get inlined.

…

On Sat 16 Jun, 2018, 6:55 PM Jason Quense, ***@***.***> wrote: I think we only want to inline react-reconciler, otherwise the bundle will include React and dom-helpers which will be a big bundle increase for folks since they will have duplicated copies of react, and anyone using dom-helpers will also be duplicated right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC9BumBhr5jyhXkCcmRMa6QB35Q1mvwRks5t9QdcgaJpZM4UqVWL> .

jquense · 2018-06-18T13:30:20Z

sure but it will still make the practical bundle size of ppl using RDL bigger if we let it do that. I don't think that's what we want

ManasJayanth · 2018-06-18T13:44:40Z

I think I misunderstood earlier. You're right. We're on the same page. I have re-introduced other deps back in package.json and marked them external for rollup.

Inlining was not seen in simple mode

ManasJayanth · 2018-06-18T15:22:14Z

Please compare the build sizes between commit 4a53dec and . This is because previously we didn’t inline npm deps in the cjs bundles.

I added fbjs so that we reuse some of the code thats bundled. It helps dedup some code.

Added index.js so that minified bundle is required too. Previously only unminified bundle was a part of package.json's main.

Had to use ADVANCED mode for closure compiler. We might have to add build level checks.

I had to remove flow defs for the host config too, for the time being. Will revisit tomorrow or so.

gaearon · 2018-06-18T15:22:55Z

Had to use ADVANCED mode for closure compiler

This is pretty hard to get working, I wouldn't recommend. Why do you need it?

ManasJayanth · 2018-06-18T15:24:16Z

$$hostconfig never got inlined in simple mode. @gaearon

ManasJayanth · 2018-06-18T15:27:04Z

Hello world app bundled (created using CRA) fell down to 23.76 KB

gaearon · 2018-06-18T15:29:08Z

It falling down doesn't mean everything works correctly though. I'd be very cautious.

If it doesn't get inlined, maybe we can explore something on our side to fix this.

ManasJayanth · 2018-06-18T15:35:53Z

Hello world app bundled (created using CRA) fell down to 23.76 KB

Sorry. This wasn't meant you justify use of advanced mode. I wanted to report the changes in build size. Probably should have updated my previous comment. You're right about the safety of the build.

I did try a couple of things like avoid unnecessary function calls so that gcc can statically analyse hostconfig. Eventually ran out of ideas.

ManasJayanth · 2018-06-18T15:40:51Z

@gaearon Can you please share some ideas to help inline the host config? My initial attempts were to make sure the object is statically analysable. I could have missed though.

ManasJayanth · 2018-06-19T02:04:43Z

I couldn't get the following to inline in simple mode :( Tested it on the online tool too

function log(m) {
  console.log(m);
}
log(m);

ManasJayanth · 2018-06-22T04:06:01Z

Just reporting it here. Dan is right about GCC - I got bit by GCC's advanced mode in a project I was working on.

ManasJayanth · 2018-07-01T12:52:19Z

I stand corrected. Figured why GCC didn't inline my code earlier: log was declared global. Following does get inlined.

function main(){
  function log(m) {
    console.log(m);
  }

  log('m');
  log('m');
  log('m');
  log('m');
  log('m');
  log('m');
  log('m');

}

main()

Inlined code

function main() {
  console.log("m");
  console.log("m");
  console.log("m");
  console.log("m");
  console.log("m");
  console.log("m");
  console.log("m");
}
main();

ManasJayanth · 2018-07-01T12:52:54Z

Reverting the advanced level changes shortly

ManasJayanth · 2018-07-16T15:02:59Z

I observed the following:

a.js

(function main() {

  const config = {
    log(m) {
      console.log(m);
    }
  }

  function reconciler(c) {
    const log = c.log;
    log('jere');
  }

  reconciler(config);

}())

a.min.js

(function() {
  (function(a) {
    a = a.log;
    a("jere");
  })({
    log: function(a) {
      console.log(a);
    }
  });
})();

b.js

(function main() {

  const config = {
    log(m) {
      console.log(m);
    }
  }

  function reconciler(c) {
    const log = c.log;
    c.log('here');
    // log('jere');
  }

  reconciler(config);

}())

b.min.js

(function() {
  console.log("here");
})();

The transform script moves host config inside the reconciler() to help GCC inline code better

ManasJayanth · 2018-07-29T23:53:03Z

Explanation

Trying to understand GCC's behaviour

I tried replicating the bundle's code behaviour of wrapping a config object and tried running GCC on it.

I got GCC to inline the following:

  const config = {
    log(m) {
      console.log(m);
    }
  }

  function reconciler(c) {
    const log = c.log;
    c.log('here');
    // log('jere');
  }

  reconciler(config);

But not the following which resembles the bundle better

  const config = {
    log(m) {
      console.log(m);
    }
  }

  function reconciler(c) {
    const log = c.log;
    log('jere');
    return {
      foo() {
        // More side effects
      }
    }
  }

  reconciler(config);

I tried a couple of other variations too. Reported it on SO and Google Groups. Tweeted about it too.

Most reliable response was from one of the maintainers of GCC on SO.

There is no one answer. The compiler uses heuristics and side effect calculations to decide when to inline. The compiler is also less likely to inline in the global scope than in nested scopes.

Thats when I decided to transform to the structure of our bundle to resemble React DOM's.

The babel transform

Basically, the scripts transforms,

  function createInstance() {
  }
  function appendChild() {
  }
  const config = {
    createInstance: createInstance
    appendChild: appendChild
  }
  function reconciler(config) {
    var ax = config.createInstance,
          by = config.appendChild;

   // ...          
  }
  reconciler(config);

to

  // const config = { ... }  is removed
  function reconciler(config) {
      var ax = function createInstance() {
      }
      var by = function appendChild() {
      }
      // ...          
  }
  reconciler({});

Gotchas

The transform depends on the line Reconciler(__DEV__ ? HostConfigDev : HostConfigProd) in the code. This is because of the reconciler(config); to reconciler({}); transform.

I figure what Reconciler(__DEV__ ? HostConfigDev : HostConfigProd) looks like in the bundle using mozilla's sourcemap consumer lib, and remove it from the ast and perform the rest of the operations.

I am still inspecting both transformed and the inlined code (using the newly added debug mode in GCC) and see if I can spot any potential issues. Another set of eyes would really help.

Will this transform work unconditionally?

Two things are absolutely necessary,

Any change to Reconciler(__DEV__ ? HostConfigDev : HostConfigProd) must be reflected in the transform script.
Config must to written in modules so that Rollup declares them as functions and const/let bindings. The transformer reliable on VariableDeclarations and FunctionDeclarations. Host configs must not longer be of the form { shouldSetTextContent(){ ... }, createInstance() {...} } as the transformer doesn't account for them

Please let me know your thoughts.

jquense · 2018-08-13T14:07:51Z

@prometheansacrifice what is the practical benefit we are getting here again, do we know how many bytes were saving this way? It's kinda convoluted so i'm not sure the added fragility to the build is worth it unless its a significant savings yeah?

jquense · 2018-08-13T14:08:09Z

flow-typed/react-reconciler.js

-      text: string,
-    ): void,
-  };
+  // declare export type HostConfig<T, P, I, TI, HI, PI, C, CC, CX, PL> = {


why are they commented out?

Those types, particularly ssr and mutation blocks need to be flattened.

jquense · 2018-08-13T14:08:58Z

package.json

-    "react": "^16.3.0",
-    "react-reconciler": "0.10.0",
-    "warning": "^3.0.0"
+    "fbjs": "^0.8.17"


i really don't want a dep on fbjs if we can help it, its more of an internal FB project, and REact itself is moving away from including it

Got it. It helped us avoid duplicate declarations for warning and other modules.

yeah i wonder if it's worth making out own one for that? I have a package react-tackle-box i dump random common stuff into, if it's helpful i could put stuff there

ManasJayanth · 2018-08-13T14:16:50Z

@jquense Did you get a chance to look at the GCC'd bundle?

jquense · 2018-08-13T14:18:39Z

I haven't had a chance to check yet :/ anything in particular you want me to see? Sorry it's been a while!

ManasJayanth · 2018-08-13T14:24:28Z

Run the build with debug: true. You notice that all host config has completely disappeared from the bundle. Bundle size drops a few KBs.

I understand the fragility you mentioned. Maybe we could put this behind a --experimental flag? I was wondering if upstream would support SweetJS style macros officially to inline config (but could take a serious look at it)

Without modifiying the AST, I exhausted all my options. Upstream works differently. Actually this transform tries to mimick the upstream AST after bundling.

ManasJayanth added 2 commits June 16, 2018 08:50

Inline npm deps so that GCC can inline

4a53dec

Log bundle size

598d471

ManasJayanth added the WIP label Jun 16, 2018

Update reconciler to 0.12.0

672a0f7

ManasJayanth force-pushed the inline-host-config branch from 841cd67 to 672a0f7 Compare June 18, 2018 13:20

ManasJayanth added 2 commits June 18, 2018 20:37

Inline conditional module loading

c12d12d

Use Java version of closure compiler

fc720b1

Inlining was not seen in simple mode

ManasJayanth requested a review from jquense June 18, 2018 15:20

ManasJayanth removed the WIP label Jun 18, 2018

ManasJayanth added the WIP label Jun 19, 2018

Back to using gcc js and simple mode

7f8202f

ManasJayanth force-pushed the inline-host-config branch from a7bc370 to 7f8202f Compare July 1, 2018 13:18

ManasJayanth added 2 commits July 26, 2018 16:55

Experimental: WIP: Adds a babel transform to move the host config

63abb8c

The transform script moves host config inside the reconciler() to help GCC inline code better

Fixes variable renaming bug

ad36f42

ManasJayanth removed the WIP label Jul 30, 2018

jquense reviewed Aug 13, 2018

View reviewed changes

ManasJayanth mentioned this pull request Sep 1, 2018

Appear to build in development mode #32

Open

Inline host config #30

Are you sure you want to change the base?

Inline host config #30

Uh oh!

Conversation

ManasJayanth commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManasJayanth commented Jun 16, 2018

Uh oh!

jquense commented Jun 16, 2018

Uh oh!

ManasJayanth commented Jun 16, 2018 via email

Uh oh!

jquense commented Jun 16, 2018

Uh oh!

ManasJayanth commented Jun 16, 2018 via email

Uh oh!

jquense commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManasJayanth commented Jun 18, 2018

Uh oh!

ManasJayanth commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Jun 18, 2018

Uh oh!

ManasJayanth commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManasJayanth commented Jun 18, 2018

Uh oh!

gaearon commented Jun 18, 2018

Uh oh!

ManasJayanth commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManasJayanth commented Jun 18, 2018

Uh oh!

ManasJayanth commented Jun 19, 2018

Uh oh!

ManasJayanth commented Jun 22, 2018

Uh oh!

ManasJayanth commented Jul 1, 2018

Uh oh!

ManasJayanth commented Jul 1, 2018

Uh oh!

ManasJayanth commented Jul 16, 2018

Uh oh!

ManasJayanth commented Jul 29, 2018

Explanation

Trying to understand GCC's behaviour

The babel transform

Gotchas

Will this transform work unconditionally?

Uh oh!

jquense commented Aug 13, 2018

Uh oh!

jquense Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

ManasJayanth Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

jquense Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

ManasJayanth Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

jquense Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

ManasJayanth commented Aug 13, 2018

Uh oh!

jquense commented Aug 13, 2018

Uh oh!

ManasJayanth commented Aug 13, 2018

Uh oh!

ManasJayanth commented Jun 16, 2018 •

edited

Loading

jquense commented Jun 18, 2018 •

edited

Loading

ManasJayanth commented Jun 18, 2018 •

edited

Loading

ManasJayanth commented Jun 18, 2018 •

edited

Loading

ManasJayanth commented Jun 18, 2018 •

edited

Loading