Avoid setTimeout in waitOnElement #321
Merged
Conversation
This file contains 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
julienw
reviewed
Oct 3, 2023
|
Default numbers on my machine: With this rAF PR: |
flashdesignory
approved these changes
Oct 4, 2023
Could you clarity what you mean by this? |
|
Generally we've seen some slow-downs due to CPU throttling that's caused by the initial load. Slower network does have an impact on the first iteration of the benchmark. And specifically for FlightJS prepare-phase there is a hard-coded 50ms delay, which is enough to clock the CPU down and cause an initial ramp-up delay on the sync phase. We recently had a bug where Chrome kept the CPU busy with an elaborate noop-loop (repeating posting of posting idle-tasks that did no work). By removing this we caused regressions on the FlightJS score since we no longer kept the CPU warm. If possible I'd like to not incentivise a "sunspider-mode" where we do some artificial GC work only to keep the CPU warm. I see various options here:
|
|
I like the option
That is, take only the second half of this PR which removes the setTimeout call. That seems like a clear and simple improvement, while the first half (which removes the rAF when the element exists) is less clear to me, and also affects much more content. |
bgrins
reviewed
Nov 8, 2023
| } else { | ||
| setTimeout(resolveIfReady, 50); | ||
| } | ||
| let callback = resolveIfReady; |
There was a problem hiding this comment.
This is a nit, but I'd find it simpler to follow if kept the basic structure in place and just replaced the setTimeout with a rAF
const resolveIfReady = () => {
const element = this.querySelector(selector);
if (element) {
window.requestAnimationFrame(() => {
resolve(element);
});
} else {
window.requestAnimationFrame(resolveIfReady);
}
};Alternatively we could refactor this to only look for the element inside the rAF, which could potentially lead to one less rAF if the element does end up existing after the first rAF is done.
const resolveIfReady = () => {
window.requestAnimationFrame(() => {
const element = this.querySelector(selector);
if (element) {
resolve(element);
} else {
resolveIfReady();
}
});
};There was a problem hiding this comment.
There is no real benefit to delay the lookup at this point, since the only workload where the content isn't there yet is the perf dashboard.
Eventually I'd like to remove the additional rAF delay here alltogether if it's not needed (but I prefer doing this separate to be able to easily measure the perf impact). WDYT?
There was a problem hiding this comment.
As discussed in the call, we can keep the implementation as you have here
|
Latest Numbers Default version (setTimeout-based)New version (rAF-based) |
bgrins
approved these changes
Nov 16, 2023
rniwa
approved these changes
Nov 16, 2023
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.
waitOnElementuses a 50ms timeout, which was historically motivated by FlightJS that used requirejs which had a hard-coded 50ms pause.We've observed fragile CPU scaling behavior with this approach in Speedometer 2.1. V3.0 is less affected by this since the perf-dashboard is the only workload the regularly needs another cycle to wait (all other workloads are ready after
onloadhas fired).Easy-change:
Replacing the 50ms with a rAF callback eliminates a potentially very long break when the element isn't ready yet.
Debatable change:
We can directly resolve the promise if the element is there and avoid another rAF callback. If we're using the rAF-based measurement, this should make no difference except reducing the idle time.
Detailed measurements coming up.