refactor: set maxIdlePreloads to be constant over time #7846
Conversation
🦋 Changeset detectedLatest commit: cc50897 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
maiieul
force-pushed
the
set-maxIdlePreloads-to-be-constant-over-time
branch
from
125cd04 to
5e8d71c
Compare
commit: |
There was a problem hiding this comment.
I don't like this change: When a new preload request comes in, and it is higher priority than what is in the queue, it and its dependencies will have to wait until less urgent preloads are done.
By making the parallel amount depend on the probability, we have headroom for more urgent preloads.
Is that wrong?
|
I think this is wrong. With my versionWhen you set 20 max idle preloads and you click on something, you will have 20 ongoing preloads before the click, then after the click, the limit will extend to the amount of bundles to preload (e.g. ~50). Until the limit is down to 20 preloads, the browser won't register new preloads. So when it gets to 19, it will theoretically add a new preload, but will only start the downloading if it has enough bandwidth (this is decided by the browser). In practice, it seems that the event preloads all finish around the same time, and new idle preloads only start after the event preloads are finished. In screenshot below, only two idle preloads are registered before all event preloads are finished (selected in blue), and even these two only start downloading after the event preloads are finished. All the following idle preloads start after the event preloads are finished. Ideally we could tell the browser to stop the ongoing idle preloading until the event preloads are finished, but there's no working API for that atm. With your versionWhen you set 20 max idle preloads, the amount will actually decrease towards a lower value (e.g. 10), slowing down the average idle preloads speed. When you click, it's the same scenario, except the idle preloads will take a little bit less network indeed. But it's not a valuable tradeoff imo, because we sacrifice overall preload speed, with slightly faster reprio speed. |
|
Ok fair enough, simpler code and it you see it improve things, so good. Could you fix the conflict? |
maiieul
force-pushed
the
set-maxIdlePreloads-to-be-constant-over-time
branch
from
5e8d71c to
cc50897
Compare
maiieul
force-pushed
the
set-maxIdlePreloads-to-be-constant-over-time
branch
from
cc50897 to
0e456fb
Compare
| @@ -78,10 +78,9 @@ export const trigger = () => { | |||
| const inverseProbability = bundle.$inverseProbability$; | |||
| const probability = 1 - inverseProbability; | |||
| const allowedPreloads = graph | |||
| ? // The more likely the bundle, the more simultaneous preloads we want to allow | |||
| Math.max(1, config.$maxIdlePreloads$ * probability) | |||
| ? config.$maxIdlePreloads$ | |||
| : // While the graph is not available, we limit to 2 preloads | |||
What is it?
Description
It's better to have this value be constant, otherwise it is hard for the developer to control it.
Also increased the ssrPreloads limit (before bundle-graph is loaded) to 5 since 2 seems like it can create a lot of waterfalls no? (I didn't benchmark this ^^)
Checklist
pnpm change