Different SPDY Hpack constant table for startup time and footprint (60KB).

Different SPDY Hpack constant table for startup time and footprint (60KB).

The hpack constant table was 40-60 KB (gcc: 40 KB, clang: 60KB), for 
storing 2 KB of data. Not optimal.

By making it an ordinary static data table we save 60 KB with clang:

Total change: -64153 bytes
==========================
  1 added, totalling +2056 bytes across 1 sources
  1 removed, totalling -66215 bytes across 1 sources
  1 grown, for a net change of +14 bytes (367 bytes before, 381 bytes after) across 1 sources
  1 shrunk, for a net change of -8 bytes (10504 bytes before, 10496 bytes after) across 1 sources

and save 40 KB with gcc:

Total change: -41382 bytes
==========================
  1 added, totalling +2056 bytes across 1 sources
  2 removed, totalling -43597 bytes across 2 sources
  1 grown, for a net change of +167 bytes (341 bytes before, 508 bytes after) across 1 sources
  1 shrunk, for a net change of -8 bytes (10576 bytes before, 10568 bytes after) across 1 sources

BUG=394269
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284698

[Daniel Bratell, 2014-07-16 09:37:53]

Please take a look at this implementation change for the hpack constant table.
I've run the testcases and they still pass (and they fail if I change a bit
somewhere).

[Ryan Hamilton, 2014-07-18 15:55:21]

On 2014/07/16 09:37:53, Daniel Bratell wrote:
> Please take a look at this implementation change for the hpack constant table.
> I've run the testcases and they still pass (and they fail if I change a bit
> somewhere).

I'll let Johnny reply here. 

This file is kept in sync with an internal version so we'll need to make a
corresponding change to the internal version.

[Johnny, 2014-07-21 15:37:28]

The internal version of this file uses '0b' literal syntax to express these
constants. Using binary literals allows a reader to more easily inspect and
compare against the specification. Unfortunately Chromium must support compilers
not supporting that syntax, and thus the bits32() kludge was introduced to keep
merges sane.

As Ryan says, we'd need to duplicate something like this change internally, and
it's not well motivated in that context (since 0b literals are a clearer
expression).

IMO the best solution is to figure out a way to evaluate bits32 at compile time,
if at all possible. BOOST_BINARY might be good model.

[Daniel Bratell, 2014-07-21 17:08:33]

On 2014/07/21 15:37:28, Johnny wrote:
> The internal version of this file uses '0b' literal syntax to express these
> constants. Using binary literals allows a reader to more easily inspect and
> compare against the specification. Unfortunately Chromium must support
compilers
> not supporting that syntax, and thus the bits32() kludge was introduced to
keep
> merges sane.
> 
> As Ryan says, we'd need to duplicate something like this change internally,
and
> it's not well motivated in that context (since 0b literals are a clearer
> expression).
> 
> IMO the best solution is to figure out a way to evaluate bits32 at compile
time,
> if at all possible. BOOST_BINARY might be good model.

Can't you use this code on the other side as well and tadaa, easy to merge?

[Daniel Bratell, 2014-07-21 17:27:16]

On 2014/07/21 17:08:33, Daniel Bratell wrote:
> On 2014/07/21 15:37:28, Johnny wrote:
> > The internal version of this file uses '0b' literal syntax to express these
> > constants. Using binary literals allows a reader to more easily inspect and
> > compare against the specification. Unfortunately Chromium must support
> compilers
> > not supporting that syntax, and thus the bits32() kludge was introduced to
> keep
> > merges sane.
> > 
> > As Ryan says, we'd need to duplicate something like this change internally,
> and
> > it's not well motivated in that context (since 0b literals are a clearer
> > expression).
> > 
> > IMO the best solution is to figure out a way to evaluate bits32 at compile
> time,
> > if at all possible. BOOST_BINARY might be good model.
> 
> Can't you use this code on the other side as well and tadaa, easy to merge?

Ok, I see what you write: "since 0b literals are a clearer expression", but I
don't see the need to see the generated bits. I mean, you don't worry about the
compiler generating the correct instructions, so why worry about the generator
generating the correct bits?

Would it be better if I made the data generated during builds from a text file?

[Johnny, 2014-07-21 19:41:11]

> Ok, I see what you write: "since 0b literals are a clearer expression", but I
> don't see the need to see the generated bits. I mean, you don't worry about
the
> compiler generating the correct instructions, so why worry about the generator
> generating the correct bits?

This is a lessor concern, but is one of readability and verifiability. Right now
the reader can directly and usefully compare the table with the specification
document, and know that there's no further magic between the table and the
binary. That's a nice property.

> Would it be better if I made the data generated during builds from a text
file?

Changes to this file are made internally first, and are then merged into
Chromium. That presents a couple of challenges. One is regarding copyright (a
Googler would need to author the change). The latter is that the change ought to
be motivated in isolation in the internal CL (which in this case it isn't, since
0b syntax is available).

Since this is an area of the codebase which is very likely to change in the
future, we'd really like to keep them in sync. bits32() is a Chromium-only
kludge which was added during such a merge, to maintain easier visual diff
comparisons, while working round the lack of support for 0b syntax. Thus my
opinion that the best fix is to replace bits32() with a compile-time evaluated
macro.

[Daniel Bratell, 2014-07-22 06:23:33]

On 2014/07/21 19:41:11, Johnny wrote:
> > Ok, I see what you write: "since 0b literals are a clearer expression", but
I
> > don't see the need to see the generated bits. I mean, you don't worry about
> the
> > compiler generating the correct instructions, so why worry about the
generator
> > generating the correct bits?
> 
> This is a lessor concern, but is one of readability and verifiability. Right
now
> the reader can directly and usefully compare the table with the specification
> document, and know that there's no further magic between the table and the
> binary. That's a nice property.
> 
> > Would it be better if I made the data generated during builds from a text
> file?
> 
> Changes to this file are made internally first, and are then merged into
> Chromium. That presents a couple of challenges. One is regarding copyright (a
> Googler would need to author the change). The latter is that the change ought
to
> be motivated in isolation in the internal CL (which in this case it isn't,
since
> 0b syntax is available).
> 
> Since this is an area of the codebase which is very likely to change in the
> future, we'd really like to keep them in sync. bits32() is a Chromium-only
> kludge which was added during such a merge, to maintain easier visual diff
> comparisons, while working round the lack of support for 0b syntax. Thus my
> opinion that the best fix is to replace bits32() with a compile-time evaluated
> macro.

I looked at the boost macros and they were extremely magic, complicated and
verbose. Also, I don't know if using them as is would be in line with all kinds
of licenses. Hmm, maybe we should check with darin or someone?

[Daniel Bratell, 2014-07-22 09:49:56]

Pushed a new version that shows the binary strings.

[Johnny, 2014-07-22 13:31:49]

Thanks, the comments improve things. Bringing in BOOST_BINARY would, as you say,
be non-trivial. This is reasonable, so LGTM.

[Daniel Bratell, 2014-07-22 14:21:48]

On 2014/07/22 13:31:49, Johnny wrote:
> Thanks, the comments improve things. Bringing in BOOST_BINARY would, as you
say,
> be non-trivial. This is reasonable, so LGTM.

Thanks, and I hope you can figure out a way to keep it in sync with the internal
code without too much work!

[Daniel Bratell, 2014-07-22 14:21:53]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-07-22 14:22:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bratell@opera.com/393573002/60001

[commit-bot: I haz the power, 2014-07-22 16:21:53]

Change committed as 284698