Remove duplicated code in slice/index.rs

[scottmcm]

Looks like const fn is far enough along now that we can just not have these two copies any more, and have one call the other.

[rustbot]

r? @joboet

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

[scottmcm]

"Execution context was destroyed"? I'll try re-kicking CI...

[joboet]

@bors try @rust-timer queue
slice::range might be used in some performance-sensitive areas...

[rust-bors]

☀️ Try build successful (CI)
Build commit: 01347f5 (01347f57b14165507e80fdcea75f1b162433bed0, parent: 94a0cd15f5976fa35e5e6784e621c04e9f958e57)

[rust-timer]

Finished benchmarking commit (01347f5): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.7%]	16
Regressions ❌ (secondary)	0.6%	[0.1%, 1.6%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.1%, 0.7%]	16

Max RSS (memory usage)

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

Cycles

Results (primary -2.1%, secondary -4.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-4.2%	[-5.1%, -3.2%]	2
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Binary size

Results (primary -0.0%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	5
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.1%	[-0.3%, -0.0%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.3%, 0.0%]	9

Bootstrap: 473.018s -> 471.612s (-0.30%)
Artifact size: 385.68 MiB -> 385.71 MiB (0.01%)

[scottmcm]

Weird perf result. doc is unhappy about icounts, but then bootstrap is faster and the cycle numbers are all green -- even for doc.

[asder8215]

Looking at Compiler Explorer with a simple example on .start_bound().copied() vs .start_bound().cloned() on Range<usize>, the assembly looks roughly the same, but the path reaching .LBB0_2/.LBB0_3 for .copied() uses one less mov instruction than the path reaching .LBB1_2/.LBB1_3 (even if I take off the #[unsafe(no_mangle)], it still uses 5 mov vs 6 mov instruction with copied() vs cloned()). Not sure if one less mov instruction would easily be detected through benchmarking like this though.

[hkBst]

Looking at Compiler Explorer

I would expect identical code, and IIANM that is what I'm seeing. Where do you see a difference?

[asder8215]

@hkBst From what I'm seeing here, it looks like there are 5 mov instructions in .LBB0_2/.LBB0_3 (which is for .copied()) and 6 mov instructions in .LBB1_2/.LBB1_3 (which is for .cloned())? Are you seeing something different from the output? Also, correct me if I'm misinterpreting this or reading this incorrectly.

[hkBst]

@asder8215 Ah right, that is curious. But this is without optimizations, so I still expect it to make no difference when optimizations are enabled.

[scottmcm]

With optimizations they come out completely identical, yup: https://godbolt.org/z/axEbj3azP

(Note that I changed them to take parameters and return them, because otherwise both things optimize to NOPs.)

[scottmcm]

Hmm, I rebased and got some codegen test failures that went away with an inline, so let's see if that was the core problem

@bors try @rust-timer queue

[asder8215]

@scottmcm Actually, I just took a look at your godbolt link and from the looks of it, I'm not sure if it actually shows the assembly output of .start_bound().copied() vs .start_bound().cloned() functions itself. This assembly output:

test_one:
        xor     eax, eax
        mov     rdx, rdi
        ret

test_two:
        xor     eax, eax
        mov     rdx, rdi
        ret

Feels like it's missing stuff like retrieving the argument from the stack pointer (because an argument Range<usize> is passed into this function), acknowledging that you're grabbing the start_bound() from Range<usize> (which the Range struct has two different fields, start and end), actually tell you what's going on internally within .copied() and .cloned(), and I'm pretty sure it would be calling convention to return stuff from a function by storing the content into a %rax register. For all I know mov rdx, rdi could be preparing an argument for a function call that is not shown here.

And it seems like this filtering of asm output on -O or release mode is an ongoing issue within compiler explorer and rust lang as noted here:

• [BUG]: Functions may not appear in output when using rust >= v1.75.0 compiler-explorer/compiler-explorer#5939
• [BUG]: Functions may not appear in output when using rust >= v1.75.0 compiler-explorer/compiler-explorer#5939

[scottmcm]

Feels like it's missing stuff like retrieving the argument from the stack pointer (because an argument Range is passed into this function),

Range<usize> gets the ScalarPair backend representation, so it's passed as the two usizes in registers, not via the stack.

Then start_bound() on a Range<usize> always returns Bound::Inclusive(_), which is discriminant zero, thus the xor eax, eax, and the mov rdx, rdi is just moving the start from the register where it was an input to the expected register for the second part of the ScalarPair return.

So yes, this function really is that trivial after optimizations.

If you look at the MIR https://godbolt.org/z/rdajfv9PE you'll see that even before LLVM does anything, test_one is already simplified down (thanks to mir inlining and other such things) to just

    bb0: {
        StorageLive(_2);
        _2 = copy (_1.0: usize);
        _0 = Bound::<usize>::Included(move _2);
        StorageDead(_2);
        return;
    }

test_two is a bit more complex, but you can pull up the LLVM opt pipeline view https://godbolt.org/z/fEE868vb8 to see that the call is there originally, but it gets inlined in LLVM then SRoA eliminates all the allocas.

[rust-bors]

☀️ Try build successful (CI)
Build commit: ba32940 (ba32940c41dc4790dc46c1040ed7d340e7441c8e, parent: 36e2b8a3a7aad93f8a92db6d254b746aa94ed6da)

[rust-timer]

Finished benchmarking commit (ba32940): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

Results (primary -1.5%, secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-2.4%	[-2.5%, -2.3%]	2
All ❌✅ (primary)	-1.5%	[-1.5%, -1.5%]	1

Cycles

Results (secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.7%]	15
Regressions ❌ (secondary)	0.1%	[0.0%, 0.4%]	39
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.7%]	16

Bootstrap: 473.793s -> 473.111s (-0.14%)
Artifact size: 397.77 MiB -> 397.80 MiB (0.01%)

[scottmcm]

Good call running perf, @joboet !

With the added #[inline] the bot agrees that we're at nothing now, as it should be.

[asder8215]

Oh that's really interesting @scottmcm

Thanks for going into detail about the optimization!

[jhpratt]

r? jhpratt

@bors r+ rollup

[rust-bors]

📌 Commit a3f169c has been approved by jhpratt

It is now in the queue for this repository.