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•4 days 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•3 days 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•2 days 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-prefetchis 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-sslis 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. 🙂
Description
•