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

self-profiling: Add events for everything except trait selection. #65208

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@michaelwoerister
Copy link
Contributor

michaelwoerister commented Oct 8, 2019

This is the followup PR to #64840.

Trait selection events are still missing (at least those not covered by regular queries).

r? @wesleywiser (or @Mark-Simulacrum if @wesleywiser is not available at the moment)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Oct 8, 2019

Just to make sure -- this won't run into the problem we hit earlier with non-contained events, right? I think the answer is no, because all added events are scope-specific. But I'd like to ask to make sure.

r=me modulo that concern

@@ -295,6 +299,9 @@ pub fn collect_crate_mono_items(
let mut inlining_map = MTLock::new(InliningMap::new());

{
let _prof_timer = tcx.prof
.generic_activity("monomorphization_collector_graph_walk");

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Oct 8, 2019

Member

I could be wrong -- but I think we already have events with spaces in them, in which case this can probably be nicer if we just use spaces directly (both in this event and generally).

This comment has been minimized.

Copy link
@wesleywiser

wesleywiser Oct 8, 2019

Member

Yeah, we do. Unless there's a strong reason to use underscores (for instance, if this activity was just the name of the enclosing function), I'd recommend using spaces as well.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 9, 2019

Author Contributor

I'm not sure. In my opinion it's more like: unless there is a strong reason for doing otherwise, it's better to keep any kind of identifiers as simple as possible (e.g. allowing only things that would also make valid Rust identifiers). Otherwise one might end up with headaches down the road, like having to weirdly escape things in commandline strings, or not knowing if trailing whitespace is significant or not. The identifiers emitted here will go through various processing tools (many of which we don't now about yet). I'd rather keep things simple.

This comment has been minimized.

Copy link
@wesleywiser

wesleywiser Oct 9, 2019

Member

Seems reasonable to me.

// In the first step, we place all regular monomorphizations into their
// respective 'home' codegen unit. Regular monomorphizations are all
// functions and statics defined in the local crate.
let mut initial_partitioning = place_root_mono_items(tcx, mono_items);
let mut initial_partitioning = {
let _prof_timer = tcx.prof.generic_activity("cgu_partitioning_place_roots");

This comment has been minimized.

Copy link
@wesleywiser

wesleywiser Oct 8, 2019

Member

cgu partitioning: place roots perhaps?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

☔️ The latest upstream changes (presumably #65223) made this pull request unmergeable. Please resolve the merge conflicts.

@wesleywiser

This comment has been minimized.

Copy link
Member

wesleywiser commented Oct 9, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

📌 Commit 38e0b5c has been approved by wesleywiser

@wesleywiser

This comment has been minimized.

Copy link
Member

wesleywiser commented Oct 9, 2019

Woops... missed the merge conflict message.

@bors r-

r=me with merge resolved

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Oct 9, 2019

Just to make sure -- this won't run into the problem we hit earlier with non-contained events, right? I think the answer is no, because all added events are scope-specific. But I'd like to ask to make sure.

Yes, the RAII-based API should make sure that something like that can't happen.

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:sp-events-review-2 branch from 38e0b5c to ceb1a9c Oct 9, 2019
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Oct 9, 2019

@bors r=wesleywiser

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

📌 Commit ceb1a9c has been approved by wesleywiser

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Oct 9, 2019

Thanks for the review!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

⌛️ Testing commit ceb1a9c with merge 321ccbe...

bors added a commit that referenced this pull request Oct 9, 2019
…iser

self-profiling: Add events for everything except trait selection.

This is the followup PR to #64840.

Trait selection events are still missing (at least those not covered by regular queries).

r? @wesleywiser (or @Mark-Simulacrum if @wesleywiser is not available at the moment)
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 9, 2019

☀️ Test successful - checks-azure
Approved by: wesleywiser
Pushing 321ccbe to master...

@bors bors added the merged-by-bors label Oct 9, 2019
@bors bors merged commit ceb1a9c into rust-lang:master Oct 9, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191009.17 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.