Use a set when checking visited workspace members

[Kobzol]

While profiling Cargo, I started gathering a set of micro-optimizations on the side that help for stress tests, e.g. Zed. Most of those did not seem worth landing on their own, but this one provided non-trivial improvements, while being a reasonably simple change, so I thought I'd post it as a PR to see what you think.

When finding workspace members, the self.members.contains check can be quite hot. Not necessarily because of traversing through a Vec, because that itself is quite fast, but Eq for PathBuf is not necessarily so fast. The same result can be gained by switching the workspace members field to a set, but that would have

On Zed, this was a ~1.5% wall-time and ~4% icount improvement, on bors it was also a ~1.5% wall-time win.

Benchmark 1 (4 runs): /projects/personal/rust/cargo/cargo-baseline check
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          1.53s  ± 4.42ms    1.52s  … 1.53s           0 ( 0%)        0%
  peak_rss            269MB ±  275KB     268MB …  269MB          1 (25%)        0%
  cpu_cycles         3.03G  ± 11.2M     3.02G  … 3.05G           0 ( 0%)        0%
  instructions       6.25G  ±  729K     6.25G  … 6.26G           0 ( 0%)        0%
  cache_references    201M  ±  756K      200M  …  202M           0 ( 0%)        0%
  cache_misses       58.1M  ±  328K     57.7M  … 58.5M           0 ( 0%)        0%
  branch_misses      19.6M  ±  115K     19.5M  … 19.7M           0 ( 0%)        0%

Benchmark 2 (4 runs): ./cargo-perf check
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          1.50s  ± 4.12ms    1.50s  … 1.51s           0 ( 0%)        ⚡-  1.8% ±  0.5%
  peak_rss            269MB ±  258KB     269MB …  269MB          0 ( 0%)          +  0.1% ±  0.2%
  cpu_cycles         2.96G  ± 13.2M     2.95G  … 2.98G           0 ( 0%)        ⚡-  2.4% ±  0.7%
  instructions       6.00G  ±  847K     6.00G  … 6.00G           0 ( 0%)        ⚡-  4.1% ±  0.0%
  cache_references    199M  ±  740K      198M  …  200M           0 ( 0%)          -  0.8% ±  0.6%
  cache_misses       57.3M  ±  317K     56.9M  … 57.6M           0 ( 0%)          -  1.5% ±  1.0%
  branch_misses      19.3M  ±  187K     19.2M  … 19.6M           0 ( 0%)          -  1.2% ±  1.4%

I think that it shouldn't be behavior altering, but I'll defer to CI and tests.

[weihanglo]

This looks generally good to me, though I guess after #17169 we should leverage IndexSet directly.

View changes since the review

[Kobzol]

I assume you meant HashSet? :) Because we don't need to deal with iteration order here. Changed it to HashSet.

[weihanglo]

Sorry not being clearer. I meant instead of Vec we use IndexSet for storing members.

[Kobzol]

Ah, I see! Did that, and confirmed that the performance is essentially the same as the previous version of this PR, at least on Zed. The second run is the previous version, the third run is IndexSet.

Benchmark 1 (4 runs): /projects/personal/rust/cargo/cargo-baseline check
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          1.47s  ± 36.0ms    1.44s  … 1.52s           0 ( 0%)        0%
  peak_rss            268MB ±  436KB     267MB …  268MB          0 ( 0%)        0%
  cpu_cycles         2.85G  ± 74.9M     2.78G  … 2.96G           0 ( 0%)        0%
  instructions       5.23G  ±  174K     5.23G  … 5.23G           0 ( 0%)        0%
  cache_references    197M  ±  676K      197M  …  198M           0 ( 0%)        0%
  cache_misses       57.8M  ±  380K     57.4M  … 58.2M           0 ( 0%)        0%
  branch_misses      17.8M  ±  106K     17.7M  … 17.9M           1 (25%)        0%

Benchmark 2 (4 runs): ./cargo-set check
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          1.43s  ± 11.8ms    1.42s  … 1.45s           0 ( 0%)          -  2.8% ±  3.2%
  peak_rss            268MB ±  203KB     268MB …  268MB          0 ( 0%)          +  0.1% ±  0.2%
  cpu_cycles         2.74G  ± 9.39M     2.73G  … 2.75G           0 ( 0%)          -  3.8% ±  3.2%
  instructions       5.02G  ± 9.14M     5.02G  … 5.04G           0 ( 0%)        ⚡-  3.9% ±  0.2%
  cache_references    194M  ± 1.39M      193M  …  196M           0 ( 0%)          -  1.6% ±  1.0%
  cache_misses       55.8M  ±  544K     55.2M  … 56.4M           0 ( 0%)        ⚡-  3.5% ±  1.4%
  branch_misses      17.7M  ±  167K     17.5M  … 17.9M           0 ( 0%)          -  1.0% ±  1.4%

Benchmark 3 (4 runs): ./cargo-perf check
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          1.42s  ± 4.44ms    1.41s  … 1.42s           0 ( 0%)          -  3.7% ±  3.0%
  peak_rss            268MB ±  275KB     268MB …  268MB          0 ( 0%)          +  0.0% ±  0.2%
  cpu_cycles         2.73G  ± 9.98M     2.72G  … 2.74G           0 ( 0%)          -  4.2% ±  3.2%
  instructions       5.02G  ± 67.0K     5.02G  … 5.02G           1 (25%)        ⚡-  4.0% ±  0.0%
  cache_references    195M  ±  634K      194M  …  196M           1 (25%)          -  1.1% ±  0.6%
  cache_misses       56.9M  ±  147K     56.7M  … 57.0M           0 ( 0%)          -  1.7% ±  0.9%
  branch_misses      17.4M  ± 70.2K     17.3M  … 17.5M           0 ( 0%)        ⚡-  2.6% ±  0.9%

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, epage, weihanglo

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[weihanglo]

Thanks!

View changes since this review