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

Use standalone .js bundle in dist tarball rather than individual JS files #3030

Merged
merged 8 commits into from
Apr 8, 2017

Conversation

Daniel15
Copy link
Member

@Daniel15 Daniel15 commented Apr 1, 2017

Summary

Instead of including all the raw JS files in the dist tarball, just use the single Yarn JS file that's built as part of the build, along with a few other files that are required. This significantly reduces the number of files in the tarball:

C:\src\yarn\dist (bundle-as-dist) (yarn@0.23.0-0)
λ find .
.
./bin
./bin/node-gyp-bin
./bin/node-gyp-bin/node-gyp
./bin/node-gyp-bin/node-gyp.cmd
./bin/yarn
./bin/yarn.cmd
./bin/yarn.js
./bin/yarnpkg
./bin/yarnpkg.cmd
./lib
./lib/v8-compile-cache.js
./lib/yarn-cli.js
./LICENSE
./package.json

There are three .js files in the archive:

  • lib/v8-compile-cache.js: Speeds up instantiation time by using the V8 code cache (https://www.npmjs.com/package/v8-compile-cache). This needs to be separate as it has to load before the bulk of the application code is loaded, so it can not be bundled
  • lib/yarn-cli.js: Contains all the bundled Yarn code
  • bin/yarn.js: Entry point to the app, just like today. Loads v8-compile-cache then loads yarn-cli

This change means that only the JavaScript files that are actually used are included, resulting in a nice file size reduction for the installation packages:

Differences are due to differing compression algorithms: Debian packages use xz or LZMA, RedHat uses gzip, Windows installer uses Cabinet

They're also slightly faster to extract:
image 3

Testing was performed on my desktop computer (Intel Core i5 6500, Samsung 850 Evo 1TB SSD, Windows 10), with testing for Linux stuff (like installing the Debian package) tested in a Docker container.

Raw data: https://docs.google.com/spreadsheets/d/1d8jdf3DU_GUFdotlPl08PkYa8SkzStK2tgnQ54ivsm0/edit?usp=sharing

Performance is very slightly faster when using v8-compile-cache along with the bundled file, but it's not extremely significant (yarn --version went from 0.19s to 0.14s on my BuyVM server). The difference might be bigger on servers with slower disks (HDD) or with more overloaded servers.

I also deleted the build-dist.ps1 file because we should be able to assume that Bash is available on Windows, particularly if Git is installed (as it comes with Git Bash). I need to verify that this works on AppVeyor.

Test plan
I tested the Debian package and Windows installer on my system. Both work as expected. I also manually inspected the tarball to ensure files were in the right place and bin/yarn still works as expected.

Wait for CI to pass before merging!

Closes #2966

cc @yarnpkg/core

@Daniel15
Copy link
Member Author

Daniel15 commented Apr 1, 2017

The Windows build is getting "stdout is not a tty" but it works fine on my computer... Will need to take a closer look.

@bestander
Copy link
Member

bestander commented Apr 1, 2017 via email

@cpojer
Copy link
Contributor

cpojer commented Apr 1, 2017 via email

@bestander
Copy link
Member

Love it so much!
Let me know when ready to merge.
What do you think of getting rid of roadrunner as well?
The extra cache layer for people who run from master branch does not make much impact.

@Daniel15
Copy link
Member Author

Daniel15 commented Apr 4, 2017

It looks like there's a build error in Node.js 4.x as build-webpack.js is using syntax it doesn't support: https://ci.appveyor.com/project/kittens/yarn/build/1942/job/33t035v195s3k19f

I'll update that, and then this should be ready to merge.

@cdaringe
Copy link

cdaringe commented Apr 7, 2017

does this affect:

  • support requests? if we use a bundle, the trace may be less helpful in the generated log file
  • debug-ability?

w/ how many issues are open ATM, it would make sense to me that support take precedence over these perf improvements (which do look great, btw).

just sharing my opinion!

@Daniel15
Copy link
Member Author

Daniel15 commented Apr 7, 2017

debug-ability?

People that are debugging Yarn itself would probably be using Yarn directly from this Git repo, which isn't affected by these changes

support requests? if we use a bundle, the trace may be less helpful in the generated log file

This is one thing I was worried about, however all the function names remain intact (no minification is performed) so stack traces shouldn't be too bad.

We could ship two tarballs, Debian packages and Windows installers if needed - a "debug" one that's like the tarball today, and a "release" one that's the smaller combined source file.

@bestander
Copy link
Member

From my experience of debugging Yarn a lot, a single file build almost never was a problem

@Daniel15
Copy link
Member Author

Daniel15 commented Apr 8, 2017

I'm going to merge this so we can get it into the next RC. We can iterate as needed. The AppVeyor build is passing, and the CircleCI failure is due to unrelated test timeouts (the actual build itself passed).

@Daniel15 Daniel15 merged commit 3af60cf into master Apr 8, 2017
@Daniel15 Daniel15 deleted the bundle-as-dist branch April 8, 2017 21:40
cp artifacts/yarn-legacy-*.js dist/lib/yarn-cli.js
cp bin/yarn-bundle-entry.js dist/bin/yarn.js
cp bin/{yarn,yarnpkg,*.cmd} dist/bin/
cp -r bin/node-gyp-bin dist/bin/
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to copy node-gyp from node_modules, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, good catch! Do we need it? I tried yarn install bcrypt and it seemed to work, but perhaps I have a globally-installed node-gyp or something like that. I can test on a fresh VM and see if it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I wonder if Yarn should install node-gyp when needed (ie. run yarn global add node-gyp) rather than bundling it? I think bundling node-gyp would also require us to bundle all of its dependencies, so we'll end up with a giant node_modules directory bundled with Yarn again :(

Copy link
Member

Choose a reason for hiding this comment

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

I agree.
That actually will solve the issue of single js file distributions.
Maybe Yarn could check if node-gyp binary is available and if not then install it.

Copy link
Member

Choose a reason for hiding this comment

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

The down side is that in some cases we Yarn might need sudo permissions?
What if Yarn would install node-gyp locally next to yarn.js?

Choose a reason for hiding this comment

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

Does this change anything about #2266?

Copy link
Member Author

Choose a reason for hiding this comment

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

@teohhanhui - We'll most likely update Yarn to automatically install node-gyp if needed (see #3114)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants