Report initiator url for resources fetched in event handler. [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
// Currently, resource timing context is only propagated on the main thread.Guohui DengThis block and the one below should be flag-guarded.
We'll want to check the overhead of this too on speedometer (with the feature enabled. I sometimes create a separate dependent branch enabling the feature to test this). I'm not sure if you have access to pinpoint, but the instructions are https://chromium.googlesource.com/chromium/src/+/main/docs/speed/perf_trybots.md (although it's speedometer3 now, not too). I'm happy to kick off a job for this too. I _think_ this should be okay, but we'll want to evaluate.
Added flag guard. Sorry I forgot this again and thank you for your patience!
Regarding the test: I can sign in and open the "run a try job" page so it looks like I have access! I will try to start the test later and if I run into problems I will ask for help. 😊
std::optional<scheduler::TaskAttributionTracker::TaskScope>Guohui DengNote: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
From the perspective of "initiator url" I much prefer propagating the "resource timing context" from script that sets up the handler, i.e., take the "context" at the "registration" (more details below). When I saw the planning of the AsyncContext I noticed they prefer propagating the "variables" along the script execution flow -- which makes sense for other applications of TaskAttribution(like soft navigation) and AsyncContext. I vaguely remembered I asked if asyncContext can accommodate this different propagating flow, and the answer was "yes with some extra work". So I hope this won't cause too much pain for you guys. If it adds too much work I will be happy to change. (maybe speaking with WebPerf WG too)
//////////////
The reason I prefer the current approach is that, the typical use case is that we "predict" the next resource fetch by previous resource that is fetched. In addition, the prediction is never reliable, because of the limitations that we have. (one noticeable limitation is that we can only pick up one script, instead of both the script that caused the event and the script that registered the handler). I think it's advantageous to report only "the script that registers the handler" because:
a) "registration script" would be expected to happen sooner. Typically, once the handler is registered, we can expect that the resource fetched in the handler can happen at any time.
b) "registration script" always exists. Events can result from user (and I would guess this is not a small portion) so that no "initiator" caused the event. Even if we make a "fallback" to report the "registering", then more different "initiator"s can be reported for the same resource and make the prediction more difficult. In addition, I prefer simpler definition.
What do you think?
class RegisteredEventListener finalGuohui DengIs this needed for `NativeEventListener` as well as JS event listeners? Feels like `JSBasedEventListener` or `JSEventListener` might be a better place for this?
It should be indeed in `JSBasedEventListener`. I made changes accordingly.
Related details
- 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 3 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Can all of this move into JSBasedEventListener, i.e. the constructor?
Aside from improving encapsulation, I think it might be required anyway for event attributes, e.g. `onfoo` for event "foo", when changing the listener. E.g.
```
document.onfoo = listener1;
// ... sometime later ...
document.onfoo = listener2;
```
In that case, IIUC, we update the callback in the existing registered listener and don't go down this code path. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/events/event_target.cc;l=822-825;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1
std::optional<scheduler::TaskAttributionTracker::TaskScope>Guohui DengNote: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
From the perspective of "initiator url" I much prefer propagating the "resource timing context" from script that sets up the handler, i.e., take the "context" at the "registration" (more details below). When I saw the planning of the AsyncContext I noticed they prefer propagating the "variables" along the script execution flow -- which makes sense for other applications of TaskAttribution(like soft navigation) and AsyncContext. I vaguely remembered I asked if asyncContext can accommodate this different propagating flow, and the answer was "yes with some extra work". So I hope this won't cause too much pain for you guys. If it adds too much work I will be happy to change. (maybe speaking with WebPerf WG too)
//////////////
The reason I prefer the current approach is that, the typical use case is that we "predict" the next resource fetch by previous resource that is fetched. In addition, the prediction is never reliable, because of the limitations that we have. (one noticeable limitation is that we can only pick up one script, instead of both the script that caused the event and the script that registered the handler). I think it's advantageous to report only "the script that registers the handler" because:a) "registration script" would be expected to happen sooner. Typically, once the handler is registered, we can expect that the resource fetched in the handler can happen at any time.
b) "registration script" always exists. Events can result from user (and I would guess this is not a small portion) so that no "initiator" caused the event. Even if we make a "fallback" to report the "registering", then more different "initiator"s can be reported for the same resource and make the prediction more difficult. In addition, I prefer simpler definition.What do you think?
w.r.t extra work, it's probably fine -- but long term it might depend on how AsyncContext gets implemented. Right now, we're setting up the context (for propagation) before dispatching the event, and you're hooking into a point after that, so it works. If we change this to also hook into events, we'll just have to make sure we get the ordering right.
---
I don't have a strong opinion on the feature itself, just want to make sure it's been thought through. But this should probably be documented in the explainer and discussed at WebPerfWG at some point (doesn't block this IMO).
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Similarly here, can this move into the js_based_event_listener? There's an `InvokeInternal()` method that runs to actually invoke the listener, and you could hook in there. You might need to create a helper method in the common base class to create the TaskScope.
Related details
- Guohui Deng
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Can all of this move into JSBasedEventListener, i.e. the constructor?
Aside from improving encapsulation, I think it might be required anyway for event attributes, e.g. `onfoo` for event "foo", when changing the listener. E.g.
```
document.onfoo = listener1;// ... sometime later ...
document.onfoo = listener2;
```In that case, IIUC, we update the callback in the existing registered listener and don't go down this code path. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/events/event_target.cc;l=822-825;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1
Done
std::optional<scheduler::TaskAttributionTracker::TaskScope>Guohui DengNote: This is a divergence from task attribution, and I'm not sure if you want that or not in all cases.
This implements "registration" semantics, where context is captured during at event listener registration time. The alternative is "Dispatch" semantics follow execution flow, e.g. an event fires async because an a certain API was called, e.g. XHR. Sometimes these are the same, sometimes they're different. (see also https://github.com/tc39/proposal-async-context/blob/master/WEB-INTEGRATION.md#events)
Task Attribution has instrumented some events using dispatch context, e.g. XHR events, some filesystem events, postMessage/MessageChannel, etc. In those cases, you might be overwriting the context with a different value, and I'm not sure if that's what you'll want or not. One option is to use your approach as a fallback if the context isn't set.
Unfortunately how AsyncContext (and ultimately Task Attribution) will handle events is a WIP and a bit of a moving target, so I'm not sure how you'll want to handle that or what best fits this API. This is a bit of a unique situation (for soft navs too) where we're building a feature on top of something else that isn't stable, and could be worth discussion more offline about timelines, etc.
Scott HaseleyFrom the perspective of "initiator url" I much prefer propagating the "resource timing context" from script that sets up the handler, i.e., take the "context" at the "registration" (more details below). When I saw the planning of the AsyncContext I noticed they prefer propagating the "variables" along the script execution flow -- which makes sense for other applications of TaskAttribution(like soft navigation) and AsyncContext. I vaguely remembered I asked if asyncContext can accommodate this different propagating flow, and the answer was "yes with some extra work". So I hope this won't cause too much pain for you guys. If it adds too much work I will be happy to change. (maybe speaking with WebPerf WG too)
//////////////
The reason I prefer the current approach is that, the typical use case is that we "predict" the next resource fetch by previous resource that is fetched. In addition, the prediction is never reliable, because of the limitations that we have. (one noticeable limitation is that we can only pick up one script, instead of both the script that caused the event and the script that registered the handler). I think it's advantageous to report only "the script that registers the handler" because:a) "registration script" would be expected to happen sooner. Typically, once the handler is registered, we can expect that the resource fetched in the handler can happen at any time.
b) "registration script" always exists. Events can result from user (and I would guess this is not a small portion) so that no "initiator" caused the event. Even if we make a "fallback" to report the "registering", then more different "initiator"s can be reported for the same resource and make the prediction more difficult. In addition, I prefer simpler definition.What do you think?
w.r.t extra work, it's probably fine -- but long term it might depend on how AsyncContext gets implemented. Right now, we're setting up the context (for propagation) before dispatching the event, and you're hooking into a point after that, so it works. If we change this to also hook into events, we'll just have to make sure we get the ordering right.
---
I don't have a strong opinion on the feature itself, just want to make sure it's been thought through. But this should probably be documented in the explainer and discussed at WebPerfWG at some point (doesn't block this IMO).
got it. I will take care of these. Thanks.
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {Similarly here, can this move into the js_based_event_listener? There's an `InvokeInternal()` method that runs to actually invoke the listener, and you could hook in there. You might need to create a helper method in the common base class to create the TaskScope.
Done. As discussed, I moved the code to be besides "InvokeInternal()" and made additional tests for this.
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |