Remove Predictor code
Categories
(Core :: Networking, task, P2)
Tracking
()
People
(Reporter: valentin, Unassigned)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [necko-triaged][necko-priority-queue])
We had a conversation about the Predictor and our current recommendation is that we remove the implementation:
- It uses the cache to save metadata causing extra cache work and contention.
- It does extra parsing work on the main thread.
- It's functionality is similar to rel=prefetch and rel=dns-prefetch
- Additional complexity and runtime cost are not balanced by noticeable perf improvements.
Reporter | ||
Comment 1•9 months ago
|
||
We did a few try runs, and it seems the predictor still seems to improve performance somewhat.
We'll do a proper experiment, and keep only the parts of the predictor that still make sense instead.
Comment 2•9 months ago
|
||
Valentin, can you share the try runs? I'm curious.
One thing I noticed is that network.predictor.enable-prefetch
is enabled for IS_EARLY_BETA_OR_EARLIER
. So this is enabled in our ci performance tests and local tests, but it's not what we are shipping.
Ideally, for performance evaluation and tests, the configurations would be identical unless we had a reason to hold a feature back.
network.predictor.enable-hover-on-ssl
is currently disabled, which I think is correct given that this could be a potential privacy leak.
So I'm wondering what the Predictor actually does in a release build?
Startup prediction and redirection prediction? Not actually clear to me!
Reporter | ||
Comment 3•9 months ago
|
||
I have a try run here
(In reply to Andrew Creskey [:acreskey] from comment #2)
Valentin, can you share the try runs? I'm curious.
One thing I noticed is that
network.predictor.enable-prefetch
is enabled forIS_EARLY_BETA_OR_EARLIER
. So this is enabled in our ci performance tests and local tests, but it's not what we are shipping.
Ideally, for performance evaluation and tests, the configurations would be identical unless we had a reason to hold a feature back.
I agree. The prefetch bit only covers what the predictor does - see here
If prefetching is disabled, the predictor will either preconnect or preresolve the URI.
network.predictor.enable-hover-on-ssl
is currently disabled, which I think is correct given that this could be a potential privacy leak.So I'm wondering what the Predictor actually does in a release build?
Startup prediction and redirection prediction? Not actually clear to me!
PREDICT_STARTUP or LEARN_STARTUP don't appear to be used outside a small unit test.
Unfortunately, redirect prediction also seems to be unimplemented
As far as I can tell, the major benefit of the predictor comes from doing the preconnect and preresolve earlier than normal - but that's just a guess. I'll have to actually dig into the code some more to figure out why this is working at all. 🙂
Comment 4•9 months ago
|
||
I added more runs to the try push.
It's a bit of a Christmas tree in terms of red and green.
It looks like removing it improves cold load and regresses warm loads.
Comment 5•9 months ago
|
||
I'll compare this on Android over the weekend when the CI device pools are less busy.
Comment 6•9 months ago
•
|
||
The device pool appears to be overtaxed, but I'm generally seeing regressions in CI, Android, when we disable the predictor.
Try push
Medium confidence regressions of ~4-7% in fcp, lcp on in amazon, bing. (all warm pageloads)
I'll include the predictor as a variant in biweekly Fenix pageload tests.
Reporter | ||
Comment 7•9 months ago
|
||
I filed bug 1898465 to make sure we don't send any IPC messages when the predictor is disabled.
My next steps are to move the PredictForLink code outside the pref/Outside the predictor.
Description
•