improve QPACK encoding efficiency

[marten-seemann]

Opening this issue to point to https://github.com/marten-seemann/qpack. I won't have the time to work on QPACK, but this should be a good first issue for people who would like to contribute.

The current QPACK implementation is as primitive as it could be: It doesn't use the dynamic table. Furthermore, it doesn't even use the static table (for encoding headers). It literally just uses string literals, so it doesn't provide much of a compression at all.

[jstordeur]

Hi Marten,

I'd like to take a stab at this but I have a few questions.

I've read quickly through the QPACK RFC . I thought I'd maybe start by the decoder given that it's supposed to be the simplest part of the two.
If I understand correctly each QUIC connections has a decoder and encoder and they're used for all streams on the connections. There are also two dedicated streams used by the decoder/encoder to communicate with their counterparts and send ACKs, or dynamic table insertions.

Questions:

• Is my understanding correct?
• If the decoder is shared between the streams, what is the purpose of saveBuf. As far as I can tell there is no guarantee that the next time the decoder is called it will be to operate on the same header block, so I'd expect the decoder to be stateless except for the dynamic table.
• What's the purpose of this mutex?

[marten-seemann]

Hello @jstordeur,

Currently the qpack implementation doesn't use the dynamic table at all, and we might need to change the design of it a bit to make that work.

The best exercise to start with before tackling the dynamic table is adding static table capabilities to the encoder (quic-go/qpack#8). That should be a fairly easy change, which will have a huge impact on encoding efficiency. Right now, the qpack implementation only uses the static table for decoding requests, but when encoding, it always uses string literals.

Is my understanding correct?

Yes.

If the decoder is shared between the streams, what is the purpose of saveBuf. As far as I can tell there is no guarantee that the next time the decoder is called it will be to operate on the same header block, so I'd expect the decoder to be stateless except for the dynamic table.

The saveBuf buffers incomplete header blocks. It's mostly copied from the Go standard library HPACK implementation. As we're currently using DecodeFull it's ok to have this buffer, since it will only be used for a single stream at a time. Thinking about it, I'm not sure if this doesn't contain a bug. What if the peer splits up a HEADERS frame into two parts?
If we actually need this buffering, it would probably make sense to have a more general qpack.Decoder struct that contains the members to handle the dynamic table, and then to add a qpack.Decoder.NewStream() method, which would return what is currently called qpack.Decoder.

[Villaquiranm]

Hello @marten-seemann does this issue still needs someone to work on it ? I already start looking to the implementation document and this seems like something I'll like to do with some guidance.