Skip to content

std: Add performance warnings to HashMap::get_disjoint_mut #139307

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
Merged
merged 2 commits into from
Apr 24, 2025
Merged

std: Add performance warnings to HashMap::get_disjoint_mut #139307

merged 2 commits into from
Apr 24, 2025

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Apr 3, 2025

Closes #139296

The get_disjoint_mut in HashMap also performs a complexity O(n^2) check. So we need to be reminded of that as well.

https://github.com/rust-lang/hashbrown/blob/b5b0655a37e156f9798ac8dd7e970d4adba9bf90/src/raw/mod.rs#L1216-L1220

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2025

r? @thomcc

rustbot has assigned @thomcc.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 3, 2025
@mattfbacon
Copy link
Contributor

To be honest I don't understand why, why not just sort the list of hashes and check neighbours (n log n)?

@xizheyin
Copy link
Contributor Author

xizheyin commented Apr 3, 2025

I'm not sure. I think what you said seems reasonable.We can wait for the members of libs to discuss it. Maybe we can change the implementation.

@mattfbacon
Copy link
Contributor

mattfbacon commented Apr 3, 2025 •
edited
Loading

So, we can't do it with the hashes due to collisions, and T doesn't have PartialOrd.

However, I think it would be possible to do everything up to getting the raw pointer of each element to return, then use this technique there. The performance is worse in the case of an overlap, but I think that is irrelevant since it panics in that case.

But honestly it's probably not worth it, most use cases are probably 2 or 3 keys where the perf would probably even be worse. So I would go ahead with documenting it.

@zirconium-n
Copy link
Contributor

So, we can't do it with the hashes due to collisions, and T doesn't have PartialOrd.

However, I think it would be possible to do everything up to getting the raw pointer of each element to return, then use this technique there. The performance is worse in the case of an overlap, but I think that is irrelevant since it panics in that case.

But honestly it's probably not worth it, most use cases are probably 2 or 3 keys where the perf would probably even be worse. So I would go ahead with documenting it.

Actually std delegates to hashbrown which is indeed already comparing the pointers, so you can sort it. However just like what you said, it's not like we are really going to query 100 keys, and there's unsafe version of API for that anyway.

@xizheyin
std: Add performance warnings to HashMap::get_disjoint_mut
fe03f76 
Signed-off-by: xizheyin <xizheyin@smail.nju.edu.cn>
@xizheyin
Copy link
Contributor Author

r? libs

@rustbot rustbot assigned joboet and unassigned thomcc Apr 14, 2025
joboet
joboet approved these changes Apr 20, 2025
View reviewed changes
@xizheyin @joboet
Use currently for futher improvement
16381b3 
Co-authored-by: Jonas Böttiger <jonasboettiger@icloud.com>
@xizheyin xizheyin requested a review from joboet April 20, 2025 13:20
@joboet
Copy link
Member

joboet commented Apr 23, 2025

Thanks!
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 23, 2025

📌 Commit 16381b3 has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 24, 2025
@Zalathar
Rollup merge of rust-lang#139307 - xizheyin:issue-139296, r=joboet
83ecddd 
std: Add performance warnings to HashMap::get_disjoint_mut

Closes rust-lang#139296

The `get_disjoint_mut` in `HashMap` also performs a complexity O(n^2) check. So we need to be reminded of that as well.

https://github.com/rust-lang/hashbrown/blob/b5b0655a37e156f9798ac8dd7e970d4adba9bf90/src/raw/mod.rs#L1216-L1220
@Zalathar Zalathar mentioned this pull request Apr 24, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2025
@bors
Auto merge of rust-lang#140231 - Zalathar:rollup-6bg0hhp, r=Zalathar
157a2b2 
Rollup of 23 pull requests

Successful merges:

 - rust-lang#139261 (mitigate MSVC alignment issue on x86-32)
 - rust-lang#139307 (std: Add performance warnings to HashMap::get_disjoint_mut)
 - rust-lang#139700 (Autodiff flags)
 - rust-lang#139752 (set subsections_via_symbols for ld64 helper sections)
 - rust-lang#139809 (Don't warn about `v128` in wasm ABI transition)
 - rust-lang#139852 (StableMIR: Implement `CompilerInterface`)
 - rust-lang#139945 (Extend HIR to track the source and syntax of a lifetime)
 - rust-lang#140028 (`deref_patterns`: support string and byte string literals in explicit `deref!("...")` patterns)
 - rust-lang#140139 (rustc_target: Adjust RISC-V feature implication)
 - rust-lang#140143 (Move `sys::pal::os::Env` into `sys::env`)
 - rust-lang#140148 (CI: use aws codebuild for job dist-arm-linux)
 - rust-lang#140150 (fix MAX_EXP and MIN_EXP docs)
 - rust-lang#140172 (Make algebraic functions into `const fn` items.)
 - rust-lang#140177 ([compiletest] Parallelize test discovery)
 - rust-lang#140181 (Remove `synstructure::Structure::underscore_const` calls.)
 - rust-lang#140184 (Update doc of cygwin target)
 - rust-lang#140186 (Rename `compute_x` methods)
 - rust-lang#140187 ([AIX] Handle AIX dynamic library extensions within c-link-to-rust-dylib run-make test)
 - rust-lang#140191 (Remove git repository from git config)
 - rust-lang#140194 (minicore: Have `//@ add-core-stubs` also imply `-Cforce-unwind-tables=yes`)
 - rust-lang#140195 (triagebot: label minicore changes w/ `A-test-infra-minicore` and ping jieyouxu on changes)
 - rust-lang#140202 (Make #![feature(let_chains)] bootstrap conditional in compiler/)
 - rust-lang#140214 (Remove comment about handling non-global where bounds with corresponding projection)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 24, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2025
@bors
Auto merge of rust-lang#140239 - matthiaskrgr:rollup-75felo8, r=matth…
dc8fe1f 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#134446 (Stabilize the `cell_update` feature)
 - rust-lang#139307 (std: Add performance warnings to HashMap::get_disjoint_mut)
 - rust-lang#139450 (Impl new API `std::os::unix::fs::mkfifo` under feature `unix_fifo`)
 - rust-lang#139809 (Don't warn about `v128` in wasm ABI transition)
 - rust-lang#139852 (StableMIR: Implement `CompilerInterface`)
 - rust-lang#139945 (Extend HIR to track the source and syntax of a lifetime)
 - rust-lang#140028 (`deref_patterns`: support string and byte string literals in explicit `deref!("...")` patterns)
 - rust-lang#140181 (Remove `synstructure::Structure::underscore_const` calls.)
 - rust-lang#140232 (Remove unnecessary clones)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cb3c5d7 into rust-lang:master Apr 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 24, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2025
@rust-timer
Unrolled build for rust-lang#139307
ff09981 
Rollup merge of rust-lang#139307 - xizheyin:issue-139296, r=joboet

std: Add performance warnings to HashMap::get_disjoint_mut

Closes rust-lang#139296

The `get_disjoint_mut` in `HashMap` also performs a complexity O(n^2) check. So we need to be reminded of that as well.

https://github.com/rust-lang/hashbrown/blob/b5b0655a37e156f9798ac8dd7e970d4adba9bf90/src/raw/mod.rs#L1216-L1220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaslueg lukaslueg lukaslueg left review comments

@joboet joboet Awaiting requested review from joboet

Assignees

@joboet joboet

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.88.0
Development

Successfully merging this pull request may close these issues.

Inconsistent warning on get_disjoint_mut
8 participants
@xizheyin @rustbot @mattfbacon @zirconium-n @joboet @bors @lukaslueg @thomcc