-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Override Waker::clone_from
to avoid cloning Waker
s unnecessarily
#96979
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Could you add a tracking issue here as well? |
triage:
|
I think I did that already. |
I think that overriding I agree that it is less discoverable though. |
That is a very good point, I forgot that method existed. I think I will change this PR to implement that instead — a note in the documentation would probably be sufficient. |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Waker::update
Waker::clone_from
to avoid cloning Waker
s unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semi-related to this PR. Should Arc
get a similar impl?
@joshtriplett ping from triage, no movement for 4 months. What's the status of this PR? @SabrinaJewson FYI you may need to squash your commits |
Please rebase to remove the master-merge. r? @workingjubilee This looks plausible, but I have one question: Are we sure it's okay to elide the possible clone effect? |
834ed29
to
6cdb7df
Compare
@@ -382,6 +388,13 @@ impl Clone for Waker { | |||
waker: unsafe { (self.waker.vtable.clone)(self.waker.data) }, | |||
} | |||
} | |||
|
|||
#[inline] | |||
fn clone_from(&mut self, source: &Self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if it'd make sense to do this here or another PR, but it might be good to add docs here to aid discoverability (I believe rustdoc pops open trait impls by default when an associated item has docs? or at least it'll catch the eye when scanning the page), and also especially since the will_wake docs link here. I can't remember it off the top of my head, but I'm almost certain there's prior art in the standard library of adding docs to associated items of an impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can handle that in a followup. It might be a good idea to doc ~all the overridden clone_from
s in std as a single PR.
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@bors r=workingjubilee |
☀️ Test successful - checks-actions |
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot] This PR pulls the following changes from the upstream repository: * rust-lang/rust#117585 * rust-lang/rust#117576 * rust-lang/rust#96979 * rust-lang/rust#117191 * rust-lang/rust#117179 * rust-lang/rust#117574 * rust-lang/rust#117537 * rust-lang/rust#117608 * rust-lang/rust#117596 * rust-lang/rust#117588 * rust-lang/rust#117524 * rust-lang/rust#116017 * rust-lang/rust#117504 * rust-lang/rust#117469 * rust-lang/rust#116218 * rust-lang/rust#117589 * rust-lang/rust#117581 * rust-lang/rust#117503 * rust-lang/rust#117590 * rust-lang/rust#117583 * rust-lang/rust#117570 * rust-lang/rust#117562 * rust-lang/rust#117534 * rust-lang/rust#116894 * rust-lang/rust#110340 * rust-lang/rust#113343 * rust-lang/rust#117579 * rust-lang/rust#117094 * rust-lang/rust#117566 * rust-lang/rust#117564 * rust-lang/rust#117554 * rust-lang/rust#117550 * rust-lang/rust#117343 * rust-lang/rust#115274 * rust-lang/rust#117540 * rust-lang/rust#116412 * rust-lang/rust#115333 * rust-lang/rust#117507 * rust-lang/rust#117538 * rust-lang/rust#117533 * rust-lang/rust#117523 * rust-lang/rust#117520 * rust-lang/rust#117505 * rust-lang/rust#117434 * rust-lang/rust#117535 * rust-lang/rust#117510 * rust-lang/rust#116439 * rust-lang/rust#117508 Co-authored-by: Ben Wiederhake <BenWiederhake.GitHub@gmx.de> Co-authored-by: SabrinaJewson <sejewson@gmail.com> Co-authored-by: J-ZhengLi <lizheng135@huawei.com> Co-authored-by: koka <koka.code@gmail.com> Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com> Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com> Co-authored-by: lengyijun <sjtu5140809011@gmail.com> Co-authored-by: Zalathar <Zalathar@users.noreply.github.com> Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de> Co-authored-by: Philipp Krones <hello@philkrones.com> Co-authored-by: y21 <30553356+y21@users.noreply.github.com> Co-authored-by: bors <bors@rust-lang.org> Co-authored-by: bohan <bohan-zhang@foxmail.com>
Finished benchmarking commit (fee5518): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 637.169s -> 636.185s (-0.15%) |
Hey friends. I am trying to understand the practicality of this feature. Let's consider the following example: use std::task::{Context, Poll, Waker};
use std::pin::Pin;
use futures::task::noop_waker_ref;
#[derive(Debug, Clone)]
struct MyTask {
id: usize,
}
struct MyFuture {
waker: Waker,
task: MyTask,
}
impl MyFuture {
fn new(id: usize) -> Self {
let task = MyTask { id };
let waker = noop_waker_ref();
MyFuture { waker: waker.clone(), task }
}
}
impl futures::Future for MyFuture {
type Output = ();
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let cloned_waker = self.waker.clone();
println!("Cloned Waker address: {:?}", &cloned_waker as *const Waker);
// `clone_from` used here
// Note: `Waker::clone_from(cx.waker(), &self.waker)` returns
// ----------------- ^^^^^^^^^^ types differ in mutability
// |
// arguments to this function are incorrect
//
// = note: expected mutable reference `&mut Waker`
// found reference `&Waker`
let mut optimized_waker = cx.waker().clone_from(&&self.waker);
println!("Optimized Waker address: {:?}", std::ptr::addr_of!(optimized_waker));
Poll::Ready(())
}
}
fn main() {
// Create two MyFuture with the same task ID
let my_future1 = MyFuture::new(42);
let my_future2 = MyFuture::new(42);
let mut pinned1 = Box::pin(my_future1);
let mut pinned2 = Box::pin(my_future2);
// Create a dummy Context
let mut cx = Context::from_waker(&noop_waker_ref());
// Poll the futures
let _result1 = futures::Future::poll(pinned1.as_mut(), &mut cx);
let _result2 = futures::Future::poll(pinned2.as_mut(), &mut cx);
} This would result with the following being displayed on the terminal: Cloned Waker address: 0x7fffaf9e6538
Optimized Waker address: 0x7fffaf9e6597
Cloned Waker address: 0x7fffaf9e6538
Optimized Waker address: 0x7fffaf9e6597 As evident from the output, both 'Cloned Waker' and 'Optimized Waker' share identical addresses for each corresponding pair. This suggests that the Any additional info about this feature would be appreciated. Thanks! |
There are a couple of issues here. Firstly, you have the argument order the wrong way round: Secondly, using Thirdly, cloning the Even when one builds a waker via the
To test this feature properly, you thus have to make a custom waker that performs some sideëffect on clone and/or drop. You can see in the output of this program does not clone in the |
Thanks for the clear explanation @SabrinaJewson! That's really helpful. I tried your example on both 1.74 and 1.75, and noticed the following optimization being done in this PR: First poll:
With `clone`:
cloning
waker is Waker { data: 0x0, vtable: 0x55fc7102d140 }
With `clone_from`:
- cloning
- dropping
waker is Waker { data: 0x0, vtable: 0x55fc7102d140 }
Second poll:
With `clone`:
cloning
dropping
waker is Waker { data: 0x0, vtable: 0x55fc7102d140 }
With `clone_from`:
- cloning
- dropping
waker is Waker { data: 0x0, vtable: 0x55fc7102d140 }
dropping
dropping So, |
Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 16, in accordance with the upstream changes. * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported. * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it, resulting in an illegal instruction fault during the build. Ref. rust-lang/rust#117231 Upstream changes: Version 1.75.0 (2023-12-28) ========================== - [Stabilize `async fn` and return-position `impl Trait` in traits.] (rust-lang/rust#115822) - [Allow function pointer signatures containing `&mut T` in `const` contexts.] (rust-lang/rust#116015) - [Match `usize`/`isize` exhaustively with half-open ranges.] (rust-lang/rust#116692) - [Guarantee that `char` has the same size and alignment as `u32`.] (rust-lang/rust#116894) - [Document that the null pointer has the 0 address.] (rust-lang/rust#116988) - [Allow partially moved values in `match`.] (rust-lang/rust#103208) - [Add notes about non-compliant FP behavior on 32bit x86 targets.] (rust-lang/rust#113053) - [Stabilize ratified RISC-V target features.] (rust-lang/rust#116485) Compiler -------- - [Rework negative coherence to properly consider impls that only partly overlap.] (rust-lang/rust#112875) - [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.] (rust-lang/rust#116493) - [Consider alias bounds when computing liveness in NLL.] (rust-lang/rust#116733) - [Add the V (vector) extension to the `riscv64-linux-android` target spec.] (rust-lang/rust#116618) - [Automatically enable cross-crate inlining for small functions] (rust-lang/rust#116505) - Add several new tier 3 targets: - [`csky-unknown-linux-gnuabiv2hf`] (rust-lang/rust#117049) - [`i586-unknown-netbsd`] (rust-lang/rust#117170) - [`mipsel-unknown-netbsd`] (rust-lang/rust#117356) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.] (rust-lang/rust#96979) - [Implement `BufRead` for `VecDeque<u8>`.] (rust-lang/rust#110604) - [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.] (rust-lang/rust#110729) - [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.] (rust-lang/rust#113747) - [Implement `Default` for `ExitCode`.] (rust-lang/rust#114589) - [Guarantee representation of None in NPO] (rust-lang/rust#115333) - [Document when atomic loads are guaranteed read-only.] (rust-lang/rust#115577) - [Broaden the consequences of recursive TLS initialization.] (rust-lang/rust#116172) - [Windows: Support sub-millisecond sleep.] (rust-lang/rust#116461) - [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl] (rust-lang/rust#100806) - [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.] (rust-lang/rust#115108) Stabilized APIs --------------- - [`Atomic*::from_ptr`] (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr) - [`FileTimes`] (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html) - [`FileTimesExt`] (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html) - [`File::set_modified`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified) - [`File::set_times`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times) - [`IpAddr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical) - [`Ipv6Addr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical) - [`Option::as_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice) - [`Option::as_mut_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice) - [`pointer::byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add) - [`pointer::byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset) - [`pointer::byte_offset_from`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from) - [`pointer::byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub) - [`pointer::wrapping_byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add) - [`pointer::wrapping_byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset) - [`pointer::wrapping_byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub) These APIs are now stable in const contexts: - [`Ipv6Addr::to_ipv4_mapped`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped) - [`MaybeUninit::assume_init_read`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read) - [`MaybeUninit::zeroed`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed) - [`mem::discriminant`] (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html) - [`mem::zeroed`] (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html) Cargo ----- - [Add new packages to `[workspace.members]` automatically.] (rust-lang/cargo#12779) - [Allow version-less `Cargo.toml` manifests.] (rust-lang/cargo#12786) - [Make browser links out of HTML file paths.] (rust-lang/cargo#12889) Rustdoc ------- - [Accept less invalid Rust in rustdoc.] (rust-lang/rust#117450) - [Document lack of object safety on affected traits.] (rust-lang/rust#113241) - [Hide `#[repr(transparent)]` if it isn't part of the public ABI.] (rust-lang/rust#115439) - [Show enum discriminant if it is a C-like variant.] (rust-lang/rust#116142) Compatibility Notes ------------------- - [FreeBSD targets now require at least version 12.] (rust-lang/rust#114521) - [Formally demote tier 2 MIPS targets to tier 3.] (rust-lang/rust#115238) - [Make misalignment a hard error in `const` contexts.] (rust-lang/rust#115524) - [Fix detecting references to packed unsized fields.] (rust-lang/rust#115583) - [Remove support for compiler plugins.] (rust-lang/rust#116412)
…lnay Document overrides of `clone_from()` in core/std As mentioned in rust-lang#96979 (comment) Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source". I reused some of the wording over and over for similar impls, but I'm not sure that the wording is actually *good*. Would appreciate feedback about that. Also, now some of these seem to provide pretty specific guarantees about behavior (e.g. will reuse the exact same allocation iff the len is the same), but I was basing it off of the docs for [`Box::clone_from`](https://doc.rust-lang.org/1.75.0/std/boxed/struct.Box.html#method.clone_from-1) - I'm not sure if providing those strong guarantees is actually good or not.
Rollup merge of rust-lang#122201 - coolreader18:doc-clone_from, r=dtolnay Document overrides of `clone_from()` in core/std As mentioned in rust-lang#96979 (comment) Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source". I reused some of the wording over and over for similar impls, but I'm not sure that the wording is actually *good*. Would appreciate feedback about that. Also, now some of these seem to provide pretty specific guarantees about behavior (e.g. will reuse the exact same allocation iff the len is the same), but I was basing it off of the docs for [`Box::clone_from`](https://doc.rust-lang.org/1.75.0/std/boxed/struct.Box.html#method.clone_from-1) - I'm not sure if providing those strong guarantees is actually good or not.
Document overrides of `clone_from()` in core/std As mentioned in rust-lang/rust#96979 (comment) Specifically, when an override doesn't just forward to an inner type, document the behavior and that it's preferred over simply assigning a clone of source. Also, change instances where the second parameter is "other" to "source". I reused some of the wording over and over for similar impls, but I'm not sure that the wording is actually *good*. Would appreciate feedback about that. Also, now some of these seem to provide pretty specific guarantees about behavior (e.g. will reuse the exact same allocation iff the len is the same), but I was basing it off of the docs for [`Box::clone_from`](https://doc.rust-lang.org/1.75.0/std/boxed/struct.Box.html#method.clone_from-1) - I'm not sure if providing those strong guarantees is actually good or not.
This would be very useful for futures — I think it’s pretty much always what they want to do instead of
*waker = cx.waker().clone()
.Tracking issue: #98287
r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs