More explicit representation of script fetch parallelism #1225
Conversation
and the bundled queryFeatureSupport('*'). This also includes
fixes to some bugs in parsing of these URLs (some of the
checks were missing).
there was another comment about the same line.
| |ig|'s [=interest group/bidding url=]. | ||
| 1. Let |biddingScript| be the result of [=waiting for script body from a fetcher=] given |
There was a problem hiding this comment.
Is this the right way to describe a non-blocking wait on a fetch? Reading the stuff this and the above line link to, it sounds like we queue a task off thread...and then synchronously wait for it to complete on the main thread.
There was a problem hiding this comment.
You're correct --- it doesn't change anything at this call sight [1]. Seller scripts are improved to:
- Start one fetch early, not have one every time we want to call scoreAd() (!)
- Have some asynchrony/overlap in timing between the fetch start and use.
... the plan to get both of those for bidders is to basically make a giant map and fetch all the scripts, as a separate change.
...but to fully fix parallelism, we need to make the actual bidding happen all in parallel, too, so the blocking happens off-thread. Which is tricky because of shared data structures.
My immediate interest was in making the dependency on the headers for cross-origin trusted seller signals stuff more explicit.
There was a problem hiding this comment.
...but to fully fix parallelism,
So the implementation has parallelism, the spec currently doesn't adequately describe it?
spec.bs
Outdated
|
|
||
| <dl dfn-for="script fetcher"> | ||
| : <dfn>script body</dfn> | ||
| :: A [=byte sequence=], null or failure. Initially null. The body of the script. |
There was a problem hiding this comment.
Comma after null. At least the Oxford comma seems to be pretty universal, at this point?
| 1. Otherwise, set set |fetcher|'s [=script fetcher/script body=] to |responseBody|. | ||
| 1. Let |failureSteps| be a set of steps that take an [=exception=] <var ignore>e</var>, and | ||
| perform the following: | ||
| 1. Set set |fetcher|'s [=script fetcher/script body=] to failure. |
There was a problem hiding this comment.
I can't figure out if fetch calls processResponse on errors. The fetch spec never actually mentions calling fetch/processResponse, just setting it / passing it as an argument, as far as I can see.
There was a problem hiding this comment.
I think fetch/processResponse gets copied to fetchParams/process response:
And that will eventually get used in the function called here:
https://fetch.spec.whatwg.org/#ref-for-fetch-finale:~:text=Otherwise%2C%20run%20fetch%20response%20handover%20given%20fetchParams%20and%20response.
...and this does look like it's used for errors, though there is a good chance I am wrong since my ability to follow the fetch spec is poor.
...but I have a hard time following the fetch spec myself.
There was a problem hiding this comment.
I think you're right - I completely missed that they renamed "processResponse" to "process response" when passing it as an argument. :(
|
Any chance you may be able to look at this, Mike? Dominic appears to be unavailable... |
SHA: 032f159 Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 032f159 Reason: push, by morlovich Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Some changes in WICG#1340 and WICG#1225 broke the spec build. @xtlsheep @morlovich
Some changes in #1340 and #1225 broke the spec build. @xtlsheep @morlovich
This turns script fetches into asynchronous operations, with separate fetch start and waiting for results, with a separate
operation to wait for just the cross-origin trusted seller signals authorization. This representation also makes it easier to show which fetches are shared/reused, rather than making it look like we keep re-fetching things.
Only seller scripts really take advantage of this capabilities in this change.
Preview | Diff