Skip to content
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

Merged
merged 1 commit into from Aug 24, 2020

Conversation

biluohc
Copy link
Contributor

@biluohc biluohc commented Dec 7, 2019

Closes #1907.

@biluohc
Copy link
Contributor Author

biluohc commented Dec 7, 2019

loom::sync::Arc(not support ?Sized) looks is different with std::sync::Arc?

But Box<Fn> can not clone, maybe I should use Arc<Box<_>> ?

use std::fmt;
#[cfg(not(loom))]
use std::sync::Arc;
#[cfg(loom)]
use crate::loom::sync::Arc;
error[E0277]: the size for values of type `(dyn std::ops::Fn() -> std::string::String + std::marker::Send + std::marker::Sync + 'static)` cannot be known at compilation time
  --> tokio/src/runtime/builder.rs:58:5
   |
58 |     pub(super) thread_name: ThreadNameFn,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn std::ops::Fn() -> std::string::String + std::marker::Send + std::marker::Sync + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required by `loom::sync::arc::Arc`

error[E0277]: the size for values of type `(dyn std::ops::Fn() -> std::string::String + std::marker::Send + std::marker::Sync + 'static)` cannot be known at compilation time
  --> tokio/src/runtime/blocking/pool.rs:35:5
   |
35 |     thread_name: ThreadNameFn,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `(dyn std::ops::Fn() -> std::string::String + std::marker::Send + std::marker::Sync + 'static)`
   = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required by `loom::sync::arc::Arc`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.

@onsails
Copy link

onsails commented Mar 2, 2020

that's really useful feature for profiling apps using perf record and perf report. Currently there is no ability to focus on a certain thread in perf report.

Copy link
Member

@hawkw hawkw left a 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 Show resolved Hide resolved
@@ -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>>;
Copy link
Member

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...

Copy link
Member

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.

tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
@biluohc
Copy link
Contributor Author

biluohc commented Mar 3, 2020

Thanks for your review, I also want to use std::sync::Arc directly.

Do I need to squash the commits to one?

@biluohc biluohc force-pushed the threadname branch 2 times, most recently from 06b19a2 to 5416579 Compare March 3, 2020 04:08
@biluohc
Copy link
Contributor Author

biluohc commented Mar 3, 2020

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)

@Darksonn
Copy link
Contributor

Darksonn commented Mar 3, 2020

The cargo fmt command is not quite able to find all of the files, so I recommend you use this command instead:

rustfmt --check --edition 2018 $(find . -name '*.rs' -print)

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime labels Jul 25, 2020
Copy link
Contributor

@Darksonn Darksonn left a 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!

@Darksonn
Copy link
Contributor

Sorry. This got a bit lost. Can you update the merge conflict?

@biluohc
Copy link
Contributor Author

biluohc commented Aug 24, 2020

Please deal with it as soon as possible, thanks @Darksonn .

@Darksonn Darksonn merged commit 4e12299 into tokio-rs:master Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can you add a name_prefix method to 0.2 runtime ’s builder?
4 participants