data packet unmarshal binary: remove uneeded copy

[drakkan]

No description provided.

[drakkan]

some interesting numbers here:

drakkan/sftpgo#69 (comment)

this patch give a 20% performance improvement for the user who posted the results. In my tests, on my laptop, the improvement is smaller

[puellanivis]

Looks good. The []byte is made in `recvPacket' and then unused except for this copy.

[drakkan]

@puellanivis Thanks!

I wonder if we can improve this code too:

       bb, err := m.MarshalBinary()
       ...
        // Slide packet down 4 bytes to make room for length header.
        packet := append(bb, make([]byte, 4)...) // optimistically assume bb has capacity
        copy(packet[4:], bb)

I need to find some time to benchmark the different ways to prepend packet (fixed 4 bytes) to bb that could be bigger.

For what I can undestand reading pkg/sftp code there are no other obvious optimizations

[puellanivis]

prepending is known to be an expensive operation, which is why Go does not have a builtin primitive for it, that would parallel append.

Although, looking at it, it might be better to just allocate the 4 bytes at the head right from the start, and just skip over them. Then later we can inject the length into it.

[drakkan]

the latest patch, applied on the top of my allocator branch, makes pkg/sftp upload performance very similar to my scp implementation.

I don't know why statvfs test fails on MacOS, it use sshFxpStatvfsPacket, my change doesn't modify that packet

[eikenb]

Travis has made it impossible to trigger re-tests so I'll pull this down and check locally. The integration tests can be a bit overly fragile.

[edit] Wait... I dont' have a Mac, so I can't check it locally. :P Guess I'll push up a test branch to trigger Travis. Seriously thinking about switching to CircleCI.

[eikenb]

I triggered 2 travis runs and they both came back fine. So just going to ignore the failure and merge this.

[eikenb]

@drakkan Thanks for the PR!
@puellanivis Thanks for the review!