Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nhooyr opened this issue Jan 31, 2020 · 14 comments
Open

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

nhooyr opened this issue Jan 31, 2020 · 14 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Milestone

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Jan 31, 2020

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
Copy link
Contributor

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
Copy link
Contributor

as commented Jan 31, 2020

Possibly related #18930

@nhooyr
Copy link
Contributor Author

nhooyr commented Jan 31, 2020

@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
Copy link
Contributor Author

nhooyr commented Jan 31, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

nhooyr commented Jan 31, 2020

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
Copy link
Contributor Author

nhooyr commented Jan 31, 2020

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

@klauspost
Copy link
Contributor

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
Copy link
Contributor

klauspost commented Feb 1, 2020

@nhooyr You can test klauspost/compress#216

Slightly clunky API to remain backwards compatible.

@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 1, 2020

Will do this weekend, thanks @klauspost

@nhooyr
Copy link
Contributor Author

nhooyr commented Feb 2, 2020

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.

@ALTree ALTree added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance labels Feb 3, 2020
@ALTree ALTree added this to the Unplanned milestone Feb 3, 2020
@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 14, 2020

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

klauspost/compress#216 (comment)

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

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 14, 2020

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

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 14, 2020

Adding it would close #32371 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Projects
None yet
Development

No branches or pull requests

4 participants