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(vendor): support modifying remote files in vendor folder without checksum errors #23979

Merged
merged 19 commits into from
May 28, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented May 25, 2024

Includes:

This is actually a lockfile perf investigation of moving the lockfile verification to be done in deno_graph's loader, but it has the side effect of enabling the ability to modify remote modules in the vendor folder without causing checksum errors.

Benchmarks

% git clone https://github.com/jsr-io/jsr
% git switch deno_graph_slow
% cd frontend
% hyperfine --warmup 3 "../../deno/deno_old run -A repro.ts" "../../deno/target/release/deno run -A repro.ts"

With "vendor": true (note: this PR doesn't fix the underlying issue that makes this scenario slow... I will follow up with that later):

Benchmark 1: ../../deno/deno_old run -A repro.ts
  Time (mean ± σ):     538.0 ms ±   5.5 ms    [User: 444.0 ms, System: 101.5 ms]
  Range (min … max):   531.9 ms … 548.6 ms    10 runs
 
Benchmark 2: ../../deno/target/release/deno run -A repro.ts
  Time (mean ± σ):     458.5 ms ±   3.1 ms    [User: 366.4 ms, System: 100.5 ms]
  Range (min … max):   453.4 ms … 463.7 ms    10 runs
 
Summary
  ../../deno/target/release/deno run -A repro.ts ran
    1.17 ± 0.01 times faster than ../../deno/deno_old run -A repro.ts

With "vendor": false (slightly slower because it now has to verify checksums):

Benchmark 1: ../../deno/deno_old run -A repro.ts
  Time (mean ± σ):     539.5 ms ±   3.7 ms    [User: 447.8 ms, System: 99.8 ms]
  Range (min … max):   536.2 ms … 546.0 ms    10 runs
 
Benchmark 2: ../../deno/target/release/deno run -A repro.ts
  Time (mean ± σ):     463.6 ms ±   8.3 ms    [User: 371.3 ms, System: 100.8 ms]
  Range (min … max):   456.0 ms … 484.2 ms    10 runs

// ideally the two error messages would be the same... this one comes from
// deno_cache and the one above comes from deno_graph. The thing is, in deno_cache
// (source of this error) it makes sense to include the url in the error message
// because it's not always used in the context of deno_graph
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

if let Some(lockfile) = &self.lockfile {
let mut lockfile = lockfile.lock();
// validate the integrity of all the modules
graph_lock_or_exit(graph, &mut lockfile);
Copy link
Member Author

Choose a reason for hiding this comment

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

Handled in graph_roots_valid.

@dsherret dsherret marked this pull request as ready for review May 28, 2024 00:29
@dsherret dsherret requested a review from bartlomieju May 28, 2024 01:15
deno_graph = { version = "=0.75.2", features = ["tokio_executor"] }
deno_doc = { version = "=0.137.0", features = ["html", "syntect"] }
deno_emit = "=0.41.0"
deno_graph = { version = "=0.77.0", features = ["tokio_executor"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems some wpt tests are failing due to denoland/deno_graph#483

See https://github.com/denoland/deno/actions/runs/9262872207

I'm unsure how to interpret the wpt test output. Are we incorrectly failing now?

Copy link
Member

Choose a reason for hiding this comment

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

I have updated the expectations. We were previously failing these tests, now the tests pass. The expectations file was assuming the tests to fail though, so them passing caused the suite to fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM, went through the PR 👍

@dsherret dsherret merged commit 448fe67 into denoland:main May 28, 2024
17 checks passed
@dsherret dsherret deleted the deno_graph_load_perf branch May 28, 2024 18:58
dsherret added a commit that referenced this pull request May 29, 2024
Slight perf regression when updating deno_lockfile in
#23979
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