a faster hash for ActivationsKey #14915

Eh2406 · 2024-12-10T18:52:24Z

This is a perf improvement I suggested #14665 (comment)

I mostly want this landed to make it easier to compare the cost and benefits of more complicated changes. As this is the thing to compare against.

rustbot · 2024-12-10T18:52:29Z

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

The source is the field the most expensive to do Eq on and the one least likely to be different. By moving it to the end calles to comparing keys that are different returns faster.

Eh2406 · 2024-12-10T19:53:29Z

Force pushed to split up into separate commits for easier review.

src/cargo/util/interning.rs

epage · 2024-12-10T20:00:04Z

src/cargo/core/resolver/types.rs

+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.0.fast_hash(state);
+        self.1.hash(state);
+        // self.2.hash(state); // Packages that only differ by SourceId are rare enough to not be worth hashing


So we have 3 improvments

Re-order

Pointer hash on the name

Skipping the source hash

Do we know how much of an improvement each one of these is? I'm mostly wondering about skipping the source hash as that feels icky for some reason.

Not going to give this feeling too much weight though. I'll mostly defer to you on this part.

Do we know how much of an improvement each one of these is?

I just collected overall performance numbers, I will try and get you some intermediate numbers.

I'm mostly wondering about skipping the source hash as that feels icky for some reason.

Can you articulate why? HashMaps can devolve to O(n) if skip fields are the only thing distinguishing n items, but we specifically commented to record why we didn't think that would happen. Skipping a field when hashing is a pretty standard performance trick.

Eh2406 · 2024-12-10T20:30:19Z

Running cargo r -r -- -t 100 -m cargo --with-solana --filter solana on https://github.com/Eh2406/pubgrub-crates-benchmark on the index commit hash: 15350ef77c6486d35d20bb1ca81f2bc051500fb6

c2b0d50

           Cargo CPU time: 22966.32s == 382.77min
                Wall time:   549.50s ==   9.16min

7a67e88

           Cargo CPU time: 21391.77s == 356.53min
                Wall time:   520.00s ==   8.67min

That is a 7% improvement, which is nothing to sneeze at given the size of the change.

Eh2406 · 2024-12-10T21:42:24Z

Re-order

f2b4998

           Cargo CPU time: 21846.75s == 364.11min
                Wall time:   515.96s ==   8.60min

So 5% of the improvement! (It would be reasonable to land just this part.)

Re-order

Pointer hash on the name

Doing thees 2 gets us:

           Cargo CPU time: 22780.97s == 379.68min
                Wall time:   540.51s ==   9.01min

That is slower... let me re-run that.

           Cargo CPU time: 21908.46s == 365.14min
                Wall time:   526.55s ==   8.78min

So lets not do that.

Re-order

Skipping the source hash

For completeness lets try this combination:

           Cargo CPU time: 20367.65s == 339.46min
                Wall time:   480.66s ==   8.01min

Oddly that looks like a winner. Let me try it again, just in case.

           Cargo CPU time: 21254.67s == 354.24min
                Wall time:   499.23s ==   8.32min

So push incoming.

epage

Thanks!

Update cargo 18 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..7847c03965260b5dcc8d93218d6af295a717abb6 2024-12-06 21:56:56 +0000 to 2024-12-13 18:06:39 +0000 - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

Update cargo 19 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..769f622e12db0001431d8ae36d1093fb8727c5d9 2024-12-06 21:56:56 +0000 to 2024-12-14 04:27:35 +0000 - test(build-std): dont require rustup (rust-lang/cargo#14933) - fix(base): Support bases in patches in virtual manifests (rust-lang/cargo#14931) - fix(resolver): Report invalid index entries (rust-lang/cargo#14927) - feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928) - fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923) - fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926) - fix(script): Don't override the release profile (rust-lang/cargo#14925) - feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917) - fix(resolver): Don't report all versions as rejected (rust-lang/cargo#14921) - fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897) - fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911) - a faster hash for ActivationsKey (rust-lang/cargo#14915) - feat(build-script): Pass CARGO_CFG_FEATURE (rust-lang/cargo#14902) - fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913) - chore: update auto-label to include build-rs crate (rust-lang/cargo#14912) - refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908) - feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910) - fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899) - SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)

rustbot assigned weihanglo Dec 10, 2024

rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2024

Eh2406 added 3 commits December 10, 2024 19:35

move ActivationsKey and SemverCompatibility

2525532

chagne ActivationsKey into a struct

a699239

reorder ActivationKey so that the source is last

f2b4998

The source is the field the most expensive to do Eq on and the one least likely to be different. By moving it to the end calles to comparing keys that are different returns faster.

Eh2406 force-pushed the faster_activationkey_hash branch from c103add to 42216fd Compare December 10, 2024 19:51

a faster hash for ActivationsKey

91e6bf8

Eh2406 force-pushed the faster_activationkey_hash branch from 42216fd to 91e6bf8 Compare December 10, 2024 19:52

cargo fmt

7a67e88

epage reviewed Dec 10, 2024

View reviewed changes

src/cargo/util/interning.rs Outdated Show resolved Hide resolved

epage reviewed Dec 10, 2024

View reviewed changes

oddly this is fasster

2cafa28

epage approved these changes Dec 10, 2024

View reviewed changes

epage enabled auto-merge December 10, 2024 21:52

epage added this pull request to the merge queue Dec 10, 2024

Merged via the queue into rust-lang:master with commit 27a4f98 Dec 10, 2024
20 checks passed

Eh2406 deleted the faster_activationkey_hash branch December 11, 2024 00:08

Eh2406 mentioned this pull request Dec 11, 2024

Faster conflict cache #14922

Closed

weihanglo mentioned this pull request Dec 13, 2024

Update cargo rust-lang/rust#134278

Merged

rustbot added this to the 1.85.0 milestone Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a faster hash for ActivationsKey #14915

a faster hash for ActivationsKey #14915

Eh2406 commented Dec 10, 2024

rustbot commented Dec 10, 2024

Eh2406 commented Dec 10, 2024

epage Dec 10, 2024

Eh2406 Dec 10, 2024

Eh2406 commented Dec 10, 2024

Eh2406 commented Dec 10, 2024

epage left a comment

a faster hash for ActivationsKey #14915

a faster hash for ActivationsKey #14915

Conversation

Eh2406 commented Dec 10, 2024

rustbot commented Dec 10, 2024

Eh2406 commented Dec 10, 2024

epage Dec 10, 2024

Choose a reason for hiding this comment

Eh2406 Dec 10, 2024

Choose a reason for hiding this comment

Eh2406 commented Dec 10, 2024

Eh2406 commented Dec 10, 2024

epage left a comment

Choose a reason for hiding this comment