u408661 Assignee
	Description • 8 years ago

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?

	Martin Thomson [:mt:]
	Comment 1 • 8 years ago

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?

	u408661 Assignee
	Comment 2 • 8 years ago

(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.

	u408661 Assignee
	Comment 3 • 8 years ago

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?

	Martin Thomson [:mt:]
	Comment 4 • 8 years ago

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.

	Patrick McManus [:mcmanus]
	Comment 5 • 8 years ago

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.

	u408661 Assignee
	Comment 6 • 8 years ago

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

	u408661 Assignee
	Comment 12 • 8 years ago

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

	Patrick McManus [:mcmanus]
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8792014 [details]
Bug 1296280 (part 0a) - Update to node-http2 3.3.6

https://reviewboard.mozilla.org/r/79274/#review78200

	Patrick McManus [:mcmanus]
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8792015 [details]
Bug 1296280 (part 0b) - Make debugging node h2 server work better

https://reviewboard.mozilla.org/r/79276/#review78202

	Patrick McManus [:mcmanus]
	Comment 15 • 8 years ago
mozreview-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()

	Patrick McManus [:mcmanus]
	Comment 16 • 8 years ago
mozreview-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

	Patrick McManus [:mcmanus]
	Comment 17 • 8 years ago
mozreview-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

	u408661 Assignee
	Comment 18 • 8 years ago

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.

	u408661 Assignee
	Comment 19 • 8 years ago
mozreview-review

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 :)

	Benjamin Smedberg
	Comment 20 • 8 years ago
mozreview-review

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.

	Pulsebot
	Comment 24 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 25 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/29ce8a4f9c82
https://hg.mozilla.org/mozilla-central/rev/a602d7e2e3bf
https://hg.mozilla.org/mozilla-central/rev/56980923e6ca
https://hg.mozilla.org/mozilla-central/rev/d37cfef9b235
https://hg.mozilla.org/mozilla-central/rev/5194eeb6ca2d

	Gerry Chang [:gchang]
	Comment 26 • 8 years ago

Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.

	Patrick McManus [:mcmanus]
	Comment 27 • 8 years ago

feature not a defect. no impact on 51.

	Kohei Yoshino [:kohei]
	Comment 28 • 8 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/increased-hpack-maximum-table-size-breaks-specific-servers/

	Florian Scholz (Open Web Docs)
	Comment 29 • 7 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/52#HTTP