rfc7692 context-takeover implemention

[smith-30]

related #339

[garyburd]

There's a lot to review here. I'll start with configuration. The Upgrader fields should be:

// EnableCompression specify if the server should attempt to negotiate per
// message compression (RFC 7692). 
EnableCompression bool

 // AllowClientContextTakeover specifies whether the server will negotiate client context
// takeover for per message compression. Context takeover improves compression at the
// the cost of using more memory.
AllowClientContextTakeover bool

// AllowServerContextTakeover specifies whether the server will negotiate server context
// takeover for per message compression.  Context takeover improves compression at the
// the cost of using more memory.
AllowServerContextTakeover bool

The Dialer should have similar fields.

Use Conn.SetCompressionLevel to specify the compression level. The comment on that method should be updated to state that changes to the compression level are ignored after the first message is written with context takeover.

The writer should lazily create the flate writer because the compression level is not known until the first call.

[smith-30]

@garyburd Sorry for the delay after reviewing it.
I fixed the part you pointed out.

[smith-30]

hi, Is there progress? I'm happy if you give feedback. 😃

[garyburd]

It will be another few days before I have time to review this further. There's a lot to review here.

Please test the feature with Autobahn. See https://github.com/gorilla/websocket/tree/master/examples/autobahn.

[smith-30]

@garyburd thanks! I will try to pass the test at Autobahn.

[smith-30]

@garyburd I did a test but there was no new error. However, in both branches case 12.4.5 ~ 6, 12.4.8 ~ 11, 12.4.13 ~ 12.4.18 have failed. Can we ignore this error?

PR branch's result https://smith-30.github.io/websocket/autobahn/context-takeover/index.html

master branch's result https://smith-30.github.io/websocket/autobahn/master/index.html

[garyburd]

The tests 12.4.5 ~ 6, 12.4.8 ~ 11, 12.4.13 ~ 12.4.18 fail because the test times out. I opened an issue for this.

Did you edit to the test application to enable context takeover?

[smith-30]

The tests 12.4.5 ~ 6, 12.4.8 ~ 11, 12.4.13 ~ 12.4.18 fail because the test times out. I opened an issue for this.

thanks!

Did you edit to the test application to enable context takeover?

sorry, I didn't add context-takeover option. I added it and re-run. Except for the existing timeout test, it seems that all of the tests are passed.

server.go https://github.com/gorilla/websocket/pull/342/commits/658a2fb894f477e0c12f4b2e8be94a7c1111736b

result https://smith-30.github.io/websocket/autobahn/context-takeover/index.html

[smith-30]

modified conflict.

[smith-30]

I prepared a branch that organized commit so I will replace it if necessary. this branch is created by

$ git merge --squash feature/context-takeover

[JordanP]

Could you make a benchmark with and without CompressionContextTakeOver and compare the memory allocation/memory usage. I ran some tests internally and I noticed that memory usage was multiplied by 10. So I am not sure, maybe there's something wrong with my bench or a memory leak in the code...

[theckman]

@smith-30 have you had the opportunity to look at the comment from @JordanP around the size of magnitude of the memory increase?

[smith-30]

@theckman I'm sorry, I missed replying and I was late. thank you for your comment.

First of all, @JordanP pointed out an increase in memory usage, but since it has not happened with my product, I do not know if I write test code. By the way, the benchmark environment I tried is 3 for api server and 180 for client. This is because it is a very high transmission frequency that one client sends.

I think that it will take 1-2 weeks for that response.

After that, I will also modify the comments. Thank you for review.

[JordanP]

On my side, my workload is different: tens of thousand clients, each sending one Websocket message every 5sec (i.e not frequently).

[smith-30]

So, in this implementation, I think that it has a maximum dictionary of 32 kb to share between client and server. Therefore, using context - takeover will increase memory usage.

[hexdigest]

Hi everyone!

Is there a chance to see this PR in master? Benchmarks show no performance degradation:

Master:

goos: linux
goarch: amd64
BenchmarkWriteNoCompression-4     	 5000000	       331 ns/op	      48 B/op	       1 allocs/op
BenchmarkWriteWithCompression-4   	  300000	      4942 ns/op	     164 B/op	       3 allocs/op
BenchmarkBroadcast/NoCompression-4         	     200	   9503267 ns/op	       0 B/op	       0 allocs/op
BenchmarkBroadcast/WithCompression-4       	      50	  30607582 ns/op	 1602198 B/op	   30010 allocs/op
BenchmarkBroadcast/NoCompressionPrepared-4 	     200	  10139983 ns/op	     167 B/op	       1 allocs/op
BenchmarkBroadcast/WithCompressionPrepared-4         	     100	  10455902 ns/op	   12451 B/op	       3 allocs/op

PR:

BenchmarkWriteNoCompression-4                      	 5000000	       325 ns/op	      48 B/op	       1 allocs/op
BenchmarkWriteWithCompression-4                    	  300000	      4921 ns/op	     164 B/op	       3 allocs/op
BenchmarkWriteWithCompressionOfContextTakeover-4   	  300000	      4806 ns/op	      60 B/op	       2 allocs/op
BenchmarkBroadcast/NoCompression-4                 	     200	   9663945 ns/op	       0 B/op	       0 allocs/op
BenchmarkBroadcast/WithCompression-4               	      50	  29305592 ns/op	 1529686 B/op	   30006 allocs/op
BenchmarkBroadcast/NoCompressionPrepared-4         	     200	  10326534 ns/op	     166 B/op	       1 allocs/op
BenchmarkBroadcast/WithCompressionPrepared-4       	     100	  10340785 ns/op	   12396 B/op	       3 allocs/op

Since this feature is optional why not have it in master?

[JordanP]

@hexdigest great that you have interest in this too. Could you share some numbers about your workload ? With compression enabled, how much more memory does your app need ? What about the CPU cost, did you measure some impact with/without this ?

[hexdigest]

@JordanP I don't think these are legit questions. These two modes: with context takeover and with no context takeover serve different purposes. In my case, there are a few thousand long living clients that send ~1 message per second where every message is more or less the same small payload (around 200 bytes) with very few differences. For my case, having context takeover is crucial because we're paying for the traffic.

I understand that the current compression implementation in the master branch uses the pool to minimize the overhead of memory allocations. This approach is especially good for the applications with hundreds of thousands of short living connections where each client rarely sends big chunks of random data. Unfortunately this is not my and @smith-30 use.

Per message CPU costs are shown here:

BenchmarkWriteWithCompression-4                    	  300000	      4921 ns/op	     164 B/op	       3 allocs/op
BenchmarkWriteWithCompressionOfContextTakeover-4   	  300000	      4806 ns/op	      60 B/op	       2 allocs/op

With same compression level context takeover has less impact on CPU per message because there are less allocations. Of course there are hidden costs for allocating/freeing memory for the flate reader/writer for each connection because the pool can't be used in the case of context takeover mode. But we're ready to pay this price in exchange for 10x-20х better compression. Considering the fact that this feature is optional and disabled by default why not?

[verebes]

@smith-30 & @elithrar, what is the reason this PR has not yet been merged? How can I help to finish it?

[smith-30]

@verebes I think it's because there is no maintainer to make the merge decision.. https://github.com/gorilla/websocket/issues/370