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.

Sign up for GitHub

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

Jump to bottom

perf: analyze cjs re-exports in parallel #23894

Merged
merged 5 commits into from
May 21, 2024
Merged

perf: analyze cjs re-exports in parallel #23894

merged 5 commits into from
May 21, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented May 19, 2024 •
edited
Loading

This causes it to go concurrently through cjs modules handling re-exports as they're discovered rather than in batches.

In very simple scenarios this will be about the same performance or a few ms slower, but in worst case scenarios it will be much faster (this is with a fresh node analysis cache on each run):

Benchmark 1: deno run -A main.ts
  Time (mean ± σ):      4.540 s ±  0.034 s    [User: 4.383 s, System: 0.333 s]
  Range (min … max):    4.497 s …  4.607 s    10 runs
 
Benchmark 2: ../deno/deno_main run -A main.ts
  Time (mean ± σ):      4.474 s ±  0.045 s    [User: 4.316 s, System: 0.317 s]
  Range (min … max):    4.390 s …  4.537 s    10 runs
 
Benchmark 3: ../deno/target/release/deno run -A main.ts
  Time (mean ± σ):      1.413 s ±  0.010 s    [User: 4.852 s, System: 0.539 s]
  Range (min … max):    1.402 s …  1.432 s    10 runs
 
Summary
  ../deno/target/release/deno run -A main.ts ran
    3.17 ± 0.04 times faster than ../deno/deno_main run -A main.ts
    3.21 ± 0.03 times faster than deno run -A main.ts

Note that with a cache it's a few ms slower:

Benchmark 1: ../deno/deno_main run -A main.ts
  Time (mean ± σ):     352.3 ms ±   1.6 ms    [User: 437.2 ms, System: 121.8 ms]
  Range (min … max):   349.5 ms … 354.5 ms    10 runs
 
Benchmark 2: ../deno/target/release/deno run -A main.ts
  Time (mean ± σ):     357.0 ms ±   2.5 ms    [User: 431.1 ms, System: 132.2 ms]
  Range (min … max):   353.9 ms … 361.9 ms    10 runs
 
Summary
  ../deno/deno_main run -A main.ts ran
    1.01 ± 0.01 times faster than ../deno/target/release/deno run -A main.ts

Closes #23859

@dsherret dsherret marked this pull request as ready for review May 20, 2024 18:50
@dsherret dsherret changed the title perf: analyze cjs reexports in parallel perf: analyze cjs re-exports in parallel May 20, 2024
@dsherret dsherret requested a review from bartlomieju May 20, 2024 18:50
dsherret
dsherret commented May 20, 2024
View reviewed changes
ext/node/analyze.rs Outdated
.unwrap();
let mut analyze_futures: FuturesUnordered<
LocalBoxFuture<Result<_, AnyError>>,
> = FuturesUnordered::new();
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 should create an unsync version of this (not joinset, which will spawn futures)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea. Can you open an issue in deno_unsync?

Copy link
Member Author

Choose a reason for hiding this comment

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

denoland/deno_unsync#9

bartlomieju
bartlomieju approved these changes May 20, 2024
View reviewed changes
ext/node/analyze.rs Outdated
.unwrap();
let mut analyze_futures: FuturesUnordered<
LocalBoxFuture<Result<_, AnyError>>,
> = FuturesUnordered::new();
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea. Can you open an issue in deno_unsync?

ext/node/analyze.rs Outdated
if !handled_reexports.insert(reexport_specifier.clone()) {
continue;
}
let mut handled_reexports: HashSet<ModuleSpecifier> = HashSet::default();
Copy link
Member

Choose a reason for hiding this comment

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

Pre-allocate?

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 don't know how many modules there will be.

dsherret
dsherret commented May 20, 2024
View reviewed changes
ext/node/analyze.rs Outdated
};
analysis_result = analyze_futures.select_next_some() => {
// 2. Look at the analysis result and resolve its exports and re-exports
let (reexport_specifier, referrer, analysis) = analysis_result?;
Copy link
Member Author

Choose a reason for hiding this comment

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

Something I just realized is we should make the errors deterministic if there are multiple.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in a1291ce

@dsherret dsherret requested a review from bartlomieju May 21, 2024 00:10
@dsherret dsherret merged commit 3d6ba1e into denoland:main May 21, 2024
17 checks passed
@dsherret dsherret deleted the perf_cjs_re_exports_parallel branch May 21, 2024 14:35
bartlomieju pushed a commit that referenced this pull request May 21, 2024
@dsherret @bartlomieju
perf: analyze cjs re-exports in parallel (#23894)
08247fa
bartlomieju pushed a commit that referenced this pull request May 21, 2024
@dsherret @bartlomieju
perf: analyze cjs re-exports in parallel (#23894)
4b08244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju Awaiting requested review from bartlomieju

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Analyze CJS re-exports in parallel
2 participants
@dsherret @bartlomieju