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.

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

Editorial: decrease traversals for module graph fetching #2971

Merged
merged 4 commits into from
Sep 6, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Aug 28, 2017

Previously, we could invoke the internal module script graph fetching
procedure once for each combination of node and ancestor list, i.e. once
for each node and for each path to the node. This could be up to O(2^n)
for n nodes, as seen in
https://gist.github.com/domenic/d877f8b2e6d6ae62a9c94f916739e4db.

This change simplifies the model by using a single visited set per
top-level fetch, which ensures that the internal module script graph
fetching procedure is only invoked once for each node.

This change is technically editorial, as it has no visible, testable
effects. But it makes it much easier for implementers to follow the
specification as written without causing themselves massive slowdowns.


/cc @whatwg/modules. Tagging "do not merge yet" as we may have further fixes that could disturb this coming over the next day or two. But I wanted to get this out for review, and ideally the other fixes will just layer on top.

@hiroshige-g in particular to review. Please let us know if you'd like your name in the acknowledgements in a different format (e.g. Kanji)

@domenic domenic added do not merge yet Pull request must not be merged per rationale in comment topic: script labels Aug 28, 2017
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Aug 31, 2017
@domenic
Copy link
Member Author

domenic commented Aug 31, 2017

Removing "do not merge yet" as I am now confident these changes will survive future work.

Previously, we could invoke the internal module script graph fetching
procedure once for each combination of node and ancestor list, i.e. once
for each node and for each path to the node. This could be up to O(2^n)
for n nodes, as seen in
https://gist.github.com/domenic/d877f8b2e6d6ae62a9c94f916739e4db.

This change simplifies the model by using a single visited set per
top-level fetch, which ensures that the internal module script graph
fetching procedure is only invoked once for each node.

This change is technically editorial, as it has no visible, testable
effects. But it makes it much easier for implementers to follow the
specification as written without causing themselves massive slowdowns.
source Outdated
<var>url</var>, <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
set</var>.</p></li>
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!

source Outdated
@@ -86945,12 +86944,12 @@ 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
<var>destination</var> and an optional <var>visited set</var>, run these steps. The algorithm
Copy link
Contributor

@hiroshige-g hiroshige-g Sep 5, 2017

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.

@hiroshige-g
Copy link
Contributor

Is it clear that the visited set is passed by reference (at least not passed by value), i.e. the single instance of the visited set is shared across all subroutines under one top-level invocation of "fetch the descendants of and instantiate"?

Also, could you make two top-level call sites of "internal module script graph fetching procedure", i.e. those in "fetch a module script graph" and in "fetch a module worker script graph", pass a new empty set instead of "a new empty list" as the visited set parameter?

@domenic
Copy link
Member Author

domenic commented Sep 5, 2017

Is it clear that the visited set is passed by reference (at least not passed by value)

This is generally implicit in web specs. For making it explicit, see whatwg/infra#139 .

Other comments addressed, I think :)

source Outdated
@@ -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!

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

@hiroshige-g
Copy link
Contributor

lgtm.

This change is technically editorial, as it has no visible, testable effects.
Probably this PR causes a subtle behavior change on the choice of error reported (if we fix the order of network fetch etc.), because this PR how errors are propagated of Step 7.2 of "fetch the descendants".
However this is not a problem, because the behavior of Step 7.2 of "fetch the descendants" is anyway non-deterministic (i.e. if the order of network fetch is changed, then the choice of error reported is changed), and the error propagation will be removed soon.

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

rs=me

@domenic domenic merged commit 6b20dfb into master Sep 6, 2017
@domenic domenic deleted the module-fixes branch September 6, 2017 20:37
MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017
1. This CL applies the spec change by
whatwg/html#2971.
Since [1], the code has already been behaving like the spec PR 2971, and this CL
makes the code align with PR 2971 more completely, by removing AncestorList
and adding spec comments.

2. This CL also flattens the code structure of ModuleTreeLinker, which is
enabled by the spec PR 2971, to make the code simpler to make further
optimizations easy.
That is, instead of creating a ModuleTreeLinker for each module script
(i.e. for each "fetch the descendants" call) in a module graph, this CL
creates a single ModuleTreeLinker that corresponds to a top-level module graph
script.

Most of fetching-related classes/methods, including
- ModuleTreeLinker::DependencyModuleClient
- ModuleTreeLinker::FetchDescendants()
- ModuleTreeLinker::NotifyOneDescendantFinished()
are merged into the single method NotifyModuleLoadFinished() that implements
the main body of "fetch the descendants" and
"internal module script graph fetching procedure".

This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
directly, as we no longer have to share it across multiple ModuleTreeLinkers.
Modulator::FetchTreeInternal() is removed as we no longer create
ModuleTreeLinkers for subtree fetching via Modulator.

3. This CL applies a part of
whatwg/html#2991, particularly introduces
FindFirstParseError() that corresponds to "find the first parse error" and
use it to find the error to be reported, instead of propagating errors
based on Step 6.1 and 6.2 of "fetch the descendants".

(*) In some subtle cases, this might cause behavior changes in error reporting,
but these changes shouldn't be significant, because anyway the spec before
PR 2991 (and thus the previous implementation) behaves nondeterministically
in some similarly subtle cases.

This CL is an intermediate step to apply spec PRs 2971 and 2991.
This CL refactors largely ModuleTreeLinker with keeping the existing behavior
mostly (except for (*)), and subsequent CLs will apply the behavior changes
and remaining code structure changes introduced by PR 2991.

Bug: 763597
Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d
Reviewed-on: https://chromium-review.googlesource.com/583552
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503034}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017
This reverts commit 5a168df.

Reason for revert: find-it says this bloke the linux build

Original change's description:
> Flatten ModuleTreeLinker
> 
> 1. This CL applies the spec change by
> whatwg/html#2971.
> Since [1], the code has already been behaving like the spec PR 2971, and this CL
> makes the code align with PR 2971 more completely, by removing AncestorList
> and adding spec comments.
> 
> 2. This CL also flattens the code structure of ModuleTreeLinker, which is
> enabled by the spec PR 2971, to make the code simpler to make further
> optimizations easy.
> That is, instead of creating a ModuleTreeLinker for each module script
> (i.e. for each "fetch the descendants" call) in a module graph, this CL
> creates a single ModuleTreeLinker that corresponds to a top-level module graph
> script.
> 
> Most of fetching-related classes/methods, including
> - ModuleTreeLinker::DependencyModuleClient
> - ModuleTreeLinker::FetchDescendants()
> - ModuleTreeLinker::NotifyOneDescendantFinished()
> are merged into the single method NotifyModuleLoadFinished() that implements
> the main body of "fetch the descendants" and
> "internal module script graph fetching procedure".
> 
> This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
> directly, as we no longer have to share it across multiple ModuleTreeLinkers.
> Modulator::FetchTreeInternal() is removed as we no longer create
> ModuleTreeLinkers for subtree fetching via Modulator.
> 
> 3. This CL applies a part of
> whatwg/html#2991, particularly introduces
> FindFirstParseError() that corresponds to "find the first parse error" and
> use it to find the error to be reported, instead of propagating errors
> based on Step 6.1 and 6.2 of "fetch the descendants".
> 
> (*) In some subtle cases, this might cause behavior changes in error reporting,
> but these changes shouldn't be significant, because anyway the spec before
> PR 2991 (and thus the previous implementation) behaves nondeterministically
> in some similarly subtle cases.
> 
> This CL is an intermediate step to apply spec PRs 2971 and 2991.
> This CL refactors largely ModuleTreeLinker with keeping the existing behavior
> mostly (except for (*)), and subsequent CLs will apply the behavior changes
> and remaining code structure changes introduced by PR 2991.
> 
> Bug: 763597
> Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d
> Reviewed-on: https://chromium-review.googlesource.com/583552
> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503034}

TBR=hiroshige@chromium.org,ksakamoto@chromium.org,kouhei@chromium.org,nhiroki@chromium.org

Change-Id: I297bca8d91c91f49e4ef1e46a01a1af90a7e67ef
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 763597
Reviewed-on: https://chromium-review.googlesource.com/674784
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503059}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017
This reverts commit 5a168df.

Reason for revert: Seems to be breaking ModuleTreeLinkerTest.FetchTreeWith3Deps1Fail on:
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20TSan%20Tests/builds/11696

Original change's description:
> Flatten ModuleTreeLinker
> 
> 1. This CL applies the spec change by
> whatwg/html#2971.
> Since [1], the code has already been behaving like the spec PR 2971, and this CL
> makes the code align with PR 2971 more completely, by removing AncestorList
> and adding spec comments.
> 
> 2. This CL also flattens the code structure of ModuleTreeLinker, which is
> enabled by the spec PR 2971, to make the code simpler to make further
> optimizations easy.
> That is, instead of creating a ModuleTreeLinker for each module script
> (i.e. for each "fetch the descendants" call) in a module graph, this CL
> creates a single ModuleTreeLinker that corresponds to a top-level module graph
> script.
> 
> Most of fetching-related classes/methods, including
> - ModuleTreeLinker::DependencyModuleClient
> - ModuleTreeLinker::FetchDescendants()
> - ModuleTreeLinker::NotifyOneDescendantFinished()
> are merged into the single method NotifyModuleLoadFinished() that implements
> the main body of "fetch the descendants" and
> "internal module script graph fetching procedure".
> 
> This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
> directly, as we no longer have to share it across multiple ModuleTreeLinkers.
> Modulator::FetchTreeInternal() is removed as we no longer create
> ModuleTreeLinkers for subtree fetching via Modulator.
> 
> 3. This CL applies a part of
> whatwg/html#2991, particularly introduces
> FindFirstParseError() that corresponds to "find the first parse error" and
> use it to find the error to be reported, instead of propagating errors
> based on Step 6.1 and 6.2 of "fetch the descendants".
> 
> (*) In some subtle cases, this might cause behavior changes in error reporting,
> but these changes shouldn't be significant, because anyway the spec before
> PR 2991 (and thus the previous implementation) behaves nondeterministically
> in some similarly subtle cases.
> 
> This CL is an intermediate step to apply spec PRs 2971 and 2991.
> This CL refactors largely ModuleTreeLinker with keeping the existing behavior
> mostly (except for (*)), and subsequent CLs will apply the behavior changes
> and remaining code structure changes introduced by PR 2991.
> 
> Bug: 763597
> Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d
> Reviewed-on: https://chromium-review.googlesource.com/583552
> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503034}

TBR=hiroshige@chromium.org,ksakamoto@chromium.org,kouhei@chromium.org,nhiroki@chromium.org

Change-Id: I84c8f8f443bdbb1d5fbe8cb52d2cb513c7890d2c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 763597
Reviewed-on: https://chromium-review.googlesource.com/674510
Reviewed-by: calamity <calamity@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503063}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017
This CL relands https://chromium-review.googlesource.com/c/chromium/src/+/583552
with a fix for test failure in release builds:
Move FindFirstParseError() call out of DCHECK().

Original CL description:

1. This CL applies the spec change by
whatwg/html#2971.
Since [1], the code has already been behaving like the spec PR 2971, and this CL
makes the code align with PR 2971 more completely, by removing AncestorList
and adding spec comments.

2. This CL also flattens the code structure of ModuleTreeLinker, which is
enabled by the spec PR 2971, to make the code simpler to make further
optimizations easy.
That is, instead of creating a ModuleTreeLinker for each module script
(i.e. for each "fetch the descendants" call) in a module graph, this CL
creates a single ModuleTreeLinker that corresponds to a top-level module graph
script.

Most of fetching-related classes/methods, including
- ModuleTreeLinker::DependencyModuleClient
- ModuleTreeLinker::FetchDescendants()
- ModuleTreeLinker::NotifyOneDescendantFinished()
are merged into the single method NotifyModuleLoadFinished() that implements
the main body of "fetch the descendants" and
"internal module script graph fetching procedure".

This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
directly, as we no longer have to share it across multiple ModuleTreeLinkers.
Modulator::FetchTreeInternal() is removed as we no longer create
ModuleTreeLinkers for subtree fetching via Modulator.

3. This CL applies a part of
whatwg/html#2991, particularly introduces
FindFirstParseError() that corresponds to "find the first parse error" and
use it to find the error to be reported, instead of propagating errors
based on Step 6.1 and 6.2 of "fetch the descendants".

(*) In some subtle cases, this might cause behavior changes in error reporting,
but these changes shouldn't be significant, because anyway the spec before
PR 2991 (and thus the previous implementation) behaves nondeterministically
in some similarly subtle cases.

This CL is an intermediate step to apply spec PRs 2971 and 2991.
This CL refactors largely ModuleTreeLinker with keeping the existing behavior
mostly (except for (*)), and subsequent CLs will apply the behavior changes
and remaining code structure changes introduced by PR 2991.

Bug: 763597
Change-Id: I4136db0db4bc8a0cdc7f5c23b18787b405d8c98f
Reviewed-on: https://chromium-review.googlesource.com/674748
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503126}
MXEBot pushed a commit to mirror/chromium that referenced this pull request Sep 21, 2017
The web platform tests testing module script errors are not deterministic
until we have the new algorithm being proposed at HTML PRs below:
- whatwg/html#2971
- whatwg/html#2991

This CL marks the test as flaky until we land the new algorithm.

TBR=hiroshige@chromium.org

Bug: 767093, 763597
Change-Id: I9ef17f2ae6dc51acbef4dbab1f7f7c7438d17eb7
Reviewed-on: https://chromium-review.googlesource.com/676403
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503304}
nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018
1. This CL applies the spec change by
whatwg/html#2971.
Since [1], the code has already been behaving like the spec PR 2971, and this CL
makes the code align with PR 2971 more completely, by removing AncestorList
and adding spec comments.

2. This CL also flattens the code structure of ModuleTreeLinker, which is
enabled by the spec PR 2971, to make the code simpler to make further
optimizations easy.
That is, instead of creating a ModuleTreeLinker for each module script
(i.e. for each "fetch the descendants" call) in a module graph, this CL
creates a single ModuleTreeLinker that corresponds to a top-level module graph
script.

Most of fetching-related classes/methods, including
- ModuleTreeLinker::DependencyModuleClient
- ModuleTreeLinker::FetchDescendants()
- ModuleTreeLinker::NotifyOneDescendantFinished()
are merged into the single method NotifyModuleLoadFinished() that implements
the main body of "fetch the descendants" and
"internal module script graph fetching procedure".

This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
directly, as we no longer have to share it across multiple ModuleTreeLinkers.
Modulator::FetchTreeInternal() is removed as we no longer create
ModuleTreeLinkers for subtree fetching via Modulator.

3. This CL applies a part of
whatwg/html#2991, particularly introduces
FindFirstParseError() that corresponds to "find the first parse error" and
use it to find the error to be reported, instead of propagating errors
based on Step 6.1 and 6.2 of "fetch the descendants".

(*) In some subtle cases, this might cause behavior changes in error reporting,
but these changes shouldn't be significant, because anyway the spec before
PR 2991 (and thus the previous implementation) behaves nondeterministically
in some similarly subtle cases.

This CL is an intermediate step to apply spec PRs 2971 and 2991.
This CL refactors largely ModuleTreeLinker with keeping the existing behavior
mostly (except for (*)), and subsequent CLs will apply the behavior changes
and remaining code structure changes introduced by PR 2991.

Bug: 763597
Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d
Reviewed-on: https://chromium-review.googlesource.com/583552
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503034}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 5a168dfca798f0088b3257f21781a66b4132ba7b
nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018
This reverts commit 5a168dfca798f0088b3257f21781a66b4132ba7b.

Reason for revert: find-it says this bloke the linux build

Original change's description:
> Flatten ModuleTreeLinker
> 
> 1. This CL applies the spec change by
> whatwg/html#2971.
> Since [1], the code has already been behaving like the spec PR 2971, and this CL
> makes the code align with PR 2971 more completely, by removing AncestorList
> and adding spec comments.
> 
> 2. This CL also flattens the code structure of ModuleTreeLinker, which is
> enabled by the spec PR 2971, to make the code simpler to make further
> optimizations easy.
> That is, instead of creating a ModuleTreeLinker for each module script
> (i.e. for each "fetch the descendants" call) in a module graph, this CL
> creates a single ModuleTreeLinker that corresponds to a top-level module graph
> script.
> 
> Most of fetching-related classes/methods, including
> - ModuleTreeLinker::DependencyModuleClient
> - ModuleTreeLinker::FetchDescendants()
> - ModuleTreeLinker::NotifyOneDescendantFinished()
> are merged into the single method NotifyModuleLoadFinished() that implements
> the main body of "fetch the descendants" and
> "internal module script graph fetching procedure".
> 
> This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
> directly, as we no longer have to share it across multiple ModuleTreeLinkers.
> Modulator::FetchTreeInternal() is removed as we no longer create
> ModuleTreeLinkers for subtree fetching via Modulator.
> 
> 3. This CL applies a part of
> whatwg/html#2991, particularly introduces
> FindFirstParseError() that corresponds to "find the first parse error" and
> use it to find the error to be reported, instead of propagating errors
> based on Step 6.1 and 6.2 of "fetch the descendants".
> 
> (*) In some subtle cases, this might cause behavior changes in error reporting,
> but these changes shouldn't be significant, because anyway the spec before
> PR 2991 (and thus the previous implementation) behaves nondeterministically
> in some similarly subtle cases.
> 
> This CL is an intermediate step to apply spec PRs 2971 and 2991.
> This CL refactors largely ModuleTreeLinker with keeping the existing behavior
> mostly (except for (*)), and subsequent CLs will apply the behavior changes
> and remaining code structure changes introduced by PR 2991.
> 
> Bug: 763597
> Change-Id: I0ef38c5ebf462fa7f02093f1725ea0014b80585d
> Reviewed-on: https://chromium-review.googlesource.com/583552
> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
> Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503034}

TBR=hiroshige@chromium.org,ksakamoto@chromium.org,kouhei@chromium.org,nhiroki@chromium.org

Change-Id: I297bca8d91c91f49e4ef1e46a01a1af90a7e67ef
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 763597
Reviewed-on: https://chromium-review.googlesource.com/674784
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503059}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: e81cbfd9f8b19631241e926ebfe66aabedc0ef53
nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018
This CL relands https://chromium-review.googlesource.com/c/chromium/src/+/583552
with a fix for test failure in release builds:
Move FindFirstParseError() call out of DCHECK().

Original CL description:

1. This CL applies the spec change by
whatwg/html#2971.
Since [1], the code has already been behaving like the spec PR 2971, and this CL
makes the code align with PR 2971 more completely, by removing AncestorList
and adding spec comments.

2. This CL also flattens the code structure of ModuleTreeLinker, which is
enabled by the spec PR 2971, to make the code simpler to make further
optimizations easy.
That is, instead of creating a ModuleTreeLinker for each module script
(i.e. for each "fetch the descendants" call) in a module graph, this CL
creates a single ModuleTreeLinker that corresponds to a top-level module graph
script.

Most of fetching-related classes/methods, including
- ModuleTreeLinker::DependencyModuleClient
- ModuleTreeLinker::FetchDescendants()
- ModuleTreeLinker::NotifyOneDescendantFinished()
are merged into the single method NotifyModuleLoadFinished() that implements
the main body of "fetch the descendants" and
"internal module script graph fetching procedure".

This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
directly, as we no longer have to share it across multiple ModuleTreeLinkers.
Modulator::FetchTreeInternal() is removed as we no longer create
ModuleTreeLinkers for subtree fetching via Modulator.

3. This CL applies a part of
whatwg/html#2991, particularly introduces
FindFirstParseError() that corresponds to "find the first parse error" and
use it to find the error to be reported, instead of propagating errors
based on Step 6.1 and 6.2 of "fetch the descendants".

(*) In some subtle cases, this might cause behavior changes in error reporting,
but these changes shouldn't be significant, because anyway the spec before
PR 2991 (and thus the previous implementation) behaves nondeterministically
in some similarly subtle cases.

This CL is an intermediate step to apply spec PRs 2971 and 2991.
This CL refactors largely ModuleTreeLinker with keeping the existing behavior
mostly (except for (*)), and subsequent CLs will apply the behavior changes
and remaining code structure changes introduced by PR 2991.

Bug: 763597
Change-Id: I4136db0db4bc8a0cdc7f5c23b18787b405d8c98f
Reviewed-on: https://chromium-review.googlesource.com/674748
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503126}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: b2ca65cb5a61eb6f69a573a336cbd1f9ea794fcd
nareix pushed a commit to nareix/webrtc.third_party that referenced this pull request Mar 5, 2018
The web platform tests testing module script errors are not deterministic
until we have the new algorithm being proposed at HTML PRs below:
- whatwg/html#2971
- whatwg/html#2991

This CL marks the test as flaky until we land the new algorithm.

TBR=hiroshige@chromium.org

Bug: 767093, 763597
Change-Id: I9ef17f2ae6dc51acbef4dbab1f7f7c7438d17eb7
Reviewed-on: https://chromium-review.googlesource.com/676403
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503304}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: e35c1229109f3eb213a4f62ebb27b1b864e5ba10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants