-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add an option to prime the network cache by loading each test suite in an iframe first. #330
Conversation
…n an iframe first.
9c887f6
to
4260e87
Compare
IIRC the server doesn't set cache headers (and if it did, then the reported effect would be at least mitigated by being contained within the first iteration without requiring a first pass like this) |
@@ -346,7 +346,7 @@ export class BenchmarkRunner { | |||
const iterationEndLabel = "iteration-end"; | |||
for (let i = 0; i < iterationCount; i++) { | |||
performance.mark(iterationStartLabel); | |||
await this._runAllSuites(); | |||
await this._runAllSuites(!i && params.cacheResources); |
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.
How about putting this as explicit separate "cacheResources"
step with explicit labels?
I think it would still make sense to have this step for slower network speeds. We could give it a shot with ts_proxy or a similar setup? |
But would this have any effect today, since the server isn't sending cache headers? If/when we started caching then any potential benefit to this change shrinks since 9/10 iterations will already be able skip the network. And on the margin I worry this change will incentive engines to do too-aggressive per-process/tab caching the first time a page is loaded (because it will improve the score by pulling work out of measured iterations). That's not really a good outcome - lots of pages are just loaded once and it'd be better have that be faster than to speed up potential subsequent load+interactions that may not ever happen. |
On our perf waterfall we measure with a clean profile, which adds additional noise for the first load (and even worse for local runs). |
The most straightforward way to mitigate any network speed side effects would be to update browserbench.org to set cache headers for static content. Without doing that, AFAICT this PR won't have any practical effect other than incentivizing engines to warm up JIT caches etc during page load (which seems like a bad incentive). |
Here are a couple of preview URLs with headers for all resources:
|
At least the effects on v2.1 are not mitigate with cache-headers (maybe this is a different issue here?) The main issue is that on slower-networks you keep the CPU busy for longer (and thus warm), while sometimes on faster networks we drop to the lower clockspeeds in the setTimeout delays. Let's have this as an option in the developerMode so we can experiment easily with it. Whether or not we enable this by default is another discussion. |
The consensus here seems to abandon this PR, and instead pursue adding caching header on browserbench.org instead. |
No description provided.