Skip to content
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.

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

Tracking Issue for map_many_mut #97601

Open
1 of 3 tasks
Urgau opened this issue May 31, 2022 · 14 comments
Open
1 of 3 tasks

Tracking Issue for map_many_mut #97601

Urgau opened this issue May 31, 2022 · 14 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Urgau
Copy link
Member

Urgau commented May 31, 2022

Feature gate: #![feature(map_many_mut)]

This is a tracking issue for the HashMap::get_many{,_unchecked}_mut functions.

Attempts to get mutable references to N values in the map at once.

Public API

// in HashMap

pub fn get_many_mut<Q: ?Sized, const N: usize>(
    &mut self,
    ks: [&Q; N]
) -> Option<[&'_ mut V; N]>
where
    K: Borrow<Q>,
    Q: Hash + Eq;

pub unsafe fn get_many_unchecked_mut<Q: ?Sized, const N: usize>(
    &mut self,
    ks: [&Q; N],
) -> Option<[&'_ mut V; N]>
where
    K: Borrow<Q>,
    Q: Hash + Eq;

Steps / History

Unresolved Questions

@Urgau Urgau added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 31, 2022
@ibraheemdev
Copy link
Member

Would it be better for this to return Result<[Option<&mut V>; N], DuplicateKeys>?

@Urgau
Copy link
Member Author

Urgau commented Jun 6, 2022

Would it be better for this to return Result<[Option<&mut V>; N], DuplicateKeys>?

I don't think so, returning a Result<_, DuplicateKeys> would different from any other get methods on HashMap; they all return Option<_> of something; And returning a individual Option for every entry would completely change the semantic of the function and make things more difficult for peoples who wants all or nothing to achieve it.

@Arnavion
Copy link

And returning a individual Option for every entry would completely change the semantic of the function and make things more difficult for peoples who wants all or nothing to achieve it.

Sure, but the current implementation makes things more difficult for people who want to do multiple lookups in parallel and then do some mutations based on which keys exist and don't exist.

Returning [Option<&mut>] would not only provide strictly more information, but can be converted to Option<[&mut]> using result.try_map(std::convert::identity) (as suggested by AstrallyForged on IRC).


If I follow the history correctly, the libstd implementation returns Option<[&mut]> because hashbrown's does. hashbrown's implementation originally returned [Result<&mut>] but this was then changed to Option<[&mut]> to align with libstd's [T]::get_many_mut. So perhaps we need to start there.

bors added a commit to rust-lang-ci/rust that referenced this issue Jun 21, 2022
Use get_many_mut to reduce the cost of setting up check cfg values

This PR use the newly added [`get_many_mut`](rust-lang#97601) function in [`HashMap`](https://doc.rust-lang.org/nightly/std/collections/hash_map/struct.HashMap.html#method.get_many_mut) to reduce the cost of setting up the initial check cfg values.

cc `@petrochenkov`
@workingjubilee
Copy link
Member

Bikesheddy note: this is very similar to the notion of a "gather" (a la Simd::gather_*) and that term is used extensively in similar interfaces (though it is also called something like "indexed vector load" in some cases).

@forrestli74
Copy link

This seems very restrictive on the number of elements at the same time. Would be nice to have a version that supports Vec or Iterator of some sort. We don't have to support the general form now, but should think about how to design the API that adding general form is easy.

@mqudsi
Copy link
Contributor

mqudsi commented Dec 1, 2022

Are there any performance benefits to this other than eliminating a for loop caller-side? Can someone explain the motivation or a use-case for this, given that the code this replaces is rather short and to-the-point?

@smarnach
Copy link
Contributor

smarnach commented Dec 5, 2022

@mqudsi It's currently impossible in safe code to get simultaneous mutable references to different values in a HashMap, and the unsafe code needed to do this in a sound way is not straightforward at all. You can easily get shared references to multiple values, which is why there is only get_many_mut() and no get_many(), but for multiple mutable references the new function is helpful.

@Uriopass
Copy link
Contributor

What about BTreeMap ? Would it be possible to have get_many_mut and get_many_unchecked_mut there too ?

@Darksonn
Copy link
Contributor

and the unsafe code needed to do this in a sound way is not straightforward at all

Can it be done at all?

@nagisa
Copy link
Member

nagisa commented May 18, 2023

Rather than returning references to values, could this be an entry-based API instead? That would forgo the questions about people wanting to potentially make multiple lookups without them necessarily failing if some of them are missing. The current proposed function could easily be built on top of an entry-based API, but vice-versa is not possible.

@Arnavion
Copy link

Does it work to have multiple Entrys in flight simultaneously? Changing one of them from Vacant to Occupied might require reallocation which would invalidate the other Occupieds.

tamird added a commit to aya-rs/aya that referenced this issue Aug 11, 2023
Store programs in a `hashbrown::HashMap` and expose `get_many_mut`. We
can revisit this dependency when
rust-lang/rust#97601 is resolved.
@JustForFun88
Copy link

This seems very restrictive on the number of elements at the same time. Would be nice to have a version that supports Vec or Iterator of some sort...

I implemented a similar thing in this pull request: rust-lang/hashbrown#408

@ilyvion
Copy link

ilyvion commented Nov 11, 2023

This seems very restrictive on the number of elements at the same time. Would be nice to have a version that supports Vec or Iterator of some sort...

I implemented a similar thing in this pull request: rust-lang/hashbrown#408

I don't see how that does what was requested. As far as I can tell based on that PR, your changes still require a const N: usize, i.e. it's a decided-at-compile-time number of queries and results, not a decided-at-runtime number of queries and results like Vec or a non-array-based iterator could support.

@DrAlta
Copy link

DrAlta commented Mar 16, 2024

What about BTreeMap ? Would it be possible to have get_many_mut and get_many_unchecked_mut there too ?

I assume they are waiting for this to reach stable for HashMaps before bringing it to BTreeMap.

But if you just want something that works

https://github.com/DrAlta/rust_quality_of_life.git

implements .get_many_mut() for BTreeMap in the GetManyMut trait

Thou it doesn't go poking around the innards of the BTreeMap it just iters over it and returns the wanted bits.

@Urgau Urgau added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests