add optional allocator

[drakkan]

after processing a packet we keep in memory the allocated slices and we reuse
them for new packets.
Slices are allocated in:

• recvPacket
• when we receive a sshFxpReadPacket (downloads)

The allocated slices have a fixed size = maxMsgLength.

Allocated slices are referenced to the request order id and are marked for reuse
after a request is served in maybeSendPackets.

The allocator is added to the packetManager struct and it is cleaned at the end
of the Serve() function.

This allocation mode is optional and disabled by default.

I tested several different approachs, please take a look at my branches here:

• https://github.com/drakkan/sftp/tree/allocator
• https://github.com/drakkan/sftp/tree/allocator2

the included benchmark show that this is the better approach. Here are some profiling results:

Upload 1GB file

Showing nodes accounting for 1116.80MB, 97.66% of 1143.54MB total
Dropped 63 nodes (cum <= 5.72MB)
Showing top 10 nodes out of 16
      flat  flat%   sum%        cum   cum%
 1110.30MB 97.09% 97.09%  1110.30MB 97.09%  golang.org/x/crypto/ssh.(*connectionState).readPacket
       6MB  0.52% 97.62%       10MB  0.87%  golang.org/x/crypto/ssh.Marshal (inline)
    0.50MB 0.044% 97.66%    10.50MB  0.92%  golang.org/x/crypto/ssh.(*channel).adjustWindow
         0     0% 97.66%    17.95MB  1.57%  github.com/drakkan/sftpgo/sftpd.Configuration.handleSftpConnection
         0     0% 97.66%    17.95MB  1.57%  github.com/pkg/sftp.(*RequestServer).Serve
         0     0% 97.66%    15.45MB  1.35%  github.com/pkg/sftp.(*conn).recvPacket (inline)
         0     0% 97.66%        6MB  0.52%  github.com/pkg/sftp.(*packetManager).controller
         0     0% 97.66%    15.45MB  1.35%  github.com/pkg/sftp.recvPacket
         0     0% 97.66%    10.50MB  0.92%  golang.org/x/crypto/ssh.(*channel).Read
         0     0% 97.66%    10.50MB  0.92%  golang.org/x/crypto/ssh.(*channel).ReadExtended

Download 1GB file

Showing nodes accounting for 34.34MB, 80.94% of 42.43MB total
Showing top 10 nodes out of 131
      flat  flat%   sum%        cum   cum%
   15.84MB 37.34% 37.34%    15.84MB 37.34%  github.com/pkg/sftp.(*allocator).GetPage
       5MB 11.78% 49.12%     6.50MB 15.32%  golang.org/x/crypto/ssh.Marshal
    2.50MB  5.89% 55.01%     4.50MB 10.61%  github.com/pkg/sftp.orderedPackets.Sort
       2MB  4.71% 59.73%        2MB  4.71%  internal/reflectlite.Swapper
       2MB  4.71% 64.44%        2MB  4.71%  github.com/pkg/sftp.(*packetManager).incomingPacket
       2MB  4.71% 69.15%        2MB  4.71%  strings.genSplit
    1.50MB  3.54% 72.69%     1.50MB  3.54%  github.com/pkg/sftp.makePacket
    1.50MB  3.54% 76.22%     8.45MB 19.91%  github.com/pkg/sftp.fileget
       1MB  2.36% 78.58%        1MB  2.36%  golang.org/x/crypto/ssh.(*connectionState).readPacket
       1MB  2.36% 80.94%        1MB  2.36%  github.com/pkg/sftp.(*packetManager).readyPacket

Fixes #334

[drakkan]

@puellanivis thank you for your review. Please let me know if I need to change other things.

[puellanivis]

Wanted to add some feedback on circumstances on my end. I do intend to evaluate the last changes, and you’ve probably covered all the important stuff. But work has been nuts with the COVID stuff, and work from home, and everything has just been quite hectic here, and i have not been able to give this the attention it deserves.

I’m sure I’ll be back to being able to provide the help to get this improvement put in soon.

[drakkan]

@puellanivis no problem, take your time and thanks for all your suggestions

[drakkan]

The allocator is now enabled in SFTPGo. I don't expect any issue, anyway this patch will be now tested by more users. Thanks

[puellanivis]

Awesome. I think the code is probably all set here, so with some actual real-world testing, we can then have extra safety on top.

[eikenb]

@puellanivis Do you think this is good to merge? I keep meaning to review it but I've just had no time. I had a bit today and did a quick skim review and things looked good. I'll trust your opinion and if you think it is ready, please go ahead and merge it (or let me know and I can do it if you don't want to).

[puellanivis]

Yeah, I think any concerns I would have would be lint-like comments anyways. The solid chunk of concerns have all be addressed.

[drakkan]

a polite ping here.

I would like to release SFTPGo 1.0.0 before July 15, do you think this patch can be merged before then?
If this is not possibile I'll continue to use my pkg/sftp branch for the release too, thank you!

[puellanivis]

Oh, sorry. I was up-to-my-eyeballs in stuff as of late.

[puellanivis]

We’ll probably need to release at least a version 1.11.1 right?

Should we instead do a 1.12? I think we’re reasonably backwards compatible that all users of the code will still work with the same behavior if untouched, right?

[drakkan]

We’ll probably need to release at least a version 1.11.1 right?

Should we instead do a 1.12? I think we’re reasonably backwards compatible that all users of the code will still work with the same behavior if untouched, right?

Thank you! Yes it should be full backwards compatible.

Please note that I pushed by mistake #355 to this branch (without the test case) since I included it in SFTPGo 1.0.0, do you want a patch to revert it? Or a patch to add the test case only? Sorry my bad

[puellanivis]

Uh, depends on if it’s actually a bug or not. Various parts of the SFTP standard dictate that we should be using POSIX path not per-OS filepath so… 🤷

[eikenb]

Should we instead do a 1.12?

I think so. Not so much due to compatibility issues but to help highlight all the performance work drakkan contributed.

I'm going to add a 1.12.0 release milestone and add all the relevant issues/prs to it. So we have some semblance of a changelog.