Create an old program to be used in snapshot. #3644

kitsonk · 2020-01-10T00:18:29Z

Resolves #3111

This PR caches an "oldProgram" which is effectively the AST of all the foundational libraries as part of the snapshot, which then is re-used when an additional compiler invocation happens.

Original PR text for context

@ry @bartlomieju I'm pretty sure this approach will speed up TypeScript compilation, but I am running across a couple of issues that aren't easy for me to figure out how to resolve. The build is failing during the creation of the compiler snapshot at the unwrap in core bindings:

deno/core/bindings.rs

Lines 438 to 440 in 525784e

    
           let op_id = v8::Local::<v8::Uint32>::try_from(info.get_argument(0)) 
        
             .unwrap() 
        
             .value() as u32;

I am not really getting a good error message here (feels like we might want to improve that), but I suspect this is because the code in the snapshot actually needs access to a single op from the CLI:

deno/cli/ops/compiler.rs

Lines 23 to 26 in 525784e

    
           i.register_op( 
        
             "fetch_asset", 
        
             s.core_op(json_op(s.stateful_op(op_fetch_asset))), 
        
           );

Which also would need to have access to all the inlined text resources. It needs to read out some of that, to generate the AST, so that that AST can be reused later (warming up the compiler). I am not sure how to provide OPS to an isolate during snapshotting.

bartlomieju · 2020-01-10T00:35:23Z

@kitsonk if snapshot isolate needs that op you'll probably want to create it in deno_typescript/ops.rs.

If you have troubles I can look into that in the morning.

bartlomieju · 2020-01-10T00:38:08Z

Hmmm, it looks like that's not the problem. I think it's already used. To be hones the error in CI looks like Deno.core.dispatch(SOME_OP_ID, control) is called with undefined instead of a number, which suggests that Deno.core.getOps() was not called and did not hydrate op id store.

I got intrigued let me tinker with it

kitsonk · 2020-01-10T00:58:27Z

@bartlomieju ah, that is true, I think some of the boilerplate ops stuff hasn't run yet in the compiler.ts. I need to move it to denoMain I think. Let me see if that works, thanks!

bartlomieju · 2020-01-10T01:00:15Z

@kitsonk this patch will get you further

diff --git a/cli/js/compiler.ts b/cli/js/compiler.ts
index 02bec614..b5a6375f 100644
--- a/cli/js/compiler.ts
+++ b/cli/js/compiler.ts
@@ -5,6 +5,8 @@
 import "./globals.ts";
 import "./ts_global.d.ts";
 
+import { core } from "./core.ts";
+import * as dispatch from "./dispatch.ts";
 import { TranspileOnlyResult } from "./compiler_api.ts";
 import { setRootExports } from "./compiler_bundler.ts";
 import {
@@ -85,6 +87,16 @@ self.workerMain = workerMain;
 let oldProgram: ts.Program;
 
 ((): void => {
+  const ops = core.ops();
+  console.error("ops received, op map: ", JSON.stringify(ops));
+  // TODO(bartlomieju): this is a prototype, we should come up with
+  // something a bit more sophisticated
+  for (const [name, opId] of Object.entries(ops)) {
+    const opName = `OP_${name.toUpperCase()}`;
+    // Assign op ids to actual variables
+    // TODO(ry) This type casting is gross and should be fixed.
+    ((dispatch as unknown) as { [key: string]: number })[opName] = opId;
+  }
   const host = new Host({
     bundle: false,
     writeFile(): void {}
diff --git a/deno_typescript/lib.rs b/deno_typescript/lib.rs
index 347779d2..fec77f5a 100644
--- a/deno_typescript/lib.rs
+++ b/deno_typescript/lib.rs
@@ -204,6 +204,10 @@ pub fn mksnapshot_bundle_ts(
   state: Arc<Mutex<TSState>>,
 ) -> Result<(), ErrBox> {
   let runtime_isolate = &mut Isolate::new(StartupData::None, true);
+  runtime_isolate.register_op(
+    "fetch_asset",
+    compiler_op(state.clone(), ops::json_op(ops::fetch_asset)),
+  );
   let source_code_vec = std::fs::read(bundle)?;
   let source_code = std::str::from_utf8(&source_code_vec)?;
 
diff --git a/deno_typescript/ops.rs b/deno_typescript/ops.rs
index e4534959..208abaf4 100644
--- a/deno_typescript/ops.rs
+++ b/deno_typescript/ops.rs
@@ -135,3 +135,20 @@ pub fn set_emit_result(s: &mut TSState, v: Value) -> Result<Value, ErrBox> {
   s.emit_result = Some(v);
   Ok(json!(true))
 }
+
+
+#[derive(Deserialize)]
+struct FetchAssetArgs {
+  name: String,
+}
+
+pub fn fetch_asset(
+  _s: &mut TSState, v: Value
+) -> Result<Value, ErrBox> {
+  let args: FetchAssetArgs = serde_json::from_value(v)?;
+  if let Some(source_code) = deno::js::get_asset(&args.name) {
+    Ok(json!(source_code))
+  } else {
+    panic!("op_fetch_asset bad asset {}", args.name)
+  }
+}

This is really band-aid and it just ask to do stuff described in #3116

kitsonk · 2020-01-10T01:20:31Z

Ok, thanks... yeah, moving to denoMain means it still doesn't get called until part of the snapshot. You are right, your patch should get me closer!

kitsonk · 2020-01-10T03:04:33Z

@bartlomieju got further, but hitting some sort of strange snapshot error in shared_queue.js now:

global handle not serialized: 0x3bae09a0a80d: [JSArrayBuffer] in OldSpace
 - map: 0x3bae08281f41 <Map(HOLEY_ELEMENTS)> [FastProperties]
 - prototype: 0x3bae0926b11d <Object map = 0x3bae08281f69>
 - elements: 0x3bae080406e1 <FixedArray[0]> [HOLEY_ELEMENTS]
 - embedder fields: 2
 - backing_store: 0x7f8e4b84d800
 - byte_length: 13612
 - external
 - shared
 - properties: 0x3bae080406e1 <FixedArray[0]> {}
 - embedder fields = {
    0, aligned pointer: 0x0
    0, aligned pointer: 0x0
 }
global handle not serialized: 0x3bae0981d2a1: [Function] in OldSpace
 - map: 0x3bae08280281 <Map(HOLEY_ELEMENTS)> [FastProperties]
 - prototype: 0x3bae09261915 <JSFunction (sfi = 0x3bae08204f21)>
 - elements: 0x3bae080406e1 <FixedArray[0]> [HOLEY_ELEMENTS]
 - function prototype: 
 - initial_map: 
 - shared_info: 0x3bae0926db19 <SharedFunctionInfo handleAsyncMsgFromRust>
 - name: 0x3bae0926d639 <String[#22]: handleAsyncMsgFromRust>
 - builtin: CompileLazy
 - formal_parameter_count: 2
 - kind: NormalFunction
 - context: 0x3bae0981d035 <FunctionContext[25]>
 - code: 0x3bae00043481 <Code BUILTIN CompileLazy>
 - source code: (opId, buf) {
    if (buf) {
      // This is the overflow_response case of deno::Isolate::poll().
      asyncHandler(opId, buf);
    } else {
      while (true) {
        const opIdBuf = shift();
        if (opIdBuf == null) {
          break;
        }
        asyncHandler(...opIdBuf);
      }
    }
  }
 - properties: 0x3bae080406e1 <FixedArray[0]> {
    #length: 0x3bae08200339 <AccessorInfo> (const accessor descriptor)
    #name: 0x3bae082002f5 <AccessorInfo> (const accessor descriptor)
    #arguments: 0x3bae0820026d <AccessorInfo> (const accessor descriptor)
    #caller: 0x3bae082002b1 <AccessorInfo> (const accessor descriptor)
    #prototype: 0x3bae0820037d <AccessorInfo> (const accessor descriptor)
 }
 - feedback vector: feedback metadata is not available in SFI

--- stderr


#
# Fatal error in ../../../../../.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/rusty_v8-0.0.23/v8/src/api/api.cc, line 812
# Check failed: handle_checker.CheckGlobalAndEternalHandles().
#
#
#
#FailureMessage Object: 0x7ffee884a730

kitsonk · 2020-01-10T05:27:09Z

I thought maybe it was because I wasn't setting an asyncHandler, but doing that doesn't fix the issue.

I narrowed it down, to if I call core.dispatch() with any op, the op completes fine, but then when to go take the snapshot I get the serialization error... which seems to be related to the init() getting called and setting up the shared core. If I remove the maybe_init() from dispatch() everything seems to work.

I've run into other problems with re-using the old program, but they are now just TypeScript compiler related.

kitsonk · 2020-01-10T05:54:36Z

benchmarks on my machine with master:

Benchmark #4: /Users/kkelly/github/deno/target/debug/deno run --reload tests/002_hello.ts

  Time (mean ± σ):      4.255 s ±  0.100 s    [User: 5.331 s, System: 0.077 s]
 
  Range (min … max):    4.148 s …  4.432 s
 
Benchmark #5: /Users/kkelly/github/deno/target/debug/deno run --reload tests/003_relative_import.ts

  Time (mean ± σ):      4.454 s ±  0.155 s    [User: 5.425 s, System: 0.094 s]
 
  Range (min … max):    4.247 s …  4.690 s

with this patch:

Benchmark #4: /Users/kkelly/github/deno/target/debug/deno run --reload tests/002_hello.ts

  Time (mean ± σ):      2.726 s ±  0.050 s    [User: 3.373 s, System: 0.061 s]
 
  Range (min … max):    2.670 s …  2.822 s
 
Benchmark #5: /Users/kkelly/github/deno/target/debug/deno run --reload tests/003_relative_import.ts

  Time (mean ± σ):      2.792 s ±  0.111 s    [User: 3.411 s, System: 0.065 s]
 
  Range (min … max):    2.710 s …  3.092 s

🎉 🎉

Still not as quick as I would like it, but hey, that is pretty good.

cli/js/compiler.ts

bartlomieju · 2020-01-10T09:44:06Z

core/shared_queue.js

@@ -180,7 +180,6 @@ SharedQueue Binary Layout
  }

  function dispatch(opId, control, zeroCopy = null) {
-    maybeInit();


This is very tricky and brittle, ideally we'd want to remove this completely, but if tests are green for now I'm okay 👍

We shove maybeInit() eveywhere because we weren't sure... clearly it is overkill. I can't see of a code path that gets here and initting the shared queue during snapshotting causes a serialization problem.

That's right it's very convoluted. I already started playing with completely removing shared_queue but surprisingly I get horrible performance and need to put some more work. Ditto, if it passes tests I'm okay with that

cli/js/compiler.ts

bartlomieju · 2020-01-10T09:45:20Z

deno_typescript/lib.rs

@@ -204,6 +204,10 @@ pub fn mksnapshot_bundle_ts(
  state: Arc<Mutex<TSState>>,
 ) -> Result<(), ErrBox> {
  let runtime_isolate = &mut Isolate::new(StartupData::None, true);
+  runtime_isolate.register_op(
+    "fetch_asset",


It may be worth adding a comment there that this op is added just to handle hydration of oldProgram - ideally we'll use CompilerIsolate here (after landing #3116)

actually, I realised we don't need this OP as part of the cli when we do it this way, I can strip that out... because everything gets parsed and becomes part of the snapshot, basically the op never fires and doesn't need to be registered in the compiler isolate.

kitsonk · 2020-01-12T10:06:07Z

@bartlomieju I went back to fix my patch (removing the fetch_asset didn't work because I still had one hiding out there), and I rebased, but it appears the rebase broke my patch. Trying to narrow it down, but it seems that there is some sort of hanging array out there now:

creating snapshot...
global handle not serialized: 0x110409835735: [JSArrayBuffer] in OldSpace
 - map: 0x110408281181 <Map(HOLEY_ELEMENTS)> [FastProperties]
 - prototype: 0x1104082c6b21 <Object map = 0x1104082811a9>
 - elements: 0x1104080406e1 <FixedArray[0]> [HOLEY_ELEMENTS]
 - embedder fields: 2
 - backing_store: 0x10b5a7000
 - byte_length: 1048576
 - detachable
 - properties: 0x1104080406e1 <FixedArray[0]> {}
 - embedder fields = {
    0, aligned pointer: 0x0
    0, aligned pointer: 0x0
 }

--- stderr


#
# Fatal error in ../../../../../.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/rusty_v8-0.0.23/v8/src/api/api.cc, line 812
# Check failed: handle_checker.CheckGlobalAndEternalHandles().
#
#
#
#FailureMessage Object: 0x7ffee7413730

bartlomieju · 2020-01-12T10:27:12Z

@bartlomieju I went back to fix my patch (removing the fetch_asset didn't work because I still had one hiding out there), and I rebased, but it appears the rebase broke my patch. Trying to narrow it down, but it seems that there is some sort of hanging array out there now:

creating snapshot...
global handle not serialized: 0x110409835735: [JSArrayBuffer] in OldSpace
 - map: 0x110408281181 <Map(HOLEY_ELEMENTS)> [FastProperties]
 - prototype: 0x1104082c6b21 <Object map = 0x1104082811a9>
 - elements: 0x1104080406e1 <FixedArray[0]> [HOLEY_ELEMENTS]
 - embedder fields: 2
 - backing_store: 0x10b5a7000
 - byte_length: 1048576
 - detachable
 - properties: 0x1104080406e1 <FixedArray[0]> {}
 - embedder fields = {
    0, aligned pointer: 0x0
    0, aligned pointer: 0x0
 }

--- stderr


#
# Fatal error in ../../../../../.cargo/registry/src/jackfan.us.kg-1ecc6299db9ec823/rusty_v8-0.0.23/v8/src/api/api.cc, line 812
# Check failed: handle_checker.CheckGlobalAndEternalHandles().
#
#
#
#FailureMessage Object: 0x7ffee7413730

I'll try locally and get back to you

bartlomieju · 2020-01-12T10:34:46Z

@kitsonk my bad, please apply this patch:

diff --git a/core/isolate.rs b/core/isolate.rs
index 372ef185..5cbd5a5b 100644
--- a/core/isolate.rs
+++ b/core/isolate.rs
@@ -672,6 +672,7 @@ impl Isolate {
     let mut hs = v8::HandleScope::new(&mut locker);
     let scope = hs.enter();
     self.global_context.reset(scope);
+    self.shared_response_buf.reset(scope);
 
     let snapshot_creator = self.snapshot_creator.as_mut().unwrap();
     let snapshot = snapshot_creator

bartlomieju

LGTM!

bartlomieju · 2020-01-12T11:18:46Z

cli/js/dispatch.ts

+/** **WARNING:** This is only available during the snapshotting process and is
+ * unavailable at runtime. */
+export let OP_FETCH_ASSET: number;


bartlomieju · 2020-01-12T11:38:47Z

@kitsonk really impressive!

cold_relative_import - 0.81s -> 0.49s
cold_hello - 0.78s -> 0.49s

It also caused a big decrement in syscall count when compilation occurs.

However executable size grew from 52Mb to 56,6Mb (because compiler snapshot grew). I wonder if now we could not include all the assets in binary after snapshot step.

Anyway, awesome results!

Ref denoland#3712. This change allowed the deno_typescript crate to reference cli/js/lib.deno_runtime.d.ts which breaks "cargo package". We intend to reintroduce this a revised version of this patch later once "cargo package" is working and tested. This reverts commit 737ab94.

Ref #3712. This change allowed the deno_typescript crate to reference cli/js/lib.deno_runtime.d.ts which breaks "cargo package". We intend to reintroduce a revised version of this patch later once "cargo package" is working and tested. This reverts commit 737ab94.

kitsonk changed the title ~~[WIP] Create an old program to be used in snapshot.~~ Create an old program to be used in snapshot. Jan 10, 2020

kitsonk marked this pull request as ready for review January 10, 2020 05:56

bartlomieju reviewed Jan 10, 2020

View reviewed changes

kitsonk added 6 commits January 12, 2020 13:20

Create an old program to be used in snapshot.

1b91b31

Move oldProgram init to denoMain

b390ce3

Expose ops in deno_typescript

7a7abaa

Fix retrieving source files from cache

39ab9df

Linting

6d6d285

Cleanup of assets and remove fetch_asset from cli

52d296b

kitsonk force-pushed the compiler-speed branch from 17d70cf to 52d296b Compare January 12, 2020 10:03

Inline bundle static text during snapshotting

581d422

bartlomieju approved these changes Jan 12, 2020

View reviewed changes

bartlomieju merged commit 737ab94 into denoland:master Jan 12, 2020

This was referenced Jan 12, 2020

don't include assets in binary #3661

Merged

deno test is slow (pref: Dynamic imports) #2789

Closed

ry mentioned this pull request Feb 24, 2020

Warm up snapshot #781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an old program to be used in snapshot. #3644

Create an old program to be used in snapshot. #3644

kitsonk commented Jan 10, 2020 •

edited

Loading

bartlomieju commented Jan 10, 2020

bartlomieju commented Jan 10, 2020

kitsonk commented Jan 10, 2020

bartlomieju commented Jan 10, 2020

kitsonk commented Jan 10, 2020

kitsonk commented Jan 10, 2020

kitsonk commented Jan 10, 2020

kitsonk commented Jan 10, 2020

bartlomieju Jan 10, 2020

kitsonk Jan 10, 2020

bartlomieju Jan 10, 2020

bartlomieju Jan 10, 2020

kitsonk Jan 10, 2020

kitsonk commented Jan 12, 2020

bartlomieju commented Jan 12, 2020

bartlomieju commented Jan 12, 2020

bartlomieju left a comment

bartlomieju Jan 12, 2020

bartlomieju commented Jan 12, 2020 •

edited

Loading

	let op_id = v8::Local::<v8::Uint32>::try_from(info.get_argument(0))
	.unwrap()
	.value() as u32;

	i.register_op(
	"fetch_asset",
	s.core_op(json_op(s.stateful_op(op_fetch_asset))),
	);

Create an old program to be used in snapshot. #3644

Create an old program to be used in snapshot. #3644

Conversation

kitsonk commented Jan 10, 2020 • edited Loading

bartlomieju commented Jan 10, 2020

bartlomieju commented Jan 10, 2020

kitsonk commented Jan 10, 2020

bartlomieju commented Jan 10, 2020

kitsonk commented Jan 10, 2020

kitsonk commented Jan 10, 2020

kitsonk commented Jan 10, 2020

kitsonk commented Jan 10, 2020

bartlomieju Jan 10, 2020

Choose a reason for hiding this comment

kitsonk Jan 10, 2020

Choose a reason for hiding this comment

bartlomieju Jan 10, 2020

Choose a reason for hiding this comment

bartlomieju Jan 10, 2020

Choose a reason for hiding this comment

kitsonk Jan 10, 2020

Choose a reason for hiding this comment

kitsonk commented Jan 12, 2020

bartlomieju commented Jan 12, 2020

bartlomieju commented Jan 12, 2020

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju Jan 12, 2020

Choose a reason for hiding this comment

bartlomieju commented Jan 12, 2020 • edited Loading

kitsonk commented Jan 10, 2020 •

edited

Loading

bartlomieju commented Jan 12, 2020 •

edited

Loading