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

Replace all uses of log::log_enabled with Debug printers #74876

Merged
merged 3 commits into from
Jul 30, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 28, 2020

cc @RalfJung this touches a bunch of logging in the miri engine. There are some visual changes, mainly that in several cases we stop prepending lines with the module path and just have a newline.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 28, 2020

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned eddyb Jul 28, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 28, 2020

cc @hawkw

Copy link
Contributor

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

nifty!

}
}

struct DebugStats<'tcx>(TyCtxt<'tcx>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for encapsulating the type inside debug_stats.

@RalfJung
Copy link
Member

There are some visual changes, mainly that in several cases we stop prepending lines with the module path and just have a newline.

What does this mean, do you have an example?
It's probably okay though.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 29, 2020

Basically instead of

path::to::module: foo bar boo
path::to::module: mep mep mep

we now get

path::to::module foo bar foo
mep mep mep

in cases where we had lots of trace! statements right after each other which are now bundled as a single formattign within one trace! statement

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 29, 2020

📌 Commit b81d164 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2020
@bors
Copy link
Contributor

bors commented Jul 30, 2020

⌛ Testing commit b81d164 with merge 1799d31...

@bors
Copy link
Contributor

bors commented Jul 30, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: RalfJung
Pushing 1799d31 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2020
@bors bors merged commit 1799d31 into rust-lang:master Jul 30, 2020
Comment on lines +650 to +651
#[doc(hidden)]
pub struct RenderAllocation<'a, 'tcx, Tag, Extra> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the doc hidden?
Also this might be pub(crate) if it suit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may get called from miri. I hid it from docs because the function is supposed to be the entry point for this and the only reason this is public is because impl Debug didn't work. It can work though, and I think we should just figure out how to make it work, but it'll require some lifetime experiments with the InterpCx

@oli-obk oli-obk deleted the lumberjack_disable branch July 30, 2020 07:19
@@ -56,7 +56,7 @@ impl<'mir, 'tcx> InterpCx<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>> {
self.copy_op(place.into(), dest)?;

self.return_to_block(ret.map(|r| r.1))?;
self.dump_place(*dest);
trace!("{:?}", self.dump_place(*dest));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, why are these Debug impls? They are definitely made for human consumption, so sounds more like a case of Display to me TBH... (but changing it now is probably not worth it because Miri also needs adjusting).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, the choice was fairly arbitrary

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Aug 3, 2020

This was a slight performance regression on the ctfe stress test.

I suspect we're optimizing more poorly or something like that?

Edit: it's also possible that this was just noise, unclear -- but seems unlikely.

@mati865
Copy link
Contributor

mati865 commented Aug 3, 2020

Given the changes in this PR it's possible const_eval_raw got slightly slower, this also manifests in other benchmarks but the difference could be within error margin. CTFE bench is extreme case though, normal crates won't notice it.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 4, 2020

hmm... maybe some targetted placement of inline attributes can get us the perf back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.