self-profiling: Support recording query keys

[michaelwoerister]

This PR makes self-profiling able to record query keys. The implementation is not as efficient as it could be yet (all query keys except for DefIds cause string data to be duplicated) and the rendered strings could be nicer too. But the implementation is functional and introduces the basic framework for emitting per-query-invocation event data.

I tried to add proper documentation on how everything works. Let me know if more documentation is needed.

r? @wesleywiser

@Mark-Simulacrum, heads up: This updates measureme to 0.7.0 which means that summarize on perf.rlo needs to be update accordingly once this is merged.

[Mark-Simulacrum]

Can we add something to rustc (e.g., an unstable --print option) that prints out the SHA commit of the summarize that rustc needs?

Essentially I'd like to for now be able to add some code to perf.rlo which will essentially build the summarize from the repo via git checkout $SHA && cargo build --release or so, and I think it makes sense for rustc to be the source of truth rather than trying to maintain a map of rustc commits to summarize versions inside perf.

[michaelwoerister]

@Mark-Simulacrum I would prefer making the measureme tools available as a rustup component. That would be a more general solution (and something we want to do anyway). What do you think?

[Mark-Simulacrum]

That would definitely be better! I don't personally have much time to spend figuring out how to hook that up; I can help review such a patch though.

[michaelwoerister]

The one downside of any approach that relies on rustc or rustup is that we can't support older versions of rustc. I.e back-collection would not work or would need still another mechanism.

[Mark-Simulacrum]

Back collection passively works by just having an older summarize in PATH whereas the new one would be "optionally" in PATH, so that should mostly just naturally fall out :)

[wesleywiser]

Since this touches some of the query infrastructure, let's do a perf run just to make sure we don't accidentally regress performance.

@bors try
@rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit ff5a81d with merge fbe42f6...

[michaelwoerister]

Yes, good idea! The self-profiling code exercised by perf.rlo is expected to be a bit slower than with measureme 0.5 (because we are emitting more string-index data) but should still be faster than measureme 0.4 (which still used pairs of start/end events instead of single interval events).

[Mark-Simulacrum]

Hm, I suspect that perf will fail to run due to the summarize incompatibility, but we can try I guess! Certainly the try build will help a future run if I find some time to do it manually.

[michaelwoerister]

We should still get correct instruction counts and wall-times, right? Just no self-profiling data.

[Mark-Simulacrum]

I think perf today just dies if self profile fails to work (which is probably not ideal, but perf leans on fail fast today a bit too hard probably).

[bors]

☀️ Try build successful - checks-azure
Build commit: fbe42f6 (fbe42f6764c1fe6b337f3b60132b9b1a780a881f)

[rust-timer]

Queued fbe42f6 with parent c605199, future comparison URL.

[rust-timer]

Finished benchmarking try commit fbe42f6, comparison URL.

[wesleywiser]

Looks like @Mark-Simulacrum was right.

[michaelwoerister]

I think perf today just dies if self profile fails to work (which is probably not ideal, but perf leans on fail fast today a bit too hard probably).

Yeah, making that more resilient would be a nice to have.

[michaelwoerister]

I don't think I'll have time to implement the Rustup component approach at the moment (I'll be afk for the rest of the year). What's the quickest way to unblock this? Doing one last manual update of summarize on perf.rlo?

[wesleywiser]

Looking at the trace file generated while compiling cargo, I see that recording query keys takes 200ms. Since this is opt in, I think there's little risk of regressing performance, but just to be safe, let's not roll this up.

@bors r+ rollup=never

[bors]

📌 Commit ff5a81d has been approved by wesleywiser

[bors]

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

[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 query-keys-in-self-profiling (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 query-keys-in-self-profiling --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_interface/util.rs
Auto-merging src/librustc_data_structures/profiling.rs
CONFLICT (content): Merge conflict in src/librustc_data_structures/profiling.rs
Auto-merging src/librustc_codegen_ssa/base.rs
CONFLICT (content): Merge conflict in src/librustc_codegen_ssa/base.rs
Auto-merging src/librustc/ty/query/plumbing.rs
CONFLICT (content): Merge conflict in src/librustc/ty/query/plumbing.rs
Auto-merging src/librustc/ty/query/mod.rs
Auto-merging src/librustc/ty/query/config.rs
CONFLICT (content): Merge conflict in src/librustc/ty/query/config.rs
Auto-merging src/librustc/dep_graph/graph.rs
CONFLICT (content): Merge conflict in src/librustc/dep_graph/graph.rs
Auto-merging Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

[bors]

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

[michaelwoerister]

This is rebased now. I did not add support for function parameters (as might be used by e.g. extra_verbose_generic_activities) as part of the rebase. I'll do that in another PR.

[michaelwoerister]

@bors r=wesleywiser

[bors]

📌 Commit ad65e3e has been approved by wesleywiser

[bors]

⌛ Testing commit ad65e3e with merge f795e8a...

[bors]

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

[michaelwoerister]

@Mark-Simulacrum: seems like this broke perf.rlo after all. Maybe something is wrong with the fallback logic for summarize?

[Mark-Simulacrum]

Hm, we were on 0.7.0 and not 0.7.1 -- I've now changed that. Is that a possible cause? I suppose probably no. It also turns out I think I may have forgotten to actually deploy the change, which I've now done :)

[michaelwoerister]

Hm, we were on 0.7.0 and not 0.7.1 -- I've now changed that. Is that a possible cause?

No, 0.7.1 just fixes a compilation error on big endian platforms.

It also turns out I think I may have forgotten to actually deploy the change, which I've now done :)

Yes, that would explain it :)