-
Notifications
You must be signed in to change notification settings - Fork 142
[worklets] Multiple fixes to the {{Worklet/import()}} algorithm. #375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Instead of relying on "in parallel" this change makes all the thread hopping explicit via. message passing and queue tasks on the appropriate event loop. - Fixes #372, changes arguments to fetch a module script graph. - Fixes #370, running scripts in parallel. Now the script explicitly are run within a task queued on the worklet global scope's event loop. - Fixes #318, actually runs the event loop. - Fixes #230, treats "fetch a module worker script graph" as asynchronous. - Probably fixes #225 ? I think I'm grabbing the correct state at each thread hop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to review the last part, but I gotta go for a bit. Figured I'd submit what I have so far. Looks a lot better already.
worklets/Overview.bs
Outdated
@@ -40,7 +40,7 @@ urlPrefix: https://html.spec.whatwg.org/multipage/webappapis.html; type: dfn; | |||
text: environment settings object | |||
text: event loop | |||
text: event loop processing model | |||
text: fetch a module script tree | |||
url: #fetch-a-module-worker-script-tree; text: fetch a module worker script graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't have to do this. I'm fairly certain HTML exports this term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
urlPrefix: list- | ||
text: append | ||
text: is empty | ||
text: remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are definitely exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I need to go through all the anchors and clean this up properly.
worklets/Overview.bs
Outdated
queues</a> are not used). The <a>event loop</a> is created by the <a>create a | ||
associated <a>browsing context</a>. Only tasks associated with the author invoking | ||
{{Worklet/import()}} use the <a>event loop</a>, in addition to the <a>microtask queue</a> when the | ||
user-agent invokes callbacks. The <a>event loop</a> is created by the <a>create a | ||
WorkletGlobalScope</a> algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the user-agent callbacks also use tasks? Otherwise promise code that resolves within the same task can get intertwined with callbacks coming from elsewhere leading to weird inconsistencies. I doubt that's how anyone would actually implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, ok I've reworded this. I can also remove this as its not strictly needed.
worklets/Overview.bs
Outdated
When a user agent is to <dfn>create a WorkletGlobalScope</dfn>, for a given |worklet|, it | ||
<em>must</em> run the following steps: | ||
When a user agent is to <dfn>create a WorkletGlobalScope</dfn>, given |workletGlobalScopeType|, | ||
|moduleResponsesMap|, and |outsideSettings| it <em>must</em> run the following steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comma before it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
the <a>relevant settings object</a> of <b>this</b>. | ||
|
||
3. If this fails, reject |promise| with a "<a>SyntaxError</a>" <a>DOMException</a> and abort | ||
4. If this fails, reject |promise| with a "<a>SyntaxError</a>" <a>DOMException</a> and abort | ||
these steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to actually return the promise as well here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use the Infra Standard and you do, you can say "If resolvedModuleURL is failure, then reject promise with a SyntaxError and return promise."
Nit: resolvedModuleURL is better as moduleURLRecord.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
8. Let |workletGlobalScopeType| be |worklet|'s <a>worklet global scope type</a>. | ||
|
||
9. Ensure that there is at least one {{WorkletGlobalScope}} in the <a>worklet's | ||
WorkletGlobalScopes</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you want here instead is a simple "If/Otherwise". Ensure might lead folks to think they can just make one up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|
||
The user-agent may also create additional {{WorkletGlobalScope}}s at this time. | ||
|
||
5. Let |outsideSettings| be the <a>relevant settings object</a> of <b>this</b>. | ||
Asynchronously wait for this step to complete before continuing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just say "Wait" (no need for asynchronous). You're already in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
<br> | ||
|
||
When the user-agent is to <dfn>fetch and invoke a worklet script</dfn> given |workletGlobalScope|, | ||
|resolvedModuleURL|, |moduleResponsesMap|, |outsideSettings|, |pendingTaskList| and |promise| the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comma before "and"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and before "the"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|resolvedModuleURL|, |moduleResponsesMap|, |outsideSettings|, |pendingTaskList| and |promise| the | ||
user-agent <em>must</em> run the following steps: | ||
|
||
Note: This algorithm should be run within the <a>worklet global scope execution environment</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No normative keywords in notes. (You can say "is to be run".)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a bunch more nits in the latter part. Still nothing major I think.
worklets/Overview.bs
Outdated
2. Let |script| by the result of <a>fetch a worklet script</a> given |resolvedModuleURL|, | ||
|moduleResponsesMap|, |outsideSettings|, and |insideSettings|. | ||
|
||
3. If |script| is <em>null</em> <a>queue a task</a> on |outsideSettings|'s <a>responsible event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better: "If script is null, then ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|moduleResponsesMap|, |outsideSettings|, and |insideSettings|. | ||
|
||
3. If |script| is <em>null</em> <a>queue a task</a> on |outsideSettings|'s <a>responsible event | ||
loop</a> to run the following substeps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to run these steps"
We've decided saying substeps is confusing and have started to remove it from all specifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
2. <a>Fetch a module script tree</a> given |resolvedModuleURL|, "omit", the empty string | ||
(as no cryptographic nonce is present for worklets), "not parser-inserted", | ||
"script", |outsideSettings|, and |insideSettings|. | ||
2. If |pendingTaskList| <a>is empty</a> then resolve |promise|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comma before then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|
||
To <a>perform the request</a> given |request|, perform the following steps: | ||
<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use <hr>
as a separator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|
||
1. Let |cache| be the current {{Worklet}}'s <a>module responses map</a>. | ||
When the user-agent is to <dfn>fetch a worklet script</dfn> given |resolvedModuleURL|, | ||
|moduleResponsesMap|, |outsideSettings|, and |insideSettings| the user-agent <em>must</em> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comma before "the"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, "user agent"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|
||
2. Let |url| be |request|'s <a >url</a>. | ||
Note: This algorithm should be run within the <a>worklet global scope execution environment</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No normative keywords in notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
|
||
6. <a>Fetch</a> |request|. | ||
2. Let |url| be |request|'s <a >url</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should have for=request on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
worklets/Overview.bs
Outdated
7. Let |response| be the result of <a>fetch</a> when it asynchronously | ||
completes. | ||
3. If |cache| contains an entry with key |url| whose value is "fetching", wait (<a>in | ||
parallel</a>) until that entry's value changes, then proceed to the next step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this algorithm already invoked as in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, removed.
worklets/Overview.bs
Outdated
7. Return |promise|. | ||
Note: Specifically, if a script fails to parse or fails to load over the network, it will reject the | ||
promise. If the script throws an error while first evaluating the promise should resolve as a | ||
classes may have been registered correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should and may cannot be used in notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with "will".
Done thanks, the bits I'm not entirely happy with are: Inside
|
Maybe instead of pendingTaskList you could have some kind of counter that you set to -1 as soon as you reach an error. And when it gets to 0 you fulfill. |
Done, placed it in a struct as was a bit weird passing a raw counter object around. Can easily remove it if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more nits. I hope @domenic can find some time too for a quick skim.
worklets/Overview.bs
Outdated
@@ -310,6 +306,12 @@ additional {{WorkletGlobalScope}}s should be transparent to the author. | |||
all fetched sources from the main thread to the thread which owns the {{WorkletGlobalScope}}. | |||
</div> | |||
|
|||
A <dfn>pending tasks struct</dfn> is a <a>struct</a> consiting of: | |||
- A <dfn for=pending-tasks-struct>counter</dfn>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want for="pending tasks struct" here and below. That links it to the term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed handles this correctly.
worklets/Overview.bs
Outdated
|
||
1. <a for=list>Append</a> |workletGlobalScope| to |pendingTaskList|. | ||
11. For each |workletGlobalScope| in the <a>worklet's WorkletGlobalScopes</a>, run the following | ||
substep: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just inline the step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Are you sure? If someone defined something called "pending-tasks-struct", wouldn't counter end up belonging to that? |
I thought that foo-bar and "foo bar" were equivalent in bikeshed? And if someone else defined that term it would become ambiguous and force you to decide which one? I can poke tab and ask. |
https://tabatkins.github.io/bikeshed/#definitions says no such thing. I think the reason it works is because you can give things random for values. Bikeshed doesn't enforce them to be defined things themselves (maybe it should though). |
Ah makes sense. Done. |
@domenic Did you want to do a review out-of-band later? |
Spoke to @domenic , he'll do a pass out of band later; will merge this now to do some other changes :). |
Instead of relying on "in parallel" this change makes all the thread
hopping explicit via. message passing and queue tasks on the appropriate
event loop.
Fixes [worklets] fetch a module script tree #372, changes arguments to fetch a module script graph.
Fixes [worklets] don't run scripts from "in parallel" #370, running scripts in parallel. Now the script explicitly are
run within a task queued on the worklet global scope's event loop.
Fixes Worklet event loop is not run? #318, actually runs the event loop.
Fixes "fetch a module script tree" is asynchronous #230, treats "fetch a module worker script graph" as
asynchronous.
Fixes [worklets] imports requires WorkletGlobalScope creation to be immediate #377, as now performs a thread hop to create the workletglobalscope.
Probably fixes import() should grab state before executing in parallel #225 ? I think I'm grabbing the correct state at each
thread hop.