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.

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

[Expriment] Lazy rexports + pay as you go #11576

Closed
wants to merge 8 commits into from

Conversation

stefanpenner
Copy link
Member

the theoretical max (for this optimization, i have ideas for further yet) is 70ms from 170ms. Without the costly HTMLbars stuff its about 97ms, with its about 120ms, mobile android appears to be much more of a win.

Posting this here, maybe i'll have time to explore further. Or others have time to help

In theory the 70ms max can be reduce further, byleaving more inert. Safari appears to do this out-of-the-box, v8 appears to need more hints.

  • rebase
  • fix test-suite
  • make the HTMLBars code pay as you ago.
  • test on mobile devices (quick tests on my android indicate a significantly larger boost)

@stefanpenner
Copy link
Member Author

cc @hjdivad

@hjdivad
Copy link
Member

hjdivad commented Jun 29, 2015

@stefanpenner something we didn't do was separate measurements gain of reexport vs not doubling the arglist. Probably most of the gain came from reexport but if not that might suggest that there's some gain to be made in minifying module names & deps.

@stefanpenner
Copy link
Member Author

@hjdivad ya we should

@bcardarella
Copy link
Contributor

Wow, this would be awesome if it lands

@stefanpenner
Copy link
Member Author

@eviltrout @samsafron this may interest you

@stefanpenner
Copy link
Member Author

new V8 side improvements: v8/v8@33ec0b7

@@ -1138,7 +1125,7 @@ function registerLibraries() {
librariesRegistered = true;

if (environment.hasDOM) {
Ember.libraries.registerCoreLibrary('jQuery', jQuery().jquery);
Ember.libraries.registerCoreLibrary('jQuery'. require('ember-views/system/jquery'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be a , instead of .

@stefanpenner
Copy link
Member Author

cc @SamSaffron

@stefanpenner
Copy link
Member Author

For anyone interested and motivated this will likely be a non-trivial amount of work to complete, but totally possible.

I will gladly make myself available via google hangout (or in person if you are relatively local to me) to come up with plan of attack.

cc @dgeb this is somewhat related to your container work, so you may also be on-board.

note: this should also play into the future "swelt" builds and tree shaking plans

@cibernox
Copy link
Contributor

@stefanpenner I think I will have time to help this this. Startup time improvement is certainly something I'd like to improve.

Could you briefly explain me a bit more how to tackle it and in particular how to benchmark if i'm making progress?

Also I know that ember itself was about to get refactored into es6 modules. Will be better to wait for that before tacking this? Alternatively if the modules structure has been fully decided I could modularize the internals myself.

@stefanpenner
Copy link
Member Author

@cibernox I would love to have you help with this, I believe this work and land incrementally and will likely only improve the pure es6 build of the future.

We should sync up on a hangout later this week.

@chadhietala
Copy link
Contributor

After @krisselden and I did investigation around this I'm not sure where the value add is. In Ember you need roughly all of the modules upfront and it's better to make these eager rather than lazy to avoid V8's lazy-preparse on modules that you need pretty much immediately. We hope to upstream the work we did to allow for the overhead of the internal to be much less. The only thing blocking this is refactoring the broccoli plugin Kris and I worked on to account for the new "index" files and some circular dependencies on ember-debug.

@chadhietala
Copy link
Contributor

More longer term I believe the packages themselves need to be refactored to explicitly expose APIs and that the packages use rollup to tree shake and create 1 large module per package.

@stefanpenner
Copy link
Member Author

@chadhietala that isn't true.

Many costly modules are included and ultimately instantiated by mistake. In addition this is also an incremental step to continue to decouple and improve the lazy/eager/svelte plans.

Without this step, we will hinder future work and with it we will see improvements incrementally.

@kiwiupover
Copy link
Contributor

One of the issues I ran into looking into this work was the circular includes in the tests.

@stefanpenner
Copy link
Member Author

@kiwiupover yes. a large included task here is some much needed cleanup. Which will nicely compound value with some future plans.

@kiwiupover
Copy link
Contributor

@stefanpenner what is the next step in unraveling the tests to remove the circular dependencies.

@stefanpenner
Copy link
Member Author

@stefanpenner what is the next step in unraveling the tests to remove the circular dependencies.

I believe you just described the next task in fix test-suite

@kiwiupover
Copy link
Contributor

@stefanpenner too right it is the next task. I want to help I will hit up @krisselden for help.

@cibernox
Copy link
Contributor

@stefanpenner @chadhietala So, at the end, is still unclear if this will bring performance benefits? I just have some OSS time that I'd like to use to tackle the modularization of ember itself to bring tree shaking asap to the ember ecosystem, and I thought this could be helpful to gain some knowledge about how ember is divided internally.

@stefanpenner
Copy link
Member Author

@stefanpenner @chadhietala So, at the end, is still unclear if this will bring performance benefits?

their are performance benefits.

@stefanpenner
Copy link
Member Author

In Ember you need roughly all of the modules upfront

Many modules are not, for example in my tests 20% total worth of just HTMLBars modules which ember totally overrides.

A quick-scan of https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application.js#L1046-L1104 already yields many eager registrations that should become pay-as-you-go. Remember our plan is to work towards svelte builds, if we are forced to eagerly consume everything that doesn't work.

How many apps use all of Ember.Select, LegacyEach, Checkbox, TextArea, TextField and all of the locations services? Individually these may not seem relevant, but when we compound all the extra baggage is adds up quickly.

If we follow those optional pieces back we will see they are the retainers of many more modules. The remaining retainers of costly & potentially un-used code can then be focused on.

One example of this is, CollectionView which view/index eagerly consumes but is actually guarded behind ENABLE_LEGACY_VIEW_SUPPORT, which actually guards more accidentally eagerly compute modules:

  • Ember.CoreView
  • Ember.View
  • Ember.View.states
  • Ember.View.cloneStates
  • Ember.View._Renderer
  • Ember.ContainerView;
  • Ember.CollectionView;

A bonus, as we make these lazy they will be easier to extract as add-ons, further reducing byte size.

and it's better to make these eager rather than lazy to avoid V8's lazy-preparse on modules that you need pretty much immediately.

This is actually unclear to me, my experiments suggested we will need to tune this and one way or the other wont be ideal.

It is also worth noting, especially as users want to run small parts of their apps in service-workers or web-workers, we shouldn't force them to eagerly pay for the aspects of ember they wont use (like the view layer).

@locks
Copy link
Contributor

locks commented Jan 21, 2017

@stefanpenner is this still relevant?

@stefanpenner stefanpenner deleted the lazy-rexports branch April 5, 2017 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants