code review 133200044: go.crypto/ssh: reuse buffer during WriteExtended

go.crypto/ssh: reuse buffer during WriteExtended

It was pointed out on golang-nuts that WriteExtended allocates a large buffer for every call to writePacket. This can be optimised a little without going to the extent of setting up a buffer pool.

We don't have great benchmarks of this, but initial tests show

benchmark                          old ns/op     new ns/op     delta
BenchmarkEndToEnd                  12417961      12004715      -3.33%
BenchmarkMarshalKexInitMsg         10251         10472         +2.16%
BenchmarkUnmarshalKexInitMsg       22723         22225         -2.19%
BenchmarkMarshalKexDHInitMsg       1301          1285          -1.23%
BenchmarkUnmarshalKexDHInitMsg     717           745           +3.91%

benchmark             old MB/s     new MB/s     speedup
BenchmarkEndToEnd     84.44        87.64        1.04x

benchmark             old allocs     new allocs     delta
BenchmarkEndToEnd     107            75             -29.91%

benchmark             old bytes     new bytes     delta
BenchmarkEndToEnd     2616693       1346890       -48.53%

[dave_cheney.net, 2014-08-30 02:22:40]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.crypto

[hanwen-google, 2014-09-03 08:34:06]

https://codereview.appspot.com/133200044/diff/60001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/133200044/diff/60001/ssh/channel.go#newcode245
ssh/channel.go:245: packet = packet[0:0:cap(packet)]
why not simply packet[:0] ?

https://codereview.appspot.com/133200044/diff/60001/ssh/channel.go#newcode247
ssh/channel.go:247: packet = append(packet, uint8(c.remoteId>>24),
uint8(c.remoteId>>16), uint8(c.remoteId>>8), uint8(c.remoteId))
Let's not do this. If this is a problem (which I don't think it is), the
compiler should be inlining it for us. The same for the other manual PutUint32
inlining below.

[hanwen-google, 2014-09-03 08:34:48]

please CC me directly on SSH changes. I don't follow the dev list closely.

[hanwen-google, 2014-09-03 08:42:22]

https://codereview.appspot.com/133200044/diff/60001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/133200044/diff/60001/ssh/channel.go#newcode251
ssh/channel.go:251: length := len(todo)
reuse 'space'

[gobot, 2014-12-19 05:12:22]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/133200044 is best) in the description in your
new CL.

Thanks very much.