Use self profile infrastructure for -Z time and -Z time-passes

[Zoxc]

There's no longer indentation for -Z time and -Z time-passes and duplicate timers between self profiling and -Z time-passes have been removed.

r? @wesleywiser

[bors]

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

[bjorn3]

There's no longer indentation for -Z time and -Z time-passes

I think the indentation is useful. Is there some way to re-introduce it?

[wesleywiser]

Looks great!

[wesleywiser]

r=me when rebased

[Zoxc]

@bjorn3 Indentation doesn't work with multi-threading though. If you want a better overview you can use -Z self-profile + crox.

[Zoxc]

@bors r=wesleywiser

[bors]

📌 Commit 19e37c4 has been approved by wesleywiser

[bors]

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

[Zoxc]

@bors r=wesleywiser

[bors]

📌 Commit 1fb4b076368d4ab21550955e2b429874097d3967 has been approved by wesleywiser

[bors]

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

1. git checkout time-refactor (switch to your branch)
2. git fetch upstream master (retrieve the latest master)
3. git rebase upstream/master -p (rebase on top of it)
4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
5. git push self time-refactor --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/librustc_session/session.rs
Auto-merging src/librustc_mir/monomorphize/partitioning.rs
CONFLICT (content): Merge conflict in src/librustc_mir/monomorphize/partitioning.rs
Auto-merging src/librustc_mir/monomorphize/collector.rs
CONFLICT (content): Merge conflict in src/librustc_mir/monomorphize/collector.rs
Auto-merging src/librustc_codegen_ssa/base.rs
CONFLICT (content): Merge conflict in src/librustc_codegen_ssa/base.rs
Auto-merging src/librustc_codegen_ssa/back/write.rs
CONFLICT (content): Merge conflict in src/librustc_codegen_ssa/back/write.rs
Auto-merging src/librustc/lib.rs
Auto-merging src/librustc/hir/map/mod.rs
CONFLICT (content): Merge conflict in src/librustc/hir/map/mod.rs
Automatic merge failed; fix conflicts and then commit the result.

[bors]

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

[Zoxc]

@bors r=wesleywiser

[bors]

📌 Commit 5a485ce has been approved by wesleywiser

[bors]

⌛ Testing commit 5a485ce with merge 7785834...

[bors]

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

[michaelwoerister]

I wished this had been coordinated with #67397, which is now broken. It also regressed the event cleanup, introducing duplicate events (e.g. "indexing HIR" vs "build_hir_map") and adds a separate line for each codegen unit again (e.g. in https://perf.rust-lang.org/detailed-query.html?commit=33640f0e03af2fb31ce380d5389d5545f24ce29a&benchmark=syn-opt&run_name=baseline%20incremental).

Self-profiling has more restrictions and is more user-facing than -Ztime-passes. Changing events can break downstream tooling. An integration of the two features has to be done carefully.

[michaelwoerister]

@Zoxc & @wesleywiser: I'm proposing to revert this PR. It breaks the ongoing self-profiling work and perf.rlo detail views. It also doesn't look like a clean integration of the two features with no clear distinction of what should be a generic activity, what a sparse pass and what a generic pass. I think time-passes and self-profiling can use the same infrastructure at some point but now is too early.

[Zoxc]

@michaelwoerister How does it break #67397? I don't see anything that would conflict.

And if our tooling can't handle more events yet we could just leave the pass events off by default?

[Zoxc]

I suggest that we:

• Rename -Z time passes to have the naming style of -Z self-profile. I'm not sure why you went with C-style function names here?
• Remove the duplicated events that I missed
• Turn off generic_pass events by default and bring back the equivalent generic_events.

I've been using self-profiling with -Z time events and crox to optimize the compiler recently and I'd like to have this available without carrying a patch around.

[michaelwoerister]

I gave my reasoning for going with names without spaces here: #65208 (comment)

New event IDs are not a problem. The tooling can handle that if parameters are handled correctly. For new event kinds things are trickier, some tooling might crash when encountering new kinds, some might ignore them and give wrong numbers.

I'm OK with the steps you suggest if we also

• replace generic_pass_event_kind and sparse_pass_event_kind with generic_activity_event_kind, and
• find a way of also getting rid of the generic-pass and sparse-pass event filters. If they provide useful information they can just be regular generic-activity events. Whether they generate console output or not can be handled independently of their event kind and event filters.

I also think it would be good get rid of the Session::time and Session::timer methods and go with sess.prof.generic_activity(...) instead (or something like sess.prof.generic_activity_verbose(...) that reads the -Ztime(-passes) settings).

[pnkfelix]

After some private discussion with @nikomatsakis and @michaelwoerister , I am inclined to revert this PR, mainly based on it regressing perf.rlo detailed view as noted by @michaelwoerister, namely the rows prefixed with "codegen passes" in perf link:

Query/Function	Time	% Total Time	Full Executions	Loading
...
codegen passes [3z2mfowzhqp1o742]	0.000	0.00%	0	0.000
collecting mono items	0.000	0.00%	0	0.000
codegen passes [3rekknkyfilt37nt]	0.000	0.00%	0	0.000
codegen passes [trthz5azyug3sfc]	0.000	0.00%	0	0.000
codegen passes [1erb9ctdzy7h21xs]	0.000	0.00%	0	0.000
...

[pnkfelix]

Update to previous comment: @michaelwoerister has just pointed out to me that the recently posted PR #67988 might contain a fix to the issue here.

I'll wait for more information on that before proceeding with a revert of PR #67777

[nnethercote]

The removal of indentation is very unfortunate. It makes the output much harder to interpret. There's not really any point having stacks of indented timers if the output doesn't indicate them.