-
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
bump tracing version #98626
bump tracing version #98626
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
@@ -291,7 +291,7 @@ pub enum InternKind { | |||
/// tracks where in the value we are and thus can show much better error messages. | |||
/// Any errors here would anyway be turned into `const_err` lints, whereas validation failures | |||
/// are hard errors. | |||
#[tracing::instrument(level = "debug", skip(ecx))] | |||
#[instrument(level = "debug", skip(ecx))] |
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'd prefer if the tracing in the interpreter used the trace
level. Currently we make debug
already so drowned in noise that we basically lose the entire point of even having that distinction.
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 is just for keeping the previous thing. I would like for the default to be trace
and then opt in a few to debug
@@ -531,7 +531,7 @@ macro_rules! define_queries_struct { | |||
|
|||
$($(#[$attr])* | |||
#[inline(always)] | |||
#[tracing::instrument(level = "trace", skip(self, tcx))] | |||
#[tracing::instrument(level = "trace", skip(self, tcx), ret)] |
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.
#[tracing::instrument(level = "trace", skip(self, tcx), ret)] | |
#[instrument(level = "trace", skip(self, tcx), ret)] |
does that work?
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.
nope, the macro is invoked $somewhere else, and it's weird to have to add an import at the call site in order for the macro expansion to compile
compiler/rustc_middle/src/ty/fold.rs
Outdated
ControlFlow::Break(FoundFlags) | ||
} else { | ||
ControlFlow::CONTINUE | ||
} | ||
} | ||
|
||
#[inline] | ||
#[instrument(level = "trace")] | ||
#[instrument(skip(self), level = "trace", ret)] | ||
fn visit_const(&mut self, c: ty::Const<'tcx>) -> ControlFlow<Self::BreakTy> { | ||
let flags = FlagComputation::for_const(c); | ||
trace!(r.flags=?flags); |
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.
trace!(r.flags=?flags); | |
trace!(c.flags=?flags); |
small nits, then r=me r? @lcnr |
@bors try @rust-timer queue probably won't negatively impact perf, but better to be sure |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit d8dd65118a3e14df8ec1a9bf4e9389519f369d86 with merge 341e73079846b165ae541bfce020672a57c7ff78... |
☀️ Try build successful - checks-actions |
Queued 341e73079846b165ae541bfce020672a57c7ff78 with parent 493c960, future comparison URL. |
Finished benchmarking commit (341e73079846b165ae541bfce020672a57c7ff78): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Footnotes |
both the keccak and the cranelift regression seem to be
seems like the |
@bors try @rust-timer queue include=cranelift,keccak |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit c7f5245e6ed0f142d33ca6d7ba747ebeee3c37fd with merge 7b2a49496c07afd88c5fc948d16d2557d78a9b3a... |
☀️ Try build successful - checks-actions |
Queued 7b2a49496c07afd88c5fc948d16d2557d78a9b3a with parent a9eb9c5, future comparison URL. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit a4375cb with merge dfe07b2522d238f735a246a7434d284316bf2749... |
☀️ Try build successful - checks-actions |
Queued dfe07b2522d238f735a246a7434d284316bf2749 with parent 9fa62f2, future comparison URL. |
Finished benchmarking commit (dfe07b2522d238f735a246a7434d284316bf2749): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Footnotes |
If it's ok, I'll switch back on author for a comment on the latest perf run (PR itself is already r'ed). @rustbot author |
no clue what happened here, but maybe we should just take the perf hit until
|
while unfortunate, I am fine with that 👍 r=me after updating the PR description (or actually adding the rest of your work back in 😁) |
@bors r=lcnr |
☀️ Test successful - checks-actions |
Finished benchmarking commit (b96fa1a): 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.
Footnotes |
Bump tracing dependency to 0.1.35 to give us features like printing the return value of functions