etw payload support [chromium/src : main]
Todd Reifsteck (Gerrit)
Todd Reifsteck added 2 comments![]( "Open in Gerrit")
Adding Bruce Dawson to reviewers to see if we can get a speed up a prompt review as this issue is causing us to have problems with using ETW to diagnose/investigate issues in Edge. Thanks so much!
// Untemplate version of WreiteEvent, taking series of TlmField objects.```suggestion
// Untemplated version of WriteEvent, taking series of TlmField objects.
```
Related details
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
// Untemplate version of WreiteEvent, taking series of TlmField objects.```suggestion
// Untemplated version of WriteEvent, taking series of TlmField objects.
```
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bruce Dawson (Gerrit)
Bruce Dawson voted and added 5 comments![]( "Open in Gerrit")
Votes added by Bruce Dawson
| Code-Review | +1 |
5 comments
uint8_t in_type_ = 2 /* TlgInANSISTRING*/;For consistency there should be a space before */
namespace perfetto {Can/should this be done as:
namespace perfetto::protos::pbzero {
?
See line 25.
// The maximum number of fields is 6, which is including maximum of 2 debugInstead of a comment can you add a DCHECK just before the call to WriteEvent?
in_type_ = 2 /* TlgInANSISTRING*/;For consistency there should be a space after /* and before */
Related details
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 4 comments![]( "Open in Gerrit")
For consistency there should be a space before */
Update to match the format
Can/should this be done as:
namespace perfetto::protos::pbzero {
?See line 25.
Thanks, updated the name spaces.
// The maximum number of fields is 6, which is including maximum of 2 debugInstead of a comment can you add a DCHECK just before the call to WriteEvent?
@bruce Dawson, remove the comment as there is no limit on number of arguments we can handle here.
For consistency there should be a space after /* and before */
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bruce Dawson (Gerrit)
Bruce Dawson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Bruce Dawson
| Code-Review | +1 |
2 comments
Thanks for the comment fixes. One more textual nit.
When generating etw event, trace arguments are not populated,The body of git commit messages should be wrapped at 72 columns.
Related details
- Chris Davis
- Eric Seckler
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
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 3 comments![]( "Open in Gerrit")
Overall LGTM, but I worry about added overhead (ETW users are the main interested)
I suggested ways to mitigate, feel free to ignore or do in a follow-up.
std::vector<std::unique_ptr<TlmFieldBase>> etw_payloads;Since most events don't have debug_annotations, you could eihter
- add a fast path that calls WriteEvent() early as before, and avoid allocations.
- Use absl::InlineVector that fits the minimum number of elements.
// Pack the event data.
std::vector<EVENT_DATA_DESCRIPTOR> descriptors(descriptors_count);
uint8_t descriptors_index = 2;
for (const auto& field : fields) {
const auto& value = *field;
value.FillEventDescriptor(&descriptors[descriptors_index]);
descriptors_index += value.GetDataDescCount();
}One idea to reduce overhead:
- Wrap std::vector<EVENT_DATA_DESCRIPTOR> and char metadata[kMaxEventMetadataSize] in a helper class
- The caller inserts elements by calling a (templated) method on the helper (instead of vector insert), fields get serialized right away (with FillEventDescriptor), this avoid unique_ptr allocation for each field, and a second vector.
Related details
- Chris Davis
- Eric Seckler
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chris Davis
- Eric Seckler
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
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")
Thanks for taking this on btw!
Myungsub Kim (Gerrit)
Myungsub Kim added 3 comments![]( "Open in Gerrit")
When generating etw event, trace arguments are not populated,The body of git commit messages should be wrapped at 72 columns.
Thanks, I updated the commit message.
std::vector<std::unique_ptr<TlmFieldBase>> etw_payloads;Since most events don't have debug_annotations, you could eihter
- add a fast path that calls WriteEvent() early as before, and avoid allocations.
- Use absl::InlineVector that fits the minimum number of elements.
Thanks for the InlinedVector suggestion. changed to it and set the the default size to 6.
// Pack the event data.
std::vector<EVENT_DATA_DESCRIPTOR> descriptors(descriptors_count);
uint8_t descriptors_index = 2;
for (const auto& field : fields) {
const auto& value = *field;
value.FillEventDescriptor(&descriptors[descriptors_index]);
descriptors_index += value.GetDataDescCount();
}One idea to reduce overhead:
- Wrap std::vector<EVENT_DATA_DESCRIPTOR> and char metadata[kMaxEventMetadataSize] in a helper class
- The caller inserts elements by calling a (templated) method on the helper (instead of vector insert), fields get serialized right away (with FillEventDescriptor), this avoid unique_ptr allocation for each field, and a second vector.
@Etienne, thanks for the suggestion but I will have to ask you more help on that implementation as I am not clear. So, I will pass on this iteration for now.
Related details
- Chris Davis
- Eric Seckler
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
// Pack the event data.
std::vector<EVENT_DATA_DESCRIPTOR> descriptors(descriptors_count);
uint8_t descriptors_index = 2;
for (const auto& field : fields) {
const auto& value = *field;
value.FillEventDescriptor(&descriptors[descriptors_index]);
descriptors_index += value.GetDataDescCount();
}Myungsub KimOne idea to reduce overhead:
- Wrap std::vector<EVENT_DATA_DESCRIPTOR> and char metadata[kMaxEventMetadataSize] in a helper class
- The caller inserts elements by calling a (templated) method on the helper (instead of vector insert), fields get serialized right away (with FillEventDescriptor), this avoid unique_ptr allocation for each field, and a second vector.
@Etienne, thanks for the suggestion but I will have to ask you more help on that implementation as I am not clear. So, I will pass on this iteration for now.
Checking on as "Resolved" to satisfy the check-in condition. Please let me know if this isn't supposed to be like this.
Related details
- Chris Davis
- Eric Seckler
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler voted and added 6 comments![]( "Open in Gerrit")
Votes added by Eric Seckler
| Code-Review | +1 |
6 comments
When generating etw event, trace arguments are not populated,nit: now?
Deligate for TrackEventStateTracker was modified to take SequenceStatenit: Delegate
to pass SequenceState directly to ETWInterceptor::Delegate::OnTrackEven() instead.nit: OnTrackEvent
// TODO(crbug.com/40276149): Consider exporting thread time onceHalf of this comment got removed
TlmFieldVector etw_payloads;Consider computing the required length of this in advance (e.g. N + M*event.track_event.debug_annotations_size())
// Pack the event data.
std::vector<EVENT_DATA_DESCRIPTOR> descriptors(descriptors_count);
uint8_t descriptors_index = 2;
for (const auto& field : fields) {
const auto& value = *field;
value.FillEventDescriptor(&descriptors[descriptors_index]);
descriptors_index += value.GetDataDescCount();
}Myungsub KimOne idea to reduce overhead:
- Wrap std::vector<EVENT_DATA_DESCRIPTOR> and char metadata[kMaxEventMetadataSize] in a helper class
- The caller inserts elements by calling a (templated) method on the helper (instead of vector insert), fields get serialized right away (with FillEventDescriptor), this avoid unique_ptr allocation for each field, and a second vector.
Myungsub Kim@Etienne, thanks for the suggestion but I will have to ask you more help on that implementation as I am not clear. So, I will pass on this iteration for now.
Checking on as "Resolved" to satisfy the check-in condition. Please let me know if this isn't supposed to be like this.
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 5 comments![]( "Open in Gerrit")
When generating etw event, trace arguments are not populated,nit: now?
I am not sure what do you mean for this though. Can you help?
Deligate for TrackEventStateTracker was modified to take SequenceStateMyungsub Kimnit: Delegate
Thanks.
to pass SequenceState directly to ETWInterceptor::Delegate::OnTrackEven() instead.Myungsub Kimnit: OnTrackEvent
Thanks for the catch
// TODO(crbug.com/40276149): Consider exporting thread time onceHalf of this comment got removed
Thanks
TlmFieldVector etw_payloads;Consider computing the required length of this in advance (e.g. N + M*event.track_event.debug_annotations_size())
I have set the default size to 6, I will try out this shortly.
Related details
- Chris Davis
- Eric Seckler
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eric Seckler
| Code-Review | +1 |
1 comment
When generating etw event, trace arguments are not populated,Myungsub Kimnit: now?
I am not sure what do you mean for this though. Can you help?
Ah I just wasn't following this sentence.
"trace arguments are not populated" -> does this describe the status-quo before the patch? Your next subclause talks about adding support to do this, I guess. Semantically, there's an issue there 😊
Here's an alternative formulation:
"This patch adds support to emit legacy trace arguments into ETW events by converting these arguments into additional ETW fields."
...
Related details
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
When generating etw event, trace arguments are not populated,Myungsub Kimnit: now?
Eric SecklerI am not sure what do you mean for this though. Can you help?
Ah I just wasn't following this sentence.
"trace arguments are not populated" -> does this describe the status-quo before the patch? Your next subclause talks about adding support to do this, I guess. Semantically, there's an issue there 😊
Here's an alternative formulation:
"This patch adds support to emit legacy trace arguments into ETW events by converting these arguments into additional ETW fields."
...
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
TlmFieldVector etw_payloads;Myungsub KimConsider computing the required length of this in advance (e.g. N + M*event.track_event.debug_annotations_size())
I have set the default size to 6, I will try out this shortly.
- Bruce Dawson
- Chris Davis
- Eric Seckler
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
// Pack the event data.
std::vector<EVENT_DATA_DESCRIPTOR> descriptors(descriptors_count);
uint8_t descriptors_index = 2;
for (const auto& field : fields) {
const auto& value = *field;
value.FillEventDescriptor(&descriptors[descriptors_index]);
descriptors_index += value.GetDataDescCount();
}Myungsub KimOne idea to reduce overhead:
- Wrap std::vector<EVENT_DATA_DESCRIPTOR> and char metadata[kMaxEventMetadataSize] in a helper class
- The caller inserts elements by calling a (templated) method on the helper (instead of vector insert), fields get serialized right away (with FillEventDescriptor), this avoid unique_ptr allocation for each field, and a second vector.
Myungsub Kim@Etienne, thanks for the suggestion but I will have to ask you more help on that implementation as I am not clear. So, I will pass on this iteration for now.
Eric SecklerChecking on as "Resolved" to satisfy the check-in condition. Please let me know if this isn't supposed to be like this.
General +1 to considering reducing overhead 😊
Patch 13 has this implemented, please take a look. Thanks
Eric Seckler (Gerrit)
Eric Seckler voted Code-Review+1![]( "Open in Gerrit")
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler added 2 comments![]( "Open in Gerrit")
void WriteEvent(const T& value) {I'd consider renaming this to WriteField, since that's what you pass as an arg?
const TlmUInt64Field timestamp_event(s/_event/_field/ ?
Related details
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 3 comments![]( "Open in Gerrit")
I'd consider renaming this to WriteField, since that's what you pass as an arg?
thanks
const TlmUInt64Field timestamp_event(Myungsub Kims/_event/_field/ ?
thanks, good suggestion.
Related details
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 7 comments![]( "Open in Gerrit")
absl::variant<std::string, uint64_t, int64_t, bool, double> value_;Could this be string_view? Considering pbzero::DebugAnnotation already has storage for the string and will outlive TlmFieldDebugAnnotation.
TlmFieldDebugAnnotation::TlmFieldDebugAnnotation(
std::string_view name,
perfetto::protos::pbzero::DebugAnnotation_Decoder& annotation)
: TlmFieldBase(name) {
CHECK_NE(Name(), nullptr);
if (annotation.has_bool_value()) {
in_type_ = 4 /* TlgInUINT8 */;
out_type_ = 3 /* TlgOutBOOLEAN */;
value_ = annotation.bool_value();
} else if (annotation.has_int_value()) {
in_type_ = 9;
value_ = annotation.int_value();
} else if (annotation.has_uint_value()) {
in_type_ = 10;
value_ = annotation.uint_value();
} else if (annotation.has_string_value()) {
in_type_ = 2 /* TlgInANSISTRING */;
value_.emplace<std::string>(annotation.string_value().data,
annotation.string_value().size);
} else if (annotation.has_legacy_json_value()) {
in_type_ = 2 /* TlgInANSISTRING */;
value_.emplace<std::string>(annotation.legacy_json_value().data,
annotation.legacy_json_value().size);
} else if (annotation.has_pointer_value()) {
in_type_ = 21 /* TlgInINTPTR */;
value_ = annotation.pointer_value();
} else if (annotation.has_double_value()) {
in_type_ = 12 /* TlgInDOUBLE */;
value_ = annotation.double_value();
}
}Suggestion: I think we could leverage existing TlmField classes and avoid TlmFieldDebugAnnotation with a variant, by having a function that calls MultiEtwPayloadHandler::WriteField directly:
```
void WriteDebugAnnotationTlmField(
std::string_view name,
perfetto::protos::pbzero::DebugAnnotation_Decoder& annotation,
MultiEtwPayloadHandler& handler) {
if (annotation.has_bool_value()) {
handler.WriteField(TlmIntBoolField(name, annotation.bool_value()));
} else (annotation.has_int_value()) {
handler.WriteField(TlmInt64Field(name, annotation.int_value()));
}
...
else if (annotation.has_string_value()) {
handler.WriteField(TlmMbcsStringField(name, annotation.string_value()));
}
...
}
```
return provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],Nit: we could clear provider_ here, and
- CHECK_EQ(nullptr, provider_) in destructor to enforce EventEnd gets called
- CHECK_NE(nullptr, provider_) at the beginning of EventEnd to enforce it's only called once.
absl::InlinedVector<TlmFieldDebugAnnotation, 2> debug__fields;
for (auto it = event.track_event.debug_annotations(); it; ++it) {
perfetto::protos::pbzero::DebugAnnotation_Decoder annotation(*it);
debug__fields.emplace_back(GetDebugAnnotationName(annotation), annotation);
}Following up on previous comment about string_view, we might not need additional storage for strings, so this could directly call WriteField()?
std::string_view ETWInterceptor::Delegate::GetDebugAnnotationName(Nit: this can be a free function (non-member) in anonymous namespace.
absl::variant<const char*, std::string> name_;Same as TlmFieldDebugAnnotation, I think this could just be a string_view, since the data comes from either string literals, or pbzero::DebugAnnotation which outlives this.
Additionally add a comment on constructor to mention the data is expected to oulive this class.
Related details
- Bruce Dawson
- Chris Davis
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 7 comments![]( "Open in Gerrit")
absl::variant<std::string, uint64_t, int64_t, bool, double> value_;Could this be string_view? Considering pbzero::DebugAnnotation already has storage for the string and will outlive TlmFieldDebugAnnotation.
If we can use std::string_view instead of std::string, it would been great as we don't have to copy but TlmField string has to be a null terminated string hence I couldn't use it as DebugAnnotation isn't null terminated.
TlmFieldDebugAnnotation::TlmFieldDebugAnnotation(Suggestion: I think we could leverage existing TlmField classes and avoid TlmFieldDebugAnnotation with a variant, by having a function that calls MultiEtwPayloadHandler::WriteField directly:
```
void WriteDebugAnnotationTlmField(
std::string_view name,
perfetto::protos::pbzero::DebugAnnotation_Decoder& annotation,
MultiEtwPayloadHandler& handler) {
if (annotation.has_bool_value()) {
handler.WriteField(TlmIntBoolField(name, annotation.bool_value()));
} else (annotation.has_int_value()) {
handler.WriteField(TlmInt64Field(name, annotation.int_value()));
}
...
else if (annotation.has_string_value()) {
handler.WriteField(TlmMbcsStringField(name, annotation.string_value()));
}
...
}
```
Good suggestion but we can't use std::string_view so we will need new class for it and couple more (pointer and boolean) those are not used anywhere else. Rather keep it under single class as TlmFieldDebugAnnotation.
return provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],Nit: we could clear provider_ here, and
- CHECK_EQ(nullptr, provider_) in destructor to enforce EventEnd gets called
- CHECK_NE(nullptr, provider_) at the beginning of EventEnd to enforce it's only called once.
Added those checks and clear provider_ on EndEvent. Thanks.
absl::InlinedVector<TlmFieldDebugAnnotation, 2> debug__fields;
for (auto it = event.track_event.debug_annotations(); it; ++it) {
perfetto::protos::pbzero::DebugAnnotation_Decoder annotation(*it);
debug__fields.emplace_back(GetDebugAnnotationName(annotation), annotation);
}Following up on previous comment about string_view, we might not need additional storage for strings, so this could directly call WriteField()?
Due to std::string_view can't be used in the Etw field, we can't do that.
std::string_view ETWInterceptor::Delegate::GetDebugAnnotationName(Nit: this can be a free function (non-member) in anonymous namespace.
moved out from Delegate class.
absl::variant<const char*, std::string> name_;Same as TlmFieldDebugAnnotation, I think this could just be a string_view, since the data comes from either string literals, or pbzero::DebugAnnotation which outlives this.
Additionally add a comment on constructor to mention the data is expected to oulive this class.
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| 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")
absl::variant<std::string, uint64_t, int64_t, bool, double> value_;Myungsub KimCould this be string_view? Considering pbzero::DebugAnnotation already has storage for the string and will outlive TlmFieldDebugAnnotation.
If we can use std::string_view instead of std::string, it would been great as we don't have to copy but TlmField string has to be a null terminated string hence I couldn't use it as DebugAnnotation isn't null terminated.
Ah I see.
There is a size deliminted string TlgInCOUNTEDANSISTRING (found at https://github.com/tpn/winsdk-10/blob/master/Include/10.0.14393.0/shared/TraceLoggingProvider.h)
in_type_ = 23
Could we add that as another TlmFieldWithConstants?
absl::variant<const char*, std::string_view> name_;The member can just be `std::string_view name_`
And the constructors becomes
```
explicit TlmFieldBase(const char* name) noexcept : name_(name, strlen(name)) {}
```
Related details
- Bruce Dawson
- Chris Davis
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
absl::variant<const char*, std::string_view> name_;The member can just be `std::string_view name_`
And the constructors becomes```
explicit TlmFieldBase(const char* name) noexcept : name_(name, strlen(name)) {}
```
`string_view` already has a `(const char*)` constructor which does the `strlen` portion. So `name_(name)` should suffice.
Related details
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
return provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],Myungsub KimNit: we could clear provider_ here, and
- CHECK_EQ(nullptr, provider_) in destructor to enforce EventEnd gets called
- CHECK_NE(nullptr, provider_) at the beginning of EventEnd to enforce it's only called once.
Added those checks and clear provider_ on EndEvent. Thanks.
The current iteration of `~MultiEtwPayloadHandler() { CHECK_EQ(nullptr, provider_); }` will start crashing in destructor if an early exit happened in the `EventEnd`.
```cpp
ULONG EventEnd(const EVENT_DESCRIPTOR& event_descriptor) {
if (!is_enabled_) {
return 0;
}
```
Considering `void ETWInterceptor::Delegate::OnTrackEvent(` is returning `void`, can we store the `const EVENT_DESCRIPTOR& event_descriptor` as member variable do the `EventEnd` in the destructor?
```cpp
MultiEtwPayloadHandler(TlmProvider* provider,
std::string_view event_name,
const EVENT_DESCRIPTOR& event_descriptor)
: provider_(provider), event_descriptor_(event_descriptor) {
is_enabled_ = provider_->IsEnabled(event_descriptor_);
if (!is_enabled_) {
return;
}
metadata_index_ = provider_->EventBegin(metadata_, event_name);
}// OTHER STUFF.
~MultiEtwPayloadHandler() { std::ignore = EventEnd(); }
private:
ULONG EventEnd() {
if (!is_enabled_) {
return 0;
}
CHECK_NE(nullptr, provider_);
ULONG ret =
provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],
descriptors_index_, event_descriptor_);
provider_ = nullptr;
return ret;
}
// OTHER STUFF.
EVENT_DESCRIPTOR event_descriptor_;
```
Then we can just remove the call to:
```cpp
etw_payload_handler.EventEnd(event_descriptor);
```
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
Myungsub KimSuggestion: I think we could leverage existing TlmField classes and avoid TlmFieldDebugAnnotation with a variant, by having a function that calls MultiEtwPayloadHandler::WriteField directly:
```
void WriteDebugAnnotationTlmField(
std::string_view name,
perfetto::protos::pbzero::DebugAnnotation_Decoder& annotation,
MultiEtwPayloadHandler& handler) {
if (annotation.has_bool_value()) {
handler.WriteField(TlmIntBoolField(name, annotation.bool_value()));
} else (annotation.has_int_value()) {
handler.WriteField(TlmInt64Field(name, annotation.int_value()));
}
...
else if (annotation.has_string_value()) {
handler.WriteField(TlmMbcsStringField(name, annotation.string_value()));
}
...
}
```
Good suggestion but we can't use std::string_view so we will need new class for it and couple more (pointer and boolean) those are not used anywhere else. Rather keep it under single class as TlmFieldDebugAnnotation.
To me it seems the approach of using temporary like: `handler.WriteField(TlmInt64Field(name, annotation.int_value()));` leads to a problem.
The `FillEventDescriptor` is implemented in the following manner:
```cpp
void TlmInt64Field::FillEventDescriptor(
EVENT_DATA_DESCRIPTOR* descriptors) const noexcept {
EventDataDescCreate(&descriptors[0], (void*)&value_, sizeof(value_));
}
```
So the object `TlmInt64Field` needs to be valid till `EventWrite` is called later, since the pointer to its member `value_` is being used there.
Since the `EventWrite` is getting called outside of `WriteField`, when the temporary `TlmInt64Field` has already been destroyed, the access will happen after the destruction.
Calling the `EventWrite` within `WriteField` will lead to multiple events getting fired.
Seems to me that the following function was deleted in order to ensure that this temporary object case leads to compilation error:
```cpp
// Ensures that this function cannot be called with temporary objects.
template <EtwFieldWithDataDescType T>
void WriteField(const T&& value) = delete;
```
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
uint8_t descriptors_index_ = 2;
absl::InlinedVector<EVENT_DATA_DESCRIPTOR, 6> descriptors_{2};nit: Can consider using a named constant to express what `6` and `2` means:
```cpp
static constexpr int kMaxPossibleDescriptors = 6;
static constexpr int kMinPossibleDescriptors = 2;
uint8_t descriptors_index_ = kMinPossibleDescriptors;
absl::InlinedVector<EVENT_DATA_DESCRIPTOR, kMaxPossibleDescriptors>
descriptors_{kMinPossibleDescriptors};
```
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
absl::InlinedVector<TlmFieldDebugAnnotation, 2> debug__fields;Consider changing to single underscore: `debug_fields`.
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
// Add debug annotations.
// There cannot be more that 2 debug annotations.
absl::InlinedVector<TlmFieldDebugAnnotation, 2> debug__fields;nit: Can consider using a named constant to explain what `2` means:
```cpp
// Add debug annotations.
static constexpr int kMaxDebugAnnotations = 2;
absl::InlinedVector<TlmFieldDebugAnnotation, kMaxDebugAnnotations> debug__fields;
```
Myungsub Kim (Gerrit)
Myungsub Kim added 8 comments![]( "Open in Gerrit")
absl::variant<std::string, uint64_t, int64_t, bool, double> value_;Myungsub KimCould this be string_view? Considering pbzero::DebugAnnotation already has storage for the string and will outlive TlmFieldDebugAnnotation.
Etienne Pierre-DorayIf we can use std::string_view instead of std::string, it would been great as we don't have to copy but TlmField string has to be a null terminated string hence I couldn't use it as DebugAnnotation isn't null terminated.
Ah I see.
There is a size deliminted string TlgInCOUNTEDANSISTRING (found at https://github.com/tpn/winsdk-10/blob/master/Include/10.0.14393.0/shared/TraceLoggingProvider.h)
in_type_ = 23
Could we add that as another TlmFieldWithConstants?
From that header file,
TlgInCOUNTEDANSISTRING means "size-prefixed MBCS char string".
- TlgInBINARY means "size-prefixed binary data".
- TlgInBINARY, TlgInCOUNTEDSTRING, and TlgInCOUNTEDANSISTRING are encoded as
a little-endian UINT16 byte-count (not a char-count) followed by the data.
So new class needs a Value with byte-count and data, string_view needs to be copied. It needs a buffer to hold byte-count and data.
Done
return provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],Thanks. Updated per suggestions.
uint8_t descriptors_index_ = 2;
absl::InlinedVector<EVENT_DATA_DESCRIPTOR, 6> descriptors_{2};nit: Can consider using a named constant to express what `6` and `2` means:
```cpp
static constexpr int kMaxPossibleDescriptors = 6;
static constexpr int kMinPossibleDescriptors = 2;
uint8_t descriptors_index_ = kMinPossibleDescriptors;
absl::InlinedVector<EVENT_DATA_DESCRIPTOR, kMaxPossibleDescriptors>
descriptors_{kMinPossibleDescriptors};
```
Replace with consts.
// Add debug annotations.
// There cannot be more that 2 debug annotations.
absl::InlinedVector<TlmFieldDebugAnnotation, 2> debug__fields;nit: Can consider using a named constant to explain what `2` means:
```cpp
// Add debug annotations.
static constexpr int kMaxDebugAnnotations = 2;
absl::InlinedVector<TlmFieldDebugAnnotation, kMaxDebugAnnotations> debug__fields;
```
Fixed as suggested.
absl::InlinedVector<TlmFieldDebugAnnotation, 2> debug__fields;Consider changing to single underscore: `debug_fields`.
Done
absl::variant<const char*, std::string_view> name_;Chandranath BhattacharyyaThe member can just be `std::string_view name_`
And the constructors becomes```
explicit TlmFieldBase(const char* name) noexcept : name_(name, strlen(name)) {}
```
`string_view` already has a `(const char*)` constructor which does the `strlen` portion. So `name_(name)` should suffice.
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| 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")
absl::variant<std::string, uint64_t, int64_t, bool, double> value_;Myungsub KimCould this be string_view? Considering pbzero::DebugAnnotation already has storage for the string and will outlive TlmFieldDebugAnnotation.
Etienne Pierre-DorayIf we can use std::string_view instead of std::string, it would been great as we don't have to copy but TlmField string has to be a null terminated string hence I couldn't use it as DebugAnnotation isn't null terminated.
Myungsub KimAh I see.
There is a size deliminted string TlgInCOUNTEDANSISTRING (found at https://github.com/tpn/winsdk-10/blob/master/Include/10.0.14393.0/shared/TraceLoggingProvider.h)
in_type_ = 23
Could we add that as another TlmFieldWithConstants?
From that header file,
TlgInCOUNTEDANSISTRING means "size-prefixed MBCS char string".
- TlgInBINARY means "size-prefixed binary data".
- TlgInBINARY, TlgInCOUNTEDSTRING, and TlgInCOUNTEDANSISTRING are encoded as
a little-endian UINT16 byte-count (not a char-count) followed by the data.So new class needs a Value with byte-count and data, string_view needs to be copied. It needs a buffer to hold byte-count and data.
Reading the implementation of TraceLoggingAnsiString(), I think we can use TlgInCOUNTEDANSISTRING. The 'prefixed' size can be emitted as a distinct call to EventDataDescCreate (with a different address in memory):
Something like this:
```
// Class that represents an event field containing MBCS data
class BASE_EXPORT TlmMbcsCountedStringField
: public TlmFieldWithConstants<2, 23> // 2 data descriptor, Type =
// TlgInCOUNTEDANSISTRING
{
public:
// value is MBCS string (assumed to be in system's default code
// page).
TlmMbcsCountedStringField(const char* name, string_view value) noexcept
: TlmFieldWithConstants(name),
value_(value.data()),
size_(value.size()) {}
string_view Value() const noexcept;
void FillEventDescriptor(EVENT_DATA_DESCRIPTOR* descriptors) const noexcept {
EventDataDescCreate(&descriptors[0], &size_, 2);
EventDataDescCreate(&descriptors[1], value_, value_.size());
}private:
const char* value_;
uint16_t size_;
};
```
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
Hey, that's a good idea of using two descriptors approach, that is going to work for std::string_view. However, we still need to address other data types as they are also writing only their address so unless we have those data saved somewhere until EventEnd() is called, they will be lost. So, I can't separate them into individual classes, but I might change std::string to std::string_view with the existing code. Will that be Ok with you?
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Ah right, you'd still need a container for all the fields.
At least we can avoid copying into a std::string doing this, sounds good!
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
absl::variant<std::string, uint64_t, int64_t, bool, double> value_;Just tried out with two descriptors but it did not work, etw event created with this method is erroring out on Windows side. I need to figure out. If it's okay, I'd like to check in the current iteration and take more time to figure it out.
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
Myungsub Kim (Gerrit)
I looked little more, it turns out we can't use 2 descriptors idea. When you have 2, you are supposed to give out two data points, So it will not make a string counnted one. You are making two different data points instead of combined one.
Each data in the descriptor is a pointer to copy the data from. So it will not be the desired data we want. (string length + string),
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chandranath Bhattacharyya (Gerrit)
Chandranath Bhattacharyya added 1 comment![]( "Open in Gerrit")
ULONG EventEnd() {
if (!is_enabled_) {
return 0;
}
CHECK_NE(nullptr, provider_);
ULONG ret =
provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],
descriptors_index_, event_descriptor_);
provider_ = nullptr;
return ret;
}It will be good to move this function to private.
Related details
- Bruce Dawson
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 1 comment![]( "Open in Gerrit")
ULONG EventEnd() {
if (!is_enabled_) {
return 0;
}
CHECK_NE(nullptr, provider_);
ULONG ret =
provider_->EventEnd(metadata_, metadata_index_, &descriptors_[0],
descriptors_index_, event_descriptor_);
provider_ = nullptr;
return ret;
}It will be good to move this function to private.
Move to private and also removed the DCHECK and cleaning _provider as it's no longer accessible,
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Etienne Pierre-Doray
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 2 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
2 comments
What do you get as error? Make sure TlmFieldDebugAnnotation::data_desc_count_ == 2.
I'm ok with landing as-is and iterate in a follow-up.
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Joe Laughlin
- Myungsub Kim
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim added 2 comments![]( "Open in Gerrit")
Thanks for the sign off. Off to submit...
Thanks for the review and sign off, I set it as 2. The issue is that, the descriptor isn't the target destination. It holds pointers to the address of data. So the end result isn't as expected like we were hoping such as (size + string data). Instead, Windows fails to copy the string as the first word on that address isn't size of the string. The test app I used to listening etw events stop responding with that field data.
Related details
- Bruce Dawson
- Chandranath Bhattacharyya
- Chris Davis
- Joe Laughlin
- Todd Reifsteck
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Myungsub Kim (Gerrit)
Myungsub Kim voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
etw payload support
This patch adds support to emit legacy trace arguments into ETW events
by converting these arguments into additional ETW fields.
Implementation:
TlmFieldBase class was renamed to TlmFieldWithConstants class and
new TlmFieldBase was added to handle various TlmField classes.
TlmFieldDebugAnnotation class was added to handle legacy trace macro payloads to convert
to etw field data.
This class will generate various etw data unlike other TlmFieldClass
that handles a specific data type.
Delegate for TrackEventStateTracker was modified to take SequenceState
to access debug annotation name from it.
This was done without modifying Perfetto SDK but perhaps we can change the SDK
to pass SequenceState directly to ETWInterceptor::Delegate::OnTrackEvent() instead.
- M base/trace_event/etw_interceptor_win.cc
- M base/trace_event/trace_logging_minimal_win.cc
- M base/trace_event/trace_logging_minimal_win.h
Code-Review: +1 by Etienne Pierre-Doray, +1 by Eric Seckler
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |