Try to avoid locals when cloning into Box/Rc/Arc #80824

cuviper · 2021-01-08T21:20:46Z

For generic T: Clone, we can allocate an uninitialized box beforehand,
which gives the optimizer a chance to create the clone directly in the
heap. For T: Copy, we can go further and do a simple memory copy,
regardless of optimization level.

The same applies to Rc/Arc::make_mut when they must clone the data.

rust-highfive · 2021-01-08T21:20:49Z

r? @kennytm

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

library/alloc/src/boxed.rs

For generic `T: Clone`, we can allocate an uninitialized box beforehand, which gives the optimizer a chance to create the clone directly in the heap. For `T: Copy`, we can go further and do a simple memory copy, regardless of optimization level.

As we did with `Box`, we can allocate an uninitialized `Rc` or `Arc` beforehand, giving the optimizer a chance to skip the local value for regular clones, or avoid any local altogether for `T: Copy`.

When only other `Weak` references remain, we can directly move the data into the new unique allocation as a plain memory copy.

cuviper · 2021-01-12T02:09:25Z

In addition to the WriteCloneIntoRaw suggestion, I also added direct copies for Rc/Arc when only other Weak references remain. This is regardless of T: Copy, because it's really a move in this case. It's still in the spirit of the rest of this PR though, as it avoids any local copy of the data.

cuviper · 2021-01-12T02:12:59Z

library/alloc/src/sync.rs

-                // there is no data race in the implicit write caused by `read`
-                // here (due to zeroing) because data is no longer accessed by
-                // other threads (due to there being no more strong refs at this
-                // point).


I have to admit, I don't really understand what this was saying about zeroing. Maybe that's outdated from the days of drop flags and such? The comment came from commit d77c4b0.

rust-log-analyzer · 2021-01-12T02:15:05Z

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 rand v0.7.3
    Checking std v0.0.0 (/checkout/library/std)
    Checking alloc v0.0.0 (/checkout/library/alloc)
    Checking core v0.0.0 (/checkout/library/core)
error[E0432]: unresolved import `crate::boxed::WriteCloneIntoRaw`
    |
    |
268 | use crate::boxed::WriteCloneIntoRaw;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `WriteCloneIntoRaw` in `boxed`

error[E0432]: unresolved import `crate::boxed::WriteCloneIntoRaw`
   |
   |
27 | use crate::boxed::{Box, WriteCloneIntoRaw};
   |                         ^^^^^^^^^^^^^^^^^ no `WriteCloneIntoRaw` in `boxed`

error[E0599]: no method named `write_clone_into_raw` found for type parameter `T` in the current scope
     |
     |
1046 |                 (**this).write_clone_into_raw(data.as_mut_ptr());
     |                          ^^^^^^^^^^^^^^^^^^^^ method not found in `T`

error[E0599]: no method named `write_clone_into_raw` found for type parameter `T` in the current scope
     |
     |
1377 |                 (**this).write_clone_into_raw(data.as_mut_ptr());
     |                          ^^^^^^^^^^^^^^^^^^^^ method not found in `T`
error: aborting due to 4 previous errors

Some errors have detailed explanations: E0432, E0599.
For more information about an error, try `rustc --explain E0432`.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `alloc`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--all-targets" "-p" "test" "-p" "term" "-p" "std" "-p" "alloc" "-p" "unwind" "-p" "proc_macro" "-p" "panic_unwind" "-p" "panic_abort" "-p" "core" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:50

cuviper · 2021-01-12T02:25:32Z

The failure is because I put the shared specialization trait in alloc::boxed, but that whole module is #[cfg(not(test))]:

rust/library/alloc/src/lib.rs

Lines 160 to 168 in fe531d5

    
           // Need to conditionally define the mod from `boxed.rs` to avoid 
        
           // duplicating the lang-items when building in test cfg; but also need 
        
           // to allow code to have `use boxed::Box;` declarations. 
        
           #[cfg(not(test))] 
        
           pub mod boxed; 
        
           #[cfg(test)] 
        
           mod boxed { 
        
               pub use std::boxed::Box; 
        
           }

There's not a great place to put this pub(crate) trait in alloc otherwise -- suggestions? Maybe a tiny new mod ptr?

kennytm · 2021-01-12T07:10:35Z

Sounds good.

or maybe inside collections next to trait SpecExtend.

cuviper · 2021-01-12T20:27:35Z

The point is dealing with pre-allocated memory, so I decided alloc::alloc makes some sense. Since it's only pub(crate), I guess we don't need to worry too much about where it goes, and it can always be moved again later.

kennytm · 2021-01-13T03:59:35Z

@bors r+

(if we have got rust-lang/rfcs#2884 this PR wouldn't need those MaybeUninit unsafety nor the specialization trick to gain performance. ah well.)

bors · 2021-01-13T03:59:37Z

📌 Commit 1f1a3b4 has been approved by kennytm

bors · 2021-01-13T11:11:40Z

⌛ Testing commit 1f1a3b4 with merge 116d1a7...

bors · 2021-01-13T13:56:07Z

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 116d1a7 to master...

rust-highfive assigned kennytm Jan 8, 2021

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

kennytm reviewed Jan 10, 2021

View reviewed changes

library/alloc/src/boxed.rs Outdated Show resolved Hide resolved

cuviper added 3 commits January 11, 2021 17:43

Specialize Rc/Arc::make_mut clones to try to avoid locals

d85df44

As we did with `Box`, we can allocate an uninitialized `Rc` or `Arc` beforehand, giving the optimizer a chance to skip the local value for regular clones, or avoid any local altogether for `T: Copy`.

Move directly when Rc/Arc::make_mut splits from Weak

f89f30f

When only other `Weak` references remain, we can directly move the data into the new unique allocation as a plain memory copy.

cuviper force-pushed the heap-clones branch from 60771d4 to f89f30f Compare January 12, 2021 02:06

cuviper commented Jan 12, 2021

View reviewed changes

move WriteCloneIntoRaw into alloc::alloc

1f1a3b4

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 Jan 13, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2021

bors merged commit 116d1a7 into rust-lang:master Jan 13, 2021

rustbot added this to the 1.51.0 milestone Jan 13, 2021

cuviper deleted the heap-clones branch September 21, 2021 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Try to avoid locals when cloning into Box/Rc/Arc #80824

Try to avoid locals when cloning into Box/Rc/Arc #80824

Uh oh!

cuviper commented Jan 8, 2021

Uh oh!

rust-highfive commented Jan 8, 2021

Uh oh!

Uh oh!

cuviper commented Jan 12, 2021

Uh oh!

cuviper Jan 12, 2021

Uh oh!

rust-log-analyzer commented Jan 12, 2021

Uh oh!

cuviper commented Jan 12, 2021 •

edited

Loading

Uh oh!

kennytm commented Jan 12, 2021

Uh oh!

cuviper commented Jan 12, 2021

Uh oh!

kennytm commented Jan 13, 2021

Uh oh!

bors commented Jan 13, 2021

Uh oh!

bors commented Jan 13, 2021

Uh oh!

bors commented Jan 13, 2021

Uh oh!

Uh oh!

Try to avoid locals when cloning into Box/Rc/Arc #80824

Try to avoid locals when cloning into Box/Rc/Arc #80824

Uh oh!

Conversation

cuviper commented Jan 8, 2021

Uh oh!

rust-highfive commented Jan 8, 2021

Uh oh!

Uh oh!

cuviper commented Jan 12, 2021

Uh oh!

cuviper Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

rust-log-analyzer commented Jan 12, 2021

Uh oh!

cuviper commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennytm commented Jan 12, 2021

Uh oh!

cuviper commented Jan 12, 2021

Uh oh!

kennytm commented Jan 13, 2021

Uh oh!

bors commented Jan 13, 2021

Uh oh!

bors commented Jan 13, 2021

Uh oh!

bors commented Jan 13, 2021

Uh oh!

Uh oh!

cuviper commented Jan 12, 2021 •

edited

Loading