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

RFC for Prebuilding Addons #118

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@arthirm

arthirm commented May 3, 2018

This RFC is about Prebuilding addons prior to building the ember app to reduce the application build time.

Below is the summary of Cold Build timings from ember serve by Prebuilding only the first level addons. (Not addons inside another addons/apps)

New App

With prebuild - 1.5 secs (73% improvement)
Without prebuild - 5.7 secs
No of addons prebuilt - 7
No of addons visited - 29

Travis-Web

With prebuild - 57.3 secs (32% improvement)
Without prebuild - 84.5 secs
No of addons prebuilt - 36
No of addons visited - 69

  • Visited refers to the number of addons that were checked to see if it meets the criteria to be prebuilt.
-Check if the `isDevelopingAddon` flag is not set
-Check if the requested tree can be prebuilt (any of the default trees that can be prebuilt) use the prebuilt addon if it exists for the same target as the app
-Finds the md5 checksum of the application target and forms a prebuiltdirectory name by appending checksum of target, addon version, tree name

This comment has been minimized.

@twokul

twokul May 3, 2018

Contributor

prebuilt directory

@arthirm

This comment has been minimized.

arthirm commented May 3, 2018

Currently the implementation assumes that the trees to prebuild will be specified in package.json which will be changed in subsequent commit.

ember-cli POC (WIP)
arthirm/ember-cli@fb5f5a8

Prebuild addon
https://github.com/arthirm/ember-cli-prebuild-addon

If the application is using the incorrect prebuilt addon, the application build will break
-The latest version of target browser changes continuously hence the prebuilt directory might not necessarily be built for the latest version of the browser. But since current last version will always be a subset of future last versions the prebuilt directory will always be valid.

This comment has been minimized.

@kanongil

kanongil May 3, 2018

But since current last version will always be a subset of future last versions the prebuilt directory will always be valid.

This assertion is not correct. Features have been removed from browsers (eg. SharedArrayBuffer due to Spectre).

You also assume that Babel / preset-env is infallible, and that the generated output is always correct. This is NOT the case. If a transpilation issue arises, it will no longer be fixed with an upstream update, but instead requires the ember addon to publish an update, though no addon code has a changed.

@kellyselden

This comment has been minimized.

Member

kellyselden commented May 3, 2018

My comment from the meeting:

Not sure about others, but I often go into node_modules to tweak random addon code. Whether I'm trying to diagnose an addon bug before submitting a PR, or just to add console logging. Then I restart my server and the new code is there. We should be aware that people do this and help them discover that this method will no longer work with prebuilt addons.

It would be possible to detect this and do the right thing, but it would be complex.

@arthirm

This comment has been minimized.

arthirm commented May 3, 2018

Meeting notes:

  1. Templates should be prebuilt only from app.
  2. Logs from prebuilt addon will be stored in a file and link to it will be printed out. When prebuilding becomes stable, it can be removed.
  3. Commands
    1. ember prebuild
    2. ember prebuild:all
    3. ember prebuild:clear (invalidate prebuild)
    4. ember prebuild:rebuild
  4. Sorting, handling cases, checksum of yarn.lock or package.lock - to generate cache key for prebuilt addon
  5. Identify version of targets used for prebuilding. Have default set of targets with explicit target file names
  6. Git ignore prebuilt directories
  7. Better logging on why build breaks while using prebuilt addons
  8. What if an addon with isDevelopingAddon set to false is changed? (prebuild on demand, watch the directory and notify to prebuild.)
  9. Prebuilding Htmlbars inline precompile
@ef4

This comment has been minimized.

Contributor

ef4 commented May 4, 2018

One concern is that there is almost certainly state/configuration other than the browser targets that influences the build. I think we need to design the prebuilt cache such that it can accomodate an arbitrarily rich summary of all the settings that influenced a particular bundle, and match those again when determining if there is a valid bundle in the cache.

(Consider for example that addons are going to soon be gaining the ability to expose different behavior when used in classic app layout vs module unification layout. Or that @ember/optional-features is now a thing that addons may choose to pay attention to when deciding how to build themselves.)

Also, if we are focused on publishing prebuilt bundles inside addons, the more targets you support, the bigger your NPM packages get. I don't really think NPM is the best vehicle for distributing many targets worth of bundles. Where NPM itself confronts this same issue for native extensions, they don't bake all the prebuilt targets into the NPM package, they offload them to storage elsewhere.

So I would rather emphasize the parts of this RFC that would enable shared, persistent build caching as the base primitive. If we focus on designing the format for the build cache that can be easily

  • read to see if there are prebuilt trees available that meet your needs
  • written to add new ones

we can then separate the question of where to store those caches and how to pick the set of targets. Maybe sometimes it will be fine to put cached builds inside npm packages. Maybe sometimes they could be separate hosting (like s3) controlled by addon authors. Maybe some companies will want to have their own shared caches that are used by all their developers -- the first person to build a new addon under a new target will do the full build and then update the shared cache for everyone else (automatically, as long as you flip a switch saying you want to share your results with the cache).

@rtablada

This comment has been minimized.

rtablada commented May 5, 2018

While I understand the want to reduce build times I’m not quite sure the value of prebuilding beyond what broccoli or similar would already have in a warm cache/temp dir.

As we begin tree shaking and possibly parallelizing builds this could become a possible source for conflicts or inconsistent results.

Something we could do instead though is to better surface this cache for things like CI where a warm tmp directory may not be available.

@jrjohnson

This comment has been minimized.

jrjohnson commented May 23, 2018

It is unclear to me after several reads where the responsibility for pre-building lives. It seems like addon authors need to ship (some/many/all) pre-built targets during NPM publish, but at the same time there is an addon for the app that handles pre-building there? And if addon and app targets don't match then what?

This leaves me confused about what the benefit is. Will I most likely just be adding ember prebuild:all to a bash alias to run before I run ember serve? Or will most addons actually ship a target that I can use? Since I, most likely, wouldn't want to trust possible stale pre-builds in production this seems like a development time feature, but in that case shouldn't this behavior just be part of the initial cold build for ember serve or a flag on that command ember serve --prebuild-addons?

@rtablada

This comment has been minimized.

rtablada commented May 23, 2018

So after taking a while to let this sit in and getting some inspiration from #ember2018 posts, I think this is a fairly convoluted way around a problem.

So there's a few painpoints that definitely do exist:

  • Initial Build Time With Many Addons
  • Watching and Build Trees that are added by addons that continue to run (but mostly use the existing cache)

This seems like a way to cut down the initial build time, but it really adds a lot of complexity to projects and user land.
What if targets published by an addon don't match my project?
Then I have to know to run ember prebuild for that addon.
Then what is best practice for what to do with those prebuilt files, should they be local only, ignored, committed: that is something that will likely be different from team to team.

Also spotting and debugging different compile targets could be really tricky since these errors could not be obvious as to what is causing the error.

The solution to that would be to add a lot of complex target diff checking into Ember CLI which makes it harder to understand what CLI is doing (which is 👎 to me when the community wants to demystify and remove complexity).

This would also mean that you're likely going to have to prebuild these addons for your app anyway because of differences in targets and babel configs.


This said, I think the problem of addons build trees being active in file watching and intermediary files in the /tmp directory is something that could be an interesting problem to solve.

As far as I know right now addon broccoli trees are active during the entire app serve command.
While the cost of watching and rebuilds are pretty limited since your addon code is unlikely to change during development (unless you are debugging like @kellyselden mentioned) this small cost does add up when multiplied over many addons with many broccoli trees each.

Something that could help with this is an option like --no-rebuild-addons (naming is hard ok?) where the addon trees are built on initial load but they are not watched and the intermediary trees are cleaned up as well.
This is a style of prebuild that could lessen the resources needed after initial load since there would be fewer broccoli trees active.

The idea of an opt out would be tricky to manage but there are those who know a lot more about CLI and Broccoli internals that might be able to run with that idea or build on it.

@ef4

This comment has been minimized.

Contributor

ef4 commented May 24, 2018

We already don’t watch addon trees by default.

@rtablada

This comment has been minimized.

rtablada commented May 25, 2018

@ef4 it may depend on the addon, but from my experience most addons are watched for changes.

I just went into one of my apps which has no config changed and changed a template from ember-power-select and this triggered a rebuild and the changes were watched.

screen shot 2018-05-25 at 8 47 07 am

The file I changed was node_modules/ember-power-select/addon/templates/power-select-multiple.hbs

@ef4

This comment has been minimized.

Contributor

ef4 commented May 25, 2018

Huh, I guess my info is out of date. The code that makes the decision is here. I don't know the history fo that TODO comment.

@arthirm

This comment has been minimized.

arthirm commented Jul 16, 2018

@kellyselden

Not sure about others, but I often go into node_modules to tweak random addon code. Whether I'm trying to diagnose an addon bug before submitting a PR, or just to add console logging.

I agree that many people will change the code in node_modules directly. One way to handle this is to exclude the addon that you want to change from using this feature. It can be provided as an env variable or can be specified in package.json of the project (app/addon). The updated RFC explains this in detail. Let me know what you think about it.

@arthirm

This comment has been minimized.

arthirm commented Jul 16, 2018

@jrjohnson

It is unclear to me after several reads where the responsibility for pre-building lives

Both addons and app can prebuild.

This leaves me confused about what the benefit is.

The major benefit can be achieved in cold build if the prebuild is stored and reused. In our large example app the babel transpilation takes half the time and transpiling addons takes half of the babel time during cold build.
I agree with you that addon and app targets might not have to match always. I believe that storing the prebuild from an application build will have more benefit.

wouldn't want to trust possible stale pre-builds in production this seems like a development time feature

Cache key takes addon version (name, package.json, babel options, target browsers) into account. The 2 cases when the code will change but the version will not change is when the addon is symlinked or when the package.json has addon pointing to gitpath instead of version. These 2 cases are handled. Currently as a first step the idea is to let individual developers use this as part of their cold build.

shouldn't this behavior just be part of the initial cold build for ember serve

With the new implementation there is no separate addon to prebuild. Right now as you said this behavior will be part of initial cold build or serve. It uses environment variable. We can decide on whether to use a flag or env variable.

@arthirm

This comment has been minimized.

arthirm commented Jul 17, 2018

@rtablada

tree shaking and possibly parallelizing builds this could become a possible source for conflicts

This should not be a source of conflict rather they should all co-exist with each other. Like how tree shaking will help in sitespeed, prebuild will help with build time.

surface this cache for things like CI where a warm tmp directory may not be available.

Yes, It can be done if the prebuild is stored in a location where it can be fetched in CI as well.

Also spotting and debugging different compile targets could be really tricky since these errors could not be obvious as to what is causing the error

Prebuild will generate a log file which will have information about the addon name, prebuild path, treeType etc. which can be used to check whether prebuild is being used and the location it fetches prebuild from.

Likewise if there is an issue with prebuild of an addon we can clear the prebuild for that addon alone or exclude the prebuild for that addon.

problem of addons build trees being active in file watching and intermediary files in the /tmp directory is something that could be an interesting problem to solve

That is a very good idea that can be explored. Let me know what you think about the updated RFC.

@arthirm

This comment has been minimized.

arthirm commented Jul 17, 2018

@ef4

One concern is that there is almost certainly state/configuration other than the browser targets that influences the build.

Currently the cache key is determined by the following

    addon.name,
    addon.pkg && Object.keys(addon.pkg).sort()
    typeof addon.options.babel === 'function' ? addon.options.babel() : addon.options.babel,
    addon.options['ember-cli-babel'],
    targetBrowsers

this can be enhanced to add more metadata to determine a valid configuration for the prebuild.

Consider for example that addons are going to soon be gaining the ability to expose different behavior when used in classic app layout vs module unification layout. Or that @ember/optional-features is now a thing that addons may choose to pay attention to when deciding how to build themselves.

We are prebuilding only the addon, addon-test-support and templates tree of an addon. My understanding is they should impact only the app code and not these trees. Please correct me if am wrong.
One way to handle continuous features that land in ember-cli like MU is to create the cache key using ember-cli version as well which can be added to the list of attributes above.

I don't really think NPM is the best vehicle for distributing many targets worth of bundles.

Yes I agree with your idea about storing the prebuilt directories. The path will be made configurable so that the addon or the app can specify the location to store prebuilt directories. Be it npm or s3 or company repo. If no path is specified it can default to npm package. Currently as a first step the idea is to let individual developers use this as part of their cold build.

@stefanpenner

This comment has been minimized.

Contributor

stefanpenner commented Sep 13, 2018

@arthirm this all looks very good and I still think this is a good idea to pursue, right now we have much churn in this area:

  • packager
  • delayed transpilation
  • @ef4 has something else cooking
  • etc. etc.

It would make me nervous to commit to this until some of the above pieces have also landed. That being said, have done the research here is awesome. That way we can utilize it when we are ready.


@arthirm that being said, one big question I have, is how does this work with babel-plugin-htmlbars-inline-precompile which is a default babel plugin, but also explicitly depends on the template compilter provided by that version of ember?

I don't think this is an insurmountable problem, but does seem like a blocker until resolved. What solutions do you see?

@arthirm

This comment has been minimized.

arthirm commented Sep 25, 2018

@stefanpenner

one big question I have, is how does this work with babel-plugin-htmlbars-inline-precompile which is a default babel plugin, but also explicitly depends on the template compilter provided by that version of ember?

The simplest approach is to include the ember version as well in the cache key while generating a prebuild. While building the app, if the ember version of the app matches with the ember version used in the cache key then the prebuilt template tree can be used. Let me know if you think of any other approach.

@arthirm

This comment has been minimized.

arthirm commented Oct 1, 2018

@stefanpenner

But the problem with associating ember version with the cache key is there may be many projects which wont be using the same ember version as the one used in prebuild. In those cases it will be less useful where it is better to prebuild templates from app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment