Issue 769151 in chromium: ☂ Modules fetched at low priority

[kou… via monorail]

Updates:
Blockedon: 763597
Cc: modul...@chromium.org

Comment #1 on issue 769151 by kou...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c1

(No comment was entered for this change.)

--
You received this message because:
1. You were specifically CC'd on the issue

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

Reply to this email to add a comment.

[ksakam… via monorail]

Comment #2 on issue 769151 by ksak...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c2

This meta bug tracks the efforts improve ES6 modules fetch requests priority.

Currently, ES6 modules are fetched at Low priority (same priority as async classic scripts).

After http://crrev.com/c/658016, HTML preload scanner triggers a fetch at High priority, as soon as it sees a <script type=module> tag. But preload scanner only sees the root node of the dependency tree; its dependent modules are still fetched at Low priority.

Link rel=modulepreload (bug 740886) could be a workaround of this, if we make modulepreload requests High priority.

[dome… via monorail]

Comment #3 on issue 769151 by dom...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c3

Why do we need a workaround? Why can't we just change Blink to perform dependent module fetches at high priority instead of low?

[ksakam… via monorail]

Comment #4 on issue 769151 by ksak...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c4

I think that's a good point. Non-async/defer classic scripts are loaded at high priority because they're parser-blocking. Modules are not blocking, but considering that
- module scripts are async by default, and
- there's no way to specify module scripts as critical resource,
it might makes sense to fetch modules at high(er) priority.

Caveat: that might have negative impact when modules are not critical for above-the-fold rendering (e.g. server-side rendered pages).

[y… via monorail]

Updates:
Cc: y...@yoav.ws

Comment #5 on issue 769151 by y...@yoav.ws: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c5

Sounds like either choice would be result in perf benefits if one case and perf regressions in the other (one case being "modules contain page critical functionality" and the other being "modules contain non-critical functionality").
Do we have any data indicating what do modules do today or what they are likely to do in the near future?
Lacking that data, the safe middle-ground would be to set the module priority to "medium".

In general, it seems like this is a good use case for the upcoming Priority Hints spec: https://github.com/WICG/priority-hints

[ksakam… via monorail]

Comment #6 on issue 769151 by ksak...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c6

Currently modules are used only in 0.0001% page loads [1] so I don't think we can make a good decision based on today's usage.
Also, our decision here could affect the usage pattern (e.g. if we make modules fetched at high priority, people may use it in render critical cases, and vice versa).

[1] https://www.chromestatus.com/metrics/feature/timeline/popularity/2062

[phist… via monorail]

Comment #7 on issue 769151 by phis...@gmail.com: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c7

What about adding to the specification somehow (as a note?) that <script type="module" async> or <script type="module" src="..." async> sets the priority for the dependencies to medium/low, otherwise high?

[y… via monorail]

Comment #8 on issue 769151 by y...@yoav.ws: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c8

Is `<script type=module async>` currently a thing? If not, since script modules are not actually sync, it'd be better to let the future Priority Hints markup to cover setting of explicit priority.

[dome… via monorail]

Comment #9 on issue 769151 by dom...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c9

> Caveat: that might have negative impact when modules are not critical for above-the-fold rendering (e.g. server-side rendered pages).

Such non-critical modules should be loaded dynamically (e.g. using import()). Maybe import() should be a lower priority; not sure.

Given that there's not a lot of module usage today, we should encourage this pattern, and I think setting <script type=module> priority to high is a good way of shaping the ecosystem.

> What about adding to the specification somehow

Priorities are not a specification concept; they are a quality of implementation issue for browsers to compete on.

> Is `<script type=module async>` currently a thing?

Yes, it is. It says to execute modules ASAP, instead of defering them until after parsing is finished. (Similar to <script async>.)

> it'd be better to let the future Priority Hints markup to cover setting of explicit priority.

We still need to pick a good default, and I don't think we should block that on this speculative future proposal.

[kou… via monorail]

Comment #10 on issue 769151 by kou...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c10

TL;DR: I'm supportive of ksakamoto@'s proposal of changing <script type=module async> priority from Low to High.

To summarize:
| | current | kouhei |
| <s> | H | ← |
| <s defer> | L | ← |
| <s async> | L | ← |
| <s t=m (implies defer)> | L | L? |
| <s t=m async> | L | H |
| dynamic import() | L | H? |

For <s t=m>, dynamic import(), kouhei@ can be convinced that it should be either L/H. I have a small preference for the following reasons:
- my personal understanding of the semantics is that <s t=m async> should be used when the module tree contains page's essential logic (which needs to be run in order for user to start consuming content / starting interaction w/ the page). <s t=m (w/o async)> should be used when the module tree doesn't quite fit the criteria above and provides add-on optional features (+1 buttons, ads, etc.).
- My understanding of dynamic import() use-case is that they are triggered when user actually requests the feature provided by the target module tree, so it feels like they should be H.

>> it'd be better to let the future Priority Hints markup to cover setting of explicit priority.
>We still need to pick a good default, and I don't think we should block that on this speculative future proposal.

I'm on the same page here. Priority Hints should help users by allowing to change priority if they have a special use case, but I think we should have a default priority which would be optimal for majority of the use cases.

[kin… via monorail]

Comment #11 on issue 769151 by kin...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c11

Just to give my thoughts here- I'm also supportive of the proposal of kouhei@ and ksakamoto@-san. (And I'm also all for letting Priority Hints cover more specific use cases- yay)

One thing that made me wonder a bit is if we should differentiate default priorities for <script type=module> and <script type=module async> or not, because doing that would likely also incentivize developers to start using <s t=m async> when they want higher-priority and <s t=m> otherwise, so this could potentially have some impact on the future usage of modules.

That's said I could be convinced that it'd be a natural outcome given what the spec'ed behavior implies. If domenic@ and other people have no particular concerns on that point (i.e. making async one and the other have different priorities) I'm cool with the current proposal.

[dome… via monorail]

Comment #12 on issue 769151 by dom...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c12

I am not so sure about keeping non-async low. I would generally use non-async whenever I want to see the page loaded before my scripts starts---for example, if I am manipulating the page's DOM. That seems fairly common.

Encouraging async is in general good, but it seems a bit sad that people might have to hack around this by using e.g. <s t=m async> and then putting the contents of their script inside `document.addEventListener("DOMContentLoaded", () => { ... })` so they get high-priority fetches but also have a DOM that they can usefully operate on.

I'm not fully familiar with the impact of low priority though. If we're going to wait until the page is done parsing to execute anyway, does low vs. high make much of a difference?

[ksakam… via monorail]

Comment #13 on issue 769151 by ksak...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c13

Thanks Domenic. I didn't realize that by using async people may need to change their script code. Now I'm convinced that it's better not to couple the load priority with whether the script is async or not.

I agree with Kouhei's opinion that dynamic import should be high priority. So, all the three cases (w/o async, async, dynamic import) are High priority by default. For non-critical modules, developers may delay calling import() until critical resources are loaded.

I'm going to create a patch. Thanks all for the discussion!

[bugdro… via monorail]

Comment #14 on issue 769151 by bugd...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c14

The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/85b03d5db61929343df6dc0fd1100ce20c89a2c0

commit 85b03d5db61929343df6dc0fd1100ce20c89a2c0
Author: Kunihiko Sakamoto <ksak...@chromium.org&gt;
Date: Thu Oct 05 09:03:53 2017

ES6 Modules: Make module scripts fetched at High priority

Before this patch, module scripts were fetched at Low priority, like
as classic async / deferred scripts. But this may not be a good default,
since module scripts tend to have many sub module scripts and fetching
them all at low priority may be too slow. (See the listed bug for more
on the discussion.)

This patch changes the fetch priority of module scripts to High (same
as classic scripts without async / defer). This applies to all module
fetches, includes dynamic import.

Bug: 769151
Change-Id: I24aa22a886e8394969447785eaf1fe0ab2eea818
Reviewed-on: https://chromium-review.googlesource.com/701937
Reviewed-by: Kinuko Yasuda <kin...@chromium.org&gt;
Reviewed-by: Kouhei Ueno <kou...@chromium.org&gt;
Commit-Queue: Kunihiko Sakamoto <ksak...@chromium.org&gt;
Cr-Commit-Position: refs/heads/master@{#506685}
[modify] https://crrev.com/85b03d5db61929343df6dc0fd1100ce20c89a2c0/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp

[ad… via monorail]

Updates:
Cc: ad...@chromium.org

Comment #15 on issue 769151 by ad...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c15

(No comment was entered for this change.)

[ad… via monorail]

Comment #16 on issue 769151 by ad...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c16

> This patch changes the fetch priority of module scripts to High (same
as classic scripts without async / defer). This applies to all module
fetches, includes dynamic import.

I'm supportive of the switch to module scripts having a High priority. I was originally concerned that we weren't providing a path to lower-priority fetching module scripts (e.g if some modules I'm loading are less critical than others), but it seems fine to punt this problem to Priority Hints to solve.

We'll probably continue thinking about developer annotation of priority/importance assignment for resources including modules in https://github.com/WICG/priority-hints/issues/7

For cross-browser predictability, should we be encouraging other vendors to adopt a similar priority for modules + dynamic import()?

Thinking about perf, my understanding is that with this change, the 10K modules benchmark would have hit onload much faster which seems like a desirable win outside of just Chrome.

[kenjibah… via monorail]

Comment #17 on issue 769151 by kenji...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c17

Addy: yes, we should try to encourage other vendors to adopt a similar priority scheme or adjust if a reasonable counter-argument is brought up. Are you looking for help to do the outreach?

[bugdro… via monorail]

Comment #18 on issue 769151 by bugd...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c18

The following revision refers to this bug:

https://chromium.googlesource.com/chromium/src.git/+/a7c59dab3b05c03db34f6ffe918a303739490abd

commit a7c59dab3b05c03db34f6ffe918a303739490abd
Author: Kunihiko Sakamoto <ksak...@chromium.org&gt;
Date: Fri Oct 13 06:41:39 2017

ES6 Modules: Add a test case for module scripts' resource priority

Bug: 769151
Change-Id: I998ef83d7a5ef851501fdbef68970af774552ab8
Reviewed-on: https://chromium-review.googlesource.com/716016
Commit-Queue: Kunihiko Sakamoto <ksak...@chromium.org&gt;
Reviewed-by: Kouhei Ueno <kou...@chromium.org&gt;
Cr-Commit-Position: refs/heads/master@{#508622}
[modify] https://crrev.com/a7c59dab3b05c03db34f6ffe918a303739490abd/third_party/WebKit/LayoutTests/http/tests/devtools/network/resource-priority-expected.txt
[modify] https://crrev.com/a7c59dab3b05c03db34f6ffe918a303739490abd/third_party/WebKit/LayoutTests/http/tests/devtools/network/resource-priority.html

[ksakam… via monorail]

Updates:
Status: Fixed

Comment #19 on issue 769151 by ksak...@chromium.org: ☂ Modules fetched at low priority
https://bugs.chromium.org/p/chromium/issues/detail?id=769151#c19

(No comment was entered for this change.)