Allow self-profiler to only record potentially costly arguments when argument recording is turned on #95689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
|
Some changes occured to rustc_codegen_gcc cc @antoyo |
rust-highfive
added
the
S-waiting-on-review
|
I've extracted the |
2 tasks
wesleywiser
reviewed
Apr 7, 2022
This allows profiling costly arguments to be recorded only when `-Zself-profile-events=args` is on: using a closure that takes an `EventArgRecorder` and call its `record_arg` or `record_args` methods.
and so that it doesn't allocate unless event argument recording is turned on
wesleywiser
approved these changes
Apr 14, 2022
|
Since this changes the self-profiler, I'm marking this as rollup=never to avoid influencing perf results on other PRs. @bors r+ rollup=never |
|
📌 Commit 1906b7e has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 1906b7e with merge d0d7abca51138e91c1ab1cf1a2d6475d05d5b57a... |
|
@bors retry yield |
|
⌛ Testing commit 1906b7e with merge f943e04a586eb976ccfbd720e88ad8179ca814ab... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
|
(was already retried, just that rust log analyzer got delayed while posting it) |
|
Yes I noticed this afterwards, a 4-hour delay to post results is … a bit slow. |
|
⌛ Testing commit 1906b7e with merge afde28b1c3c27b86f8ef6b89c23c37489fa605c1... |
|
@bors retry yield to rollup |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (febce1f): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 22, 2022
self-profiler: record spans for proc-macro expansions This PR is a follow-up to rust-lang#95473, using the arg recorder feature from rust-lang#95689: - it adds support code to easily record spans in the event's arguments, when using `generic_activity_with_arg_recorder`. - uses that to record the spans where proc-macro expansions happen in addition to their name. As for the other 2 PRs, the goal here is to provide visibility into proc-macro expansion performance, so that users can diagnose which uses of proc-macros in their code could be causing compile time issues. Some areas where I'd love feedback: - [x] the API and names: the `SpannedEventArgRecorder` trait and its method, much like rust-lang#95689 had the same question about the `EventArgRecorder` naming - [x] we don't currently have a way to record the names of the event arguments, so should `record_arg_spanned` record the span as "location: {}" or similar ?
antoyo
pushed a commit
to antoyo/rust
that referenced
this pull request
Jun 7, 2022
Allow self-profiler to only record potentially costly arguments when argument recording is turned on As discussed [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Identifying.20proc-macro.20slowdowns/near/277304909) with `@wesleywiser,` I'd like to record proc-macro expansions in the self-profiler, with some detailed data (per-expansion spans for example, to follow rust-lang#95473). At the same time, I'd also like to avoid doing expensive things when tracking a generic activity's arguments, if they were not specifically opted into the event filter mask, to allow the self-profiler to be used in hotter contexts. This PR tries to offer: - a way to ensure a closure to record arguments will only be called in that situation, so that potentially costly arguments can still be recorded when needed. With the additional requirement that, if possible, it would offer a way to record non-owned data without adding many `generic_activity_with_arg_{...}`-style methods. This lead to the `generic_activity_with_arg_recorder` single entry-point, and the closure parameter would offer the new methods, able to be executed in a context where costly argument could be created without disturbing the profiled piece of code. - some facilities/patterns allowing to record more rustc specific data in this situation, without making `rustc_data_structures` where the self-profiler is defined, depend on other rustc crates (causing circular dependencies): in particular, spans. They are quite tricky to turn into strings (if the default `Debug` impl output does not match the context one needs them for), and since I'd also like to avoid the allocation there when arg recording is turned off today, that has turned into another flexibility requirement for the API in this PR (separating the span-specific recording into an extension trait). **edit**: I've removed this from the PR so that it's easier to review, and opened rust-lang#95739. - allow for extensibility in the future: other ways to record arguments, or additional data attached to them could be added in the future (e.g. recording the argument's name as well as its data). Some areas where I'd love feedback: - the API and names: the `EventArgRecorder` and its method for example. As well as the verbosity that comes from the increased flexibility. - if I should convert the existing `generic_activity_with_arg{s}` to just forward to `generic_activity_with_arg_recorder` + `recorder.record_arg` (or remove them altogether ? Probably not): I've used the new API in the simple case I could find of allocating for an arg that may not be recorded, and the rest don't seem costly. - [x] whether this API should panic if no arguments were recorded by the user-provided closure (like this PR currently does: it seems like an error to use an API dedicated to record arguments but not call the methods to then do so) or if this should just record a generic activity without arguments ? - whether the `record_arg` function should be `#[inline(always)]`, like the `generic_activity_*` functions ? As mentioned, r? `@wesleywiser` following our recent discussion.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed on zulip with @wesleywiser, I'd like to record proc-macro expansions in the self-profiler, with some detailed data (per-expansion spans for example, to follow #95473).
At the same time, I'd also like to avoid doing expensive things when tracking a generic activity's arguments, if they were not specifically opted into the event filter mask, to allow the self-profiler to be used in hotter contexts.
This PR tries to offer:
generic_activity_with_arg_{...}-style methods. This lead to thegeneric_activity_with_arg_recordersingle entry-point, and the closure parameter would offer the new methods, able to be executed in a context where costly argument could be created without disturbing the profiled piece of code.rustc_data_structureswhere the self-profiler is defined, depend on other rustc crates (causing circular dependencies): in particular, spans. They are quite tricky to turn into strings (if the defaultDebugimpl output does not match the context one needs them for), and since I'd also like to avoid the allocation there when arg recording is turned off today, that has turned into another flexibility requirement for the API in this PR (separating the span-specific recording into an extension trait). edit: I've removed this from the PR so that it's easier to review, and opened self-profiler: record spans for proc-macro expansions #95739.Some areas where I'd love feedback:
EventArgRecorderand its method for example. As well as the verbosity that comes from the increased flexibility.generic_activity_with_arg{s}to just forward togeneric_activity_with_arg_recorder+recorder.record_arg(or remove them altogether ? Probably not): I've used the new API in the simple case I could find of allocating for an arg that may not be recorded, and the rest don't seem costly.record_argfunction should be#[inline(always)], like thegeneric_activity_*functions ?As mentioned, r? @wesleywiser following our recent discussion.