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

πŸ—πŸš€βœ¨ Significantly speed up gulp dist and gulp check-types #21939

Merged
merged 8 commits into from Apr 23, 2019
Merged

πŸ—πŸš€βœ¨ Significantly speed up gulp dist and gulp check-types #21939

merged 8 commits into from Apr 23, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Apr 23, 2019

Background:

What this PR does:

Adopts nailgun, which can start up any java binary in the background in server mode, and allow it to be reused during successive calls while eliminating the long start-up time.

Changes:

  • Switch build-system/runner/dist/runner.jar to a single jar instead of a jar-in-jar (to work with nailgun)
  • Copy the v1.0 nailgun binaries from https://github.com/facebook/nailgun to build-system/runner/nailgun/
  • Update gulpfile.js and build-system/tasks/compile.js to start up and use a nailgun server during gulp dist and gulp check-types (on supported platforms)

Running time improvements:

References:

Current release:

Fixes #10277

@rsimha
Copy link
Contributor Author

rsimha commented Apr 23, 2019

Okay, I've tested this PR a bunch of different ways, and it's yielding a significant speed up with no apparent downsides. (So far 🀞)

Ready for review!

/to @cramforce @erwinmombay @jridgewell @choumx

@rsimha
Copy link
Contributor Author

rsimha commented Apr 23, 2019

The numbers are in. On average, this PR results in a:

  • ~4x speed up of gulp dist (all extensions)
  • ~2x speed up of gulp dist (no extensions)
  • ~1.5x speed up of gulp check-types

See #21939 (comment) for more details.

@cramforce
Copy link
Member

Very cool! Can this also improve perf for local dev by keeping the compiler running in the background?

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

Amazing! Two little comments

build-system/runner/nailgun/nailgun-runner Outdated Show resolved Hide resolved
gulpfile.js Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Comments addressed.

Re: using this for local dev:

  • As written, a nailgun server will be used forgulp dist or gulp check-types on linux or macos, irrespective of whether it's local dev or CI. (The server is cleaned up whether or not the gulp task is successful.)
  • If you were asking about using this to start up an always-running compiler instance, it's definitely possible:
# Start runner.jar as a nailgun server
# You can drop the '&' and use a separate terminal for the server
$ java -XX:+TieredCompilation -server -cp \
third_party/nailgun/nailgun-server.jar:build-system/runner/dist/runner.jar \
com.facebook.nailgun.NGServer [PORT] &
NGServer 1.0.0-SNAPSHOT started on all addresses, port 2113.

# Use nailgun to compile code with runner.jar
$ echo "const foo = 42; function bar() { return foo * 2; }" > foo.js
$ third_party/nailgun/nailgun-runner [--nailgun-port PORT] \
org.ampproject.AmpCommandLineRunner -- --js foo.js
var foo=42;function bar(){return 2*foo};

# Stop nailgun server
# Ctrl+C works if the server is started without an '&'
$ third_party/nailgun/nailgun-runner [--nailgun-port PORT] ng-stop
NGServer shut down.

gulpfile.js Show resolved Hide resolved
build-system/runner/nailgun/nailgun-runner Outdated Show resolved Hide resolved
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

This brought gulp dist on my 2015 Macbook Pro from 34min to 6.58min. Also, my computer is actually usable during this time, unlike the old setup. 😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍

gulpfile.js Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Apr 23, 2019

I've tested this PR in a bunch of ways:

@rsimha rsimha merged commit 9008e3e into ampproject:master Apr 23, 2019
@rsimha rsimha deleted the 2019-04-22-ClosureNailgun branch April 23, 2019 21:20
@cramforce
Copy link
Member

RE: Local dev: Could you wrap that in a gulp task?

@rsimha
Copy link
Contributor Author

rsimha commented Apr 25, 2019

@cramforce Sent you #21977

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.

Significantly speed up gulp dist
5 participants