Add preparse optimization

[asakusuma]

Wanted to get the ball rolling on integrating broccoli-ember-preparse into the build. Preparse re-writes define statements as immediately executing functions for internal ember modules that have to be evaluated up front. This shaves off the overhead of lazily defining modules that don't need to be lazy. We're seeing about a 300ms boost on an older android phone on app boot.

This is at a POC stage. There are a few hacks:

• Wherever there is a direct require statement, the dependency has to be manually declared.
• The loader is hardcoded right now. Would be nice if it was actually pulled in the ember loader. This is not possible in it's current state without doing some ast-rewriting magic. The plumbing exists for supplying the loader file to be used by preparse, but it's not actually being used right now.

@stefanpenner @krisselden @rwjblue @chadhietala

[stefanpenner]

nice

[rwjblue]

We need to be careful about removing the ability to use/update etc internal modules. The pattern that ember-engines follows is a fairly solid one, and I believe this may make some of that impossible...

[stefanpenner]

I would love to see the output, and also be able to split eager/lazy chunks of ember. (currently far to much is forced eager...)

[asakusuma]

@rwjblue just to make sure I understand, you're saying that ember-engines access internal modules directly?

@stefanpenner is there a way to infer what's actually eager vs what is forced eager? Here is an example output: https://github.com/asakusuma/ember-preparse-patch

[rwjblue]

@asakusuma - Yes, using Ember.__loader.require. We can absolutely come up with a better pattern/path, I'm just saying we need to consider it...

[chadhietala]

@stefanpenner Almost all are eager I believe, something like 95% of them. Way too many.

@rwjblue I think this is more of an ordering thing. Since the dependency graph is pre-resolved at build time, the file should now be executed from top to bottom immediately populating the loader entries. If I remember correctly, as long as addons are south of Ember in the vendor.js things should continue to work as is. However, we should most definitely try some experiments before cutting over.

[krisselden]

@rwjblue this does not break Ember.__loader.require there are ember-cli-addons that use this has been tested with.

The purpose of this is to preload stuff directly into the loader that is required initially and to order it in the same order that it is required initially.

[krisselden]

@stefanpenner @rwjblue here is an example of a rewritten ember https://github.com/krisselden/ember-preparse-patch/blob/master/ember.prod.js @asakusuma do you have a more recent version?

[krisselden]

@asakusuma also, if this is done in ember js build we don't have to actually reparse out all the modules, we can just use the modules directly earlier in the pipeline.

[stefanpenner]

@stefanpenner Almost all are eager I believe, something like 95% of them. Way too many.

@chadhietala as described in a previous issue (which you were part of) with a little effort (and making things lazy as that issue also described) quite a large amount, especially legacy related stuff become lazy. Freeing us from many mixinApply's and additional code loading.

This work should ensure there is a reasonable path to also support lazy segments. This wouldn't require everything lazy being supporting, but just that it remains reasonably doable.

[stefanpenner]

The purpose of this is to preload stuff directly into the loader that is required initially and to order it in the same order that it is required initially.

Awesome, ya now that the output is provided (I have never actually seen the output of this) it looks great. Removing the outer closure, but not leaking anything not previously leaked, allows us to benefit from some global centric parsers, and pushing things eager (that are always eager) we avoid some wasted work.

I want to be careful, that can theoretically have control of lazy/eager even if (when we do it) it requires handwritten config/accomplish to accomplish.

[krisselden]

@stefanpenner the earlier benchmarks of lazy loading via defineProperty on Ember didn't boot an app (as far as I know), of course most of it will remain unevaluated. I would certainly be interested in knowing which modules are actually needed for a basic app. Currently 95% of ember's modules loaded when the most basic app is rendered. A portion of those are not needed, I don't have an idea what portion of those but I don't think that should be a blocker for improving the current situation.

[stefanpenner]

@stefanpenner the earlier benchmarks of lazy loading via defineProperty on Ember didn't boot an app (as far as I know),

I had a spike working

would certainly be interested in knowing which modules are actually needed for a basic app.

@krisselden I have enumerated this in the previous discussion: emberjs/ember.js#11576 (comment)

Currently 95% of ember's modules loaded when the most basic app is rendered. A portion of those are not needed, I don't have an idea what portion of those but I don't think that should be a blocker for improving the current situation.

Following the requires of legacy or largely defunct features yields a substantial sub-graph of both unneeded modules and various costly invocations such as repeated object design and mixin application that can be entirely avoided in a idiomatic 2x app.

Pushing the lazy work, to also include on-demand resolving completes the pay-go cost is a really nice win for apps not utilizing those features (ever or upfront)

In addition about 15%-20% of initial ember.js eval/run related costs were actually related to eager require of HTMLBars modules which provided features ember either doesn't use, or replaces entirely with its own constructors.

---

I am quite confused why I have to reiterate my findings and constantly lobby/justify the value of that work being pursued further, do I have a bad track record when It comes to identifying and addressing performance related issues?

The point raised each time, is ember requires 95% of the modules anyways making it lazy doesn't help, this is simply naive as a quick survey yields substantial portions of the code base that are merely artificially being retained. So switching internal imports that are used to build the initial container to on-demand resolves and adjusting a few accidental eager instantiations defeats that problem, but hinges on the lazy global work also being completed otherwise the eager consumption remains. Both efforts must be accomplish to reap the benefits. This work would enable a backwards compatible transition, while also paving the road for the much desired svelte builds.

---

If implemented correctly both efforts would yield some extremely nice wins, and enable our future plans. All i want, is to be sure this work doesn't present a hazard in lazy effort I believe is also of high value.

[krisselden]

@stefanpenner I'm confused, not asking you rejustify, I just said I didn't know the percentage. This is a win over the current situation, which loads 95% of modules, I stated I knew that some of the legacy code stuff is obviously not used but this PR does nothing to worsen that situation nor does it prevent the other from being worked on. I was just trying to make that last part clear not trying to debate you.

"If implemented correctly both efforts would yield some extremely nice wins" I 100% agree with this. I'm just trying to get a sense for what people needed for the above scope to be merged.

[krisselden]

@stefanpenner I think @asakusuma thought you when you said lazy you meant not making it eager and leaving it as define() I don't think he is aware of the defineProperty() lazy reexports strategy on the global.

[rwjblue]

Can someone explain what this actually does? I see in the output from what @krisselden linked above, that it looks fairly close to todays system (sorry for my mistake in assuming that things using Ember.__loader.require would break), but I don't understand the point or what is actually happening here. I'm sorry to need the crayon drawing breakdown here, but I tried to review https://github.com/asakusuma/broccoli-ember-preparse and https://github.com/chadhietala/define-rewriter but still don't grok it.

[asakusuma]

@rwjblue Kris can give a more in depth explanation, but the main difference in preparse vs the current build is the fact that in preparse, modules that have to be used up front are defined using an immediately executed function instead of being defined by a define statement. The reason this is faster is because there is some overhead in evaluating a module when V8 thinks it is lazy. When V8 sees the define, it goes on this lazy path which is not as fast as just evaluating the module right then and there if it has to be evaluated up front anyways.

In code terms, this:

(function (exports) {
  exports.myModule = { //stuff };
})(Ember._registry['myModule']);

is faster than this:

Em.__loader.define("myModule", ["exports"], function (exports) {
  exports.myModule = { //stuff };
});

when you have a bunch of these modules that have to be evaluated and used up front.

@krisselden @stefanpenner yes I did not know about the lazy stuff with defineProperty.

It sounds like the main conceptual issue is just making sure that we can lazily load some modules. I will work on this.

Any comments on the hacks I listed in the PR description? I supposed we could do some AST parsing to look for direct require statements to address manually declaring dependencies.

[nathanhammond]

Next dumb question: how does this work with our "Ember as an addon" story?

[stefanpenner]

Next dumb question: how does this work with our "Ember as an addon" story?
Show all checks

I think it should "just work" for now at least, once we allow deep imports into the ember bundle, we will likely need to revisit, but I believe the goal here is to only do this aggressive optimization for internal modules. Although @asakusuma or @krisselden should likely also speak to this brief to confirm my assumption.

@asakusuma @krisselden i merely want to be sure, this work doesn't block laziness (for things that can/should become lazy). Can you speak to this concern? I can gladly make myself available via hangout, to explain/share/brain-dump. I am generally +1, just want to make sure ^^ remains an options, as I believe it plays into our svelte build ideas.

[krisselden]

@nathanhammond Stef is correct, first phase of Ember as an addon will still be building the loader in. Next phase would be to build in better support for preloading into the loader.

@stefanpenner As I said this doesn't change the current situation, we will add lazy reexport, then when that removes the import it will stop being eagerly imported, because this currently is configure to follow dependencies from the "ember" module. At that point we'll need to test a minimal app to see what modules deserve to be eagerly loaded.

[stefanpenner]

@krisselden sounds good.

[stefanpenner]

@asakusuma whats pending here? Maybe a todo list or something so we can see who/what etc is blocking this from completing.

[asakusuma]

@stefanpenner I believe there is some debate as to whether we should allow the initial introduction of this feature via a "postprocess" operation on the dist (as it is now) vs a more integrated approach in the build.

Also, wondering if we can combine this with the string modules work done by @kratiahuja

[krisselden]

I've opened an issue with a TODO list

[stefanpenner]

Thanks for the pr, unfortunately we will be going in a slightly different direction. Although some of the learnings helped inform the new direction.

I want to be careful, that can theoretically have control of lazy/eager even if (when we do it) it requires handwritten config/accomplish to accomplish.

seems like we are moving to something more like ^

• trigging using parser heuristics to avoid preparse for things we need eagerly, and using preparse for stuff we most likely wont need right away
• hand tunable configurable
• will work with the above mentioned original "lazy effort"

@krisselden is pushing this to completion, he is working on some more validation now that he has something that works e2e (making sure something wasn't missed.