[iOS Tracing] Add async trace events that track JS calls [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
perfetto::Category("webkit"),Please add a description of the category with SetDescription()
uint64_t GetNextJSExecutionTraceId() {
DCHECK([NSThread isMainThread]); // Ensure non-atomic increment is safe.
static uint64_t g_trace_id = 0;
return ++g_trace_id;
}
Nit: you could use base::trace_event::GetNextGlobalTraceId()
TRACE_EVENT_END("webkit",
perfetto::NamedTrack("Injected Script", trace_id));This runs after TRACE_EVENT_BEGIN() below right? (I'm a little confused with objective-C syntax)
TRACE_EVENT_BEGIN("webkit", perfetto::DynamicString(location.ToString()),You can use perfetto::StaticString since this is a compile time string IUUC.
To make sure this works for field traces.
Here and below.
Related details
- Justin Novosad
- Mark Cogan
- Rohit Rao
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. |
Justin Novosad (Gerrit)
Justin Novosad added 4 comments![]( "Open in Gerrit")
Please add a description of the category with SetDescription()
Done
uint64_t GetNextJSExecutionTraceId() {
DCHECK([NSThread isMainThread]); // Ensure non-atomic increment is safe.
static uint64_t g_trace_id = 0;
return ++g_trace_id;
}
Nit: you could use base::trace_event::GetNextGlobalTraceId()
Done
TRACE_EVENT_END("webkit",
perfetto::NamedTrack("Injected Script", trace_id));This runs after TRACE_EVENT_BEGIN() below right? (I'm a little confused with objective-C syntax)
Yes, exactly. This is the Obj-C equivalent of a C++ lambda.
TRACE_EVENT_BEGIN("webkit", perfetto::DynamicString(location.ToString()),You can use perfetto::StaticString since this is a compile time string IUUC.
To make sure this works for field traces.
Here and below.
Unfortunately base::Location::ToString() returns a temporary string. I did not realize that dynamic strings don't work for field traces. This might not be the ideal approach then. I need to think more about this...
Related details
- Etienne Pierre-Doray
- Mark Cogan
- Rohit Rao
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 added 1 comment![]( "Open in Gerrit")
TRACE_EVENT_BEGIN("webkit", perfetto::DynamicString(location.ToString()),Justin NovosadYou can use perfetto::StaticString since this is a compile time string IUUC.
To make sure this works for field traces.
Here and below.
Unfortunately base::Location::ToString() returns a temporary string. I did not realize that dynamic strings don't work for field traces. This might not be the ideal approach then. I need to think more about this...
You could fill a proto with this (which eventually shows up the in event args) similar to
https://source.chromium.org/chromium/chromium/src/+/main:base/task/common/task_annotator.cc;l=263?q=base%20task%20annotator%20location&ss=chromium
Either by reusing task_execution, or creating a new field, but then you'd need to come up with another more static name.
Related details
- Justin Novosad
- Mark Cogan
- Rohit Rao
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. |
Mark Cogan (Gerrit)
Mark Cogan added 1 comment![]( "Open in Gerrit")
- Justin Novosad
- Mike Dougherty
- Rohit Rao
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. |
Justin Novosad (Gerrit)
Justin Novosad added 1 comment![]( "Open in Gerrit")
TRACE_EVENT_BEGIN("webkit", perfetto::DynamicString(location.ToString()),Justin NovosadYou can use perfetto::StaticString since this is a compile time string IUUC.
To make sure this works for field traces.
Here and below.
Etienne Pierre-DorayUnfortunately base::Location::ToString() returns a temporary string. I did not realize that dynamic strings don't work for field traces. This might not be the ideal approach then. I need to think more about this...
You could fill a proto with this (which eventually shows up the in event args) similar to
https://source.chromium.org/chromium/chromium/src/+/main:base/task/common/task_annotator.cc;l=263?q=base%20task%20annotator%20location&ss=chromiumEither by reusing task_execution, or creating a new field, but then you'd need to come up with another more static name.
Okay, that worked, and it was pretty easy to re-use that same proto. Thanks! The trace graph is slightly less convenient to read this way because it now uses a static event name, but all the info we care about is still there. It's a good compromise IMHO. Also, there's no observable impact on record time performance (<microsecond). I imagine the trace buffer is significantly more compact in memory this way, but I'm not sure how to measure that.
Related details
- Etienne Pierre-Doray
- Mike Dougherty
- Rohit Rao
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. |
Mike Dougherty (Gerrit)
Mike Dougherty added 1 comment![]( "Open in Gerrit")
There is a lot of piping taking place in this CL and no context in the associated bug. Is it possible to provide some additional context about why this is necessary?
Additionally, is it possible to land the core of the change (within ios/web/js_messaging/web_view_js_utils.mm) first? And then follow-up with a few smaller CLs to improve the granularity of the `location` value?
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Rohit Rao
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. |
Justin Novosad (Gerrit)
Justin Novosad added 3 comments![]( "Open in Gerrit")
There is a lot of piping taking place in this CL and no context in the associated bug. Is it possible to provide some additional context about why this is necessary?
Additionally, is it possible to land the core of the change (within ios/web/js_messaging/web_view_js_utils.mm) first? And then follow-up with a few smaller CLs to improve the granularity of the `location` value?
Done
I split this CL into 2. This part now only adds the trace events logic.
TRACE_EVENT_BEGIN("webkit", perfetto::DynamicString(location.ToString()),Justin NovosadYou can use perfetto::StaticString since this is a compile time string IUUC.
To make sure this works for field traces.
Here and below.
Etienne Pierre-DorayUnfortunately base::Location::ToString() returns a temporary string. I did not realize that dynamic strings don't work for field traces. This might not be the ideal approach then. I need to think more about this...
Justin NovosadYou could fill a proto with this (which eventually shows up the in event args) similar to
https://source.chromium.org/chromium/chromium/src/+/main:base/task/common/task_annotator.cc;l=263?q=base%20task%20annotator%20location&ss=chromiumEither by reusing task_execution, or creating a new field, but then you'd need to come up with another more static name.
Okay, that worked, and it was pretty easy to re-use that same proto. Thanks! The trace graph is slightly less convenient to read this way because it now uses a static event name, but all the info we care about is still there. It's a good compromise IMHO. Also, there's no observable impact on record time performance (<microsecond). I imagine the trace buffer is significantly more compact in memory this way, but I'm not sure how to measure that.
- Etienne Pierre-Doray
- Mike Dougherty
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Dougherty (Gerrit)
Mike Dougherty added 5 comments![]( "Open in Gerrit")
I added a few nits but in general I don't feel like I know enough about perfetto to provide great feedback on this overall. I don't love how it needs to propagate the location manually through all the layers in the follow-up CL, but maybe that's just how tracing works on other platforms too?
const base::Location& location = FROM_HERE);After the follow-up lands, would be it a good idea to remove the `FROM_HERE` default to ensure call sites pass in their own value of location?
typedef void (^ExecuteJavaScriptCompletionHandler)(id, NSError*);I don't think we need this typedef as it just adds an extra indirection for readers trying to understand which params are passed to the completion handler.
perfetto::NamedTrack("Injected Script", trace_id),I would recommend the term "Executed" or "Evaluated" instead. We use the term "Injected" to refer to JS that is added to WebFrames on webpage load, which is a specific type of JavaScript execution.
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Rohit Rao
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. |
Justin Novosad (Gerrit)
Justin Novosad added 5 comments![]( "Open in Gerrit")
I added a few nits but in general I don't feel like I know enough about perfetto to provide great feedback on this overall. I don't love how it needs to propagate the location manually through all the layers in the follow-up CL, but maybe that's just how tracing works on other platforms too?
This is really to address an iOS-specific problem. With Blink+V8 we don't do anything like this to propagate locations because Blink-based Chrome doesn't do much hard-coded script injection. The preferred approach with Blink is to use built-in browser extensions, which are trackable by other means.
After the follow-up lands, would be it a good idea to remove the `FROM_HERE` default to ensure call sites pass in their own value of location?
Yeah, that's a reasonable idea. I'll just have to check whether all call sites are indeed API wrappers. If there are places that use this directly, then maybe not.
typedef void (^ExecuteJavaScriptCompletionHandler)(id, NSError*);I don't think we need this typedef as it just adds an extra indirection for readers trying to understand which params are passed to the completion handler.
Done
#import <string_view>Justin Novosadis this necessary?
Done
I would recommend the term "Executed" or "Evaluated" instead. We use the term "Injected" to refer to JS that is added to WebFrames on webpage load, which is a specific type of JavaScript execution.
- Etienne Pierre-Doray
- Mike Dougherty
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Dougherty (Gerrit)
Mike Dougherty voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike Dougherty
| Code-Review | +1 |
1 comment
OWNERS lgtm, but please ensure you get someone familiar with TRACE_EVENT_* to review all files as well before landing
Related details
- Etienne Pierre-Doray
- Justin Novosad
- Rohit Rao
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 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
1 comment
- Justin Novosad
- Rohit Rao
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. |
Justin Novosad (Gerrit)
Justin Novosad added 1 comment![]( "Open in Gerrit")
OWNERS lgtm, but please ensure you get someone familiar with TRACE_EVENT_* to review all files as well before landing
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
8 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[iOS Tracing] Add async trace events that track JS calls
This change adds a tracing track on iOS that records the durations of
javascript execution tasks that are dispatched to webkit.
The tracing was added to the ExecuteJavaScript and
ExecuteAsyncJavaScript functions in web_view_js_utils.mm, which are the
entrypoints that all other JS execution APIs use under the hood.
- M base/trace_event/builtin_categories.h
- M ios/web/js_messaging/web_view_js_utils.h
- M ios/web/js_messaging/web_view_js_utils.mm
Code-Review: +1 by Etienne Pierre-Doray, +1 by Mike Dougherty
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |