TaskAttribution: Add BlinkTaskState trace events for microtasks [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
TaskAttributionTracker::MicrotaskTraceScope scope(self->isolate_);Can you run speedometer and make sure there is no regression?
Related details
- Michal Mocny
- 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 added 1 comment![]( "Open in Gerrit")
TaskAttributionTracker::MicrotaskTraceScope scope(self->isolate_);Can you run speedometer and make sure there is no regression?
Sure, kicked off that and Jetstream. Note: I've tried to make this do very little if the tracing category isn't enabled, hopefully it's close enough to a no-op in that case.
Related details
- Dave Tapuska
- Michal Mocny
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf-pgo/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1457a488510000
Related details
- Dave Tapuska
- Michal Mocny
- 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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf-pgo/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/167b2588510000
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
TaskAttributionTracker::MicrotaskTraceScope scope(self->isolate_);Scott HaseleyCan you run speedometer and make sure there is no regression?
Sure, kicked off that and Jetstream. Note: I've tried to make this do very little if the tracing category isn't enabled, hopefully it's close enough to a no-op in that case.
Speedometer results are neutral: https://pinpoint-dot-chromeperf.appspot.com/job/1457a488510000.
Jetstream shows -0.1 in score, but not statistically significant (and it's less than the 0.3 threshold). I'm assuming this is noise. The only thing showing stat sig are a couple improvements, which I'd also assume is noise. https://pinpoint-dot-chromeperf.appspot.com/job/167b2588510000
Combined with this being ~ a no-op, this looks all looks neutral to me.
Related details
- Dave Tapuska
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 3 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
3 comments
So fun using this locally!
into EventLoop for internal microtasks. The PromiseHook API is not
cheap, so we don't want to enable this all the time, so we only registerIs not, or may not?
Seems benchmarks suggest its okay given the way you use it? Or were the benchmarks just for the case where the expensive parts are disabled?
(Would it be hard to try benchmark with the feature enabled?)
// a task runner. Note that it's safe to remove a non-existent observer.Nit: Could you move the comment for "safe to remove" to destructor?
I think you should just document the conditions for registration here, and then the note about conditions not matching below
Related details
- Dave Tapuska
- 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 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
So fun using this locally!
Thanks for testing it!
into EventLoop for internal microtasks. The PromiseHook API is not
cheap, so we don't want to enable this all the time, so we only registerIs not, or may not?
Seems benchmarks suggest its okay given the way you use it? Or were the benchmarks just for the case where the expensive parts are disabled?
(Would it be hard to try benchmark with the feature enabled?)
I meant it'd be expensive if we were to unconditionally register a PromiseHook, but this is mitigated by putting it behind a tracing category. Cleaned up the comment a bit.
// a task runner. Note that it's safe to remove a non-existent observer.Nit: Could you move the comment for "safe to remove" to destructor?
I think you should just document the conditions for registration here, and then the note about conditions not matching below
SGTM. I lifted this from MainThreadSchedulerImpl and didn't update :).
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
Nate: would you mind taking a pass?
Related details
- Dave Tapuska
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted Code-Review+1![]( "Open in Gerrit")
- Dave Tapuska
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
+Etienne for tracing
+Leszek for PromiseHooks usage
Related details
- Dave Tapuska
- Etienne Pierre-Doray
- Leszek Swirski
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 and added 3 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
3 comments
TRACE_EVENT_BEGIN("synchronous" TRACE_EVENT_BEGIN/END is hard to get right because they must nest correctly w.r.t. *all* other trace events on the same thread (otherwise traces behave weird)
Makes sure this is always the case; I'm guessing it is the case because we're at the scheduler level but I just want to flag this.
Otherwise consider creating a specific track for these events.
bool enabled;
TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTracingCategory, &enabled);
return enabled;Nit: this is the legacy one, prefer:
`return TRACE_EVENT_CATEGORY_ENABLED(kTracingCategory)`
Related details
- Dave Tapuska
- Leszek Swirski
- 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 added 2 comments![]( "Open in Gerrit")
"synchronous" TRACE_EVENT_BEGIN/END is hard to get right because they must nest correctly w.r.t. *all* other trace events on the same thread (otherwise traces behave weird)
Makes sure this is always the case; I'm guessing it is the case because we're at the scheduler level but I just want to flag this.
Otherwise consider creating a specific track for these events.
"synchronous" TRACE_EVENT_BEGIN/END is hard to get right because they must nest correctly w.r.t. *all* other trace events on the same thread (otherwise traces behave weird)
Yep, I've definitely seen this issue before. And I'm generally hesitant to use them for that reason.
Makes sure this is always the case; I'm guessing it is the case because we're at the scheduler level but I just want to flag this.
Otherwise consider creating a specific track for these events.
Yeah I gave this some consideration and thought it should be fine because:
1. Here, there's an existing one that works fine that's being converted (it's lifetime matches an on-stack scoped object (`TaskScope`).
2. The addition in this CL is the for each individual microtask, start = start of microtask / end = end of microtask, and I think the scoping to that scheduling unit will work, as I'd assume any async traces starting in the middle of a microtask and not ending would be broken. IOW I think this is equivalent to adding a scoped TRACE_EVENT around the invoke-microtask method (which we can't access here). We've been using this locally w/ all categories enabled, and it's working very well.
One way we've been using this is to "visualize the arg values" and comparing those with the other work on the main thread, so there is an advantage to having this on the main thread track. If we run into any snags though, I'll move this.
bool enabled;
TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTracingCategory, &enabled);
return enabled;Nit: this is the legacy one, prefer:
`return TRACE_EVENT_CATEGORY_ENABLED(kTracingCategory)`
Ooh nice, thanks.
Related details
- Dave Tapuska
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
isolate_->SetPromiseHook(TaskAttributionPromiseHook);This is something to be careful about -- `Isolate::SetPromiseHook` only allows one promise hook, and we expect the embedder to handle dispatching to multiple hooks, so if some other Blink code wanted to set promise hooks, they would conflict with each other. I don't see any other uses of `SetPromiseHook` in Blink at the moment, but it might be something to think about.
Related details
- Dave Tapuska
- 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 added 1 comment![]( "Open in Gerrit")
isolate_->SetPromiseHook(TaskAttributionPromiseHook);This is something to be careful about -- `Isolate::SetPromiseHook` only allows one promise hook, and we expect the embedder to handle dispatching to multiple hooks, so if some other Blink code wanted to set promise hooks, they would conflict with each other. I don't see any other uses of `SetPromiseHook` in Blink at the moment, but it might be something to think about.
Thanks, yeah, it's similar to CPED. I also checked usage beforehand and thought about this. If we need to, we can add a management layer on top, but given performance concerns, we might not be able to add other hooks.
I'll follow up and add a check in audit_non_blink_usage.py -- there's a similar one there for CPED for the same reason.
Related details
- Dave Tapuska
- Leszek Swirski
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Leszek Swirski
- 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 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
1 comment
isolate_->SetPromiseHook(TaskAttributionPromiseHook);Scott HaseleyThis is something to be careful about -- `Isolate::SetPromiseHook` only allows one promise hook, and we expect the embedder to handle dispatching to multiple hooks, so if some other Blink code wanted to set promise hooks, they would conflict with each other. I don't see any other uses of `SetPromiseHook` in Blink at the moment, but it might be something to think about.
Thanks, yeah, it's similar to CPED. I also checked usage beforehand and thought about this. If we need to, we can add a management layer on top, but given performance concerns, we might not be able to add other hooks.
I'll follow up and add a check in audit_non_blink_usage.py -- there's a similar one there for CPED for the same reason.
I'll add the usage check in a separate CL.
Related details
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley 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
12 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
TaskAttribution: Add BlinkTaskState trace events for microtasks
This CL primarily adds trace events with task state information for
microtasks, so we can get the task_state_id. We already output this for
async task propagation when creating a TaskScope, e.g. postMessage,
setTimeout propagation, but don't for microtasks since this is handled
by v8. This will be valuable to diagnose soft nav detection issues
related to task attribution.
To add these trace events, we use v8's PromiseHook API to get callbacks
before and after running Promise-related microtask, as well as add hooks
into EventLoop for internal microtasks. Unconditionally registering a
PromiseHook would likely be too expensive, so we mitigate this by only
registering the hook when tracing is enabled for the relevant category,
and we only check this at construction time and when tracing is turned
on (via AsyncEnabledStateObserver).
This also unifies the existing trace event, renaming BlinkTaskScope to
BlinkTaskState, and moving this to a new "task_attribution" trace
category (given the additional PromiseHook overhead and main use case, I
wanted to separate this from "scheduler", which has other use cases).
- M base/trace_event/builtin_categories.h
- M base/tracing/protos/chrome_track_event.proto
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.h
- M third_party/blink/renderer/platform/scheduler/common/event_loop.cc
- M third_party/blink/renderer/platform/scheduler/public/task_attribution_tracker.h
Code-Review: +1 by Nate Chapin, +1 by Michal Mocny, +1 by Etienne Pierre-Doray, +1 by Dave Tapuska
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |