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

Change -Z time event naming scheme and make them generic activities #67988

Merged
merged 2 commits into from
Jan 9, 2020

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 7, 2020

I made the -Z time-passes only events (which encodes argument in the event id) use a extra_verbose_generic_activity function which does not emit self-profiling events.

r? @michaelwoerister
cc @wesleywiser

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

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-07T22:30:31.2451187Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-07T22:30:31.2551204Z ##[command]git config gc.auto 0
2020-01-07T22:30:31.2616492Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-07T22:30:31.2669825Z ##[command]git config --get-all http.proxy
2020-01-07T22:30:31.2806839Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67988/merge:refs/remotes/pull/67988/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@michaelwoerister
Copy link
Member

Thanks for opening this PR, @Zoxc. I'll take a look shortly.

@michaelwoerister
Copy link
Member

As a side note (haven't reviewed this in detail yet): Once #67397 lands, there is a clean way of attaching arguments to events without tripping up the tooling.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Thanks for PR, @Zoxc. I think this should fix perf.rlo and provides a clean basis for rebasing #67397 onto.

r=me with the two doc comment additions.

}
}

/// Start profiling a sparse pass. Profiling continues until the
/// Start profiling a verbose generic activity. Profiling continues until the
/// VerboseTimingGuard returned from this call is dropped.
Copy link
Member

Choose a reason for hiding this comment

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

Please add the following to the doc comment:

In addition to recording a measureme event, "verbose" generic activities also print a timing entry to the console (if the compiler is invoked with -Ztime or -Ztime-passes).

)
}

/// Start profiling a generic pass. Profiling continues until the
/// Start profiling a extra verbose generic activity. Profiling continues until the
/// VerboseTimingGuard returned from this call is dropped.
Copy link
Member

Choose a reason for hiding this comment

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

Please add the following to the doc comment:

Note that "extra verbose" generic activities do not record a measureme event. They only print a timing entry to the console (if -Ztime-passes is specified).

(it is -Z time-passes, right?)

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 8, 2020

@bors r=michaelwoerister p=1

@bors
Copy link
Contributor

bors commented Jan 8, 2020

📌 Commit dee4077212f32b86272e9d2a0a5265e06b181269 has been approved by michaelwoerister

@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 8, 2020
@Zoxc Zoxc mentioned this pull request Jan 9, 2020
@bors
Copy link
Contributor

bors commented Jan 9, 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 prof-fix (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 prof-fix --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_lint/late.rs
Auto-merging src/librustc_interface/passes.rs
CONFLICT (content): Merge conflict in src/librustc_interface/passes.rs
Auto-merging src/librustc_driver/lib.rs
Auto-merging src/librustc_codegen_ssa/back/write.rs
Auto-merging src/librustc/hir/map/mod.rs
Automatic merge failed; fix conflicts and then commit the result.

@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 9, 2020
@bors
Copy link
Contributor

bors commented Jan 9, 2020

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

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 9, 2020

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Jan 9, 2020

📌 Commit 7db4b7e has been approved by michaelwoerister

@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 9, 2020
@bors
Copy link
Contributor

bors commented Jan 9, 2020

⌛ Testing commit 7db4b7e with merge c404ce6...

bors added a commit that referenced this pull request Jan 9, 2020
Change -Z time event naming scheme and make them generic activities

I made the `-Z time-passes` only events (which encodes argument in the event id) use a `extra_verbose_generic_activity` function which does not emit self-profiling events.

r? @michaelwoerister
cc @wesleywiser
Centril added a commit to Centril/rust that referenced this pull request Jan 9, 2020
Change -Z time event naming scheme and make them generic activities

I made the `-Z time-passes` only events (which encodes argument in the event id) use a `extra_verbose_generic_activity` function which does not emit self-profiling events.

r? @michaelwoerister
cc @wesleywiser
@Centril
Copy link
Contributor

Centril commented Jan 9, 2020

@bors retry rolled up.

bors added a commit that referenced this pull request Jan 9, 2020
Rollup of 6 pull requests

Successful merges:

 - #67258 (Introduce `X..`, `..X`, and `..=X` range patterns)
 - #67358 (Add HashSet::get_or_insert_owned)
 - #67935 (Relax the Sized bounds on Pin::map_unchecked(_mut))
 - #67975 (Export public scalar statics in wasm)
 - #67988 (Change -Z time event naming scheme and make them generic activities)
 - #68006 (Recognise riscv64 in compiletest)

Failed merges:

 - #67806 (Extract `rustc_ast_passes`, move gating, & refactor linting)

r? @ghost
@bors
Copy link
Contributor

bors commented Jan 9, 2020

☀️ Test successful - checks-azure
Approved by: michaelwoerister
Pushing c404ce6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 9, 2020
@bors bors merged commit 7db4b7e into rust-lang:master Jan 9, 2020
@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

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.

5 participants