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

perf(compile): pass module source data from binary directly to v8 #26494

Merged
merged 10 commits into from
Oct 24, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Oct 23, 2024

This changes denort to pass a static reference of the moude source bytes found in the binary to v8 instead of copying it.

Memory Usage (Example 1)

async function test() {
  await import("./typescript.js");
  await import("./typescript2.js");
}

setInterval(() => {
  console.log(Deno.memoryUsage().rss);
}, 1000);
% ./scratch_main
80805888
80920576
81231872
^C
% ./scratch_new
36044800
36175872
36487168
36487168
36536320

Memory Usage (Example 2)

import "./typescript.js";
import "./typescript2.js";

setInterval(() => {
  console.log(Deno.memoryUsage().rss);
}, 1000);
% ./scratch_main                   
161644544
161742848
161742848
161775616
^C
% ./scratch_new 
97746944
97828864
97845248
97878016
^C

Performance

Benchmarking not_imported_dynamic_import.ts
Benchmark 1: ./scratch_new
  Time (mean ± σ):      15.3 ms ±   0.9 ms    [User: 10.6 ms, System: 3.4 ms]
  Range (min … max):    14.1 ms …  20.6 ms    179 runs
 
Benchmark 2: ./scratch_main
  Time (mean ± σ):      19.8 ms ±   0.6 ms    [User: 12.8 ms, System: 5.4 ms]
  Range (min … max):    18.6 ms …  23.0 ms    138 runs
 
Summary
  ./scratch_new ran
    1.30 ± 0.09 times faster than ./scratch_main
Benchmarking typescript_imports.ts
Benchmark 1: ./scratch_new
  Time (mean ± σ):     169.6 ms ±   1.2 ms    [User: 161.4 ms, System: 11.6 ms]
  Range (min … max):   167.9 ms … 171.5 ms    17 runs
 
Benchmark 2: ./scratch_main
  Time (mean ± σ):     176.1 ms ±   2.0 ms    [User: 163.7 ms, System: 15.6 ms]
  Range (min … max):   173.9 ms … 179.9 ms    16 runs
 
Summary
  ./scratch_new ran
    1.04 ± 0.01 times faster than ./scratch_main
Benchmarking many_jsr.ts
Benchmark 1: ./scratch_new
  Time (mean ± σ):      61.6 ms ±   1.4 ms    [User: 56.0 ms, System: 7.4 ms]
  Range (min … max):    59.9 ms …  67.9 ms    47 runs
 
Benchmark 2: ./scratch_main
  Time (mean ± σ):      49.9 ms ±   0.6 ms    [User: 44.8 ms, System: 7.4 ms]
  Range (min … max):    48.6 ms …  51.4 ms    58 runs
 
Summary
  ./scratch_main ran
    1.23 ± 0.03 times faster than ./scratch_new
Files
// not_imported_dynamic_import.ts
console.log("Hello");

async function test() {
  await import("./typescript.js");
  await import("./typescript2.js");
}

// typescript_imports.ts
import "./typescript.js";
import "./typescript2.js";

console.log("hello");

// many_jsr.ts
import "jsr:@std/assert";
import "jsr:@std/async";
import "jsr:@std/bytes";
import "jsr:@std/cache";
import "jsr:@std/collections";
import "jsr:@std/csv";
import "jsr:@std/data-structures";
import "jsr:@std/datetime";
import "jsr:@std/dotenv";
import "jsr:@std/encoding";
import "jsr:@std/expect";
import "jsr:@std/fmt/bytes";
import "jsr:@std/fmt/colors";
import "jsr:@std/fmt/duration";
import "jsr:@std/fmt/printf";
import "jsr:@std/front-matter";
import "jsr:@std/fs";
import "jsr:@std/html";
import "jsr:@std/http";
import "jsr:@std/ini";
import "jsr:@std/io";
import "jsr:@std/jsonc";
import "jsr:@std/log";
import "jsr:@std/media-types";
import "jsr:@std/msgpack";
import "jsr:@std/net";
import "jsr:@std/path";
import "jsr:@std/random";
import "jsr:@std/regexp";
import "jsr:@std/semver";
import "jsr:@std/streams";
import "jsr:@std/tar";
import "jsr:@std/testing/bdd";
import "jsr:@std/testing/mock";
import "jsr:@std/testing/snapshot";
import "jsr:@std/testing/time";
import "jsr:@std/testing/types";
import "jsr:@std/text";
import "jsr:@std/toml";
import "jsr:@std/ulid";
import "jsr:@std/uuid";
import "jsr:@std/yaml";
import "jsr:@david/dax";
console.log("Hello");

It's faster in many scenarios, but seems slower for the test running many JSR modules. I looked at a flamegraph and it seems to be due to denoland/deno_core#942

@dsherret dsherret changed the title perf(compile): read source data directly from binary perf(compile): pass module source data from binary directly to v8 Oct 23, 2024
@dsherret dsherret marked this pull request as ready for review October 23, 2024 22:31
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM!

cli/standalone/virtual_fs.rs Show resolved Hide resolved
cli/standalone/virtual_fs.rs Show resolved Hide resolved
cli/standalone/binary.rs Outdated Show resolved Hide resolved
cli/standalone/mod.rs Show resolved Hide resolved
Comment on lines +436 to +443
npmrc: Arc::new(ResolvedNpmRc {
default_config: deno_npm::npm_rc::RegistryConfigWithUrl {
registry_url: npm_registry_url.clone(),
config: Default::default(),
},
scopes: Default::default(),
registry_configs: Default::default(),
}),
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why we need an npmrc here :D I think it should be fine to have a "noop" npmrc that errors when it's actually used?

Copy link
Member Author

Choose a reason for hiding this comment

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

We use a global cache that's located in the binary and we use the provided npm registry url here to resolve the packages.

cli/standalone/mod.rs Outdated Show resolved Hide resolved
bytes.extend_from_slice(data);
}

let mut bytes = Vec::new();
Copy link
Member

Choose a reason for hiding this comment

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

Please pre-allocate this vector here

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's not worth the complexity because this is just for serializing and I doubt it will have much perf impact. Maybe if we see it on a flamegraph?

Copy link
Member

Choose a reason for hiding this comment

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

Sure

// 4. VFS
let (input, data) = read_bytes_with_len(input).context("vfs")?;
let vfs_dir: VirtualDirectory =
serde_json::from_slice(data).context("deserializing vfs data")?;
Copy link
Member

Choose a reason for hiding this comment

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

FYI in deno_core we use https://crates.io/crates/bincode to serialize snapshot, maybe it would be useful in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think we should do it in the future.

@dsherret dsherret requested a review from bartlomieju October 24, 2024 19:14
@dsherret dsherret enabled auto-merge (squash) October 24, 2024 19:30
@dsherret dsherret merged commit eedf243 into denoland:main Oct 24, 2024
17 checks passed
bartlomieju pushed a commit that referenced this pull request Oct 25, 2024
…6494)

This changes denort to pass a static reference of the moude source bytes found in the binary to v8 instead of copying it.
@KnorpelSenf
Copy link
Contributor

This introduced a bug that I reported in #26583.

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.

4 participants