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

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

Merged
merged 1 commit into from
Jan 5, 2020

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 1, 2020

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2020
@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 2, 2020

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

@bjorn3
Copy link
Member

bjorn3 commented Jan 2, 2020

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?

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Looks great!

@wesleywiser
Copy link
Member

r=me when rebased

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 2, 2020

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

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 2, 2020

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jan 2, 2020

📌 Commit 19e37c4 has been approved by wesleywiser

@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 Jan 2, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 2, 2020
Use self profile infrastructure for -Z time and -Z time-passes

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
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 2, 2020
Use self profile infrastructure for -Z time and -Z time-passes

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 added a commit that referenced this pull request Jan 2, 2020
Rollup of 5 pull requests

Successful merges:

 - #67636 (allow rustfmt key in [build] section)
 - #67736 (Less-than is asymmetric, not antisymmetric)
 - #67762 (Add missing links for insecure_time)
 - #67777 (Use self profile infrastructure for -Z time and -Z time-passes)
 - #67807 (Use drop instead of the toilet closure `|_| ()`)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Jan 4, 2020

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 4, 2020
@Zoxc Zoxc mentioned this pull request Jan 4, 2020
@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 4, 2020

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jan 4, 2020

📌 Commit 1fb4b076368d4ab21550955e2b429874097d3967 has been approved by wesleywiser

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2020
@bors
Copy link
Contributor

bors commented Jan 5, 2020

🔒 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 bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 5, 2020
@bors
Copy link
Contributor

bors commented Jan 5, 2020

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

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 5, 2020

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jan 5, 2020

📌 Commit 5a485ce has been approved by wesleywiser

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 5, 2020
@bors
Copy link
Contributor

bors commented Jan 5, 2020

⌛ Testing commit 5a485ce with merge 7785834...

bors added a commit that referenced this pull request Jan 5, 2020
Use self profile infrastructure for -Z time and -Z time-passes

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
Copy link
Contributor

bors commented Jan 5, 2020

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

@michaelwoerister
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

Zoxc commented Jan 6, 2020

@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
Copy link
Contributor Author

Zoxc commented Jan 6, 2020

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
Copy link
Member

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
Copy link
Member

pnkfelix commented Jan 8, 2020

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
Copy link
Member

pnkfelix commented Jan 8, 2020

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

Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
Compile some CGUs in parallel at the start of codegen

This brings the compilation time for `syntex_syntax` from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling `n` CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for `syntex_syntax`.

Based on rust-lang#67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
Compile some CGUs in parallel at the start of codegen

This brings the compilation time for `syntex_syntax` from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling `n` CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for `syntex_syntax`.

Based on rust-lang#67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
Compile some CGUs in parallel at the start of codegen

This brings the compilation time for `syntex_syntax` from 11.542s to 10.453s with 6 threads in non-incremental debug mode. Just compiling `n` CGUs in parallel at the beginning of codegen seems sufficient to get rid of the staircase effect, at least for `syntex_syntax`.

Based on rust-lang#67777.

r? @michaelwoerister
cc @alexcrichton @Mark-Simulacrum
@nnethercote
Copy link
Contributor

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.

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.

8 participants