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

Should two different defer scripts execute in the same task? #6230

Open
rik opened this issue Dec 15, 2020 · 18 comments
Open

Should two different defer scripts execute in the same task? #6230

rik opened this issue Dec 15, 2020 · 18 comments

Comments

@rik
Copy link

rik commented Dec 15, 2020

With the following HTML

<script src="long-download.js" defer></script>
<script src="short-download.js" defer></script>

If short-download.js has finished being fetched before long-download.js then both will execute in the same task.

This is visible in a Glitch demo (with 3 scripts in the same task) Glitch project to remix

Use case

While trying to split our code into several smaller scripts to prevent long blocking tasks, we noticed some defer scripts would sometime run in the same task, defeating the purpose of our "optimisation". The "sometime" was the race condition described here (that can happen because of network conditions or caches).

(conversation started in a Twitter thread with @jakearchibald)

@Yay295
Copy link
Contributor

Yay295 commented Dec 15, 2020

I don't see anything in the spec that says each script runs in its own task. https://html.spec.whatwg.org/multipage/scripting.html#the-script-element
The only two references to a task at all in this section are in step 26: "If ..., queue a task to fire an event named error at the element, and return."

Using setTimeout(fun,0) is an easy way to get your code to run in separate tasks though.

@Kaiido
Copy link
Member

Kaiido commented Dec 16, 2020

Per current texts, UAs should "spin the event loop" while waiting for the first script in the list to be ready.
There is an implied "queue a task" call made from spin the event loop, but it is conceptually the same task that gets queued again (it gets "resumed").

Usually "spin the event loop" should allow for rendering updates, but here it's quite unclear if UAs actually are forced to enter the "spin the event loop" algorithm since when script one execution is done, script two and three already have their "ready to be parser-executed" flags set since about two and four full seconds respectively.
So I guess it boils down to how should one interpret "do x until something that already occurred".

@rik
Copy link
Author

rik commented Dec 16, 2020

So I guess it boils down to how should one interpret "do x until something that already occurred".

Which is not ideal for spec text, right? :)

Using setTimeout(fun,0) is an easy way to get your code to run in separate tasks though.

I thought about this but need to confirm that the order of execution is still guaranteed. From timer initialization steps, I think step 16 gives that guarantee. "Wait until any invocations of this algorithm that had the same method context, that started before this one, and whose timeout is equal to or less than this one's, have completed."

@domfarolino
Copy link
Member

Nice investigation @Kaiido! Yeah this is tricky. Per spec, we spin the event loop until condition goal happens, and I think a strict interpretation of the spec here always has us invoking spin the event loop ; its just that by the time we actually check goal, we're already in parallel. When goal is already met, we immediately post the task to come back, but it is still async with respect to the "main thread".

I think the next step would be to figure out what all browsers do here and see how it compares to the spec. We can can either get them to converge or change the spec (although it sounds like the spec's behavior may be ideal, albeit implicit). @rik would you be able to investigate into what other browsers are doing here? Maybe a test like your Glitch demo would work to collect this data.

As we discussed Chrome will execute all "ready" deferred scripts in one task as per https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/script/html_parser_script_runner.cc;l=472;drc=50cb1c9e9c1eb0cc9e42fd5519d44e3401116ac3 but there was some talk a while back about changing this, it's just a matter of project priority.

@rik
Copy link
Author

rik commented Dec 23, 2020

@domfarolino Sorry I should have included this info in my original report. On my Mac, Safari 14.0.2, Edge 87.0.664.66 and Firefox 84.0.1 all scripts seem to execute in the same task.

@noamr
Copy link
Contributor

noamr commented Feb 16, 2023

AFAIU "spin the event loop" is like an async/await and always posts a task when it's done to continue. It means that currently the spec actually separates the deferred scripts by one per task, but the implementations do not. @domenic maybe has some historical context for this?

@noamr
Copy link
Contributor

noamr commented Feb 22, 2023

Tracking this in chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1418367

@hiroshige-g
Copy link
Contributor

I don't know its context, but I think this is one of many code paths around parsing end that are sync in Chromium but should be async in the spec. Actually, the entire https://html.spec.whatwg.org/multipage/parsing.html#stop-parsing can be executed in a single task in Chromium (e.g. https://bugs.chromium.org/p/chromium/issues/detail?id=961428).

Code change in Chromium to make this spec conformant would be simple. It might cause performance regressions due to additional async tasks (and performance improvements by avoiding long tasks) and might break websites in the wild (there are many websites that depend on this kind of Chromium's sync behavior despite not spec conformant), but probably they are not big blockers.

BTW in another similar spin-the-event-loop usage https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-incdata:spin-the-event-loop, we don't enter spin-the-event-loop if the condition is already met.

@rik
Copy link
Author

rik commented Feb 23, 2023

there are many websites that depend on this kind of Chromium's sync behavior despite not spec conformant

I'm not sure websites can rely on the sync behaviour since the grouping in one long task is dependant on a race condition.

@zcorpan
Copy link
Member

zcorpan commented May 8, 2023

The spec matches what is implemented as described here in that "spin the event loop" doesn't invoke "update the rendering".

Defer scripts are run during "the end" which runs spin the event loop between each defer/module script. So setTimeout(fn, 0) ought to run between such scripts, but rendering is not updated.

I'm not sure if there are reasons for this, it seems reasonable to allow rendering updates between scripts.

@noamr
Copy link
Contributor

noamr commented May 8, 2023

The spec matches what is implemented as described here in that "spin the event loop" doesn't invoke "update the rendering".

Ah, thanks for the clarification

Defer scripts are run during "the end" which runs spin the event loop between each defer/module script. So setTimeout(fn, 0) ought to run between such scripts, but rendering is not updated.

I'm not sure if there are reasons for this, it seems reasonable to allow rendering updates between scripts.

A plausible reason would be to prevent flashing as well as reducing redundant layouts based on partial state.
In some scenarios those would be a problem, and in other long tasks blocking input would be a problem. It's hard to say in advance which is a better strategy.

@Kaiido
Copy link
Member

Kaiido commented May 9, 2023

I don't follow why "update the rendering" would have to be called explicitely. By queuing a task (6.2) you're basically enabling the whole event loop iteration to be processed, and you have to go through "update the rendering" before your task is executed (moreover if you expect a timer task to fill in between).
Sure, UAs are allowed to early exit in update the rendering, but that rendering can happen, (and here, from the glimpse we have of the browser's heuristics, it should since the page has been blocked for more than a VSync interval).

@domfarolino
Copy link
Member

The spec matches what is implemented as described here [...]
...
Defer scripts are run during "the end" which runs spin the event loop between each defer/module script. So setTimeout(fn, 0) ought to run between such scripts

I'm confused. You're saying the spec would run setTimeout() in between defer scripts, but isn't the OP about the implementation not doing that, i.e., they run in the same task? Doesn't that mean the spec does not match what is implemented?

@zcorpan
Copy link
Member

zcorpan commented May 9, 2023

@domfarolino the glitch demo only tests changing styles, so I commented on why that doesn't happen per spec. I haven't tested what browsers do for setTimeout.

@Kaiido https://html.spec.whatwg.org/multipage/webappapis.html#spin-the-event-loop (called between defer scripts) is different from normal processing of the event loop https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

@Kaiido
Copy link
Member

Kaiido commented May 9, 2023

@Kaiido https://html.spec.whatwg.org/multipage/webappapis.html#spin-the-event-loop (called between defer scripts) is different from normal processing of the event loop https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

Could you clarify how it is different when we're between defer scripts?
The event loop must be processing at this step, otherwise we couldn't "spin" it. If the event loop is really following another processing model algorithm, could you point to where it's being defined? There must be some steps that will at least pick the queued tasks and execute them, (e.g. like what's done in navigate HTML with the networking tasks).
Spin the event loop says, pause the current JS execution; when we're ready, post a new task (step 6.2) that will resume our paused JS execution. So we're at least one task after. And since I don't see another processing model than the one you linked to for our case, this means to me that "update the rendering" should be met here.

@zcorpan
Copy link
Member

zcorpan commented May 10, 2023

Hmm, reading the algorithm for spin the event loop again I think I had the wrong idea of how it works. Indeed it doesn't have a loop to run tasks and check goal, it just waits in parallel until goal is met. So the normal event loop processing must happen in the meantime. Apologies for the confusion.

@domfarolino
Copy link
Member

So we're all aligned that the spec as-is will not run defer scripts in the same task, and between the execution of two defer scripts we fully defer to the event loop which may run other queued tasks and indeed update the rendering?

@noamr
Copy link
Contributor

noamr commented May 11, 2023

So we're all aligned that the spec as-is will not run defer scripts in the same task, and between the execution of two defer scripts we fully defer to the event loop which may run other queued tasks and indeed update the rendering?

Yes! Though rendering opportunity is implementation-specific and mentions "user agent throttling for performance reasons". Throttling rendering due to having pending scripts would be per-spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants