Skip to content

Conversation

@stormslowly
Copy link
Contributor

@stormslowly stormslowly commented Nov 21, 2025

Note: it's not a perf PR.
It simply disables PnP features in the file system while performing non-PnP-related benchmarks, and separates performance concerns.

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 21, 2025

CodSpeed Performance Report

Merging #134 will improve performances by 57.52%

Comparing chore/filesytem_opts (cfbf58f) with main (8238fcf)

Summary

⚡ 3 improvements
✅ 3 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
resolver[multi-thread] 95.9 ms 74 ms +29.49%
resolver[resolve from symlinks] 359.5 ms 228.2 ms +57.52%
resolver[single-thread] 89.3 ms 63.4 ms +40.79%

@stormslowly stormslowly marked this pull request as ready for review November 23, 2025 03:00
Copilot finished reviewing on behalf of stormslowly November 23, 2025 03:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the benchmark code to properly disable Plug'n'Play (PnP) features during non-PnP-related benchmarks, separating performance concerns. The changes introduce a new FileSystemOptions struct to control PnP behavior and update benchmarks to explicitly set PnP flags.

Key changes:

  • Added FileSystemOptions struct with enable_pnp field to configure filesystem behavior
  • Introduced FileSystemOs::new() constructor to accept custom filesystem options
  • Updated benchmark resolver creation to explicitly disable PnP for non-PnP benchmarks

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/lib.rs Exports FileSystemOptions to make it available for benchmarks
src/file_system.rs Adds FileSystemOs::new() method to accept custom options
benches/resolver.rs Refactors resolver creation to accept PnP flag and disables it for non-PnP benchmarks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

fn rspack_resolver() -> rspack_resolver::Resolver {
fn rspack_resolver(enable_pnp: bool) -> rspack_resolver::Resolver {
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The enable_pnp parameter is only used when the yarn_pnp feature is enabled. To avoid compilation warnings about unused parameters when the feature is disabled, consider either:

  1. Prefixing the parameter with an underscore: _enable_pnp: bool
  2. Or applying a #[allow(unused_variables)] attribute when the feature is disabled

Example:

fn rspack_resolver(
  #[cfg_attr(not(feature = "yarn_pnp"), allow(unused_variables))]
  enable_pnp: bool
) -> rspack_resolver::Resolver {
Suggested change
fn rspack_resolver(enable_pnp: bool) -> rspack_resolver::Resolver {
fn rspack_resolver(
#[cfg_attr(not(feature = "yarn_pnp"), allow(unused_variables))]
enable_pnp: bool
) -> rspack_resolver::Resolver {

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@stormslowly stormslowly merged commit c518a8a into main Nov 23, 2025
25 checks passed
@stormslowly stormslowly deleted the chore/filesytem_opts branch November 23, 2025 05:47
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