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

refactor: remove tsc snapshot #27987

Merged
merged 7 commits into from
Feb 10, 2025
Merged

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Feb 6, 2025

Removes the TSC snapshot to unblock the V8 upgrade (which enables shared RO heap, and is incompatible with multiple snapshots).

this compresses the sources and declaration files as well, which leads to a roughly 4.2MB reduction in binary size.

this currently adds about 80ms to deno check times, but code cache isn't wired up for the extension code (namely 00_typescript.js) yet

❯ hyperfine --warmup 5 -p "rm -rf ~/Library/Caches/deno/check_cache_v2" "../../deno/target/release-lite/deno check main.ts" "deno check main.ts"
Benchmark 1: ../../deno/target/release-lite/deno check main.ts
  Time (mean ± σ):     184.2 ms ±   2.2 ms    [User: 378.3 ms, System: 48.2 ms]
  Range (min … max):   181.5 ms … 189.9 ms    15 runs

Benchmark 2: deno check main.ts
  Time (mean ± σ):     107.4 ms ±   1.2 ms    [User: 155.3 ms, System: 23.9 ms]
  Range (min … max):   105.3 ms … 109.6 ms    26 runs

Summary
  deno check main.ts ran
    1.72 ± 0.03 times faster than ../../deno/target/release-lite/deno check main.ts

cli/tsc/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM, but how hard is it to get this using the code cache?

Edit: I read the description now.

@bartlomieju
Copy link
Member

Let's wait with merging until we have deno_core upgrade ready.

@bartlomieju bartlomieju mentioned this pull request Feb 7, 2025
cli/build.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

🎉

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, let's go once the conflicts are resolved

@bartlomieju bartlomieju enabled auto-merge (squash) February 10, 2025 16:18
@bartlomieju bartlomieju merged commit 174e496 into denoland:main Feb 10, 2025
17 checks passed
@nathanwhit nathanwhit deleted the remove-tsc-snapshot branch February 11, 2025 09:44
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Feb 11, 2025
bartlomieju added a commit that referenced this pull request Feb 11, 2025
This reverts commit 174e496.

That commit caused panic on startup like
#28047
or #28044.

Reverting for now, until we figure out what's going wrong.
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Feb 11, 2025
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.

3 participants