support compression in websockets #611 #2162
Conversation
|
HI @cooliscool, thanks for your contribution 🙏 As you noted, the feature is still marked as experimental and may result in decreased performance, we would prefer to not add it by default. Add a It would be useful also to add benchmarks with compression enabled and not. |
|
Thanks for the review @codebien |
|
i've added 'compression' param and tests for it. |
Codecov Report
@@ Coverage Diff @@
## master #2162 +/- ##
==========================================
+ Coverage 72.71% 72.76% +0.04%
==========================================
Files 184 184
Lines 14571 14586 +15
==========================================
+ Hits 10596 10613 +17
+ Misses 3333 3331 -2
Partials 642 642
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…ool/k6 into websocketCompression#611
Added benchmarks - with and without compressionHere's what I did :ws handler will send a message of 1KB to the client after Upgrade, then close the websocket benchmarked this for the two scenarios - with and without compression from the benchmark, time taken for a single operation is more - when compression is enabled - which could be explained by the overhead caused by encoding and decoding. (the advantages in network latency due to compression may not be captured here properly - because there's no real network, everthing is in memory right?) PS: I had to separate the two benchmarks into different functions because of the increased cyclop complexity. |
There was a problem hiding this comment.
Mostly LGTM, thanks! 🙇
The only nitpick I have is that it seems that a lot of the code in BenchmarkCompressionEnabled() and BenchmarkCompressionDisabled() can probably be moved to a common function that just accepts compressionEnabled as a parameter.
Also, what are the results of actually running the benchmarks through something like benchstat?
|
i could shrink the code to this : func BenchmarkCompression(b *testing.B) {
const textMessage = 1
tb, samples, rt := newRuntime(b)
sr := tb.Replacer.Replace
cases := []struct {
handler string
testCode string
compressionEnabled bool
}{
{
"/ws-compression-enabled-echo",
sr(`
var res = ws.connect("WSBIN_URL/ws-compression-enabled-echo", {"compression":"deflate"}, (socket) => {
socket.on('message', (data) => {
socket.close()
})
});
`),
true,
},
{
"/ws-compression-disabled-echo",
sr(`
var res = ws.connect("WSBIN_URL/ws-compression-disabled-echo", {}, (socket) => {
socket.on('message', (data) => {
socket.close()
})
});
`),
false,
},
}
for i := 0; i < 2; i++ {
i := i
kbData := bytes.Repeat([]byte("0123456789"), 100)
tb.Mux.HandleFunc(cases[i].handler, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
// upgrade connection, send the first (long) message, disconnect
upgrader := websocket.Upgrader{
EnableCompression: cases[i].compressionEnabled,
ReadBufferSize: 1024,
WriteBufferSize: 1024,
}
conn, e := upgrader.Upgrade(w, req, w.Header())
if e != nil {
b.Fatalf(cases[i].handler+" cannot upgrade request: %v", e)
return
}
if err := conn.WriteMessage(textMessage, kbData); err != nil {
b.Fatalf(cases[i].handler+" cannot write message: %v", err)
return
}
e = conn.Close()
if e != nil {
b.Logf("error while closing connection in "+cases[i].handler+": %v", e)
return
}
}))
}
go func() {
ctxDone := tb.Context.Done()
for {
select {
case <-samples:
case <-ctxDone:
return
}
}
}()
b.ResetTimer()
b.Run("compression-enabled", func(b *testing.B) {
for i := 0; i < b.N; i++ {
if _, err := rt.RunString(cases[0].testCode); err != nil {
b.Error(err)
}
}
})
b.Run("compression-disabled", func(b *testing.B) {
for i := 0; i < b.N; i++ {
if _, err := rt.RunString(cases[1].testCode); err != nil {
b.Error(err)
}
}
})
}but this still has a cyclomatic complexity of Benchstat results: |
|
LGTM as well, I would also prefer to get them in one function instead of multiple. Looking at the code I think you can have 1 endpoint with the compression enabled and just not enable it on the client ? It shouldn't be used if the client doesn't have it enabled I hope? I wonder about multiple body sizes, but I guess that wouldn't really matter for localhost networking 🤔 |
Yes. It shouldn't matter. Thanks for the suggestion. |
#611
Here's my research on compression in Websockets :
permessage-deflate compression is defined in RFC7692 and gorilla/websockets has implemented this as an experimental feature.
read this here
permessage-deflate will work only if both client and server supports this extension - and successfully negotiates it through
Sec-Websocket-Extensionsheader.If
EnableCompressionis set to true, gorilla/websocket will automatically try to negotiate compression with peer.Upon successful negotiation any message received in compressed form will be automatically decompressed by gorilla/websocket. So compression/decompression is taken care by gorilla/websocket if its supported.
Also, use of compression is experimental and may result in decreased performance.
the official RFC7692 has only defined deflate algorithm for compression in the specification as of now ( deflate from permessage-deflate). In future, say both a client and server wants to support algorithm 'foo' then this has to be negotiated through extension 'permessage-foo'. Few newer ones being discussed by IETF are 'permessage-bzip2', 'permessage-lz4', 'permessage-snappy'. These have not yet made it into the official RFCs. I'm not sure if any browsers are having experimental support for additional permessage algorithms.
More about in this stackoverflow question
I'd like to propose a PR for enabling
EnableCompression: trueto make compression work wherever it's supported.