Skip to content

[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

Merged
merged 5 commits into from
Apr 11, 2017

Conversation

bfgeek
Copy link
Contributor

@bfgeek bfgeek commented Apr 6, 2017

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.

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.
@tabatkins tabatkins added the ready label Apr 6, 2017
@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 6, 2017

@annevk @domenic Can you have a look at this? It's a fairly large rework of the Worklet#import method.

Copy link
Member

@annevk annevk left a 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.

@@ -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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

urlPrefix: list-
text: append
text: is empty
text: remove
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are definitely exported.

Copy link
Contributor Author

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.

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.
Copy link
Member

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.

Copy link
Contributor Author

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.

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma before it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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.
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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>.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


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.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<br>

When the user-agent is to <dfn>fetch and invoke a worklet script</dfn> given |workletGlobalScope|,
|resolvedModuleURL|, |moduleResponsesMap|, |outsideSettings|, |pendingTaskList| and |promise| the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comma before "and"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and before "the"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

|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>.
Copy link
Member

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".)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@annevk annevk left a 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.

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
Copy link
Member

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 ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

|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:
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma before then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


To <a>perform the request</a> given |request|, perform the following steps:
<br>
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma before "the"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, "user agent"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


2. Let |url| be |request|'s <a >url</a>.
Note: This algorithm should be run within the <a>worklet global scope execution environment</a>.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


6. <a>Fetch</a> |request|.
2. Let |url| be |request|'s <a >url</a>.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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.
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, removed.

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.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with "will".

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 6, 2017

Done thanks, the bits I'm not entirely happy with are:

Inside fetch and invoke a worklet script:

  1. If there are multiple global scopes which fail the parsing, the promise will get re-rejected multiple times one time for each global scope in the list. This is fine as if one fails parsing, all should fail the parsing, but might be a bit strange.

  2. Using the |workletGlobalScope| as the item in |pendingTaskList| but that might be fine.

@annevk
Copy link
Member

annevk commented Apr 6, 2017

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.

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 6, 2017

Done, placed it in a struct as was a bit weird passing a raw counter object around. Can easily remove it if needed.

Copy link
Member

@annevk annevk left a 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.

@@ -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>.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed handles this correctly.


1. <a for=list>Append</a> |workletGlobalScope| to |pendingTaskList|.
11. For each |workletGlobalScope| in the <a>worklet's WorkletGlobalScopes</a>, run the following
substep:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just inline the step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@annevk
Copy link
Member

annevk commented Apr 7, 2017

Bikeshed handles this correctly.

Are you sure? If someone defined something called "pending-tasks-struct", wouldn't counter end up belonging to that?

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 7, 2017

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://rawgit.com/bfgeek/7925f35bb41afeb11faaec2d7c9f65a6/raw/405c0dd94f2f3d1f212f1477a587f55fbc6a80ff/worklets.html#pending-tasks-struct-counter

@annevk
Copy link
Member

annevk commented Apr 7, 2017

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).

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 7, 2017

Ah makes sense. Done.

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 10, 2017

@domenic Did you want to do a review out-of-band later?

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 11, 2017

Spoke to @domenic , he'll do a pass out of band later; will merge this now to do some other changes :).

@bfgeek bfgeek merged commit 7c5c4f8 into master Apr 11, 2017
@tabatkins tabatkins removed the ready label Apr 11, 2017
@domenic domenic mentioned this pull request Apr 24, 2017
@plinss plinss deleted the worklets-import branch February 17, 2018 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants