Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Oct 25, 2023

No description provided.

@rniwa rniwa force-pushed the add-cache-resources-option branch from 9c887f6 to 4260e87 Compare October 25, 2023 21:58
@bgrins
Copy link
Contributor

bgrins commented Oct 25, 2023

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);
Copy link
Contributor

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?

@camillobruni
Copy link
Contributor

I think it would still make sense to have this step for slower network speeds.
We've occasionally seen different scores on v2.1 depending on how slow/fast the network is.
Hopefully this would limit the effects a bit?

We could give it a shot with ts_proxy or a similar setup?

@bgrins
Copy link
Contributor

bgrins commented Nov 2, 2023

I think it would still make sense to have this step for slower network speeds.

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.

@camillobruni
Copy link
Contributor

On our perf waterfall we measure with a clean profile, which adds additional noise for the first load (and even worse for local runs).
I'd say it's not something we want to cover directly in Speedometer v3, so I'd vouch for limiting these effects.

@bgrins
Copy link
Contributor

bgrins commented Nov 2, 2023

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).

@bgrins
Copy link
Contributor

bgrins commented Nov 2, 2023

Here are a couple of preview URLs with headers for all resources:

Cache-Control: public, max-age=15552000: https://with-cache-headers--speedometer-preview.netlify.app/
Cache-Control: no-store: https://with-no-store--speedometer-preview.netlify.app

@camillobruni
Copy link
Contributor

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.

@rniwa
Copy link
Member Author

rniwa commented Nov 15, 2023

The consensus here seems to abandon this PR, and instead pursue adding caching header on browserbench.org instead.

@rniwa rniwa closed this Nov 15, 2023
@rniwa rniwa deleted the add-cache-resources-option branch February 29, 2024 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants