1612111 - Experimental hack to not be dependent on doing stuff on the main thread in nsHttpChannel

Status: NEW

(Core :: Networking: HTTP, task, P3)

Description

[Andrew Creskey [:acreskey]]

Determine if the web extension, or other, hooks in the http connection can be disabled for a performance experiment.

Comment 1

[Honza Bambas (:mayhemer)]

The idea behind this bug is to provide only experimental patches to save some main thread loops in nsHttpChannel where we can. This means to look at the code and think a bit. Roughly, do off MT: classification callback, cache available callback...

I may merge this with bug 1545909.

Comment 3

[Honza Bambas (:mayhemer)]

Matt asked me to give this bug to him.

Matt, the idea for this bug is to:

• track the "request preparation" path in nsHttpChannel (e.g. as you proposed through MOZ_LOGing) and find all callbacks triggered on the main thread
• hack as much as possible of them to be completely removed (bypassed) or moved to a background thread, if easy to do

Examples of callbacks we can bypass:

• classification (assume all requests are not trackers); note that will automatically rule out tailing
• proxy resolution (assume mProxyInfo = nullptr all the time)
• cache entry opening (for always-cold loads this will have no effect, no cache entries to use there)

Comment 5

[Honza Bambas (:mayhemer)]

Note that if moving off the main thread is blocked by a gHttpHandler->On* call, we can simply remove those calls for this experiment altogether. W/o any addons installed and devtools enabled this has zero effect, no one is listening.

Comment 6

[Andrew Creskey [:acreskey]]

I have tried out this change of Matt's which disables various callbacks:
https://hg.mozilla.org/try/rev/ec5b985d844fa95f33dd599d08b48bf51ff1dfbc
Results can be seen here as limitHops, to be compared against fenix
https://docs.google.com/spreadsheets/d/1p6EabhB3pjSwpK9wB_IxzPPkOQB4gsafUcaf4pDeA1U/edit#gid=1315002968

A second run of the change can be seen here
https://docs.google.com/spreadsheets/d/1p6EabhB3pjSwpK9wB_IxzPPkOQB4gsafUcaf4pDeA1U/edit#gid=294628143

Overall this is not helping the network metrics like requestStart

It's interesting that when running this through raptor on the try server, it looks to seriously hurt performance on some sites, (while it helps on others)
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=84ef9def20e6976e4549b8aa4e4d659e46f02d5d&newProject=try&newRevision=8d58399dbad3083e2d814648b99eb03cf345b521&framework=10

This makes me wonder if there are parts of this change (maybe the cache access) that are not helping, while others parts are?

Comment 7

[Honza Bambas (:mayhemer)]

I don't see a cache opening bypass in the change.

The should-upgrade change seems wrong.
The should-intercept change as well.

those two can break sites and have nothing to do with the intent to save MT loops. Only changes I see is the classification bypass that can change cookie and connection sharing behavior and the proxy resolve loop (two MT hops).

Looks like we still don't know what is going on, afterall?

Comment 8

[Andrew Creskey [:acreskey]]

Looks like we still don't know what is going on, afterall?

I would say that we only know that the initial network request is significantly slower in e10s android.
The only change that's improved that is adding an early speculative connect: see Bug 1612575
also visible here:
https://docs.google.com/spreadsheets/d/1p6EabhB3pjSwpK9wB_IxzPPkOQB4gsafUcaf4pDeA1U/edit#gid=1775866602

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P1'.
:dragana, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[Greg Hess]

Set priority as P3, currently planning this after OMT on the Content Process is completed.