If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Reduce size of HuffmanIncomingTables

RESOLVED FIXED in Firefox 48

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8728726 [details] [diff] [review]
Reduce size of HuffmanIncomingTables

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)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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 make_incoming_tables.py 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)
(Assignee)

Comment 4

2 years ago
Created attachment 8729238 [details] [diff] [review]
Reduce size of HuffmanIncomingTables

Note that I took advantage of the fact that C and C++ allow trailing commas in
struct initialization. E.g. see
http://stackoverflow.com/questions/7043372/int-a-1-2-weird-comma-allowed-any-particular-reason
Attachment #8729238 - Flags: review?(hurley)
(Assignee)

Updated

2 years ago
Attachment #8728726 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
And try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=365f98501bb4
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]
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/676fc640c532865c16cda8102b9c3ddc2e8357fa
Bug 1255239 - Reduce size of HuffmanIncomingTables. r=hurley.

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/676fc640c532
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.