Closed bjuulp closed 1 year ago
Hey man, that looks awesome, thanks for contributing this! I will review the PR and also see if I can break it 😉
Have you profiled this? Is the performance difference visible?
I actually didn't profile it. The problem on my side was that, without this change, I could estimate that the worst case memory consumption would be unnecessarily high on the industrial PC I'm working with and the server would therefore increase the risk that we would inducing swapping as the PC is already very loaded. Furthermore, in our setup, there can be many clients fetching the same few large files at the same time. So, I knew on forehand that memory mapped files would yield better performance in our environment.
Indeed, back when I implemented that part of fineftp, I chose the speed of transmission over memory consumption. In my tests I got fineFTP to use the entire capacity of a 1Gbit Ethernet connection and way higher speeds on localhost. But I achieved that mainly by having big buffers (couple of megabytes) per file transmit. Hopefully I will find some time to check out your changes this week. Next week I am on vacation, so please don't worry too much, if this PR will be open for a while.
Sounds good. Enjoy your vacation! I will proceed locally with the changes in the current PR and then update my local repo later when the changes get onto master.
Hey @bjuulp: I finally started working on this PR. I have created some GTests, but they all fail on Linux and macOS when I use your memory mapped branch: https://github.com/eclipse-ecal/fineftp-server/actions?query=branch%3Atest%2Fmemory_mapped_files
With my old approach, the tests work on Linux and macOS (but 1 of them fails on Windows. Shame on me.) https://github.com/eclipse-ecal/fineftp-server/actions?query=branch%3Afeature%2Ftesting
Can you maybe take a look if that is fixable?
Sorry for my late reply. I've just created a pull request that should fix some of the errors. As mentioned in the message of the pull request, there is still one test error on Windows due to a race condition. Most likely because curl doesn't wait for the server to say that it has fully received the file before sending the RNFR and RNTO commands - at least that is what I think, but a Wireshark trace would be needed to confirm that (which I haven't had time to take).
I am closing this, as I already merged (and improved) it in the feature/memory_mapped_files branch
This change implies that files to be sent will be mapped into memory. This should speed up file transfer and reduce heap memory consumption.
Note that this also fixes a problem on Windows in relation to ASCII transfer: On Windows, even ASCII files should be transferred as binary files because the FTP spec state that such files should contain the carriage return and line feed characters at lined endings, and that just happens to be matching the binary encoding of Windows ASCII files.
On Unix, ASCII transfer will (still) be broken because the required translation from Unix line endings to Windows line endings is absent. But that's also a problem in the current master (prior to the present pull request).