Refactor event-loop processing model to perform rendering as a task

[noamr]

Instead of "updating the rendering" after each task, we wait for rendering opportunities in parallel, and post a special task to update the rendering. This is more in line with existing implementations, and fixes weird cases where e.g. a microtrask queue can run without a current task.

See #9824
(Not a behavior change)

• At least two implementers are interested (and none opposed):
  • N/A: this is supposed to reflect current status, not a behavior change
• Tests are written and can be reviewed and commented upon at:
  • This is observable via the longtasks API, see https://wpt.fyi/results/longtask-timing/longtask-in-raf.html
• Implementation bugs are filed:
  *N/A
• The top of this comment includes a clear commit message to use.

---

💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 31, 2024, 2:53 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

error code: 524

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

[Kaiido]

Thanks for this, this seems closer to the observed behavior indeed.
I just have a few (non-authoritative) small notes regarding the design, but it seems to work well overall.

[smaug----]

FWIW, this seems relatively close to what Gecko does, or perhaps I should say very close.

[Kaiido]

For the little it's worth, LGTM now.

I just wonder if we should have WPTests for this?
You said in #9824 that the fact it's a task is visible through the longtask-timing API, and we can probably add a small test that the timestamp is correctly updated after a long task (testing for the race-condition case), but maybe it's already there, I must admit I didn't check yet.
WDYT?

[noamr]

For the little it's worth, LGTM now.

I just wonder if we should have WPTests for this? You said in #9824 that the fact it's a task is visible through the longtask-timing API, and we can probably add a small test that the timestamp is correctly updated after a long task (testing for the race-condition case), but maybe it's already there, I must admit I didn't check yet. WDYT?

It's already there! I actually became interested in fixing this after finding out that this test passes but shouldn't:
https://wpt.fyi/results/longtask-timing/longtask-in-raf.html

[Kaiido]

Ah great.
But it just crossed my mind that maybe the Worker event-loop's update the rendering may also need the same refactor?

[noamr]

Ah great. But it just crossed my mind that maybe the Worker event-loop's update the rendering may also need the same refactor?

Workers don't update the rendering.

[Kaiido]

Workers don't update the rendering.

Oh, yes they do. That's how we do update the placeholder <canvas> of an OffscreenCanvas that lives in a Worker.

However the implications for a Worker are a bit different in that close() is supposed to discard all queued tasks, but that'd would be problematic for the update the rendering tasks, as we actually probably want the update the rendering to happen in such case, or at least a lighter version of it without rAF callbacks like Firefox currently does: https://glitch.com/edit/#!/raf-in-closing-worker ¹. This will become even more problematic once #9979 lands (if it ever lands...), but the current status is not interoperable anyway, so you can probably simply move the worker's update the rendering along with the window's one without much issue, and we'll handle the problematic cases later on separately.

But if you don't feel confident doing it here, I guess that can also be tackled in a follow-up PR, though I should note (mostly as a reminder to myself) that at least in #9979 I was expecting the link to update the rendering would encompass both the window and worker event-loops versions, which is not the case anymore with the changes here.

Footnotes

1. In case the editor fails to open, the output is at https://raf-in-closing-worker.glitch.me/ ↩

[noamr]

Workers don't update the rendering.

Oh, yes they do. That's how we do update the placeholder <canvas> of an OffscreenCanvas that lives in a Worker.

However the implications for a Worker are a bit different in that close() is supposed to discard all queued tasks, but that'd would be problematic for the update the rendering tasks, as we actually probably want the update the rendering to happen in such case, or at least a lighter version of it without rAF callbacks like Firefox currently does: https://glitch.com/edit/#!/raf-in-closing-worker 1. This will become even more problematic once #9979 lands (if it ever lands...), but the current status is not interoperable anyway, so you can probably simply move the worker's update the rendering along with the window's one without much issue, and we'll handle the problematic cases later on separately.

But if you don't feel confident doing it here, I guess that can also be tackled in a follow-up PR, though I should note (mostly as a reminder to myself) that at least in #9979 I was expecting the link to update the rendering would encompass both the window and worker event-loops versions, which is not the case anymore with the changes here.

Footnotes

1. In case the editor fails to open, the output is at https://raf-in-closing-worker.glitch.me/ ↩

Oh yes, the update-canvas-from-worker thing. I think we should do that in a follow up.

[domenic]

This is a tough one...

[noamr]

Thanks, it's a tough one indeed... Fixed some and commented on some.

[domenic]

Thanks for all the work here! I did a final round of nit-fixing and formatting, and will now merge.