-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Improve archiver performance for small files #3955
Improve archiver performance for small files #3955
Conversation
@MichaelEischer thanks for working on this. I used the same folder as described there #3917 default read concurrency of 2
So much faster than before. I noticed that before the PR restic showed at maximum as many files beeing processed as read concurrency was. Now the number varies a lot and about 4 to 10 files are shown simultaneously. read concurrency of 6
So now using read concurrency doesn't improve speed. However overall speed is decreased a bit compared to previous implementation when increasing the read concurrency. |
That is somewhat expected with this PR. However, showing more files than before does not mean that restic is concurrently reading from more files than before.
I wouldn't consider that minimal performance difference to be too relevant. However, I have to admit that I don't have any real idea why the PR should result in a slightly higher backup time than before. |
I've done a test with the previous version again. With read conccurrency of 6 it is now 1:41 I've also tested the new restic version again. With read concurrency of 6 it takes 1:47 The difference for the faster speed is that the VM that was building the PR build was still running. With VM closed I guess Windows is caching the whole folder in RAM. So this PR is about 6 % slower than the older implementation when using it with a higher read concurrency. |
ac575af
to
16baafb
Compare
Did some tests again. Previously I used for the test https://beta.restic.net/latest/ Previously used restic version without this PR (all benchmarks before this post) So speed differences resp. faster results were because of newer go version. Sorry for not thinking about this before. So regarding performance this PR can safely be merged. |
16baafb
to
c4d55a9
Compare
What remains is to decide how to handle the much higher number of files displayed as part of the status message. It will likely confuse users... |
As far as I understood with this pull request restic still reads by default 2 files in parallel. So the question is how does this then happen that there are now displayed more files than previously? |
That is somewhat easy to explain. Let's assume we have a small file which only consists of a single chunk. Then restic reads the file (at this point it shows up on the status bar), hands it over to the blob saver which stores it in the repository, once that is completed the file is reported as complete and removed from the status bar. Only the first step actually involves reading the file from the disk. In particular, for maximum compression it takes quite some time to store the chunk in the repository. Previously the read concurrency applied to this whole sequence, which means that restic cannot make use of more CPU cores that the read concurrency, and that the next file is only requested from disk when the previous one has completed processing. That way the CPU and disk are both underutilized. With this PR, the read concurrency only applies to the step which actually reads the file. Thus, reading new files and compressing the old one happens concurrently. The status bar currently shows all files which are in any of these processing steps. This has the consequence that all files which are currently read or saved to the repository are reported.
The least confusing way is probably to hide files from the status bar once the file reading step is complete. The main complexity of that approach is that this introduces a new intermediate state in the output. The verbose output must still be able to report the final amount of data stored for a file. This means that we'd have to send an additional signal to the progress output once the file reading part is complete. But I guess that's better than confusing everyone. |
I think the problem is that restic users are accustomed that only the files are shown that restic reads currently from disk. Or the other option is the cosmetic implementation which needs quite some time for no real functional gain. |
c0c614c
to
0825948
Compare
That just feels wrong. It gives the impression that we're too lazy to implement a proper status output. I've implemented the variant which just hides all files which already have completed reading from disk. It's actually less messy to implement than I've expected. |
0825948
to
b7c80fe
Compare
After reading and chunking all data in a file, the FutureFile still has to wait until the FutureBlobs are completed. This was done synchronously which results in blocking the file saver and prevents the next file from being read. By replacing the FutureBlob with a callback, it becomes possible to complete the FutureFile asynchronously.
Previously, the old status text remained until it was overwritten.
As the FileSaver is asynchronously waiting for all blobs of a file to be stored, the number of active files is higher than the number of files from which restic is reading concurrently. Thus to not confuse users, only display files in the status from which restic is currently reading.
b7c80fe
to
c0f34af
Compare
LGTM. I've slightly tweaked a comment and added the |
Damn... Hats off. |
What does this PR change? What problem does it solve?
As discovered in #3917 (comment) the
backup
command is slower for small files (up to about 5-6 chunks) than it could be. The main bottleneck appears to be the FileSaver, as increasing the--read-concurrency
also drastically improves the backup performance. The FileSaver proceeds in two steps, first it reads the file, splits it into chunks and passes it to the BlobSaver and secondly it waits for all blobs to complete saving (the upload happens asynchronously). Depending on the blob size it can take a few milliseconds to process it. During this time the FileSaver is blocked and not performing any useful work.This PR changes the second phase of the FileSaver to work asynchronously using callbacks. For this the BlobSaver no longer returns a FutureBlob but instead calls a callback once a blob was saved. This provides the necessary means to asynchronously complete the FutureFile. Similar changes to FutureTree will have to wait at least until #3880 is merged.
The asynchronous file completion is protected using a mutex to avoid synchronization problems.
Asynchronously completing a file has a somewhat unexpected side effect. The status will now often list more than two files at the same time, even though the read concurrency is limited to two. Although this look a bit unexpected, it is actually correct. The status displays which files restic is currently working on, and not which file restic is reading from. However, I wonder whether we should change that to avoid confusing users.
The second commit in this PR changes the status output to clear no longer used status lines. With a frequently changing number of currently processed files, this ended up being confusing.
Performance measurements:
Command:
restic backup -n --exclude .git linux
using a checkout of Linux 6.0.CPU: AMD Ryzen 7 PRO 5850U ; Repository stored on NVME disk (warmed up page cache).
Times reported are those printed in the backup command summary.
I'm aware that the test dataset is too small to properly differentiate the different variants, but the trends should be obvious :-) .
Was the change previously discussed in an issue or on the forum?
See #3917.
Checklist
[ ] I have added documentation for relevant changes (in the manual).changelog/unreleased/
that describes the changes for our users (see template).gofmt
on the code in all commits.