SelfProfiler API refactoring and part one of event review #64840
Conversation
|
The job 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 |
5da752a to
bb62d47
Compare
|
☔ The latest upstream changes (presumably #64864) made this pull request unmergeable. Please resolve the merge conflicts. |
|
shall the TimingGuard be moved to the measureme crate is useful for other then rustc. |
| /// Start profiling a query being blocked on a concurrent execution. | ||
| /// Profiling continues until `query_blocked_end` is called. | ||
| #[inline(always)] | ||
| pub fn query_blocked_start(&self, query_name: QueryName) { |
There was a problem hiding this comment.
Should query_blocked_start() and query_blocked_end() be updated to work with the TimerGuard api?
There was a problem hiding this comment.
I initially had it that way but these calls are only used in exactly one spot and that becomes more complicated with the TimerGuard approach.
| @@ -465,21 +465,17 @@ impl<'tcx> TyCtxt<'tcx> { | |||
|
|
|||
| let result = if let Some(result) = result { | |||
| profq_msg!(self, ProfileQueriesMsg::CacheHit); | |||
| self.sess.profiler(|p| p.record_query_hit(Q::NAME)); | |||
There was a problem hiding this comment.
I was pondering this question too. I opted to remove it because we already record the incr_cache_loading event above and strictly speaking this is not an in-memory cache hit. However, I also want to replace the query_provider event right below with something that indicates that this is not a regular query provider invocation but was triggered instead of a cache load.
Yes, that's true. There's nothing in there that couldn't also be part of measureme.
Hm, maybe you are right. Having separate start and stop events makes sure that the nesting structure of events is always correct (without relying on possibly flaky timing functions). But maybe this could be done more efficiently somehow and then let post-processing tools reconstruct the structure. I think both should be done in a subsequent PR though. Recording overhead is not much of a problem at the moment (at least with default event filters). |
4c29a36 to
28de983
Compare
28de983 to
d942622
Compare
|
Rebased and typos fixed. |
|
@bors r+ |
|
📌 Commit d942622 has been approved by |
…i-refactor, r=wesleywiser SelfProfiler API refactoring and part one of event review This PR refactors the `SelfProfiler` a little bit so that most profiling methods are RAII-based. The codegen backend code already had something similar, this refactoring pulls this functionality up into `SelfProfiler` itself, for general use. The second commit of this PR is a review and update of the existing events we are already recording. Names have been made more consistent. CGU names have been removed from event names. They will be added back in when function parameter recording is implemented. There is still some work to be done for adding new events, especially around trait resolution and the incremental system. r? @wesleywiser
…i-refactor, r=wesleywiser SelfProfiler API refactoring and part one of event review This PR refactors the `SelfProfiler` a little bit so that most profiling methods are RAII-based. The codegen backend code already had something similar, this refactoring pulls this functionality up into `SelfProfiler` itself, for general use. The second commit of this PR is a review and update of the existing events we are already recording. Names have been made more consistent. CGU names have been removed from event names. They will be added back in when function parameter recording is implemented. There is still some work to be done for adding new events, especially around trait resolution and the incremental system. r? @wesleywiser
…i-refactor, r=wesleywiser SelfProfiler API refactoring and part one of event review This PR refactors the `SelfProfiler` a little bit so that most profiling methods are RAII-based. The codegen backend code already had something similar, this refactoring pulls this functionality up into `SelfProfiler` itself, for general use. The second commit of this PR is a review and update of the existing events we are already recording. Names have been made more consistent. CGU names have been removed from event names. They will be added back in when function parameter recording is implemented. There is still some work to be done for adding new events, especially around trait resolution and the incremental system. r? @wesleywiser
…i-refactor, r=wesleywiser SelfProfiler API refactoring and part one of event review This PR refactors the `SelfProfiler` a little bit so that most profiling methods are RAII-based. The codegen backend code already had something similar, this refactoring pulls this functionality up into `SelfProfiler` itself, for general use. The second commit of this PR is a review and update of the existing events we are already recording. Names have been made more consistent. CGU names have been removed from event names. They will be added back in when function parameter recording is implemented. There is still some work to be done for adding new events, especially around trait resolution and the incremental system. r? @wesleywiser
Rollup of 11 pull requests Successful merges: - #64649 (Avoid ICE on return outside of fn with literal array) - #64722 (Make all alt builders produce parallel-enabled compilers) - #64801 (Avoid `chain()` in `find_constraint_paths_between_regions()`.) - #64805 (Still more `ObligationForest` improvements.) - #64840 (SelfProfiler API refactoring and part one of event review) - #64885 (use try_fold instead of try_for_each to reduce compile time) - #64942 (Fix clippy warnings) - #64952 (Update cargo.) - #64974 (Fix zebra-striping in generic dataflow visualization) - #64978 (Fully clear `HandlerInner` in `Handler::reset_err_count`) - #64979 (Update books) Failed merges: - #64959 (syntax: improve parameter without type suggestions) r? @ghost
| @@ -380,6 +323,9 @@ pub fn start_async_codegen<B: ExtraBackendMethods>( | |||
| ) -> OngoingCodegen<B> { | |||
| let (coordinator_send, coordinator_receive) = channel(); | |||
| let sess = tcx.sess; | |||
|
|
|||
| sess.prof.generic_activity_start("codegen_and_optimize_crate"); | |||
There was a problem hiding this comment.
this activity causes the events to get in the faulty order
i get a panic added som printout to see what event was the problem.
when I removed this trace it worked again
thread 'main' panicked at 'previous event on thread wasn't the start event , stopEvent Event { event_kind: "GenericActivity", label: "codegen_crate", additional_data: [], timestamp: SystemTime { tv_sec: 117, tv_nsec: 342291500 }, timestamp_kind: End, thread_id: 2 }, Event that do not match as a startEvent Event { event_kind: "GenericActivity", label: "codegen_and_optimize_crate", additional_data: [], timestamp: SystemTime { tv_sec: 62, tv_nsec: 748803800 }, timestamp_kind: Start, thread_id: 2 }', measureme/src/profiling_data.rs:140:
There was a problem hiding this comment.
created #65137 to remove this event
There was a problem hiding this comment.
I created #65137 to remove the event
…iser self-profiling: Add events for everything except trait selection. This is the followup PR to #64840. Trait selection events are still missing (at least those not covered by regular queries). r? @wesleywiser (or @Mark-Simulacrum if @wesleywiser is not available at the moment)
…-work-item-event-names, r=wesleywiser self-profiling: Remove module names from some event-ids in codegen backend. Event-IDs are not supposed to contain argument values. Event-IDs are the equivalent of function names. Proper support for parameters will be added to self-profiling down the line. This PR fixes an oversight from rust-lang#64840. r? @wesleywiser
…-work-item-event-names, r=wesleywiser self-profiling: Remove module names from some event-ids in codegen backend. Event-IDs are not supposed to contain argument values. Event-IDs are the equivalent of function names. Proper support for parameters will be added to self-profiling down the line. This PR fixes an oversight from rust-lang#64840. r? @wesleywiser
This PR refactors the
SelfProfilera little bit so that most profiling methods are RAII-based. The codegen backend code already had something similar, this refactoring pulls this functionality up intoSelfProfileritself, for general use.The second commit of this PR is a review and update of the existing events we are already recording. Names have been made more consistent. CGU names have been removed from event names. They will be added back in when function parameter recording is implemented.
There is still some work to be done for adding new events, especially around trait resolution and the incremental system.
r? @wesleywiser