[tracing] Make ObservedProperty traceable 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. |
Lalit Maganti (Gerrit)
Lalit Maganti added 1 comment![]( "Open in Gerrit")
perfetto::internal::TrackRegistry::InitializeInstance();Given this a function in the internal namespace, I'd strongly suggest *not* depending on this and finding another way to expose what you need from the SDK officially.
Related details
- Etienne Pierre-Doray
- Joe Mason
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 1 comment![]( "Open in Gerrit")
perfetto::internal::TrackRegistry::InitializeInstance();Given this a function in the internal namespace, I'd strongly suggest *not* depending on this and finding another way to expose what you need from the SDK officially.
TrackRegistry::InitializeInstance is called from Tracing::Initialize(const TracingInitArgs& args), so we can indeed set the uuid in TracingInitArgs.
But the fact that we call TrackRegistry::InitializeInstance directly doesn't have much to do with this CL.
See https://source.chromium.org/chromium/chromium/src/+/main:services/tracing/public/cpp/trace_startup.cc;l=64;drc=18d241cf44a9580c0d53dfb9e5b16a9c95f164b6;bpv=1;bpt=1
that comment is stale; but we need to call TrackRegistry::InitializeInstance to untie a circular dependency: the platform provided to Tracing::Initialize needs a thread started by chrome, which may use Track API which would crash if TrackRegistry isn't registered first.
As for this particular testing call, it could probably simply call Tracing::Initialize and use the perfetto platform, but there's no need to initialize perfetto entirely, this is only needed to allow TrackEvent::SetTrackDescriptor()
Related details
- Joe Mason
- Lalit Maganti
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 1 comment![]( "Open in Gerrit")
perfetto::internal::TrackRegistry::InitializeInstance();Etienne Pierre-DorayGiven this a function in the internal namespace, I'd strongly suggest *not* depending on this and finding another way to expose what you need from the SDK officially.
TrackRegistry::InitializeInstance is called from Tracing::Initialize(const TracingInitArgs& args), so we can indeed set the uuid in TracingInitArgs.
But the fact that we call TrackRegistry::InitializeInstance directly doesn't have much to do with this CL.
See https://source.chromium.org/chromium/chromium/src/+/main:services/tracing/public/cpp/trace_startup.cc;l=64;drc=18d241cf44a9580c0d53dfb9e5b16a9c95f164b6;bpv=1;bpt=1
that comment is stale; but we need to call TrackRegistry::InitializeInstance to untie a circular dependency: the platform provided to Tracing::Initialize needs a thread started by chrome, which may use Track API which would crash if TrackRegistry isn't registered first.
As for this particular testing call, it could probably simply call Tracing::Initialize and use the perfetto platform, but there's no need to initialize perfetto entirely, this is only needed to allow TrackEvent::SetTrackDescriptor()
I added conditional in code to avoid this for now.
Related details
- Joe Mason
- Lalit Maganti
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 11 comments![]( "Open in Gerrit")
Initial comments, haven't reviewed TracingObserver yet.
perfetto::internal::TrackRegistry::InitializeInstance();Etienne Pierre-DorayGiven this a function in the internal namespace, I'd strongly suggest *not* depending on this and finding another way to expose what you need from the SDK officially.
Etienne Pierre-DorayTrackRegistry::InitializeInstance is called from Tracing::Initialize(const TracingInitArgs& args), so we can indeed set the uuid in TracingInitArgs.
But the fact that we call TrackRegistry::InitializeInstance directly doesn't have much to do with this CL.
See https://source.chromium.org/chromium/chromium/src/+/main:services/tracing/public/cpp/trace_startup.cc;l=64;drc=18d241cf44a9580c0d53dfb9e5b16a9c95f164b6;bpv=1;bpt=1
that comment is stale; but we need to call TrackRegistry::InitializeInstance to untie a circular dependency: the platform provided to Tracing::Initialize needs a thread started by chrome, which may use Track API which would crash if TrackRegistry isn't registered first.
As for this particular testing call, it could probably simply call Tracing::Initialize and use the perfetto platform, but there's no need to initialize perfetto entirely, this is only needed to allow TrackEvent::SetTrackDescriptor()
I added conditional in code to avoid this for now.
Agree that checking if initialized every time is better than initializing the registry only. I suggested adding a helper function for that.
parent_track ? *parent_track : perfetto::ProcessTrack::Current())Nit: could use a comment saying where the FrameNode track will show up in practice. "Current()" would be the browser process, right? When would this happen?)
base::TrackEvent::SetTrackDescriptor(track, track.Serialize());Now that we've got several places in the code that have to do this, it could use a utility function. Maybe `void base::tracing::InitializeTrack<TrackType>(const TrackType& track)` so that the impl can call the correct `track.Serialize()` func.
This can be a follow-up.
tracing_track_,Nit: why isn't this getting its own track? Could use a comment, if it's intentional.
class Wrapper = TrivialWrapper<PropertyType>>Does this work?
```
template <typename PropertyType,
void (ObserverType::*NotifyFunctionPtr)(const NodeType*),
typename WrapperTemplate = TrivialWrapper,
typename Wrapper = WrapperTemplate<PropertyType>>
```
That way a property decl wouldn't need to repeat the PropertyType in the wrapper:
```
ObservedProperty::NotifiesOnlyOnChanges<bool,
&PageNodeObserver::OnIsFocusedChanged,
TracedWrapper>
```
I'd really like to find a way to avoid that repetition. (Can clean it up as a followup though.)
template <typename U = PropertyType>Nit: blank line before this.
slice_is_open_ = !!value_str;Nit: I always find `!!` confusing. I'd prefer explicit `value_str != nullptr` or `bool{value_str}`.
I'd actually prefer this, to keep all the code that depends on `slice_is_open_` in the same control flow block, but it's a judgement call:
```
if (slice_is_open_) {
TRACE_EVENT_END(...);
slice_is_open_ = false;
}
if (value_str) {
TRACE_EVENT_BEGIN(...);
slice_is_open_ = true;
}
```
const PropertyType& value() const& { return value_; }Can you add a comment explaining what we discussed offline?
eg. "The && qualified method forwards `std::move(wrapper)` to `wrapper.value_`. It requires the const method to be `const&` to avoid ambiguity."
~TracedWrapper() {Please fix this WARNING reported by ClangTidy: check: modernize-use-override
annotate this function with 'override' or (rarely...
check: modernize-use-override
annotate this function with 'override' or (rarely) 'final' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-override.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-override` footer to the CL description to skip the check)
(Note: running clang-tidy on this file failed; this diagnostic might be incorrect as a result.)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
TracedWrapper(perfetto::NamedTrack track, ConverterFuncPtr converter_func)Nit: please add blank lines before method declarations. Easier to read.
Related details
- Etienne Pierre-Doray
- Lalit Maganti
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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
LGTM with nits, although I'd like to try to get rid of the duplicate types in ObservedProperty as a followup.
base::ObserverList<TracingObserver> observers_;Optional suggestion: If you make this a `base::ObserverListThreadSafe`, you won't need to store a TaskRunner or WeakPtrFactor - the sequence checker ensures RegisterObserver and UnregisterObserver are always called from the same sequence as the constructor, and then you can just call `observers_.Notify` directly. `base::ObserverListThreadSafe::Notify` takes care of posting to the correct sequence, and checking if the observer still exist when the notification arrives.
OTOH it posts once for each observer, which is slightly less efficient. But tracing doesn't start/stop often so that shouldn't matter.
If you use ObserverListThreadSafe please use `RemoveObserverPolicy::kAddingSequenceOnly` which is safer but not the default because it was added later.
void RegisterObserver(TracingObserver* observer);Nit: prefer to name these AddObserver and RemoveObserver so that base::ScopedObservation works with them.
Related details
- Etienne Pierre-Doray
- Lalit Maganti
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 12 comments![]( "Open in Gerrit")
parent_track ? *parent_track : perfetto::ProcessTrack::Current())Nit: could use a comment saying where the FrameNode track will show up in practice. "Current()" would be the browser process, right? When would this happen?)
Done
The track is added under browser when renderer isn't available, but this is fixed in a following CL.
base::TrackEvent::SetTrackDescriptor(track, track.Serialize());Now that we've got several places in the code that have to do this, it could use a utility function. Maybe `void base::tracing::InitializeTrack<TrackType>(const TrackType& track)` so that the impl can call the correct `track.Serialize()` func.
This can be a follow-up.
Good idea, Done.
Nit: why isn't this getting its own track? Could use a comment, if it's intentional.
Done.
For convenience, I find visibility is the main thing of interest w.r.t frames in renderer. It fully describes the lifetime as well.
class Wrapper = TrivialWrapper<PropertyType>>Does this work?
```
template <typename PropertyType,
void (ObserverType::*NotifyFunctionPtr)(const NodeType*),
typename WrapperTemplate = TrivialWrapper,
typename Wrapper = WrapperTemplate<PropertyType>>
```That way a property decl wouldn't need to repeat the PropertyType in the wrapper:
```
ObservedProperty::NotifiesOnlyOnChanges<bool,
&PageNodeObserver::OnIsFocusedChanged,
TracedWrapper>
```I'd really like to find a way to avoid that repetition. (Can clean it up as a followup though.)
More or less, it'd be something like `template <typename U> class WrapperTemplate = TrivialWrapper`
The big downside is that all wrappers must then be in the form Wrapper<T>, which prevents potentially having ConverterFunc as wrapper template param.
I feel the gain is't worth the limitation, WDYT?
Another option is to *only* (and always) provide the wrapper type (and have PropertyType deduced from it):
```
ObservedProperty::NotifiesOnlyOnChanges<TracedWrapper<bool>,
&PageNodeObserver::OnIsFocusedChanged>
```
But this would require changing all ObservedProperty at once.
Nit: blank line before this.
Done
Nit: I always find `!!` confusing. I'd prefer explicit `value_str != nullptr` or `bool{value_str}`.
I'd actually prefer this, to keep all the code that depends on `slice_is_open_` in the same control flow block, but it's a judgement call:
```
if (slice_is_open_) {
TRACE_EVENT_END(...);
slice_is_open_ = false;
}
if (value_str) {
TRACE_EVENT_BEGIN(...);
slice_is_open_ = true;
}
```
Done
Can you add a comment explaining what we discussed offline?
eg. "The && qualified method forwards `std::move(wrapper)` to `wrapper.value_`. It requires the const method to be `const&` to avoid ambiguity."
Done
Please fix this WARNING reported by ClangTidy: check: modernize-use-override
annotate this function with 'override' or (rarely...
check: modernize-use-override
annotate this function with 'override' or (rarely) 'final' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-override.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-override` footer to the CL description to skip the check)
(Note: running clang-tidy on this file failed; this diagnostic might be incorrect as a result.)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
TracedWrapper(perfetto::NamedTrack track, ConverterFuncPtr converter_func)Nit: please add blank lines before method declarations. Easier to read.
Done
base::ObserverList<TracingObserver> observers_;Optional suggestion: If you make this a `base::ObserverListThreadSafe`, you won't need to store a TaskRunner or WeakPtrFactor - the sequence checker ensures RegisterObserver and UnregisterObserver are always called from the same sequence as the constructor, and then you can just call `observers_.Notify` directly. `base::ObserverListThreadSafe::Notify` takes care of posting to the correct sequence, and checking if the observer still exist when the notification arrives.
OTOH it posts once for each observer, which is slightly less efficient. But tracing doesn't start/stop often so that shouldn't matter.
If you use ObserverListThreadSafe please use `RemoveObserverPolicy::kAddingSequenceOnly` which is safer but not the default because it was added later.
I find that the PostTask is simple enough and it makes it clearer this is single threaded with the exception of TracingObserverList::OnStart.
But I don't feel very strongly about this.
Nit: prefer to name these AddObserver and RemoveObserver so that base::ScopedObservation works with them.
- Lalit Maganti
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 2 comments![]( "Open in Gerrit")
class Wrapper = TrivialWrapper<PropertyType>>Etienne Pierre-DorayDoes this work?
```
template <typename PropertyType,
void (ObserverType::*NotifyFunctionPtr)(const NodeType*),
typename WrapperTemplate = TrivialWrapper,
typename Wrapper = WrapperTemplate<PropertyType>>
```That way a property decl wouldn't need to repeat the PropertyType in the wrapper:
```
ObservedProperty::NotifiesOnlyOnChanges<bool,
&PageNodeObserver::OnIsFocusedChanged,
TracedWrapper>
```I'd really like to find a way to avoid that repetition. (Can clean it up as a followup though.)
More or less, it'd be something like `template <typename U> class WrapperTemplate = TrivialWrapper`
The big downside is that all wrappers must then be in the form Wrapper<T>, which prevents potentially having ConverterFunc as wrapper template param.
I feel the gain is't worth the limitation, WDYT?Another option is to *only* (and always) provide the wrapper type (and have PropertyType deduced from it):
```
ObservedProperty::NotifiesOnlyOnChanges<TracedWrapper<bool>,
&PageNodeObserver::OnIsFocusedChanged>
```
But this would require changing all ObservedProperty at once.
I like the second one, but a big-bang conversion can wait for a followup.
base::ObserverList<TracingObserver> observers_;Etienne Pierre-DorayOptional suggestion: If you make this a `base::ObserverListThreadSafe`, you won't need to store a TaskRunner or WeakPtrFactor - the sequence checker ensures RegisterObserver and UnregisterObserver are always called from the same sequence as the constructor, and then you can just call `observers_.Notify` directly. `base::ObserverListThreadSafe::Notify` takes care of posting to the correct sequence, and checking if the observer still exist when the notification arrives.
OTOH it posts once for each observer, which is slightly less efficient. But tracing doesn't start/stop often so that shouldn't matter.
If you use ObserverListThreadSafe please use `RemoveObserverPolicy::kAddingSequenceOnly` which is safer but not the default because it was added later.
I find that the PostTask is simple enough and it makes it clearer this is single threaded with the exception of TracingObserverList::OnStart.
But I don't feel very strongly about this.
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 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] Make ObservedProperty traceable in performance manager
This CL adds a mechanism to optionally trace properties, and uses it
for a few properties in the graph. This also adds new tracing track
in FrameNodeImpl.
- M base/trace_event/builtin_categories.h
- M base/trace_event/trace_event.h
- M components/performance_manager/BUILD.gn
- M components/performance_manager/embedder/graph_features.h
- M components/performance_manager/graph/frame_node_impl.cc
- M components/performance_manager/graph/frame_node_impl.h
- M components/performance_manager/graph/frame_node_impl_describer.cc
- M components/performance_manager/graph/page_node.cc
- M components/performance_manager/graph/page_node_impl.cc
- M components/performance_manager/graph/page_node_impl.h
- A components/performance_manager/graph/properties.cc
- M components/performance_manager/graph/properties.h
- A components/performance_manager/graph/tracing_observer.cc
- A components/performance_manager/graph/tracing_observer.h
- M components/performance_manager/graph_features.cc
Code-Review: +1 by Joe Mason
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |