Record timing info for promise resolve/reject and callback #1400
Conversation
|
Maybe I lack the context, but having this in Web IDL covers a lot more than animation frames, right? Are the standards positions and implementation bugs actually cover the IDL layer change? |
It does not, this is the shim between WebIDL and long animation frames. For long animation frames to report script timing it needs to know when particular scripts are enabled. This doesn't add anything web-observable to WebIDL directly.
The explainers included the observable effect of this change as exposed by long animation frames from the start. |
|
@annevk PTAL? |
There was a problem hiding this comment.
Given https://w3c.github.io/long-animation-frames/#webidl-monkey-patches this seems incomplete? There's also numerous other ways that give you some kind of fulfilled promise this doesn't capture?
Should probably also be reviewed by @sefeng211.
Yes, there's one more monkey patch which I'm going to handle separately as it's a bit more involved.
You mean "create a resolved promise" etc? Those don't need to be recorded as they don't resolve a promise asynchronously in a task. Or do you mean something else? |
|
How do you know "resolve" or "reject" is called from a (newly-created) task? That doesn't seem like a safe assumption. |
To be more specific, this is not about it being called from a task but rather being a script entry point (which is usually a task). I guess though that this is a valid point as a spec can create a resolved promise and then react to it which would have the same effect. Perhaps a better place for this is |
|
@annevk I looked at this again and this is not an issue.
If I missed one of the paths that runs a user script I'm happy to correct this but I couldn't so far so for the extent of my research this PR is correct. |
|
By that logic, how does resolve trigger an author-provided script? |
It's a promise that was created before and already had an author-provided |
|
I think I need a higher-level explanation of the objectives here in order to be able to review. It's not entirely clear to me what is being measured. |
Sure! What we're trying to measure here is the script entry point: from the moment an author script (callback, event-listener, promise resolution, script evaluation) started running until it has a clear stack and all the related microtasks have been executed. This should give authors a hint regarding hot-spots in their code that cause long animation frames (and potential jank as a result). The start and end of an "entry point" are the places are equivalent to these spots in the HTML spec:
The time spent between those two points are "time spent in JS execution". So instead, the "start" of the entry point is recorded at the locations that can trigger script execution, and the "end" is recorded at the aforementioned "cleanup" location. Does this make more sense? |
|
A thing I don't understand is that promise resolution isn't a guarantee that script will run. And there's also no guarantee that when a specification calls resolve/reject the JS stack is empty. Why isn't this done as part of HTML's script running algorithms? It seems that already has the appropriate start and end points. @smaug---- have you looked at this? |
|
Example 1: fetch(url).then(() => { ... })If Example 2: // This might return an already resolved promise
const text = response.text()
setTimeout(() => do_something_with(await text))If
True, the worth that can happen though is that a promise resolution that takes as long time would be counted as if it was an author script.
Those checks are done inside the Long Animation Frame timing spec. If there is already a running entry point, we return early (see https://w3c.github.io/long-animation-frames/#create-script-entry-point #6). The end point is anyway called directly from HTML here when we clear the mictorask queue.
The "end" point called from HTML (see above). |
|
Hmm, but what if a task is used to fire an event and resolve a promise? You'd get two separate measurements if both are "reacted" to? Wouldn't it be more accurate to annotate the tasks? |
Separating them to script entry points is much more actionable, especially since this is in the context of a single long animation frame (which is a task + the rendering update)... e.g. if the event handler takes 30ms and the function you put in |
|
They'd both get attributed to the event handler as the JS stack will be non-empty by the time you get to the promise though. I think it also gets confusing if you resolve multiple promises in a single task. I don't like using "resolve" and "reject" as entry points coupled with the JS stack being empty as in my mind that's very much against the way promises are designed. Tying this directly to tasks (that may execute script) seems much more natural. |
That's not true, as firing an event clears the microtask queue (in "cleanup after script"). Each of those calls would be totally separate.
Sure, though that's a rare case.
See above, this would lose important information if you do other things in that task. |
You're assuming the event is dispatched before the promise is resolved? I think I'd always want to resolve the promise first, but that's a good point and something to be careful about. |
We actually had issues like this with view-transitions, and had to wrap the "resolve the promise" call with a prepare/cleanup to avoid this jumbling of execution order... (see https://html.spec.whatwg.org/multipage/browsing-the-web.html#reveal step 5). In most cases today there is only one "resolve a promise" in a task, and in the other cases I'd really recommend to do the above... The cases where "resolve a promise" calls blend their microtasks with other things your task is doing create an order-of-operation confusion and those are the exact same places where this type of measurement would also be confusing. |
Preview | Diff