[worklets] Multiple fixes to the {{Worklet/import()}} algorithm. #375

bfgeek · 2017-04-06T04:34:33Z

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.

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.

bfgeek · 2017-04-06T04:35:24Z

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

annevk

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.

annevk · 2017-04-06T05:30:22Z

worklets/Overview.bs

@@ -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


You shouldn't have to do this. I'm fairly certain HTML exports this term.

annevk · 2017-04-06T05:30:32Z

worklets/Overview.bs

+    urlPrefix: list-
+        text: append
+        text: is empty
+        text: remove


These are definitely exported.

Done. I need to go through all the anchors and clean this up properly.

annevk · 2017-04-06T05:32:31Z

worklets/Overview.bs

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


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.

I see, ok I've reworded this. I can also remove this as its not strictly needed.

annevk · 2017-04-06T05:33:41Z

worklets/Overview.bs

-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:


Comma before it.

annevk · 2017-04-06T05:37:35Z

worklets/Overview.bs

        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.


You need to actually return the promise as well here.

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.

annevk · 2017-04-06T05:40:33Z

worklets/Overview.bs

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


I think what you want here instead is a simple "If/Otherwise". Ensure might lead folks to think they can just make one up.

annevk · 2017-04-06T05:41:23Z

worklets/Overview.bs


        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.


Just say "Wait" (no need for asynchronous). You're already in parallel.

annevk · 2017-04-06T05:42:28Z

worklets/Overview.bs

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


comma before "and"

and before "the"

annevk · 2017-04-06T05:43:06Z

worklets/Overview.bs

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


No normative keywords in notes. (You can say "is to be run".)

annevk

Found a bunch more nits in the latter part. Still nothing major I think.

annevk · 2017-04-06T07:24:03Z

worklets/Overview.bs

+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


Better: "If script is null, then ..."

annevk · 2017-04-06T07:24:32Z

worklets/Overview.bs

+    |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:


"to run these steps"

We've decided saying substeps is confusing and have started to remove it from all specifications.

annevk · 2017-04-06T07:25:40Z

worklets/Overview.bs

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


Comma before then.

annevk · 2017-04-06T07:26:25Z

worklets/Overview.bs


-                To <a>perform the request</a> given |request|, perform the following steps:
+<br>


Use <hr> as a separator.

annevk · 2017-04-06T07:26:40Z

worklets/Overview.bs


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


Comma before "the"

Also, "user agent"

annevk · 2017-04-06T07:27:04Z

worklets/Overview.bs


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


No normative keywords in notes.

annevk · 2017-04-06T07:27:43Z

worklets/Overview.bs


-                    6. <a>Fetch</a> |request|.
+        2. Let |url| be |request|'s <a >url</a>.


That should have for=request on it.

annevk · 2017-04-06T07:28:24Z

worklets/Overview.bs

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


Isn't this algorithm already invoked as in parallel?

Yup, removed.

annevk · 2017-04-06T07:29:34Z

worklets/Overview.bs

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


should and may cannot be used in notes.

Replaced with "will".

bfgeek · 2017-04-06T16:09:31Z

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

Inside fetch and invoke a worklet script:

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.
Using the |workletGlobalScope| as the item in |pendingTaskList| but that might be fine.

annevk · 2017-04-06T16:23:02Z

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.

… a counter.

bfgeek · 2017-04-06T17:24:48Z

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

annevk

Couple more nits. I hope @domenic can find some time too for a quick skim.

annevk · 2017-04-07T07:36:03Z

worklets/Overview.bs

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


You want for="pending tasks struct" here and below. That links it to the term.

Bikeshed handles this correctly.

annevk · 2017-04-07T07:36:36Z

worklets/Overview.bs

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


Just inline the step?

annevk · 2017-04-07T16:32:29Z

Bikeshed handles this correctly.

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

bfgeek · 2017-04-07T16:39:10Z

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 · 2017-04-07T16:43:12Z

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 · 2017-04-07T16:52:27Z

Ah makes sense. Done.

bfgeek · 2017-04-10T22:42:19Z

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

bfgeek · 2017-04-11T16:52:41Z

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

tabatkins added the ready label Apr 6, 2017

annevk reviewed Apr 6, 2017

View reviewed changes

[worklets] Fixes from annevk review.

e7b816f

bfgeek mentioned this pull request Apr 6, 2017

[worklets] could the module responses cache and custom fetch steps be simplified? #324

Closed

[worklets] Change |pendingTaskList| to |pendingTaskStruct| containing…

e840c64

… a counter.

bfgeek mentioned this pull request Apr 6, 2017

[worklets] imports requires WorkletGlobalScope creation to be immediate #377

Closed

annevk reviewed Apr 7, 2017

View reviewed changes

[worklets] Address annevk comments, inline algorithm step.

80ac914

[worklets] s/pending-tasks-struct/"pending tasks struct"/g

d871412

bfgeek merged commit 7c5c4f8 into master Apr 11, 2017

tabatkins removed the ready label Apr 11, 2017

domenic mentioned this pull request Apr 24, 2017

[worklets] Review #396

Closed

plinss deleted the worklets-import branch February 17, 2018 20:41


		To <a>perform the request</a> given \|request\|, perform the following steps:
		<br>


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


		6. <a>Fetch</a> \|request\|.
		2. Let \|url\| be \|request\|'s <a >url</a>.

[worklets] Multiple fixes to the {{Worklet/import()}} algorithm. #375

[worklets] Multiple fixes to the {{Worklet/import()}} algorithm. #375

Uh oh!

Conversation

bfgeek commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bfgeek commented Apr 6, 2017

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bfgeek commented Apr 6, 2017 •

edited

Loading