Closed Bug 1255239 Opened 6 years ago Closed 6 years ago

Reduce size of HuffmanIncomingTables


(Core :: Networking: HTTP, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: n.nethercote, Assigned: n.nethercote)



(Whiteboard: [MemShrink][necko-active])


(1 file, 1 obsolete file)

Http2HuffmanIncoming.h has 15 HuffmanIncomingEntry arrays, each with 256
entries. On 64-bit platforms they take up 60 KiB, on 32-bit it's 30 KiB.
They are currently in the .data section which means they aren't shared and the
60/30 KiB is repeated for every process.

We can do much better. Each table has two kinds of entries:

- most/all entries: a value and a prefixLen;
- zero/a few entries: a pointer to another table;

We currently use a "fat" representation for each entry which has space for all
three fields. So every entry either looks like this:

> { nullptr, 33, 2 },

or like this:

> { &HuffmanIncoming_254, 0, 0 },

It's possible to split each table into two parts: a first part with all the
value/prefixLen entries, and a second, much smaller (possibly zero-sized) part
with all the pointer entries. This avoids the unused field(s) for each entry.

It's also possible to use bitfields to pack the value and prefixLen into 16
bits, instead of taking up 24 (which ends up as 32 once padding is added).

It's also possible to make all these tables |const|, which moves the entry 
tables out of the .data section into the .rodata section -- the removal of the
pointers was also necessary for this -- which means they will be shared between
processes instead of each process having its own copy.

So the overall saving from these changes on 64-bit is about (60*N - 7.5) KiB,
where N is the number of processes. On 32-bit it's about (30*N - 7.5) KiB.
I have been as careful as I could with this change, but I don't know how this
code is used, how well tested it is, and how any errors would manifest. I will
do a try push shortly.
Attachment #8728726 - Flags: review?(mcmanus)
Assignee: nobody → n.nethercote
Comment on attachment 8728726 [details] [diff] [review]
Reduce size of HuffmanIncomingTables

Review of attachment 8728726 [details] [diff] [review]:

there are xpcshell tests that should exercise this.. thanks!
Attachment #8728726 - Flags: review?(mcmanus) → review?(hurley)
Comment on attachment 8728726 [details] [diff] [review]
Reduce size of HuffmanIncomingTables

Review of attachment 8728726 [details] [diff] [review]:

This all looks good - just one request. Please also update in the same directory so that its output matches your modifications. The chances of having to re-generate the .h is pretty slim, but if we do something similar for future versions of HTTP, it would be nice to have an easier path towards auto-generation.

Also, just for safety's sake, I'd like to see a linux64-only, xpcshell-only try run before this lands - the huffman code has been known to be subtle in the past (not my finest coding, to be sure), so more safety = better :)
Attachment #8728726 - Flags: review?(hurley)
Note that I took advantage of the fact that C and C++ allow trailing commas in
struct initialization. E.g. see
Attachment #8729238 - Flags: review?(hurley)
Attachment #8728726 - Attachment is obsolete: true
Comment on attachment 8729238 [details] [diff] [review]
Reduce size of HuffmanIncomingTables

Review of attachment 8729238 [details] [diff] [review]:

Sorry - I reviewed this yesterday evening, but totally forgot to hit "publish". Thanks!
Attachment #8729238 - Flags: review?(hurley) → review+
Whiteboard: [MemShrink] → [MemShrink][necko-active]
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.