Closed Bug 1296280 Opened 3 years ago Closed 3 years ago

Change HPACK receive table size to 64k

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [necko-active])

Attachments

(5 files)

Let's make the max table size preffable so we can change it easily. However, before changing its default value, let's start collecting some data so we can hopefully make a better-informed decision. Right now, here's what I'm thinking:

 - max space used per session
 - max number of headers stored per session
 - when we evict, number of headers evicted (are we often evicting a lot of small headers to make room for a big one?)
 - similarly with total size (do we often end up evicting one giant header that makes room for a lot of smaller ones?)

Patrick, Martin, does this seem reasonable?
Perfect.  I assume that you would record these stats into histograms when the connection terminates.  Are you concerned that this might be affected by the duration of the session?
(In reply to Martin Thomson [:mt:] from comment #1)
> Perfect.  I assume that you would record these stats into histograms when
> the connection terminates.  Are you concerned that this might be affected by
> the duration of the session?

The first two would be reported at the end of the session, yes. They could certainly be skewed based on the duration of the session (shorter sessions would be more likely to have a lower max size and max count), but I'm hoping (with no data to back this up) that most sessions would end up in a steady(ish) state within relatively short order, so it wouldn't be that big of a deal. I could be totally wrong on this one :) Of course, for space (not count), we know the upper bound, so if we see most sessions reaching (or near) that upper bound, that's probably a good sign we should up the default pref.

The second two would be reported each time we evict, so they should be unaffected by session lifetime.
So... while working on this, methinks I've found a bug in our (my, since I'm totally responsible for this bit of the code) HPACK implementation. Look at http://searchfox.org/mozilla-central/rev/b38dbd1378cea4ae83bbc8a834cdccd02bbc5347/netwerk/protocol/http/Http2Compression.cpp#958

What's happening there right now is, when we decode a table size change, we use that to change the size of the table in our encoder. However, reading https://tools.ietf.org/html/rfc7541#section-4.2 it looks like a table size change should change the size of the table in the decoder.

My (current) reading of 7541 seems most sensible - we already have a way to signal what we're willing to commit when decoding (SETTINGS_MAX_TABLE_SIZE), but (outside the above) no way to limit what we commit when encoding. I vaguely remember discussing this with Martin in person back when this was added to the spec, though, and understanding at the time that how I coded it is what the right answer is. Perhaps the spec language changed to clarify between when I added the code and now. Or maybe I'm just dumb.

Blink seems to do what my current reading of the spec says: https://chromium.googlesource.com/chromium/src/+/master/net/spdy/hpack/hpack_decoder.cc#177, but I want to get another opinion outside my own brain and blink. Martin?
Flags: needinfo?(martin.thomson)
So the correct interpretation is definitely to change the decoder state.  Part of the reason to change this value is to signal acceptance of new settings.  You change the encoder state by simply sending this value.

I remember many discussions about these things, so won't remember specifics (besides, I'm pretty wiped out right now, it's evening Kazakh time and I have been out for three days).  Try checking on nghttp2 if you want a third opinion.  Or you can ping Hervé; but I'm sure that you will get the same answer.
Flags: needinfo?(martin.thomson)
Depends on: 1300148
I think we want to be more aggressive here and just make a change - we know what the downside is.

https://bugs.chromium.org/p/chromium/issues/detail?id=642784

chrome is making the change to 64KB - so let's stay in lockstep with that.. it seems reasonable for the desktop. ought to help avoid any bugs with non compliant senders too.

the telemetry is good - maybe get that on aurora so we can see any difference.
This change requires an update to a new version of node-http2. Since there are way more tests than just the h2 tests that depend on that now, I've pushed a try build with that patch isolated to make sure the update doesn't break anything else unexpectedly.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff8a06738580
Summary: Telemetry for h2 compression + preffable max table size → Change HPACK receive table size to 64k
And so they're both in the same place, here's the auto-triggered try run for the whole series: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a4f71bf120
Comment on attachment 8792014 [details]
Bug 1296280 (part 0a) - Update to node-http2 3.3.6

https://reviewboard.mozilla.org/r/79274/#review78200
Attachment #8792014 - Flags: review?(mcmanus) → review+
Comment on attachment 8792015 [details]
Bug 1296280 (part 0b) - Make debugging node h2 server work better

https://reviewboard.mozilla.org/r/79276/#review78202
Attachment #8792015 - Flags: review?(mcmanus) → review+
Comment on attachment 8792016 [details]
Bug 1296280 (part 1) - Add telemetry for HPACK usage.  data-review=bsmedberg

https://reviewboard.mozilla.org/r/79278/#review78206

remember the data collection stewards are reviewing all telemetry

::: netwerk/protocol/http/Http2Compression.cpp:964
(Diff revision 1)
>    MakeRoom(room, "decompressor");
>  
>    // Incremental Indexing implicitly adds a row to the header table.
>    mHeaderTable.AddElement(name, value);
>  
> +  uint32_t currentSize = mHeaderTable.ByteCount();

I haven't looked to see if that's O(1) or not.. if not you can test it against TelemetryEnabled()
Attachment #8792016 - Flags: review?(mcmanus) → review+
Comment on attachment 8792017 [details]
Bug 1296280 (part 2) - Add pref for our SETTINGS_MAX_TABLE_SIZE.

https://reviewboard.mozilla.org/r/79280/#review78210
Attachment #8792017 - Flags: review?(mcmanus) → review+
Comment on attachment 8792018 [details]
Bug 1296280 (part 3) - Set HPACK receive buffer size appropriately for the platform

https://reviewboard.mozilla.org/r/79282/#review78212
Attachment #8792018 - Flags: review?(mcmanus) → review+
Comment on attachment 8792016 [details]
Bug 1296280 (part 1) - Add telemetry for HPACK usage.  data-review=bsmedberg

Benjamin - need data collection review on the new telemetry in this patch.
Attachment #8792016 - Flags: feedback?(benjamin)
Comment on attachment 8792016 [details]
Bug 1296280 (part 1) - Add telemetry for HPACK usage.  data-review=bsmedberg

https://reviewboard.mozilla.org/r/79278/#review78226

::: netwerk/protocol/http/Http2Compression.cpp:964
(Diff revision 1)
>    MakeRoom(room, "decompressor");
>  
>    // Incremental Indexing implicitly adds a row to the header table.
>    mHeaderTable.AddElement(name, value);
>  
> +  uint32_t currentSize = mHeaderTable.ByteCount();

It's O(1), so no worries here :)
Comment on attachment 8792016 [details]
Bug 1296280 (part 1) - Add telemetry for HPACK usage.  data-review=bsmedberg

https://reviewboard.mozilla.org/r/79278/#review78566

::: toolkit/components/telemetry/Histograms.json:1994
(Diff revision 1)
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 256,
> +    "n_buckets": 50,
> +    "description": "HPACK: Number of items removed from dynamic table to make room for 1 new item",
> +    "alert_emails": "necko@mozilla.com",

Here and below, please add a specific person in addition to necko@, so that we know who's responsible for these in the future.

data-review+

For the record, we discussed on IRC: alerting will cover regressions from this. Eventually this may be valuable for release, but we'll want improved aggregation before then so the team doesn't have to maintain a bunch of custom queries. This is ok as "never" because the team does review t.m.o regularly to see trends.
Attachment #8792016 - Flags: review+
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29ce8a4f9c82
(part 0a) - Update to node-http2 3.3.6 r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/a602d7e2e3bf
(part 0b) - Make debugging node h2 server work better r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/56980923e6ca
(part 1) - Add telemetry for HPACK usage. r=bsmedberg,mcmanus data-review=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/d37cfef9b235
(part 2) - Add pref for our SETTINGS_MAX_TABLE_SIZE. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/5194eeb6ca2d
(part 3) - Set HPACK receive buffer size appropriately for the platform r=mcmanus
Depends on: 1305321
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
feature not a defect. no impact on 51.
Depends on: 1307190
You need to log in before you can comment on or make changes to this bug.