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

feat(cli, runtime): compress snapshots #13320

Merged
merged 3 commits into from
Jan 10, 2022

Conversation

evanwashere
Copy link
Contributor

@evanwashere evanwashere commented Jan 9, 2022

depends on denoland/rusty_v8@1cedb0e reversal
replaces v8 zlib compression with lz4 and zstd for better speed and compression ratio

build size
deno-new 79mb
deno-uncompressed 103mb
deno-v8-compressed 80mb
deno run with cache (cli snapshot) Mean [ms] Min [ms] Max [ms] Relative
deno-new run -A b.ts 30.4 ± 0.5 29.6 32.3 1.03 ± 0.02
deno-uncompressed run -A b.ts 29.6 ± 0.4 28.9 31.5 1.00
deno-v8-compressed run -A b.ts 44.6 ± 0.5 43.8 46.6 1.51 ± 0.03
deno cache with no cache (tsc snapshot) Mean [ms] Min [ms] Max [ms] Relative
deno-new cache -r b.ts 266.5 ± 1.6 264.1 269.0 1.05 ± 0.01
deno-uncompressed cache -r b.ts 253.8 ± 1.1 252.2 255.7 1.00
deno-v8-compressed cache -r b.ts 273.8 ± 1.4 271.7 276.2 1.08 ± 0.01
deno run with no cache (cli + tsc snapshot) Mean [ms] Min [ms] Max [ms] Relative
deno-new run -r -A b.ts 540.8 ± 16.3 530.2 580.8 1.04 ± 0.03
deno-uncompressed run -r -A b.ts 519.2 ± 1.9 517.5 522.6 1.00
deno-v8-compressed run -r -A b.ts 577.4 ± 2.0 574.7 580.9 1.11 ± 0.01
test source
// b.ts
export function foo<T>(bar: T) {
  return bar;
}

console.log(foo(1));

let i = 5;
while (i--) {
  const w = new Worker(new URL(`a.ts`, import.meta.url), { type: 'module' });

  w.terminate();
}
// a.ts
let a = 1;

export function foo<T>(bar: T) {
  return bar;
}

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2022

CLA assistant check
All committers have signed the CLA.

@ry
Copy link
Member

ry commented Jan 9, 2022

1mb saved? Is it worth the complexity?

@lucacasonato
Copy link
Member

lucacasonato commented Jan 9, 2022

@ry 33% improvement in startup time!

@piscisaureus
Copy link
Member

I think this looks really good.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

One question: why lzzzz instead of (lz4_flex)[https://crates.io/crates/lz4_flex]? The latter is the more popular crate and is pure Rust.

runtime/js.rs Outdated
Comment on lines 16 to 22
let mut vec = Vec::with_capacity(size);

unsafe {
vec.set_len(size);
}

lzzzz::lz4::decompress(&COMPRESSED_CLI_SNAPSHOT[4..], &mut vec).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably an academic concern but I think writing to uninitialized memory like this is technically undefined behavior. The blessed path is to use MaybeUninit: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#out-pointers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no undefined behavior in this case, we are just reusing previous allocation without zeroing it

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue MaybeUninit is much clearer.

Copy link
Contributor

@andreubotella andreubotella Jan 10, 2022

Choose a reason for hiding this comment

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

I've just checked, and Vec::with_capacity calls the allocator without guaranteeing that the memory has been zeroed. Sure, the allocation might've actually been reused, or it might be a new allocation that actually has been zeroed, but even if we could guarantee that it's either of those cases, I'm pretty sure the language treats the result of an allocation as uninitialized memory, which means that creating values from it, even u8 values, would be undefined behavior.

runtime/js.rs Outdated Show resolved Hide resolved
@evanwashere
Copy link
Contributor Author

One question: why lzzzz instead of (lz4_flex)[https://crates.io/crates/lz4_flex]? The latter is the more popular crate and is pure Rust.

I initially used lz4_flex for proof of concept but switched to lzzzz because it has lz4hc and is slightly faster at decompressing cli snapshot

piscisaureus added a commit to piscisaureus/safe_v8 that referenced this pull request Jan 10, 2022
It turns out that the embedder can take selectively compress snapshots,
resulting in better startup performance and a smaller binary size.
See denoland/deno#13320.
piscisaureus added a commit to piscisaureus/safe_v8 that referenced this pull request Jan 10, 2022
It turns out that the embedder can take selectively compress snapshots,
resulting in better startup performance and a smaller binary size.
See denoland/deno#13320.
piscisaureus added a commit to piscisaureus/safe_v8 that referenced this pull request Jan 10, 2022
It turns out that the embedder can selectively compress snapshots,
resulting in better startup performance and a smaller binary size.

See denoland/deno#13320.
piscisaureus added a commit to denoland/rusty_v8 that referenced this pull request Jan 10, 2022
It turns out that the embedder can selectively compress snapshots,
resulting in better startup performance and a smaller binary size.

See denoland/deno#13320.
@bartlomieju
Copy link
Member

@piscisaureus I guess we'll wait with landing this PR until we upgrade rusty_v8

@piscisaureus
Copy link
Member

@piscisaureus I guess we'll wait with landing this PR until we upgrade rusty_v8

There's no need; snapshots are already off in the current version of rusty_v8.

@piscisaureus piscisaureus merged commit b66afa2 into denoland:main Jan 10, 2022
@evanwashere evanwashere deleted the snapshot-compression branch January 11, 2022 21:15
ry added a commit to denoland/rusty_v8 that referenced this pull request Aug 23, 2022
Deno does its own snapshot compression using lz4 and zstd, so no need to
build zlib into V8. See denoland/deno#13320
ry added a commit to denoland/rusty_v8 that referenced this pull request Aug 23, 2022
Deno does its own snapshot compression using lz4 and zstd, so no need to
build zlib into V8. See denoland/deno#13320
ry added a commit to denoland/rusty_v8 that referenced this pull request Aug 23, 2022
Deno does its own snapshot compression using lz4 and zstd, so no need to
build zlib into V8. See denoland/deno#13320
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.

8 participants