625383 - HttpChunkedDecoder should not memcpy all the data bytes

Status: NEW

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

Description

[Patrick McManus [:mcmanus]]

in nsHttpChunkedDecoder.cpp when we need to identify a new chunk we parse the byte stream into a chunk len and then remove those bytes from the stream with memmove by moving the rest of the buffer up by a few bytes.

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChunkedDecoder.cpp#99

That can be a large copy of data. And if it might contain more than just the next chunk's data - it could contain chunk headers after that. Which means we will have to copy some of it yet again.

Exactly how much pain this is depends on the chunk sizes, how full our buffers are and probably how much damage it does it l2/l3 cache. Ironically, underbuffering probably helps us here.

Nonetheless - it can add up to something significant. On https://developer.mozila.org/en-US/mobile I see almost 100KB of these copies to just load that page.

It will take more than just changes to nsHttpChunkedDecoder, but we can reorient things to not assume they have to read from the beginning of the buffer and avoid the copies all together.

Comment 1

[Patrick McManus [:mcmanus]]

The fundamental blocker here is that our apis tend to be of the form "here is my buffer of size N, fill it please"... so the network layer in this case has done a large read into the user buffer and then needs to excise the chunk markers from it. (which requires memmoving all the subsequent bytes up to cover the gaps). The result is pretty much all chunked data gets memcopied.

our http/2 code faces a similar problem.. frame headers (and control frames) constantly are mixed in the data stream. It addresses this by limiting reads to frame header size to figure out the size of the data chunk, and then reading the chunk size. No moves required.

http/1 chunked is a bit messier because it is not a binary frame format; but I've done something very similar with this patch. When looking for a chunk size read either 1KB (when getting all the headers) or 6 bytes (when aligned at an unparsed chunksize marker).. when we have a parsed chunk size then just read up to the remaining chunk size. This minimizes the amount of memmove

https://tbpl.mozilla.org/?tree=Try&rev=0a69e8d07c31

The first page I ran this on reduced memmoves from 1,865KB to 12KB while adding about 500 syscalls. I think that's a win - trivial syscalls are trivial, but memory bandwidth is always a bottleneck.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: optimize http chunked decoder memmoves

Comment 3

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73caaea5844f

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73caaea5844f

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Note that this change was backed out because of bug 971001.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f3f9263459

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a9f3f9263459

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

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

Considering the reason this got backed out, it's probably not the way to go.
But in any case, we should figure out if this is still something worth pursuing, especially since we now have H2/H3.
I suspect we might end up wontfix-ing this issue.

Comment 10

[Andrew Creskey [:acreskey]]

Keeping the ni' on myself as I'm hoping to look deeper into H2/H3 perf.