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

Split out compiler snapshot #1566

Merged
merged 6 commits into from Jan 29, 2019

Conversation

3 participants
@kitsonk
Copy link
Contributor

kitsonk commented Jan 23, 2019

This PR splits out the compiler from the Deno runtime. It is working, but has at least one issue, in that stack traces for errors inside of the compiler worker are not rewritten properly, because Rust does not have the source map for this.

There is a rather dramatic change in the snapshot size. Before:

-rw-r--r--  1 kkelly  staff    19M 23 Jan 21:34 snapshot_deno.bin

And after:

-rw-r--r--  1 kkelly  staff    18M 23 Jan 21:25 snapshot_compiler.bin
-rw-r--r--  1 kkelly  staff   950K 23 Jan 20:58 snapshot_deno.bin

as far as benchmarks I am getting this on my machine before the split:

Benchmark #1: /Users/kkelly/github/deno/target/debug/deno tests/002_hello.ts
  Time (mean ± σ):     116.7 ms ±   2.8 ms    [User: 83.7 ms, System: 29.9 ms]
  Range (min … max):   111.8 ms … 121.4 ms

Benchmark #2: /Users/kkelly/github/deno/target/debug/deno tests/003_relative_import.ts
  Time (mean ± σ):     116.6 ms ±   3.8 ms    [User: 83.8 ms, System: 29.8 ms]
  Range (min … max):   110.3 ms … 123.7 ms

Benchmark #3: /Users/kkelly/github/deno/target/debug/deno tests/error_001.ts
  Time (mean ± σ):     118.2 ms ±   5.1 ms    [User: 84.6 ms, System: 29.1 ms]
  Range (min … max):   110.3 ms … 135.3 ms

  Warning: Ignoring non-zero exit code.

Benchmark #4: /Users/kkelly/github/deno/target/debug/deno tests/002_hello.ts --recompile
  Time (mean ± σ):      1.497 s ±  0.010 s    [User: 2.367 s, System: 0.088 s]
  Range (min … max):    1.480 s …  1.510 s

Benchmark #5: /Users/kkelly/github/deno/target/debug/deno tests/003_relative_import.ts --recompile
  Time (mean ± σ):      1.587 s ±  0.055 s    [User: 2.488 s, System: 0.087 s]
  Range (min … max):    1.539 s …  1.725 s

And the following after the split:

Benchmark #1: /Users/kkelly/github/deno/target/debug/deno tests/002_hello.ts
  Time (mean ± σ):      85.8 ms ±   2.2 ms    [User: 63.9 ms, System: 19.2 ms]
  Range (min … max):    82.9 ms …  93.3 ms

Benchmark #2: /Users/kkelly/github/deno/target/debug/deno tests/003_relative_import.ts
  Time (mean ± σ):      85.1 ms ±   3.7 ms    [User: 64.2 ms, System: 18.1 ms]
  Range (min … max):    80.6 ms …  97.9 ms

Benchmark #3: /Users/kkelly/github/deno/target/debug/deno tests/error_001.ts
  Time (mean ± σ):      87.2 ms ±   2.9 ms    [User: 64.8 ms, System: 18.9 ms]
  Range (min … max):    83.4 ms …  95.2 ms

  Warning: Ignoring non-zero exit code.

Benchmark #4: /Users/kkelly/github/deno/target/debug/deno tests/002_hello.ts --recompile
  Time (mean ± σ):      1.464 s ±  0.022 s    [User: 2.343 s, System: 0.079 s]
  Range (min … max):    1.425 s …  1.492 s

Benchmark #5: /Users/kkelly/github/deno/target/debug/deno tests/003_relative_import.ts --recompile
  Time (mean ± σ):      1.574 s ±  0.025 s    [User: 2.479 s, System: 0.081 s]
  Range (min … max):    1.542 s …  1.617 s

That is about a 30% "warm" startup improvement!!! (and a less impressive improvement of 2% on "cold" startup)

@kitsonk kitsonk force-pushed the kitsonk:split_compiler branch from d67b6be to 7103bb2 Jan 25, 2019

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

kitsonk commented Jan 25, 2019

Ok, fixed the issues with the source maps. Need to restore --types and figure out how to unit test the compiler.

@ry ry self-requested a review Jan 25, 2019

@ry
Copy link
Collaborator

ry left a comment

Very nice! Faster and less code - the perfect kind of patch.

Show resolved Hide resolved BUILD.gn Outdated
Show resolved Hide resolved js/compiler.ts Outdated
Show resolved Hide resolved js/compiler.ts
Show resolved Hide resolved js/compiler.ts Outdated
Show resolved Hide resolved js/deno.ts
Show resolved Hide resolved js/globals.ts
Show resolved Hide resolved js/main.ts
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Jan 25, 2019

I'm happy to land with the compiler tests disabled for now. I think the stack traces into TS are also not so important. Just make sure there's TODOs and/or issues somewhere that document these problems.

kitsonk added some commits Jan 23, 2019

@kitsonk kitsonk force-pushed the kitsonk:split_compiler branch from 7103bb2 to 2264883 Jan 29, 2019

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

kitsonk commented Jan 29, 2019

I made a solution for --types and am looking at a better longer term approach. The error stack traces I had previously fixed. There are TODO's anywhere where there is something to mop up later.

@kitsonk kitsonk changed the title [WIP] Split out compiler snapshot Split out compiler snapshot Jan 29, 2019

@ry

ry approved these changes Jan 29, 2019

Copy link
Collaborator

ry left a comment

LGTM

Awesome to have this done! I think it's been 2 or 3 months since we first discussed splitting up the compiler in this way - glad to see it completed : )

@ry ry merged commit ee9c627 into denoland:master Jan 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@@ -34,7 +34,7 @@ impl Worker {
Some(internal_channels),
));

let snapshot = snapshot::deno_snapshot();
let snapshot = snapshot::compiler_snapshot();

This comment has been minimized.

@kevinkassimo

kevinkassimo Feb 2, 2019

Contributor

We need to make this back to deno_snapshot once we expose Workers

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