1997973 - Potential performance improvement by reducing string hashing in nsHttpConnectionMgr::FindCoalescableConnection

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[John Schanck [:jschanck]]

I was doing some socket thread profiling and I noticed that we spend a lot of time in nsHttpConnectionMgr::FindCoalescableConnection, specifically in computing hash table keys with mozilla::HashString. The hash function used there has a pretty significant per-byte cost, so I put together a quick patch to key the hash table on the 4 byte value hash(key) rather than on the ~90 byte value key.

Eyeballing the profiles below, it looks like this gives a decent improvement (focus on ConnEvent::Run on the socket thread). I haven't done any rigorous benchmarking.

Before: https://share.firefox.dev/3WBW2Ap
After: https://share.firefox.dev/47TWWyC

I'll post my patch as WIP. If you want to take it, then someone who is more familiar with the surrounding code should fix the log messages, etc.

Comment 1

[John Schanck [:jschanck]]

Attachment: Bug 1997973 - prehash coalescing keys to reduce lookup time. r=#necko-reviewers

Comment 2

[Andrew Creskey [:acreskey]]

Perf compare push here. This will take a couple of days to complete.
I've started with Windows since it includes PerfStats by default (may pickup improvements that higher order metrics miss).
We can add more platforms and tests as desired.

Comment 3

[Andrew Creskey [:acreskey]]

Good find, John.

From in-progress CI tests this looks to be having an unexpectedly large effect.

wikipedia largestContentfulPaint, optfissionwarmwebrender, Windows 11     before: 126.5 ms    after: 63.86 ms    change: -49.51 % 

(Also on Amazon, CNN. And some curious regressions.)
Mann-Whitney-U view.

Ni' myself to grab CI profiles that include the socket thread.

Comment 4

[John Schanck [:jschanck]]

That's a much larger effect than I would have expected. So much so that I wonder if my patch is buggy.

Comment 5

[Andrew Creskey [:acreskey]]

It's very possible that there's something unexpected going on in the test framework as well.
From a quick peek: the fast navigations with your patch look correct. e.g. here, however I'll look in more detail.

Comment 6

[Andrew Creskey [:acreskey]]

This "before" push includes profiles of cnn.com that include the socket thread.
However I'm not actually seeing FindCoalescableConnection show up in the profiles.
e.g.
https://share.firefox.dev/3NCZIAO
or
https://share.firefox.dev/4agvU5H

Maybe because the CI tests are running via HTTPS proxy mode? See Bug 1973522

So CI doesn't look like it well be helpful here.

But this is a clear improvement to socket thread performance. Ship it!

Comment 7

[Andrew Creskey [:acreskey]]

FindCoalescableConnections is roughly 7-8% of samples in this challenging DoH scenario, bug 2009372
https://share.firefox.dev/4kAYthN

Comment 8

[Andrew Creskey [:acreskey]]

Proposing that this goes in [necko-priority-next] to help with logging, maybe test, etc.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a93c3ec592c4
https://hg.mozilla.org/integration/autoland/rev/95f5f4fc474c
prehash coalescing keys to reduce lookup time. r=necko-reviewers,jesup

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/95f5f4fc474c