code review 5728053: go.crypto/ssh: improve marshal performance

go.crypto/ssh: improve marshal performance

Atom N450, 6g

benchmark                         old ns/op    new ns/op    delta
BenchmarkMarshalKexInitMsg            96446        66675  -30.87%
BenchmarkUnmarshalKexInitMsg         155341       142715   -8.13%
BenchmarkMarshalKexDHInitMsg           9499         8340  -12.20%
BenchmarkUnmarshalKexDHInitMsg         4973         5145   +3.46%

Intel E3-1270, 6g

benchmark                         old ns/op    new ns/op    delta
BenchmarkMarshalKexInitMsg            23218        16903  -27.20%
BenchmarkUnmarshalKexInitMsg          31384        31640   +0.82%
BenchmarkMarshalKexDHInitMsg           1943         1661  -14.51%
BenchmarkUnmarshalKexDHInitMsg          915          941   +2.84%

[dave_cheney.net, 2012-03-03 09:28:15]

Hello agl@golang.org (cc: golang-dev@googlegroups.com),

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

[minux1, 2012-03-03 09:40:29]

http://codereview.appspot.com/5728053/diff/4001/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5728053/diff/4001/ssh/common.go#newcode260
ssh/common.go:260: *b = append((*b), byte(n>>8), byte(n))
Why not instead wrap encoding/binary.BigEndian.PutUint16() to
avoid code duplication and rely on compiler inlining for efficiency?

There are many inline expanding of encoding/binary.BigEndian.Uint*
and PutUint* in ssh code, I think they should all be converted to use
encoding/binary directly.

[remyoudompheng, 2012-03-03 09:45:17]

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go
File ssh/messages.go (right):

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go#newcode313
ssh/messages.go:313: case reflect.Uint8:
isn't it a []byte in most cases? it could be type-asserted and appended
directly.

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go#newcode438
ssh/messages.go:438: out = uint64(in[0])<<56 |
this is out = binary.BigEndian.Uint64(in)

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go#newcode487
ssh/messages.go:487: func marshalUint32(to []byte, n uint32) []byte {
s = marshalUint32(s, n)
looks no different from
binary.BigEndian.PutUint32(s, n)
s = s[:4]

[dave_cheney.net, 2012-03-03 10:50:47]

Hello agl@golang.org, minux.ma@gmail.com, remyoudompheng@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-03-03 10:51:11]

Thanks for the quick replies. Please take another look.

http://codereview.appspot.com/5728053/diff/4001/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5728053/diff/4001/ssh/common.go#newcode260
ssh/common.go:260: *b = append((*b), byte(n>>8), byte(n))
On 2012/03/03 09:40:29, minux wrote:
> Why not instead wrap encoding/binary.BigEndian.PutUint16() to
> avoid code duplication and rely on compiler inlining for efficiency?

Currently the compiler won't inline non leaf functions.
 
> There are many inline expanding of encoding/binary.BigEndian.Uint*
> and PutUint* in ssh code, I think they should all be converted to use
> encoding/binary directly.

Yes, I'm planning to address those in a future CL.

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go
File ssh/messages.go (right):

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go#newcode313
ssh/messages.go:313: case reflect.Uint8:
On 2012/03/03 09:45:18, remyoudompheng wrote:
> isn't it a []byte in most cases? it could be type-asserted and appended
> directly.

Done.

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go#newcode438
ssh/messages.go:438: out = uint64(in[0])<<56 |
On 2012/03/03 09:45:18, remyoudompheng wrote:
> this is out = binary.BigEndian.Uint64(in)

Done.

http://codereview.appspot.com/5728053/diff/4001/ssh/messages.go#newcode487
ssh/messages.go:487: func marshalUint32(to []byte, n uint32) []byte {
On 2012/03/03 09:45:18, remyoudompheng wrote:
> s = marshalUint32(s, n)
> looks no different from
> binary.BigEndian.PutUint32(s, n)
> s = s[:4]

Done.

[agl1, 2012-03-04 22:34:45]

*** Submitted as
http://code.google.com/p/go/source/detail?r=b87c16c531f3&repo=crypto ***

go.crypto/ssh: improve marshal performance

Atom N450, 6g

benchmark                         old ns/op    new ns/op    delta
BenchmarkMarshalKexInitMsg            96446        66675  -30.87%
BenchmarkUnmarshalKexInitMsg         155341       142715   -8.13%
BenchmarkMarshalKexDHInitMsg           9499         8340  -12.20%
BenchmarkUnmarshalKexDHInitMsg         4973         5145   +3.46%

Intel E3-1270, 6g

benchmark                         old ns/op    new ns/op    delta
BenchmarkMarshalKexInitMsg            23218        16903  -27.20%
BenchmarkUnmarshalKexInitMsg          31384        31640   +0.82%
BenchmarkMarshalKexDHInitMsg           1943         1661  -14.51%
BenchmarkUnmarshalKexDHInitMsg          915          941   +2.84%

R=agl, minux.ma, remyoudompheng
CC=golang-dev
http://codereview.appspot.com/5728053

Committer: Adam Langley <agl@golang.org>

http://codereview.appspot.com/5728053/diff/4008/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/5728053/diff/4008/ssh/common.go#newcode255
ssh/common.go:255: func (b *writeBuf) uint8(n uint8) {
Since this is just putting a function call around append(), I removed it.

http://codereview.appspot.com/5728053/diff/4008/ssh/common.go#newcode259
ssh/common.go:259: func (b *writeBuf) uint16(n uint16) {
at that point, it became cleaner to make these appendU16 and so on.

[dave_cheney.net, 2012-03-04 22:43:46]

Thanks for the adjustments to the method names, I agree your versions are an
improvement. 

On 05/03/2012, at 9:34, agl@golang.org wrote:

> *** Submitted as
> http://code.google.com/p/go/source/detail?r=b87c16c531f3&repo=crypto ***
> 
> go.crypto/ssh: improve marshal performance
> 
> Atom N450, 6g
> 
> benchmark                         old ns/op    new ns/op    delta
> BenchmarkMarshalKexInitMsg            96446        66675  -30.87%
> BenchmarkUnmarshalKexInitMsg         155341       142715   -8.13%
> BenchmarkMarshalKexDHInitMsg           9499         8340  -12.20%
> BenchmarkUnmarshalKexDHInitMsg         4973         5145   +3.46%
> 
> Intel E3-1270, 6g
> 
> benchmark                         old ns/op    new ns/op    delta
> BenchmarkMarshalKexInitMsg            23218        16903  -27.20%
> BenchmarkUnmarshalKexInitMsg          31384        31640   +0.82%
> BenchmarkMarshalKexDHInitMsg           1943         1661  -14.51%
> BenchmarkUnmarshalKexDHInitMsg          915          941   +2.84%
> 
> R=agl, minux.ma, remyoudompheng
> CC=golang-dev
> http://codereview.appspot.com/5728053
> 
> Committer: Adam Langley <agl@golang.org>
> 
> 
> 
> http://codereview.appspot.com/5728053/diff/4008/ssh/common.go
> File ssh/common.go (right):
> 
> http://codereview.appspot.com/5728053/diff/4008/ssh/common.go#newcode255
> ssh/common.go:255: func (b *writeBuf) uint8(n uint8) {
> Since this is just putting a function call around append(), I removed
> it.
> 
> http://codereview.appspot.com/5728053/diff/4008/ssh/common.go#newcode259
> ssh/common.go:259: func (b *writeBuf) uint16(n uint16) {
> at that point, it became cleaner to make these appendU16 and so on.
> 
> http://codereview.appspot.com/5728053/