-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
add thread_name_fn method to runtime::Builder, close #1907. #1921
Conversation
loom::sync::Arc(not support ?Sized) looks is different with std::sync::Arc? But use std::fmt;
#[cfg(not(loom))]
use std::sync::Arc;
#[cfg(loom)]
use crate::loom::sync::Arc;
|
that's really useful feature for profiling apps using |
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.
Thanks! I think a lot of people are going to find this useful!
I left a few relatively minor suggestions. Overall, the change looks good.
tokio/src/runtime/builder.rs
Outdated
@@ -65,6 +67,8 @@ pub struct Builder { | |||
pub(super) before_stop: Option<Callback>, | |||
} | |||
|
|||
pub(crate) type ThreadNameFn = Arc<Box<dyn Fn() -> String + Send + Sync + 'static>>; |
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.
It's a shame that we have to double-box this to make it work with loom
...but, this isn't really a hot path, so it's probably fine for now. It would be good to open an issue against loom
about allowing ?Sized
Arc
type parameters, though...
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.
Alternatively, I don't think it's super important for the thread name function to participate in loom
tests...I think it would be fine to just always use std::sync::Arc
here.
Thanks for your review, I also want to use std::sync::Arc directly. Do I need to squash the commits to one? |
06b19a2
to
5416579
Compare
Code already fomat, but format check failed. What version of the toolchain is used by ci, I have no problem locally. D/e/tokio ╍ (threadname) git log --pretty=one |head -n 9 (%1)
5416579e53fad30635559ced267308e759130f9a ThreadNameFn always use std::sync::Arc
2213fcecf7adbcb28f3ad94c5ee3c032eaad4747 Apply suggestions from code review
ba2c9dc131e05b335772fb666b4b7b7fcbb1138a fix loom test about Arc and features
147760584319d623140d96ddcb107a90351a656d add thread_name_fn method to runtime::Builder, close #1907.
1eb6131321b5bb8e0ccdb7b9433f6f0ef47821f2 codec: Add `Framed::with_capacity` (#2215)
17a6f6fabf4e4d84e5d2807f49f22fc2bd1b359c macros: fix select! documentation formatting (#2283)
ecc23c084a64188e272e2c1e9e370189358a088b chore: prepare v0.2.13 release (#2282)
937ae11f37b0acf6cff9f5808b050f5b8c6ec976 macros: fix unresolved import in pin! (#2281)
f0d18653a6b269d88a61ab37a00abd586b2de173 runtime: add threaded_scheduler to examples (#2277)
D/e/tokio ╍ (threadname) rustc -V (%1)
rustc 1.41.1 (f3e1a954d 2020-02-24)
D/e/tokio ╍ (threadname) (%1)
D/e/tokio ╍ (threadname) cargo fmt -- --check (%1)
D/e/tokio ╍ (threadname) echo $status (%1) (562ms)
0
D/e/tokio ╍ (threadname) (%1)
D/e/tokio ╍ (threadname) |
The
|
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 this looks good!
Sorry. This got a bit lost. Can you update the merge conflict? |
Please deal with it as soon as possible, thanks @Darksonn . |
Closes #1907.