Specialize slice::fill with Copy type and u8/i8/bool #81874

tesuji · 2021-02-08T05:19:19Z

I don't expect rustperf could measure any perf improvements with this changes
since slice::fill is newly added.

Godbolt link for this change: https://rust.godbolt.org/z/r3fzee.

r? @matthewjasper since this patch added new specialization.

rust-log-analyzer · 2021-02-08T05:32:04Z

The job mingw-check failed! Check out the build log: (web) (plain)

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

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
Checking which error codes lack tests...
Found 436 error codes
Found 0 error codes with no tests
Done!
tidy error: /checkout/library/core/src/slice/specialize.rs:29: undocumented unsafe
tidy error: /checkout/library/core/src/slice/specialize.rs:39: undocumented unsafe
some tidy checks failed

command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build"
expected success, got: exit code: 1

the8472 · 2021-02-08T10:02:15Z

library/core/src/slice/specialize.rs

+        for item in self.iter_mut() {
+            *item = value;
+        }
+    }


Instead of specializing separately on [u8] below you can do a if mem::size_of::<T>() == 1 here and call memset for any type that's 1 byte wide.

Hm, that's another option. Will do that! Thanks.

value now has 1-byte size. ptr::write_bytes only accepts u8 as second argument.
Should I transmute value to u8 ? Is it sound ?

Even if I want to transmute, the compiler forbids me: https://rust.godbolt.org/z/a3bsYs

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types --> <source>:38:33 | 38 | let value: u8 = std::mem::transmute(value); | ^^^^^^^^^^^^^^^^^^^ | = note: source type: `T` (this type does not have a fixed size) = note: target type: `u8` (8 bits)

transmute_copy should work

Neat! That's really work! But I wonder if that those specific specialization for [u8] matters much with #81874 (comment)

The advantage is that it doesn't rely on the optimizer, so it'll work in debug mode too or with less powerful backends.

With the size_of approach it only has to be implemented once instead of one specialization for each T.

I don't have strong opinion on this. I defer that to reviewer to decide.

Hi @bjorn3. the8472 and I were discussing if other codegen backends could handle straight
for loop to copy item and make it to memcpy when item is 1-byte size.

Do you think cranelift could be benefited from this optimization ^^?

tesuji · 2021-02-08T11:40:00Z

library/core/src/slice/specialize.rs

+    }
+}
+
+impl SpecFill<u8> for [u8] {


The only difference when not specialize for [u8] is that compiler emits a check whether the slice is empty
before the call to memcpy.
https://rust.godbolt.org/z/z9fbcT.

Maybe it is not worth to keep those specs for u8/i8/bool slice.

matthewjasper · 2021-02-14T13:20:33Z

@bors try @rust-timer queue

rust-timer · 2021-02-14T13:20:34Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-02-14T13:20:44Z

⌛ Trying commit 010e194 with merge 194efb9e2ffaa05d0d10f8cb6c97e5a83f763f5d...

bors · 2021-02-14T14:08:30Z

☀️ Try build successful - checks-actions
Build commit: 194efb9e2ffaa05d0d10f8cb6c97e5a83f763f5d (194efb9e2ffaa05d0d10f8cb6c97e5a83f763f5d)

rust-timer · 2021-02-14T14:08:31Z

Queued 194efb9e2ffaa05d0d10f8cb6c97e5a83f763f5d with parent b86674e, future comparison URL.

rust-timer · 2021-02-14T15:47:30Z

Finished benchmarking try commit (194efb9e2ffaa05d0d10f8cb6c97e5a83f763f5d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

m-ou-se · 2021-02-14T18:57:25Z

Note that slice::fill isn't used in the compiler, so a perf run doesn't really say anything here.

tesuji · 2021-02-24T01:46:49Z

Friendly ping.

matthewjasper · 2021-02-27T11:40:54Z

@bors r+

bors · 2021-02-27T11:40:55Z

📌 Commit 010e194 has been approved by matthewjasper

bors · 2021-02-27T14:54:41Z

⌛ Testing commit 010e194 with merge ec7f8d9...

bors · 2021-02-27T17:35:29Z

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing ec7f8d9 to master...

tesuji · 2021-02-28T04:22:19Z

Thanks for the merge. But quick note when things slipped is that some comments are not resolved.

fix bootstrap

446798f

rust-highfive assigned matthewjasper Feb 8, 2021

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

the8472 reviewed Feb 8, 2021

View reviewed changes

Specialize slice::fill for Copy type and u8/i8/bool

010e194

tesuji force-pushed the spec_slice_fill branch from c566a2e to 010e194 Compare February 8, 2021 11:35

tesuji changed the title ~~Specialized slice::fill with Copy type and u8/i8~~ Specialized slice::fill with Copy type and u8/i8/bool Feb 8, 2021

tesuji commented Feb 8, 2021

View reviewed changes

tesuji changed the title ~~Specialized slice::fill with Copy type and u8/i8/bool~~ Specialize slice::fill with Copy type and u8/i8/bool Feb 9, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2021

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 Feb 27, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2021

bors merged commit ec7f8d9 into rust-lang:master Feb 27, 2021

rustbot added this to the 1.52.0 milestone Feb 27, 2021

tesuji deleted the spec_slice_fill branch February 28, 2021 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specialize slice::fill with Copy type and u8/i8/bool #81874

Specialize slice::fill with Copy type and u8/i8/bool #81874

tesuji commented Feb 8, 2021 •

edited

Loading

rust-log-analyzer commented Feb 8, 2021

the8472 Feb 8, 2021 •

edited

Loading

tesuji Feb 8, 2021

tesuji Feb 8, 2021 •

edited

Loading

tesuji Feb 8, 2021 •

edited

Loading

the8472 Feb 8, 2021

tesuji Feb 9, 2021 •

edited

Loading

the8472 Feb 9, 2021

tesuji Feb 9, 2021

tesuji Feb 10, 2021

tesuji Feb 8, 2021 •

edited

Loading

matthewjasper commented Feb 14, 2021

rust-timer commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

rust-timer commented Feb 14, 2021

rust-timer commented Feb 14, 2021

m-ou-se commented Feb 14, 2021

tesuji commented Feb 24, 2021

matthewjasper commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

tesuji commented Feb 28, 2021 •

edited

Loading

Specialize slice::fill with Copy type and u8/i8/bool #81874

Specialize slice::fill with Copy type and u8/i8/bool #81874

Conversation

tesuji commented Feb 8, 2021 • edited Loading

rust-log-analyzer commented Feb 8, 2021

the8472 Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

tesuji Feb 8, 2021

Choose a reason for hiding this comment

tesuji Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

tesuji Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

the8472 Feb 8, 2021

Choose a reason for hiding this comment

tesuji Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

the8472 Feb 9, 2021

Choose a reason for hiding this comment

tesuji Feb 9, 2021

Choose a reason for hiding this comment

tesuji Feb 10, 2021

Choose a reason for hiding this comment

tesuji Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

matthewjasper commented Feb 14, 2021

rust-timer commented Feb 14, 2021

bors commented Feb 14, 2021

bors commented Feb 14, 2021

rust-timer commented Feb 14, 2021

rust-timer commented Feb 14, 2021

m-ou-se commented Feb 14, 2021

tesuji commented Feb 24, 2021

matthewjasper commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

bors commented Feb 27, 2021

tesuji commented Feb 28, 2021 • edited Loading

tesuji commented Feb 8, 2021 •

edited

Loading

the8472 Feb 8, 2021 •

edited

Loading

tesuji Feb 8, 2021 •

edited

Loading

tesuji Feb 8, 2021 •

edited

Loading

tesuji Feb 9, 2021 •

edited

Loading

tesuji Feb 8, 2021 •

edited

Loading

tesuji commented Feb 28, 2021 •

edited

Loading