Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Editorial: decrease traversals for module graph fetching #2971

Merged
merged 4 commits into from
Sep 6, 2017
Merged

Editorial: decrease traversals for module graph fetching #2971

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7768872
Editorial: decrease traversals for module graph fetching
domenic Aug 28, 2017
4e37a02
Merge steps per annevk
domenic Sep 2, 2017
61fbb05
Fixes per review
domenic Sep 5, 2017
efb17d6
Fix visited set issues per review
domenic Sep 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 27 additions & 19 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86777,8 +86777,8 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Perform the <span>internal module script graph fetching procedure</span> given
<var>url</var>, <var>settings object</var>, <var>destination</var>, <var>cryptographic
nonce</var>, <var>parser state</var>, <var>credentials mode</var>, <var>settings object</var>, a
new empty list, "<code data-x="">client</code>", and with the <var>top-level module fetch
flag</var> set. If the caller of this algorithm specified custom <span
new empty <span>set</span>, "<code data-x="">client</code>", and with the <var>top-level module
Copy link
Contributor

Choose a reason for hiding this comment

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

Er, actually we have to pass a set consisting of only one element url, instead of an empty set, as the visited set parameter for top-level callers (here and in "fetch a module worker script graph"). Otherwise, "internal module script graph fetching procedure" can be called twice on the root URL.

[Optional] Perhaps adding an assertion that the url is contained in the visited set in "internal module script graph fetching procedure" might be helpful to ensure the procedure is called only once per URL per root module script.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed (and the set-append vs. list-append); PTAL, and thanks very much for the careful review!

fetch flag</var> set. If the caller of this algorithm specified custom <span
data-x="fetching-scripts-perform-fetch">perform the fetch</span> steps, pass those along as
well.</p>

Expand All @@ -86797,10 +86797,10 @@ interface <dfn>NavigatorOnLine</dfn> {
<li><p>Perform the <span>internal module script graph fetching procedure</span> given
<var>url</var>, <var>fetch client settings object</var>, <var>destination</var>, the empty
string, "<code data-x="">not parser-inserted</code>", <var>credentials mode</var>, <var>module
map settings object</var>, a new empty list, "<code data-x="">client</code>", and with the
<var>top-level module fetch flag</var> set. If the caller of this algorithm specified custom
<span data-x="fetching-scripts-perform-fetch">perform the fetch</span> steps, pass those along as
well.</p>
map settings object</var>, a new empty <span>set</span>, "<code data-x="">client</code>", and
with the <var>top-level module fetch flag</var> set. If the caller of this algorithm specified
custom <span data-x="fetching-scripts-perform-fetch">perform the fetch</span> steps, pass those
along as well.</p>

<li><p>When the <span>internal module script graph fetching procedure</span> asynchronously
completes with <var>result</var>, asynchronously complete this algorithm with
Expand All @@ -86816,7 +86816,7 @@ interface <dfn>NavigatorOnLine</dfn> {
<p>To perform the <dfn>internal module script graph fetching procedure</dfn> given a
<var>url</var>, a <var>fetch client settings object</var>, a <var>destination</var>, a
<var>cryptographic nonce</var>, a <var>parser state</var>, a <var>credentials mode</var>, a
<var>module map settings object</var>, an <var>ancestor list</var>, a <var>referrer</var>, and a
<var>module map settings object</var>, a <var>visited set</var>, a <var>referrer</var>, and a
<var>top-level module fetch</var> flag, perform these steps. The algorithm will asynchronously
complete with either null (on failure) or a <span>module script</span> (on success).</p>

Expand All @@ -86841,8 +86841,7 @@ interface <dfn>NavigatorOnLine</dfn> {

<li><p>If the <var>top-level module fetch</var> flag is set, <span data-x="fetch the descendants
of and instantiate a module script">fetch the descendants of and instantiate</span>
<var>result</var> given <var>destination</var> and an ancestor list obtained by <span
data-x="list append">appending</span> <var>url</var> to <var>ancestor list</var>. Otherwise,
<var>result</var> given <var>destination</var> and <var>visited set</var>. Otherwise,
<span data-x="fetch the descendants of a module script">fetch the descendants of</span>
<var>result</var> given the same arguments.</p></li>

Expand Down Expand Up @@ -86945,13 +86944,11 @@ interface <dfn>NavigatorOnLine</dfn> {
</ol>

<p>To <dfn>fetch the descendants of a module script</dfn> <var>module script</var>, given a
<var>destination</var> and an optional <var>ancestor list</var>, run these steps. The algorithm
will asynchronously complete with either null (on failure) or with <var>module script</var> (on
<var>destination</var> and a <var>visited set</var>, run these steps. The algorithm will
Copy link
Contributor

@hiroshige-g hiroshige-g Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

[Optional] It is clearer to

  • Make |visited set| non-optional here in "fetch the descendants of a module script" (and in "fetch the descendants of and instantiate a module script" below),
  • Remove Step 1, and
  • Make Step 22 of "prepare a script" (i.e. the only caller of "fetch the descendants.*" that lacks the visited set argument) to explicitly create a new empty set.

By doing so, a single visited set is always created by the top-level caller and passed to all the subroutines.
(i.e. there are no chances that multiple "fetch the descendants of and instantiate a module script" calls create separate visited sets in Step 1)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like it to be optional in "fetch the descendants of and instantiate" because that is a top-level entry point for inline scripts. But I will make it non-optional in fetch descendants.

asynchronously complete with either null (on failure) or with <var>module script</var> (on
success).</p>

<ol>
<li><p>If <var>ancestor list</var> was not given, let it be an empty <span>list</span>.</p></li>

<li><p>If <var>module script</var> <span data-x="concept-module-script-is-errored">is
errored</span> or <span data-x="concept-module-script-has-instantiated">has instantiated</span>,
asynchronously complete this algorithm with <var>module script</var>, and abort these
Expand All @@ -86978,9 +86975,17 @@ interface <dfn>NavigatorOnLine</dfn> {
href="#validate-requested-module-specifiers">previously successful</a> with these same two
arguments.</p></li>

<li><p>If <var>ancestor list</var> does not <span data-x="list contains">contain</span>
<var>url</var>, <span data-x="list append">append</span> <var>url</var> to
<var>urls</var>.</p></li>
<li>
<p>If <var>visited set</var> does not <span data-x="list contains">contain</span>
<var>url</var>, then:</p>

<ol>
<li><p><span data-x="list append">Append</span> <var>url</var> to <var>urls</var>.</p></li>

<li><p><span data-x="list append">Append</span> <var>url</var> to <var>visited
Copy link
Member

Choose a reason for hiding this comment

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

It would be clearer if you merged this step with the previous step given that "set append" is slightly different from "list append" but would be identical if you merged this with the previous step. (I initially thought you wanted to change order in the set if the item was already present.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I figured out what you mean, and pushed a commit. Let me know if I got it right.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

set</var>.</p></li>
</ol>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: #set-append instead of #list-append?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh dang, I forgot they go to different places, thanks.

</li>
</ol>
</li>

Expand All @@ -86991,7 +86996,7 @@ interface <dfn>NavigatorOnLine</dfn> {
<var>module script</var>'s <span data-x="concept-module-script-nonce">cryptographic
nonce</span>, <var>module script</var>'s <span data-x="concept-module-script-parser">parser
state</span>, <var>destination</var>, <var>module script</var>'s <span>settings object</span>,
<var>module script</var>'s <span>settings object</span>, <var>ancestor list</var>, <var>module
<var>module script</var>'s <span>settings object</span>, <var>visited set</var>, <var>module
script</var>'s <span data-x="concept-module-script-base-url">base URL</span>, and with the
<var>top-level module fetch</var> flag unset. If the caller of this algorithm specified custom
<span data-x="fetching-scripts-perform-fetch">perform the fetch</span> steps, pass those along
Expand Down Expand Up @@ -87037,13 +87042,15 @@ interface <dfn>NavigatorOnLine</dfn> {
</ol>

<p>To <dfn>fetch the descendants of and instantiate a module script</dfn> <var>module
script</var>, given a <var>destination</var> and an optional <var>ancestor list</var>, run these
script</var>, given a <var>destination</var> and an optional <var>visited set</var>, run these
steps. The algorithm will asynchronously complete with either null (on failure) or with
<var>module script</var> (on success).</p>

<ol>
<li><p>If <var>visited set</var> was not given, let it be an empty <span>set</span>.</p></li>

<li><p><span data-x="fetch the descendants of a module script">Fetch the descendants of</span>
<var>module script</var>, given <var>destination</var> and <var>ancestor list</var>.</p></li>
<var>module script</var>, given <var>destination</var> and <var>visited set</var>.</p></li>

<li><p>Return from this algorithm, and run the following steps when <span data-x="fetch the
descendants of a module script">fetching the descendants of a module script</span> asynchronously
Expand Down Expand Up @@ -120585,6 +120592,7 @@ INSERT INTERFACES HERE
Henry Mason,
Henry Story,
Hermann Donfack Zeufack,
Hiroshige Hayashizaki,
Hugh Bellamy,
Hugh Guiney,
Hugh Winkler,
Expand Down