fix(driver): wait for CPU idle

[patrickhulce]

should fix #2475 (the issue raised in tastejs/hacker-news-pwas#63), the large class of remaining sites in #2293, and the last 10% that were left after #2347

🎉 🎉 🎉 🎉 🎉

Best explained by the comment:

/**
 * Installs a PerformanceObserver in the page to monitor for longtasks and resolves when there have
 * been no long tasks for at least waitForCPUQuiet ms. The promise will not resolve until the
 * `markAsResolvable` function on the return object has been called. This is to prevent promise resolution
 * before some important point in time such as network quiet or document load.
 * @param {number} waitForCPUQuiet
 * @return {{promise: !Promise, cancel: function(), markAsResolvable: function()}}
 */
_waitForCPUIdle(waitForCPUQuiet) {

This change also makes the pauseAfterNetworkQuiet heuristic no longer necessary, so it's been removed as well to keep our number of config flags the same.

[addyosmani]

Thanks for working on a fix to this, @patrickhulce! So far I think this would make a sizable difference to at least one of the issues we were previously seeing 🎉

[addyosmani]

Might be worth expanding comment to Network and CPU listeners or adding a similar line above ...const waitForCPUIdle for consistency.

[patrickhulce]

oops, meant to do that thanks!

[addyosmani]

Is the idea here to realign networkQuietThresholdMs and the new cpuQuietThresholdMs with the pauseAfterLoadMs delay instead of the lower pause thresholds previously being used?

[patrickhulce]

the historical background to pauseAfterNetworkQuiet was that we needed a way to account for CPU work after scripts have finished downloading, so it became the number for "what's the longest after network quiet we might reasonably expect a long task to occur"

now that this role has been subsumed by a bona fide CPU idle check, we don't need it anymore, however, we still want a little bit of slack in our cutoff thus 5250 instead of straight 5000

pauseAfterLoad is probably still worth keeping around for now as there are cases like the ember + fastboot where "quiet" can be reached too quickly without really reaching the load state, and it's good to see the full timeline as we try to improve the other heuristics

[addyosmani]

I'm not quite as close to this code as @brendankenny or @paulirish might be, but the general approach to using long-tasks here sgtm.

[brendankenny]

ah, this is a great idea.

evaluateScriptOnLoad will end up adding a performance observer on every pass with a non-0 cpuQuietThresholdMs, so what about just adding it once in gatherRunner.setupDriver in the style of driver.cacheNatives? I believe that would also allow dropping markAsResolvable, as it could just start checking as soon as network was done

[patrickhulce]

what about just adding it once in gatherRunner.setupDriver in the style of driver.cacheNatives

Getting rid of markAsResolvable would be nice, but I'm not sure the trade-off of moving this specific functionality that's only for waitForCPUIdle into setupDriver is that much better?

[patrickhulce]

bump :)

[wardpeet]

does it make sense to cache the native peformanceObserver as we do with Promise and Error?

[patrickhulce]

@wardpeet I think it's much less of a problem here since for Promise and Error we relied on them being the actual original classes for instanceof and protocol promise support. Here we just want the PerformanceObserver functionality without caring about the specific object it references. Much like we do with all the other browser APIs we use (Image, DOM APIs, setTimeout, etc)

[wardpeet]

@patrickhulce 👍 sounds reasonable 😄

Can't wait to have this landed!

[patrickhulce]

🏏 🏏

[patrickhulce]

🏏 🏏

[paulirish]

can we assert the rawValue is over 9000 ?
😀

[patrickhulce]

love it, gotta check those power levels

[paulirish]

this method description need an update. Can you flesh it out a bit so we won't have to look back up the PR when trying to read the code?

[patrickhulce]

done