[tracing] Guarantee a predictable process track in performance manager [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 4 comments![]( "Open in Gerrit")
std::optional<perfetto::Track> parent_track,Can this become non-optional now that the track is guaranteed to be available?
TracedWrapper<base::TaskPriority>>Nit: adding the Priority event should really be in its own commit. Not a huge deal though.
proxy.tracing_track(),Please fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
'proxy' used after it was moved (https://clang.l...
check: bugprone-use-after-move
'proxy' used after it was moved (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
I would make a helper func that that takes an AnyChildProcessHostProxy and returns the tracing track from the wrapped proxy, or Current() for the browser process, and call that from the main ProcessNodeImpl constructor (instead of passing the track as a param).
uint64_t process_track_id,Since this is unused now, should remove it from the mojom.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 5 comments![]( "Open in Gerrit")
Can this become non-optional now that the track is guaranteed to be available?
Done
Nit: adding the Priority event should really be in its own commit. Not a huge deal though.
Done
Please fix this WARNING reported by ClangTidy: check: bugprone-use-after-move
'proxy' used after it was moved (https://clang.l...
check: bugprone-use-after-move
'proxy' used after it was moved (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html)
I would make a helper func that that takes an AnyChildProcessHostProxy and returns the tracing track from the wrapped proxy, or Current() for the browser process, and call that from the main ProcessNodeImpl constructor (instead of passing the track as a param).
Done
Since this is unused now, should remove it from the mojom.
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
3 comments
// re-initialized for the same ProcessNode (eg. after a crash).Nit: this comment is now out of date. (Judgement call whether to keep the CHECK.)
// perfetto track whose parent is a child process track.Nit: "tracks".
Also I think it would be clearer to phrase this as, "This may be used to create new perfetto tracks nested under the child process track." Otherwise "parent" and "child" are used in different ways in this comment.
// Do not emit events directly to this track. This may be used to create newNit: Explain why not. (I assume it's because the child process will also be emitting events to the track, and mixing events from different processes would be confusing?)
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
// re-initialized for the same ProcessNode (eg. after a crash).Nit: this comment is now out of date. (Judgement call whether to keep the CHECK.)
Done
// perfetto track whose parent is a child process track.Nit: "tracks".
Also I think it would be clearer to phrase this as, "This may be used to create new perfetto tracks nested under the child process track." Otherwise "parent" and "child" are used in different ways in this comment.
Done
// Do not emit events directly to this track. This may be used to create newNit: Explain why not. (I assume it's because the child process will also be emitting events to the track, and mixing events from different processes would be confusing?)
Done -> emitting directly to a track whose id is the process doesn't make a lot of sense, and the UI shows these events in a very misleading way.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joe Mason
| Code-Review | +1 |
3 comments
perfetto::NamedTrack ProcessNodeImpl::CreateTracingTrack(Nit: the difference between this and GetTracingTrack is confusing. Maybe call this `CreateNestedTracingTrack`?
// auto track = perfetto::NamedTrack( "Name", id,Nit: extra space here
Related details
- Dave Tapuska
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
Nit: the difference between this and GetTracingTrack is confusing. Maybe call this `CreateNestedTracingTrack`?
Ah no, this isn't needed anymore, deleted.
// auto track = perfetto::NamedTrack( "Name", id,Etienne Pierre-DorayNit: extra space here
Done
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
| 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
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[tracing] Guarantee a predictable process track in performance manager
This CL fixes a caveat in dumping PM frame tree: it makes
child process track guaranteed to be available by having
the browser process decide on the track uuid at process creation.
The track uuid is forwarded to child processes through command line.
- M components/performance_manager/graph/frame_node_impl.cc
- M components/performance_manager/graph/frame_node_impl.h
- M components/performance_manager/graph/page_node_impl.cc
- M components/performance_manager/graph/process_node_impl.cc
- M components/performance_manager/graph/process_node_impl.h
- M components/performance_manager/graph/process_node_impl_unittest.cc
- M components/performance_manager/public/browser_child_process_host_proxy.h
- M components/performance_manager/public/mojom/coordination_unit.mojom
- M components/performance_manager/public/render_process_host_proxy.h
- M components/performance_manager/public/tracing_support.h
- M components/performance_manager/scenarios/performance_scenario_data.cc
- M components/performance_manager/tracing_support.cc
- M components/tracing/common/tracing_switches.cc
- M components/tracing/common/tracing_switches.h
- M content/browser/browser_child_process_host_impl.cc
- M content/browser/child_process_launcher.cc
- M content/browser/child_process_launcher.h
- M content/browser/child_process_launcher_helper.cc
- M content/browser/child_process_launcher_helper.h
- M content/browser/child_process_launcher_helper_posix.cc
- M content/browser/child_process_launcher_helper_posix.h
- M content/browser/renderer_host/render_process_host_impl.cc
- M content/child/child_performance_coordinator.cc
- M content/child/child_performance_coordinator_unittest.cc
- M content/public/browser/BUILD.gn
- A content/public/browser/tracing_support.cc
- A content/public/browser/tracing_support.h
- M services/tracing/public/cpp/perfetto/perfetto_traced_process.cc
- M services/tracing/public/cpp/perfetto/perfetto_traced_process.h
- M services/tracing/public/cpp/trace_startup.cc
Code-Review: +1 by Dave Tapuska, +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |