search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.8k stars 17.64k forks source link

compress/flate: Allow resetting writer with new dictionary #36919

Open nhooyr opened 4 years ago

nhooyr commented 4 years ago

As of Go 1.14, compress/flate's Writer only allows resetting the write side with the same dictionary.

In contrast, the Reader can be reset with a new dictionary.

I need this to efficiently implement context takeover compression for WebSockets.

See https://tools.ietf.org/html/rfc7692#section-7.1.1

cc @klauspost

klauspost commented 4 years ago

What is the problem you are trying to solve? Is this a 1.14 change?

I'm not sure I understand why it is a problem to supply the dictionary when resetting?

as commented 4 years ago

Possibly related https://github.com/golang/go/issues/18930

nhooyr commented 4 years ago

@klauspost

I don't want to keep a flate.Writer allocated for every connection due to memory usage. Instead, I'd rather store the rolling window myself and then grab a writer out of the pool and reset it with the window and then write a message to the connection.

nhooyr commented 4 years ago

I'm not sure I understand why it is a problem to supply the dictionary when resetting?

It's not a problem, that's exactly what I want to do but it's unsupported at the moment.

klauspost commented 4 years ago

Ah, ok, so like a Stateless compression function that accepts a dictionary or a Stateless Writer, where it keeps the history between calls, but discards everything else.

nhooyr commented 4 years ago

Ah my bad I forgot you already implemented that.

Is there an issue open to merge it into the stdlib or shall I keep this open?

nhooyr commented 4 years ago

Nvm, there is no dict option yet in your pkg.

klauspost commented 4 years ago

I don't have plans to submit it to the stdlib unless someone expresses a need for it. The threshold for adding stuff is of course a bit higher for the stdlib.

Adding it to the functions above would not be super easy, since everything related to history was pulled out to make it stateless. OTOH is would also not be massive. Having, say a max 8K dict would be feasible, but every call would of course require it to be re-indexed, so not free.

klauspost commented 4 years ago

@nhooyr You can test https://github.com/klauspost/compress/pull/216

Slightly clunky API to remain backwards compatible.

nhooyr commented 4 years ago

Will do this weekend, thanks @klauspost

nhooyr commented 4 years ago

Won't be able to get to it this weekend.

Appreciate your very quick response to this issue @klauspost, I'll try to get to get to it next week.

nhooyr commented 4 years ago

Using this in my WebSocket library and the performance has been great.

https://github.com/klauspost/compress/pull/216#issuecomment-586638251

2x faster and only 100 B allocated per op for echoing a 512 random byte message with a 8 KB dictionary.

nhooyr commented 4 years ago

Would be nice to see it in the stdlib instead to avoid the dependency.

nhooyr commented 4 years ago

Adding it would close #32371 as well.