Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
EMA based statistically adaptive thread pool design #108
EMA based statistically adaptive thread pool design #108
Changes from all commits
0e6b30f
4a1afcd
7f237b7
55664f1
ecfce3a
1de75ee
07eac68
08c8e04
2dca64d
d9785cc
9135ca0
4606893
73ccc67
186d55f
e765cee
2340250
572b80f
6664f4b
e934a5e
405b081
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
in general, humans are bad at guessing whether something should be inlined. Usually in rust it's better to leave these attributes out unless some code is going to be executed in a super hot path and you've done work to ensure the improvement is measurable
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.
This comment is not contributing to the overall discussion + review. I’ve found it quite a bit
out of context
and also from thebook
. I've made the benchmarks already with and without and decided to leave it inlined. You can see below my last run again after your comment (run shouldn't be needed because of how compiler inlines and unrolls):Mean of 10 consecutive runs of
blocking
benchmark:That is also better for users of this library indirectly.
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'm a bit skeptical that change is related to the inline here because this is a function that gets called 5 times per second, and that measurement skew is ~1ms. For a workload that lasts ~40ms, it's not clear to me how measuring things that happen on the order of once every 200ms is relevant.
I'm asking you to simplify your proposed code because we are going to have to keep it working over time, and it's important not to add bits that add complexity for humans without clear benefits.
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.
In another context there was some discussion around adding tracing (using log's
trace!
macro) to async-std. This functions seems like a good place for that, too.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.
log::trace
or the newtracing
library? wondering about how much overhead each would addThere 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 just know that the idea was thrown around, and it was probably an opt-in feature, too. @stjepang or @yoshuawuyts would know more :)
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.
log
now has kv support, which we're already using in other places too.tracing
can pick these up too, so usinglog
is probably the best choice here overall (: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 should get the
kv
support intracing-log
soon-ish? However I don't believe thatlog
supports thespan
-like macros, so async-std might need to have its own, internalspan
-like macro and dispatch conditionally to async-log or tracing.I haven't considered how the differences between async-log's
span
andtracing
's span can be bridged, but I think it's feasible iftracing'
sspan!().enter()
is used to mimic async-log's nestedspan
block.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.
@davidbarsky we could use
async-log::span
for this, probably. Though there's still a few open issues on the repo, the interface wouldn't change.From convos with Eliza during RustConf there's def a desire to bridge between the two approaches, and we mostly need to figure out how to go about it. async-rs/async-log#7 is what I've currently got (repo incoming soon), but need to check how well that works (:
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.
@yoshuawuyts Yep! I think we're on the same page about wanting to bridge the two libraries.
I'm spitballing one mechanism on bridging that gap, which consists of using mutually exclusive feature flags in a
build.rs
to dispatch to eitherasync-log
ortracing
. This would entail using anasync-std
-specificspan
macro until the gaps—if any exist—betweenasync-std
'sspan
andtracing
'sspan
are closed. I'm personally excited to dig into rust-lang/log#353!I'm sorry if what I communicated wasn't clear!