Add slice methods for indexing via an array of indices. #83608

Kimundi · 2021-03-28T15:41:23Z

Disclaimer: It's been a while since I contributed to the main Rust repo, apologies in advance if this is large enough already that it should've been an RFC.

Update:

Based on feedback, removed the &[T] variant of this API, and removed the requirements for the indices to be sorted.

Description

This adds the following slice methods to core:

impl<T> [T] {
    pub unsafe fn get_many_unchecked_mut<const N: usize>(&mut self, indices: [usize; N]) -> [&mut T; N];
    pub fn get_many_mut<const N: usize>(&mut self, indices: [usize; N]) -> Option<[&mut T; N]>;
}

This allows creating multiple mutable references to disjunct positions in a slice, which previously required writing some awkward code with split_at_mut() or iter_mut(). For the bound-checked variant, the indices are checked against each other and against the bounds of the slice, which requires N * (N + 1) / 2 comparison operations.

This has a proof-of-concept standalone implementation here: https://crates.io/crates/index_many

Care has been taken that the implementation passes miri borrow checks, and generates straight-forward assembly (though this was only checked on x86_64).

Example

let v = &mut [1, 2, 3, 4];
let [a, b] = v.get_many_mut([0, 2]).unwrap();
std::mem::swap(a, b);
*v += 100;
assert_eq!(v, &[3, 2, 101, 4]);

Codegen Examples

Click to expand!

Disclaimer: Taken from local tests with the standalone implementation.

Unchecked Indexing:

pub unsafe fn example_unchecked(slice: &mut [usize], indices: [usize; 3]) -> [&mut usize; 3] {
    slice.get_many_unchecked_mut(indices)
}

example_unchecked:
 mov     rcx, qword, ptr, [r9]
 mov     r8, qword, ptr, [r9, +, 8]
 mov     r9, qword, ptr, [r9, +, 16]
 lea     rcx, [rdx, +, 8*rcx]
 lea     r8, [rdx, +, 8*r8]
 lea     rdx, [rdx, +, 8*r9]
 mov     qword, ptr, [rax], rcx
 mov     qword, ptr, [rax, +, 8], r8
 mov     qword, ptr, [rax, +, 16], rdx
 ret

Checked Indexing (Option):

pub unsafe fn example_option(slice: &mut [usize], indices: [usize; 3]) -> Option<[&mut usize; 3]> {
    slice.get_many_mut(indices)
}

 mov     r10, qword, ptr, [r9, +, 8]
 mov     rcx, qword, ptr, [r9, +, 16]
 cmp     rcx, r10
 je      .LBB0_7
 mov     r9, qword, ptr, [r9]
 cmp     rcx, r9
 je      .LBB0_7
 cmp     rcx, r8
 jae     .LBB0_7
 cmp     r10, r9
 je      .LBB0_7
 cmp     r9, r8
 jae     .LBB0_7
 cmp     r10, r8
 jae     .LBB0_7
 lea     r8, [rdx, +, 8*r9]
 lea     r9, [rdx, +, 8*r10]
 lea     rcx, [rdx, +, 8*rcx]
 mov     qword, ptr, [rax], r8
 mov     qword, ptr, [rax, +, 8], r9
 mov     qword, ptr, [rax, +, 16], rcx
 ret
.LBB0_7:
 mov     qword, ptr, [rax], 0
 ret

Checked Indexing (Panic):

pub fn example_panic(slice: &mut [usize], indices: [usize; 3]) -> [&mut usize; 3] {
    let len = slice.len();
    match slice.get_many_mut(indices) {
        Some(s) => s,
        None => {
            let tmp = indices;
            index_many::sorted_bound_check_failed(&tmp, len)
        }
    }
}

example_panic:
 sub     rsp, 56
 mov     rax, qword, ptr, [r9]
 mov     r10, qword, ptr, [r9, +, 8]
 mov     r9, qword, ptr, [r9, +, 16]
 cmp     r9, r10
 je      .LBB0_6
 cmp     r9, rax
 je      .LBB0_6
 cmp     r9, r8
 jae     .LBB0_6
 cmp     r10, rax
 je      .LBB0_6
 cmp     rax, r8
 jae     .LBB0_6
 cmp     r10, r8
 jae     .LBB0_6
 lea     rax, [rdx, +, 8*rax]
 lea     r8, [rdx, +, 8*r10]
 lea     rdx, [rdx, +, 8*r9]
 mov     qword, ptr, [rcx], rax
 mov     qword, ptr, [rcx, +, 8], r8
 mov     qword, ptr, [rcx, +, 16], rdx
 mov     rax, rcx
 add     rsp, 56
 ret
.LBB0_6:
 mov     qword, ptr, [rsp, +, 32], rax
 mov     qword, ptr, [rsp, +, 40], r10
 mov     qword, ptr, [rsp, +, 48], r9
 lea     rcx, [rsp, +, 32]
 mov     edx, 3
 call    index_many::bound_check_failed
 ud2

Extensions

There are multiple optional extensions to this.

Indexing With Ranges

This could easily be expanded to allow indexing with [I; N] where I: SliceIndex<Self>. I wanted to keep the initial implementation simple, so I didn't include it yet.

Panicking Variant

We could also add this method:

impl<T> [T] {
    fn index_many_mut<const N: usize>(&mut self, indices: [usize; N]) -> [&mut T; N];
}

This would work similar to the regular index operator and panic with out-of-bound indices. The advantage would be that we could more easily ensure good codegen with a useful panic message, which is non-trivial with the Option variant.

This is implemented in the standalone implementation, and used as basis for the codegen examples here and there.

rust-highfive · 2021-03-28T15:41:26Z

r? @sfackler

(rust-highfive has picked a reviewer for you, use r? to override)

leonardo-m · 2021-03-28T22:15:42Z

To reduce the API surface is it a good idea to offer only the mut versions? Getting multiple immutable references is already possible in simpler ways.

And regarding the Presorted, in the D language stdlib, the sort functions return a SortedRange (https://dlang.org/phobos/std_algorithm_sorting.html#sort ), it's a type that can be converted to a regular slice, but it can also be used as is. A type that remembers the sortdness of a slice is handy, it performs a safer binary search (the compiler forbids you from performing a binary search on an unsorted range by mistake. You have assumeSorted function (https://dlang.org/phobos/std_range.html#.SortedRangeOptions ), but in debug mode it performs a statistically cheap test that the range is actually sorted) and you can use it in other cases like indexing for your functions.

leonardo-m · 2021-03-28T22:17:44Z

See also #78541 where I asked this for just two indices... that's my most common case.

Kimundi · 2021-03-28T22:39:53Z

The std lib usually provides variants for both mutable and immutable slices the same way, even if the immutable one can be expressed differently already. See split_at and split_at_mut for example.

burdges · 2021-04-04T22:44:22Z

~~I'm unsure how exactly how the rules changed, but.. this is simple enough it might not require an RFC under the current rules.~~

I give some related convenience methods around split_at_mut in rust-lang/rfcs#3100 (comment) so if this does not pass then maybe some slice enhancement crate makes sense or already exists.

bors · 2021-04-05T01:48:39Z

☔ The latest upstream changes (presumably #83530) made this pull request unmergeable. Please resolve the merge conflicts.

leonardo-m · 2021-04-05T09:05:40Z

The std lib usually provides variants for both mutable and immutable slices the same way,

Yeah, but increasing API surface has a cognitive and other kind of costs. So if a function isn't that useful, it could be better to break the symmetry.

burdges · 2021-04-05T12:38:01Z

An immutable variant often helps whenever anyone writes another method with both mutable and immutable variants, because then their two methods read identically. Our eyes run %s/[_ ]mut//g somewhat accurately over small areas. ;)

What code replaces get_many here?

let a = slice[5];
let b = slice[7];
let c = slice[13];

or

let [a,b,c] = [5,7,13].iter().map(|idx| slice[idx]).collect::<[T;3]>();

leonardo-m · 2021-04-05T12:50:53Z

On Nightly we can write shorter code now :-)

#![feature(array_map)]

let [a, b, c] = [5, 7, 13].map(|idx| slice[idx]);

burdges · 2021-04-05T16:12:21Z

Nice. :) Appears the reserve_mut variant of split_at_mut almost covers the mutable case too, except indexing requires some care.

#[derive(Default)]
pub struct Increments(Option<usize>);
impl Increments {
    fn next(&must self, o: usize) -> usize {
            if let Some(offset) = self.0.clone() {
                assert!(o > offset);
                let n = o-offset;
                *self = Some(o);
                n
            } else {
                offset = Some(o);
                o
            }
    }
}

impl<T> [T] {
    fn get_many_mut<'a,const : usize>(mut &'a mut self, offsets: [usize; N]) -> Option<[&'a mut T ; N]>
    {
        let mut off = Increments::default();
        offsets.map( |o| (&mut self).reserve_mut(off.next(o)).first_mut().unwrap() )
    }
}

branpk · 2021-04-05T16:51:33Z

Shouldn't this panic if the indices are unsorted? Seems like a pitfall that it silently returns None (which is also used to indicate the indices being out of bounds).

Also since the common case will be for these arrays to be relatively small (I would imagine 2 is the most common case), I wonder if it's even worth requiring the indices to be sorted? Or maybe there could be two separate methods? I'm thinking about other collections like HashMap where we will presumably want this operation as well, eventually. Inconsistent preconditions between HashMap and slice could easily lead to confusion/bugs.

Dylan-DPC-zz · 2021-05-28T12:53:20Z

r? @m-ou-se

m-ou-se

Thanks for making this!

Also since the common case will be for these arrays to be relatively small (I would imagine 2 is the most common case), I wonder if it's even worth requiring the indices to be sorted?

While discussing this in the libs team meeting recently, we were asking ourselves the same question. We imagine this will mostly be used with small values of N (e.g. 2 or 3), in which case checking for duplicates without requiring it to be sorted would be fine. (Or maybe it could first check if it's sorted to have a fast common case, but fall back to the O(N²) check otherwise?) (And since N will be low in most cases, maybe multiple might be a better name than many. But we can leave the bikeshedding for later. ^^)

That would solve the question of what to do for unsorted indices (panic vs returning None) by making the question irrelevant.

It'd also match better with the unchecked version. Otherwise we'd have cases where the unchecked version can safely be used, (e.g. [1, 0]), but the checked version would panic or return None, which can be confusing.

And for that same reason, I think it'd be best to weaken the requirements on the non-mut version as much as possible too.

What do you think?

A also have a few comments on the implementation:

m-ou-se · 2021-06-05T13:28:04Z

library/core/src/slice/mod.rs

+        let mut arr: mem::MaybeUninit<[&T; N]> = mem::MaybeUninit::uninit();
+        let arr_ptr = arr.as_mut_ptr();
+
+        // SAFETY: We expect `indices` to contain disjunct values that are
+        // in bounds of `self`.
+        unsafe {
+            for i in 0..N {
+                let idx = *indices.get_unchecked(i);
+                *(*arr_ptr).get_unchecked_mut(i) = &*slice.get_unchecked(idx);
+            }
+            arr.assume_init()
+        }


You can use array::map to simplify this:

Suggested change

let mut arr: mem::MaybeUninit<[&T; N]> = mem::MaybeUninit::uninit();

let arr_ptr = arr.as_mut_ptr();

// SAFETY: We expect `indices` to contain disjunct values that are

// in bounds of `self`.

unsafe {

for i in 0..N {

let idx = *indices.get_unchecked(i);

*(*arr_ptr).get_unchecked_mut(i) = &*slice.get_unchecked(idx);

}

arr.assume_init()

}

indices.map(|i| unsafe { &*slice.get_unchecked(i) })

get_unchecked() returns &T, so the &* would be unneeded. Apart from that - I started out with the .map function, but it generated worse code when I tried it. Believe me, I did not arrive at this complicated piece of unsafe code as a first choice. 😄

m-ou-se · 2021-06-05T13:29:03Z

library/core/src/slice/mod.rs

+        let mut arr: mem::MaybeUninit<[&mut T; N]> = mem::MaybeUninit::uninit();
+        let arr_ptr = arr.as_mut_ptr();
+
+        // SAFETY: We expect `indices` to contain disjunct values that are
+        // in bounds of `self`.
+        unsafe {
+            for i in 0..N {
+                let idx = *indices.get_unchecked(i);
+                *(*arr_ptr).get_unchecked_mut(i) = &mut *slice.get_unchecked_mut(idx);
+            }
+            arr.assume_init()
+        }


Suggested change

let mut arr: mem::MaybeUninit<[&mut T; N]> = mem::MaybeUninit::uninit();

let arr_ptr = arr.as_mut_ptr();

// SAFETY: We expect `indices` to contain disjunct values that are

// in bounds of `self`.

unsafe {

for i in 0..N {

let idx = *indices.get_unchecked(i);

*(*arr_ptr).get_unchecked_mut(i) = &mut *slice.get_unchecked_mut(idx);

}

arr.assume_init()

}

indices.map(|i| unsafe { &mut *slice.get_unchecked_mut(i) })

get_unchecked_mut() returns &mut T, so the &mut * would not be needed.

This would not compile due to the static borrow checker for mutable borrows

i.map(|i| unsafe { &mut *(s.get_unchecked_mut(i) as *mut _) }) to make it raw pointers does not pass miri checks.

library/core/src/slice/mod.rs

bors · 2022-06-10T03:57:20Z

☔ The latest upstream changes (presumably #91970) made this pull request unmergeable. Please resolve the merge conflicts.

Kimundi · 2022-08-06T12:22:20Z

@Ten0 It was never abandoned from my side. Rather, the idea was that once we have this merged, we could have a follow-up PR for replacing the hardcoded types with generics that do what you propose. Of course, I did not expect this PR to remain open so long...

rustbot · 2022-08-06T13:00:01Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Kimundi · 2022-08-06T13:12:26Z

@rustbot label +T-libs-api -T-libs

bors · 2022-08-20T20:08:50Z

☔ The latest upstream changes (presumably #100809) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2022-10-08T23:23:23Z

Triage:
@Kimundi can you please address the merge conflicts?

Mark-Simulacrum · 2022-11-20T16:31:59Z

Filed a tracking issue (#104642), rebased, and added the link to it from the feature gates here. I think this can go in as-is, but I noted the unresolved questions on the tracking issue in terms of refactoring/expanding the API to fit more use cases.

Normally we'd push back for an ACP but this has received attention from a number of libs-api meetings (albeit a while back) and the desire for it seems clear, even if we wish to iterate on the precise API, which can happen in future PRs.

@bors r+

bors · 2022-11-20T16:32:01Z

📌 Commit 3fe37b8 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

…earth Rollup of 7 pull requests Successful merges: - rust-lang#83608 (Add slice methods for indexing via an array of indices.) - rust-lang#95583 (Deprecate the unstable `ptr_to_from_bits` feature) - rust-lang#101655 (Make the Box one-liner more descriptive) - rust-lang#102207 (Constify remaining `Layout` methods) - rust-lang#103193 (mark sys_common::once::generic::Once::new const-stable) - rust-lang#104622 (Use clang for the UEFI targets) - rust-lang#104638 (Move macro_rules diagnostics to diagnostics module) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

# Objective - std's new APIs do the same thing as `Query::get_multiple_mut`, but are called `get_many`: rust-lang/rust#83608 ## Solution - Find and replace `get_multiple` with `get_many`

rust-highfive assigned sfackler Mar 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2021

This comment has been minimized.

Sign in to view

Kimundi changed the title ~~[WIP] Add slice methods for indexing via an array of indices.~~ Add slice methods for indexing via an array of indices. Mar 30, 2021

Kimundi mentioned this pull request Apr 4, 2021

RFC: Add a standard trait for getting many &mut to places rust-lang/rfcs#3100

Open

Kimundi force-pushed the index_many branch from 9260484 to 3697f76 Compare April 5, 2021 11:30

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2021

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2021

rust-highfive assigned m-ou-se and unassigned sfackler May 28, 2021

m-ou-se reviewed Jun 5, 2021

View reviewed changes

m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021

Arnavion mentioned this pull request Jun 16, 2022

Tracking Issue for map_many_mut #97601

Open

3 tasks

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022

cuviper mentioned this pull request Jul 15, 2022

Implement get_many_mut indexmap-rs/indexmap#238

Open

Kimundi force-pushed the index_many branch from bfb2bd0 to 712b500 Compare August 6, 2022 12:59

Mark-Simulacrum assigned Mark-Simulacrum and unassigned m-ou-se Aug 21, 2022

Mark-Simulacrum mentioned this pull request Nov 20, 2022

Tracking Issue for get_many_mut #104642

Open

3 tasks

Mark-Simulacrum force-pushed the index_many branch 2 times, most recently from 38d91fc to 95e46a4 Compare November 20, 2022 16:17

Add get_many_mut methods to slice

3fe37b8

Mark-Simulacrum force-pushed the index_many branch from 95e46a4 to 3fe37b8 Compare November 20, 2022 16:19

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 Nov 20, 2022

Manishearth mentioned this pull request Nov 22, 2022

Rollup of 7 pull requests #104702

Merged

bors merged commit 1dd515f into rust-lang:master Nov 22, 2022

rustbot added this to the 1.67.0 milestone Nov 22, 2022

ChayimFriedman2 mentioned this pull request Jul 28, 2024

Stabilize <[T]>::get_many_mut() #128318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add slice methods for indexing via an array of indices. #83608

Add slice methods for indexing via an array of indices. #83608

Kimundi commented Mar 28, 2021 •

edited

Loading

rust-highfive commented Mar 28, 2021

This comment has been minimized.

leonardo-m commented Mar 28, 2021

leonardo-m commented Mar 28, 2021 •

edited

Loading

Kimundi commented Mar 28, 2021

burdges commented Apr 4, 2021 •

edited

Loading

bors commented Apr 5, 2021

leonardo-m commented Apr 5, 2021

burdges commented Apr 5, 2021

leonardo-m commented Apr 5, 2021

burdges commented Apr 5, 2021

branpk commented Apr 5, 2021

Dylan-DPC-zz commented May 28, 2021

m-ou-se left a comment

m-ou-se Jun 5, 2021

Kimundi Jun 15, 2021

m-ou-se Jun 5, 2021

Kimundi Jun 15, 2021

bors commented Jun 10, 2022

Kimundi commented Aug 6, 2022

rustbot commented Aug 6, 2022

Kimundi commented Aug 6, 2022

bors commented Aug 20, 2022

JohnCSimon commented Oct 8, 2022

Mark-Simulacrum commented Nov 20, 2022

bors commented Nov 20, 2022

Add slice methods for indexing via an array of indices. #83608

Add slice methods for indexing via an array of indices. #83608

Conversation

Kimundi commented Mar 28, 2021 • edited Loading

Update:

Description

Example

Codegen Examples

Unchecked Indexing:

Checked Indexing (Option):

Checked Indexing (Panic):

Extensions

Indexing With Ranges

Panicking Variant

rust-highfive commented Mar 28, 2021

This comment has been minimized.

leonardo-m commented Mar 28, 2021

leonardo-m commented Mar 28, 2021 • edited Loading

Kimundi commented Mar 28, 2021

burdges commented Apr 4, 2021 • edited Loading

bors commented Apr 5, 2021

leonardo-m commented Apr 5, 2021

burdges commented Apr 5, 2021

leonardo-m commented Apr 5, 2021

burdges commented Apr 5, 2021

branpk commented Apr 5, 2021

Dylan-DPC-zz commented May 28, 2021

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Jun 5, 2021

Choose a reason for hiding this comment

Kimundi Jun 15, 2021

Choose a reason for hiding this comment

m-ou-se Jun 5, 2021

Choose a reason for hiding this comment

Kimundi Jun 15, 2021

Choose a reason for hiding this comment

bors commented Jun 10, 2022

Kimundi commented Aug 6, 2022

rustbot commented Aug 6, 2022

Kimundi commented Aug 6, 2022

bors commented Aug 20, 2022

JohnCSimon commented Oct 8, 2022

Mark-Simulacrum commented Nov 20, 2022

bors commented Nov 20, 2022

Kimundi commented Mar 28, 2021 •

edited

Loading

leonardo-m commented Mar 28, 2021 •

edited

Loading

burdges commented Apr 4, 2021 •

edited

Loading