fileget: allocate a slice with enough capacity

[drakkan]

so a new allocation is not needed in MarshalBinary and sendPacket.

Here are some profiling results while downloading a file (file size is about 1GB),

before this patch:

1254.24MB 55.18% 55.18%  1254.24MB 55.18%  github.com/pkg/sftp.sshFxpDataPacket.MarshalBinary
  991.81MB 43.63% 98.81%   991.81MB 43.63%  github.com/pkg/sftp.fileget
       1MB 0.044% 98.86%  1255.24MB 55.22%  github.com/pkg/sftp.(*packetManager).maybeSendPackets
    0.50MB 0.022% 98.88%  1260.24MB 55.44%  github.com/pkg/sftp.(*packetManager).controller
         0     0% 98.88%   991.81MB 43.63%  github.com/pkg/sftp.(*Request).call
         0     0% 98.88%   993.31MB 43.70%  github.com/pkg/sftp.(*RequestServer).Serve.func1.1
         0     0% 98.88%   993.31MB 43.70%  github.com/pkg/sftp.(*RequestServer).packetWorker
         0     0% 98.88%  1254.24MB 55.18%  github.com/pkg/sftp.(*conn).sendPacket
         0     0% 98.88%  1254.24MB 55.18%  github.com/pkg/sftp.sendPacket

with this patch:

1209.48MB 98.46% 98.46%  1209.48MB 98.46%  github.com/pkg/sftp.fileget
       2MB  0.16% 98.63%     7.50MB  0.61%  github.com/pkg/sftp.recvPacket
         0     0% 98.63%        8MB  0.65%  github.com/drakkan/sftpgo/sftpd.Configuration.handleSftpConnection
         0     0% 98.63%  1209.48MB 98.46%  github.com/pkg/sftp.(*Request).call
         0     0% 98.63%        8MB  0.65%  github.com/pkg/sftp.(*RequestServer).Serve
         0     0% 98.63%  1209.98MB 98.50%  github.com/pkg/sftp.(*RequestServer).Serve.func1.1
         0     0% 98.63%  1209.98MB 98.50%  github.com/pkg/sftp.(*RequestServer).packetWorker
         0     0% 98.63%     7.50MB  0.61%  github.com/pkg/sftp.(*conn).recvPacket (inline)

[drakkan]

Here are the profile results for uploads and downloads

file_download_profile_results.zip
file_upload_profile_results.zip

[eikenb]

@drakkan Thanks for your continued work on performance improvements. Very much appreciated.

Also thanks @puellanivis for the great job reviewing it.

My only comments would have been similar to @puellanivis about explaining why the numbers are what they are. I think the comments @drakkan added do a pretty good job.

Thanks again.

[drakkan]

@eikenb thanks, my next pull request will contain the allocator.

As you can see in the profile results above, pkg/sftp only allocates in 2 places now:

• here for downloads
• here when receiveing any packet including write packets

and we need to release the allocated slice after sending the packet to the network, here

It doesn't seem so complex, anyway, obviously, feel free to reject it.

You can see the results that we can obtain in this way here:

https://github.com/drakkan/sftpgo/blob/master/docs/performance.md

the results for the aes128gcm@openssh.com cipher show the gains that we can have with the allocator, no other patch apply for that cipher

[puellanivis]

Additional idea to simplify the allocator: why don’t we always allocate the max capacity slice, rather than a variety of different slice lengths?

In fact, if we just change the current allocations to strictly use the one length, would we see a performance increase before even doing the whole allocator? It’s possible it might simplify garbage collection, and reallocation of backing arrays.

[drakkan]

@puellanivis, I evaluated this strategy too, I think it can be used in fileget for downloads, in recvPacket the max allowed packet size is 262144 this seems too much.

As alternative strategy I thought to preallocate a slice for each rwChan but we need a way to identify each rwChan and to get this information to use the right slice, I don't know how to do this with the current code (the allocator seems less intrusive), do you have ideas? Additionally recvPacket is not related to rwChan,we receive the packet (allocating a slice) we read the type and then we send to the right channel so for uploads we have an already allocated slice inside rwChan.

Thanks!

EDIT: I'll change my allocator to only return slices of 2 size:

• max read size + 9+4 for fileget
• maxMsgLength (262144) for receiving packet

I'll make the allocator optional, it must be explicitly enabled. I'll try to send a pull request this weekend, meantime if you have other ideas please share them, thanks!