[tracing] Improve MainThreadSchedulerImpl trace events [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
TraceableState<std::optional<TaskDescriptionForTracing>,
TRACE_DISABLED_BY_DEFAULT("renderer.scheduler")>
task_description_for_tracing; // Don't use except for tracing.
TraceableState<std::optional<TaskPriority>,
TRACE_DISABLED_BY_DEFAULT("renderer.scheduler")>
task_priority_for_tracing; // Only used for tracing.These 2 are redundant with "scheduler" that adds args to RunTask (we could probably remove them)
They are also not moved to "renderer.scheduler.status" because they are high overhead.
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
perfetto::Track tracing_track() const { return *tracing_track_; }Blink Style Guide: Naming - Use 'CamelCase' for all function names. Function names should begin with an uppercase letter. Please rename `tracing_track()` to `TracingTrack()`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 4 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
4 comments
LGTM. Thanks for all the tracing improvements!
"RendererScheduler.BackgroundBudgetMs",MTSI is using "Scheduler.*" now instead of RendererScheduler. Should this too?
"RendererScheduler.FindInPageBudgetMs", scheduler_->tracing_track());Same question here.
TraceableState<std::optional<TaskDescriptionForTracing>,
TRACE_DISABLED_BY_DEFAULT("renderer.scheduler")>
task_description_for_tracing; // Don't use except for tracing.
TraceableState<std::optional<TaskPriority>,
TRACE_DISABLED_BY_DEFAULT("renderer.scheduler")>
task_priority_for_tracing; // Only used for tracing.These 2 are redundant with "scheduler" that adds args to RunTask (we could probably remove them)
They are also not moved to "renderer.scheduler.status" because they are high overhead.
ACK, I use the "scheduler" variants, so removing SG from my side. I still find it a bit unintuitive that "scheduler" adds info to existing trace events. Not sure if there's a better setup/name for that, but meh I'm used to it at this point.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 4 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +2 |
4 comments
MTSI is using "Scheduler.*" now instead of RendererScheduler. Should this too?
Sure, done.
"RendererScheduler.FindInPageBudgetMs", scheduler_->tracing_track());Etienne Pierre-DoraySame question here.
Done
TraceableState<std::optional<TaskDescriptionForTracing>,
TRACE_DISABLED_BY_DEFAULT("renderer.scheduler")>
task_description_for_tracing; // Don't use except for tracing.
TraceableState<std::optional<TaskPriority>,
TRACE_DISABLED_BY_DEFAULT("renderer.scheduler")>
task_priority_for_tracing; // Only used for tracing.Scott HaseleyThese 2 are redundant with "scheduler" that adds args to RunTask (we could probably remove them)
They are also not moved to "renderer.scheduler.status" because they are high overhead.
ACK, I use the "scheduler" variants, so removing SG from my side. I still find it a bit unintuitive that "scheduler" adds info to existing trace events. Not sure if there's a better setup/name for that, but meh I'm used to it at this point.
Ok I removed it.
still find it a bit unintuitive that "scheduler" adds info to existing trace events
altimin added at some point "config.scheduler.record_task_post_time" (for other things), which is still just a category but at least the naming is a bit more helpful.
perfetto::Track tracing_track() const { return *tracing_track_; }Blink Style Guide: Naming - Use 'CamelCase' for all function names. Function names should begin with an uppercase letter. Please rename `tracing_track()` to `TracingTrack()`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
1 comment
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[tracing] Improve MainThreadSchedulerImpl trace events
This CL
- Folds MainThreadSchedulerImpl under a common "MainThreadScheduler" track
- moves a few TraceableState to "renderer.scheduler.status" so that they are considered persistent events.
- Plumb correct name to CPUTimeBudgetPool
- M third_party/blink/renderer/platform/scheduler/common/throttling/cpu_time_budget_pool.cc
- M third_party/blink/renderer/platform/scheduler/common/throttling/cpu_time_budget_pool.h
- M third_party/blink/renderer/platform/scheduler/main_thread/find_in_page_budget_pool_controller.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h
- M third_party/blink/renderer/platform/scheduler/main_thread/page_scheduler_impl.cc
- M third_party/blink/renderer/platform/scheduler/worker/worker_thread_scheduler.cc
Code-Review: +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |