1952237 - Retarget HttpChannelParent::OnDataAvailable off-main-thread

Status: NEW

(Core :: Networking, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

This would allow us to free up the main thread in the parent process.
To achieve this we need to:

• Make CacheFileOutputStream implement nsIThreadRetargetableStreamListener so that this check passes
• Make HttpChannelParent implement nsIThreadRetargetableStreamListener
• Make HttpChannelParent::OnDataAvailable thread safe - there's currently some flow control which suspends the channel. We need to handle that to allow off-main-thread diversion
• Retarget in HttpChannelParent::OnStartRequest

Comment 1

[Kershaw Chang [:kershaw]]

We've tried to do this before in bug 1505493, but we didn't see any improvements from the try push.
However, I think it's still worth to try again and see if we can make any difference.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Thank you for pointing that out. I completely forgot about it 🙂
Do you remember if we ever confirmed if retargetting actually happened? Because from what I can tell in https://phabricator.services.mozilla.com/D22516 we never implemented nsIThreadRetargetableStreamListener for CacheFileOutputStream, which means we would only retarget when reading from the cache, but not when writing to it (hot vs cold loads).

Anyway, I think our perf tooling might be a lot better now, both in CI and profiler.
I think it's worth trying again - for a simple pageload profile nsHttpChannel::OnDataAvailable occupies quite a big chunk of the flamegraph.

Comment 3

[Sunil Mayya]

Attachment: WIP: Bug 1952237 - Retarget http channel to background thread

Comment 4

[Andrew Creskey [:acreskey]]

I'm rebasing this one to run it against the updated fenix applink tests.

Comment 5

[Andrew Creskey [:acreskey]]

The newssite applink test is reporting low and medium regressions of ~40-50ms from this patch

https://perf.compare/compare-results?baseRev=a26cd39c86f8acf373285c58553635a77e8dbd29&newRev=4ca172975c2a85708623b4712d6484d4a32eb1b6&baseRepo=try&newRepo=try&framework=15&filter_confidence=low%2Cmedium%2Chigh

I'll profile and make sure it's working as expected.