-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(page-functions): expose simulated throttling requestIdleCallback shim #11032
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
-disable-storage-reset -clear origin storage
…re applying the shim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you very much @LukasAuerMstage this looks great! 🎉
lighthouse-core/gather/driver.js
Outdated
*/ | ||
async registerRequestIdleCallbackWrap(settings) { | ||
if (settings.throttlingMethod === 'simulate') { | ||
const scriptStr = `(${pageFunctions.wrapRequestIdleCallbackString})()`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's pass through the multiplier to the function so it doesn't have to hardcode the deadline
const scriptStr = `(${pageFunctions.wrapRequestIdleCallbackString})()`; | |
const scriptStr = `(${pageFunctions.wrapRequestIdleCallbackString})(${settings.throttling.cpuSlowdownMultiplier)`; |
* RequestIdleCallback shim that calculates the remaining deadline time in order to avoid a potential lighthouse | ||
* penalty for tests run with simulated throttling. As the throttling factor is x4 the maximum remaining time is | ||
* reduced to approximately 50/4 => ~12 | ||
* @return {null} | ||
*/ | ||
/* istanbul ignore next */ | ||
function wrapRequestIdleCallback() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* RequestIdleCallback shim that calculates the remaining deadline time in order to avoid a potential lighthouse | |
* penalty for tests run with simulated throttling. As the throttling factor is x4 the maximum remaining time is | |
* reduced to approximately 50/4 => ~12 | |
* @return {null} | |
*/ | |
/* istanbul ignore next */ | |
function wrapRequestIdleCallback() { | |
* RequestIdleCallback shim that calculates the remaining deadline time in order to avoid a potential lighthouse | |
* penalty for tests run with simulated throttling. Reduces the deadline time to (50 - safetyAllowance) / cpuSlowdownMultiplier to | |
* ensure a long task is very unlikely if using the API correctly. | |
* @param {number} cpuSlowdownMultiplier | |
* @return {null} | |
*/ | |
/* istanbul ignore next */ | |
function wrapRequestIdleCallback(cpuSlowdownMultiplier) { | |
const safetyAllowanceMs = 10; | |
const maxExecutionTimeMs = Math.floor((50 - safetyAllowanceMs) / cpuSlowdownMultiplier); |
const start = Date.now(); | ||
deadline.__timeRemaining = deadline.timeRemaining; | ||
deadline.timeRemaining = () => { | ||
return Math.min(deadline.__timeRemaining(), Math.max(0, 12 - (Date.now() - start))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Math.min(deadline.__timeRemaining(), Math.max(0, 12 - (Date.now() - start))); | |
return Math.min(deadline.__timeRemaining(), Math.max(0, maxExecutionTimeMs - (Date.now() - start))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need to reflow with line length :/
requestedUrl: 'http://localhost:10200/ric-shim.html?short', | ||
finalUrl: 'http://localhost:10200/ric-shim.html?short', | ||
audits: { | ||
'interactive': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TTI can be super flaky on test machines depending on scheduling of network too, let's use total blocking time instead which is strictly time spent in tasks beyond 50ms :)
const timeStart = Date.now(); | ||
while (true) { | ||
let elapsedTime = Date.now() - timeStart; | ||
if (elapsedTime > milliseconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (elapsedTime > milliseconds) { | |
if (elapsedTime >= milliseconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise sleep(1)
will always sleep for at least 2 ms, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah thats right, i missed that
const iterations = 40; | ||
|
||
if (window.location.search.includes('short')) { | ||
window.ricTaskDuration = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seems like this window should just be a let ricTaskDuration = 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i remember correctly let/const dont move up into the global scope if they are in some sort of curly braces but i could do something like this, wdyt? Or just use var if you prefer that because that should just move up.
let ricTaskDuration;
if (window.location.search.includes('short')) {
ricTaskDuration = 1;
}
if (window.location.search.includes('long')) {
ricTaskDuration = 50;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sorry that's what I meant, declaring let ricTaskDuration = 0
up with iterations
and then overriding it if the query param matches
- pass cpu slowdown multiplier to rIC shim wrapper and calculate the maximum execution time dynamically - adjust rIC shim function comment - refactor smoke tests so that they use tbt instead of tti - fix bug in sleep function (was previously 1ms off) - add copyright header to smoke test html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,47 @@ | |||
<!DOCTYPE html> | |||
<!-- | |||
* Copyright 2018 The Lighthouse Authors. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright 2018 The Lighthouse Authors. All Rights Reserved. | |
* Copyright 2020 The Lighthouse Authors. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great :) done
'total-blocking-time': { | ||
// With the requestIdleCallback shim in place 1ms tasks should not block at all and should max add up to | ||
// 12.5 ms each, which would result in 50ms under a 4x simulated throttling multiplier and therefore in 0 tbt | ||
numericValue: '<=0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can give it a little bit of slack to avoid flakiness, maybe <=100
? going from 5500 -> 100 is still great :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense 👍 done
- change copyright header date to 2020 - change tbt smoke test value to <=100 (from previously 0) to add some room for a little variance
Thanks @LukasAuerMstage looks all good! Unfortunately you'll need to rebase after #11035 lands to fix the required status checks that are failing atm. |
@patrickhulce as a follow up at some point, could we document this somewhere? It'll be easy to forget this is happening at runtime (since most lantern stuff happens after the fact). Would |
Yeah a little "Lantern Considerations" type deal might be good to get a running list of things that need to happen for lantern to work well. |
@@ -303,6 +303,39 @@ function getNodeLabel(node) { | |||
return tagName; | |||
} | |||
|
|||
/** | |||
* RequestIdleCallback shim that calculates the remaining deadline time in order to avoid a potential lighthouse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* RequestIdleCallback shim that calculates the remaining deadline time in order to avoid a potential lighthouse | |
* RequestIdleCallback shim that calculates the remaining deadline time in order to avoid a potential |
const nativeRequestIdleCallback = window.requestIdleCallback; | ||
window.requestIdleCallback = (cb) => { | ||
const cbWrap = (deadline, timeout) => { | ||
const start = Date.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will performance.now()
work too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will, we're like split 50/50 throughout the smokes on Date.now vs. performance.now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Date.now()
is faster than performance.now()
thats why i chose it, but i dont know if it matters all that much in this case. Do you want me to refactor to performance.now()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary @LukasAuerMstage but @connorjclark were you hoping to slowly standardize on performance.now?
Summary
This PR introduces a request idle callback shim that gets applied for lighthouse tests that are run with simulated throttling.
The feature is needed as the use of the request idle callback deadline defeats the simulated throttling logic. Until the deadline hits 0 tasks can be processed within the request idle callback, which adds up to a maximum of 50ms. Lighthouse will then multiply these 50ms with 4 if simulated throttling is active, which results in a long task.
This riC shim calculates the remainig deadline with this issue in mind and will deliver a value of 12ms or less, so that once the simulated throttling is applied, the overall time consumed does not surpass 50ms and no long task gets registered.
Related Issues/PRs
#10693