Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/crypto/ssh: performance improvement #39117

Closed
drakkan opened this issue May 17, 2020 · 0 comments
Closed

x/crypto/ssh: performance improvement #39117

drakkan opened this issue May 17, 2020 · 0 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unreleased

Comments

@drakkan
Copy link
Member

drakkan commented May 17, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nicola/.cache/go-build"
GOENV="/home/nicola/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/nicola/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build437742805=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I'm using crypto/ssh for SFTP inside SFTPGo (https://github.com/drakkan/sftpgo). The performance are generally good and very similar to OpenSSH for SFTP downloads. SFTP uploads are a bit slower than OpenSSH.

Here is a profiling result:

(pprof) top                                                       
Showing nodes accounting for 673.49MB, 97.46% of 691.07MB total
Dropped 51 nodes (cum <= 3.46MB)
Showing top 10 nodes out of 45
      flat  flat%   sum%        cum   cum%
  603.99MB 87.40% 87.40%   603.99MB 87.40%  golang.org/x/crypto/ssh.(*connectionState).readPacket
      64MB  9.26% 96.66%       64MB  9.26%  golang.org/x/crypto/argon2.initBlocks
       5MB  0.72% 97.38%        8MB  1.16%  golang.org/x/crypto/ssh.Marshal (inline)
    0.50MB 0.072% 97.46%     8.50MB  1.23%  golang.org/x/crypto/ssh.(*channel).adjustWindow
         0     0% 97.46%     3.61MB  0.52%  crypto/x509.SystemCertPool
         0     0% 97.46%     3.61MB  0.52%  crypto/x509.initSystemRoots
         0     0% 97.46%     3.61MB  0.52%  crypto/x509.loadSystemRoots
         0     0% 97.46%     3.61MB  0.52%  crypto/x509.systemRootsPool (inline)
         0     0% 97.46%       64MB  9.26%  github.com/alexedwards/argon2id.ComparePasswordAndHash
         0     0% 97.46%     3.61MB  0.52%  github.com/drakkan/sftpgo/cmd.Execute
(pprof) list readPacket
Total: 691.07MB
....
  603.99MB   603.99MB    163:	fresh := make([]byte, len(packet))
         .          .    164:	copy(fresh, packet)
         .          .    165:
         .          .    166:	return fresh, err
         .          .    167:}
         .          .    168:

so we can improve upload performance avoid this allocation

What did you expect to see?

Data upload with minimal memory allocation to improve performance

What did you see instead?

a new slice is allocated for each received packet

I can try to write a patch but I would like some suggestions on the best approach to do this.
I'm thinking about a per connection circular ring buffer of slices with a configurable max size, so we can reuse the preallocated slices once the ring buffer is filled.
What do you think about? Do you have other suggestions? Thanks!
@gopherbot gopherbot added this to the Unreleased milestone May 17, 2020
@ALTree ALTree added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 17, 2020
@drakkan drakkan closed this as completed Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@drakkan @ALTree @gopherbot