[tentative] Revert "Convert ShardedHashMap to use hashbrown::HashTable"

[fmease]

#138414 (comment)
r? fmease

try-job: dist-x86_64-linux

[fmease]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ff21e44 with merge d7225dd...

[fmease]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 964533b with merge f93dc40...

[rust-log-analyzer]

The job dist-x86_64-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

file:.git/config remote.origin.url=https://github.com/rust-lang-ci/rust
file:.git/config remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config gc.auto=0
file:.git/config http.https://github.com/.extraheader=AUTHORIZATION: basic ***
file:.git/config branch.try.remote=origin
file:.git/config branch.try.merge=refs/heads/try
file:.git/config remote.upstream.url=https://github.com/rust-lang/rust
file:.git/config remote.upstream.fetch=+refs/heads/*:refs/remotes/upstream/*
file:.git/config submodule.library/backtrace.active=true
file:.git/config submodule.library/backtrace.url=https://github.com/rust-lang/backtrace-rs.git
file:.git/config submodule.library/stdarch.active=true
---
   |            ^^^^^^^^^^^^^^

[RUSTC-TIMING] unic_langid test:false 0.044
[RUSTC-TIMING] fluent_bundle test:false 0.710
error[E0599]: no method named `raw_entry_mut` found for struct `LockGuard<'_, HashMap<K, (), FxBuildHasher>>` in the current scope
    --> compiler/rustc_data_structures/src/sharded.rs:163:27
     |
163  |         let entry = shard.raw_entry_mut().from_key_hashed_nocheck(hash, value);
     |                           ^^^^^^^^^^^^^
     |
    ::: compiler/rustc_data_structures/src/sync/lock.rs:27:1
     |
27   | pub struct LockGuard<'a, T> {
     | --------------------------- method `raw_entry_mut` not found for this struct
     |
help: there is a method `get_mut` with a similar name, but with different arguments
    --> /rustc/d7225dd1929a602785fc38ac0212de064f822101/library/std/src/collections/hash/map.rs:1164:5
     |
1164 | /     pub fn get_mut<Q: ?Sized>(&mut self, k: &Q) -> Option<&mut V>
1165 | |     where
1166 | |         K: Borrow<Q>,
1167 | |         Q: Hash + Eq,
     | |_____________________^

error[E0599]: no method named `raw_entry_mut` found for struct `LockGuard<'_, HashMap<K, (), FxBuildHasher>>` in the current scope
    --> compiler/rustc_data_structures/src/sharded.rs:183:27
     |
183  |         let entry = shard.raw_entry_mut().from_key_hashed_nocheck(hash, &value);
     |                           ^^^^^^^^^^^^^
     |
    ::: compiler/rustc_data_structures/src/sync/lock.rs:27:1
     |
27   | pub struct LockGuard<'a, T> {
     | --------------------------- method `raw_entry_mut` not found for this struct
     |
help: there is a method `get_mut` with a similar name, but with different arguments
    --> /rustc/d7225dd1929a602785fc38ac0212de064f822101/library/std/src/collections/hash/map.rs:1164:5
     |
1164 | /     pub fn get_mut<Q: ?Sized>(&mut self, k: &Q) -> Option<&mut V>
1165 | |     where
1166 | |         K: Borrow<Q>,
1167 | |         Q: Hash + Eq,
     | |_____________________^

[RUSTC-TIMING] intl_memoizer test:false 0.333
error[E0599]: no method named `raw_entry` found for struct `LockGuard<'_, HashMap<K, (), FxBuildHasher>>` in the current scope
   --> compiler/rustc_data_structures/src/sharded.rs:206:15
    |
206 |         shard.raw_entry().from_hash(hash, |entry| entry.into_pointer() == value).is_some()
    |               ^^^^^^^^^
    |
   ::: compiler/rustc_data_structures/src/sync/lock.rs:27:1
    |
27  | pub struct LockGuard<'a, T> {
    | --------------------------- method `raw_entry` not found for this struct
    |
help: there is a method `entry` with a similar name, but with different arguments
   --> /rustc/d7225dd1929a602785fc38ac0212de064f822101/library/std/src/collections/hash/map.rs:883:5
    |
883 |     pub fn entry(&mut self, key: K) -> Entry<'_, K, V> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0308]: mismatched types
   --> compiler/rustc_data_structures/src/sharded.rs:206:75
    |
201 | impl<K: Eq + Hash + Copy + IntoPointer> ShardedHashMap<K, ()> {
    |      - expected this type parameter
...
206 |         shard.raw_entry().from_hash(hash, |entry| entry.into_pointer() == value).is_some()
    |                                                                           ^^^^^ expected type parameter `K`, found `*const ()`
    |
    = note: expected type parameter `K`
                  found raw pointer `*const ()`
    = note: `*const ()` implements `PartialEq<{type error}>`
help: consider swapping the equality
    |
206 -         shard.raw_entry().from_hash(hash, |entry| entry.into_pointer() == value).is_some()
206 +         shard.raw_entry().from_hash(hash, |entry| value == entry.into_pointer()).is_some()
    |

[RUSTC-TIMING] hashbrown test:false 0.823
[RUSTC-TIMING] regex test:false 1.139
   Compiling rustc_fluent_macro v0.0.0 (/checkout/compiler/rustc_fluent_macro)
---
Caused by:
    Command RUST_BACKTRACE=full python3 /checkout/x.py build --target x86_64-unknown-linux-gnu --host x86_64-unknown-linux-gnu --stage 2 library/std --rust-profile-generate /tmp/tmp-multistage/opt-artifacts/rustc-pgo --set llvm.thin-lto=false --set llvm.link-shared=true [at /checkout/obj] has failed with exit code Some(1)

Stack backtrace:
   0: <anyhow::Error>::msg::<alloc::string::String>
             at /rust/deps/anyhow-1.0.95/src/backtrace.rs:27:14
   1: <opt_dist::exec::CmdBuilder>::run
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/exec.rs:80:17
   2: <opt_dist::exec::Bootstrap>::run
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/exec.rs:181:9
   3: opt_dist::execute_pipeline::{closure#1}::{closure#0}
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/main.rs:222:13
   4: <opt_dist::timer::TimerSection>::section::<opt_dist::execute_pipeline::{closure#1}::{closure#0}, ()>
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/timer.rs:111:22
   5: opt_dist::execute_pipeline::{closure#1}
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/main.rs:211:9
   6: <opt_dist::timer::TimerSection>::section::<opt_dist::execute_pipeline::{closure#1}, opt_dist::training::RustcPGOProfile>
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/timer.rs:111:22
   7: opt_dist::execute_pipeline
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/main.rs:208:29
   8: opt_dist::main
             at /rustc/d7225dd1929a602785fc38ac0212de064f822101/src/tools/opt-dist/src/main.rs:408:18
   9: <fn() -> core::result::Result<(), anyhow::Error> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/f0cb41030579cd1a6f72bd23f38e677052d5d485/library/core/src/ops/function.rs:250:5
  10: std::sys::backtrace::__rust_begin_short_backtrace::<fn() -> core::result::Result<(), anyhow::Error>, core::result::Result<(), anyhow::Error>>
             at /rustc/f0cb41030579cd1a6f72bd23f38e677052d5d485/library/std/src/sys/backtrace.rs:152:18
  11: std::rt::lang_start::<core::result::Result<(), anyhow::Error>>::{closure#0}
             at /rustc/f0cb41030579cd1a6f72bd23f38e677052d5d485/library/std/src/rt.rs:199:18
  12: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/f0cb41030579cd1a6f72bd23f38e677052d5d485/library/core/src/ops/function.rs:284:13
  13: std::panicking::try::do_call
             at /rustc/f0cb41030579cd1a6f72bd23f38e677052d5d485/library/std/src/panicking.rs:587:40
  14: std::panicking::try
             at /rustc/f0cb41030579cd1a6f72bd23f38e677052d5d485/library/std/src/panicking.rs:550:19

[bors]

☀️ Try build successful - checks-actions
Build commit: f93dc40 (f93dc4051ffadf38f5e45016407c7c23c7c53f0b)

[Kobzol]

@bors try

(Made the job explicit in the PR description to run the full build and test suite)

[bors]

⌛ Trying commit 964533b with merge cce9209...

[fmease]

Made the job explicit in the PR description to run the full build and test suite

Wait, why? The first try build failure concerned the patch before the force push, the subsequent try build success concerned the patch after it. Everything was just fine?

[Kobzol]

Ah, sorry, I thought it failed during a merge but succeeded in a try build.

[lqd]

Same for me, we maybe should show the sha being tested in the result log to disambiguate the results when multiple builds are in flight.

[cuviper]

Maybe we can try using hashbrown's raw entry, rather than bringing it back in the standard library? It's a dead feature, we should move on...

(But I'm also surprised that the switch to HashTable hurt any performance, so I'd like to investigate that.)

[cuviper]

The perf results are not so bad to be "must revert" in my opinion either.

[Zoxc]

The result look neutral except for doc builds, which is a bit odd as the query system is quite hot for check builds. The code also looks equivalent to the old code, ignoring inlining.

[bors]

☀️ Try build successful - checks-actions
Build commit: cce9209 (cce92091ad3df0734691fd2070195f39667db6db)

[fmease]

I definitely want to avoid reverting two PRs and bringing an FCP-closed feature back to life.

I've just quickly spun up this PR to possibly start some perf experiments starting with a clean revert (which might've been overkill I admit).

But I'm also surprised that the switch to HashTable hurt any performance, so I'd like to investigate that

That would be awesome!

[rust-timer]

Finished benchmarking commit (f93dc40): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	9
Regressions ❌ (secondary)	0.6%	[0.1%, 1.5%]	31
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.2%]	15
Improvements ✅ (secondary)	-0.6%	[-0.9%, -0.1%]	33
All ❌✅ (primary)	-0.2%	[-0.9%, 0.3%]	24

Max RSS (memory usage)

Results (primary -0.8%, secondary -2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.7%	[-2.1%, -1.3%]	2
Improvements ✅ (secondary)	-2.5%	[-2.7%, -2.3%]	4
All ❌✅ (primary)	-0.8%	[-2.1%, 1.0%]	3

Cycles

Results (secondary 1.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.7%	[1.5%, 1.9%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 776.901s -> 780.89s (0.51%)
Artifact size: 365.07 MiB -> 365.10 MiB (0.01%)