script: Unify script-based "update the rendering" and throttle it to 60 FPS #38431
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
95809ce to
b857773
Compare
|
🔨 Triggering try run (#16695594746) for Linux (WPT) |
|
Marked as draft because I want to add more documentation to the code before landing this. |
|
Test results for linux-wpt from try job (#16695594746): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (23)
Stable unexpected results (3)
|
|
|
This PR introduces script driven "update the rendering", right? We need to think what would that mean for #37776. |
Sorry, I meant to write "less work." The ScriptThread already does its own "update the rendering" calls when it doesn't expect to receive ticks from the renderer (when it isn't producing display lists). In addition, the ScriptThread currently runs "update the rendering" whenever receiving an IPC message, but without running rAFs. This change makes it so that the ScriptThread runs "update the rendering" in a spec-compliant way (running rAFs), but driven by a timer and only when it detects that it's necessary. I think it brings the ScriptThread-driven "update the rendering" closer to the specification. |
b857773 to
48e14be
Compare
|
🔨 Triggering try run (#16703627347) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16703627347): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (22)
|
|
✨ Try run (#16703627347) succeeded. |
Ah, thanks for explanation. I see the bigger picture now. |
48e14be to
cbf0858
Compare
|
🔨 Triggering try run (#16707542911) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16707542911): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (21)
|
|
✨ Try run (#16707542911) succeeded. |
sagudev
reviewed
Aug 4, 2025
Comment on lines
+2339
to
+2341
| if self.font_context.web_fonts_still_loading() != 0 { | ||
| return; | ||
| } |
There was a problem hiding this comment.
thought: Would it make sense to use fonts.waiting_to_fullfill_promise here?
There was a problem hiding this comment.
I agree that this makes sense, though I would prefer to do it in a followup as I'm afraid of introducing too many more behavior changes in this one PR.
sagudev
reviewed
Aug 4, 2025
Comment on lines
+17
to
+18
| window.requestAnimationFrame(() => { | ||
| window.requestAnimationFrame(test.step_func_done(function () { |
There was a problem hiding this comment.
Hm, why do we need 2 rAFs to make this work. That does not seem right.
On related note, why isn't this tests upstreamed to WPT? I do not see anything servo specific.
There was a problem hiding this comment.
I believe this needs two frames because the resize needs to be processed according to the specification in "update the rendering":
- Forced layout: The
<iframe>element resizes, but outside of "update the rendering." - First
requestAnimationFramecallback, theDocumentinside the<iframe>element is resized according to the<iframe>resize during "update the rendering". - Second
requestAnimationFramecallback: The corresponding media-dependent styles are applied after theDocumentis resized.
It's pretty common in WPT that these sort of tests wait for two animation frames before verifying behavior. Perhaps this could be improved, but maybe it's a bit outside the scope of this change. I think this test was passing accidentally before because all IPC messages were forcing "update the rendering" between rAF callbacks, which isn't spec behavior.
There was a problem hiding this comment.
This works without any rAFs in both firefox and chromium:
window.onload = test.step_func(function() {
var frame = document.getElementById("myframe");
var frameDoc = frame.contentWindow.document;
var element = frameDoc.getElementById("testelement");
assert_equals(frame.contentWindow.getComputedStyle(element).backgroundColor, "rgb(255, 0, 0)");
frame.width = "300";
frameDoc.documentElement.offsetWidth; // Force layout
test.step_func_done(function () {
assert_equals(frame.contentWindow.getComputedStyle(element).backgroundColor, "rgb(0, 255, 0)");
})()
});
There was a problem hiding this comment.
I suspect there is something wrong about the way we are processing updates here (either the restyle or the media query update) or else Firefox and Chromium have a more optimized implementation. In any case, I think this issue existed before my change.
There was a problem hiding this comment.
Indeed, we should open issue about that.
There was a problem hiding this comment.
I've opened #38446 for this.
5c370be to
17748c6
Compare
sagudev
approved these changes
Aug 4, 2025
|
@sagudev Thank you for the review! |
|
TIMEOUT [expected PASS] /resize-observer/iframe-same-origin.html Might be related to #38446 |
|
The issue here seems to be that when a ResizeObserver starts observing, we need to run "update the rendering" because it should deliver an event before a resize happens. It seems that we need to check to see if we've started observing in |
…60 FPS Instead of running "update the rendering" at every IPC message, only run it when a timeout has occured in script. In addition, avoid updating the rendering if a rendering update isn't necessary. This should greatly reduce the amount of processing that has to happen in script. Because we are running many fewer calls to "update the rendering" it is reasonable now to ensure that these always work the same way. In particular, we always run rAF and update the animation timeline when updating the ernder In addition, pull the following things out of reflow: - Code dealing with informing the Constellation that a Pipeline has become Idle when waiting for a screenshot. - Detecting when it is time to fulfill the `document.fonts.ready` promise. The latter means that reflow can never cause a garbage collection, making timing of reflows more consistent and simplifying many callsites that need to do script queries. Followup changes will seek to simplify the way that ScriptThread-driven animation timeouts happen even simpler. Signed-off-by: Martin Robinson <mrobinson@igalia.com> Co-authored-by: Oriol Brufau <obrufau@igalia.com>
17748c6 to
8b8e081
Compare
|
🔨 Triggering try run (#16727928517) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16727928517): Flaky unexpected result (29)
Stable unexpected results that are known to be intermittent (25)
|
|
✨ Try run (#16727928517) succeeded. |
PotatoCP
added a commit
to PotatoCP/servo
that referenced
this pull request
Aug 6, 2025
…60 FPS (servo#38431) (wd) {"fail_fast": false, "matrix": [{"name": "Linux (WPT)", "workflow": "linux", "wpt": true, "profile": "release", "unit_tests": false, "build_libservo": false, "bencher": false, "build_args": "", "wpt_args": "./tests/wpt/tests/webdriver/tests/classic/ --product servodriver --headless --processes 1", "number_of_wpt_chunks": 2}]}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of running "update the rendering" at every IPC message, only run
it when a timeout has occured in script. In addition, avoid updating the
rendering if a rendering update isn't necessary. This should greatly
reduce the amount of processing that has to happen in script.
Because we are running many fewer calls to "update the rendering" it is
reasonable now to ensure that these always work the same way. In
particular, we always run rAF and update the animation timeline when
updating the ernder
In addition, pull the following things out of reflow:
become Idle when waiting for a screenshot.
document.fonts.readypromise.
The latter means that reflow can never cause a garbage collection,
making timing of reflows more consistent and simplifying many callsites
that need to do script queries.
Followup changes will seek to simplify the way that ScriptThread-driven
animation timeouts happen even simpler.
Testing: In general, this should not change testable behavior so much, though it
does seem to fix one test. The main improvement here should be that
the ScriptThread does less work.