-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add faster file download approach #2672
Closed
AlexDickerson
wants to merge
28
commits into
wabbajack-tools:main
from
AlexDickerson:downloadsRefactor
Closed
Add faster file download approach #2672
AlexDickerson
wants to merge
28
commits into
wabbajack-tools:main
from
AlexDickerson:downloadsRefactor
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…iles under an arbitrary size NonReusableDownloader leverages the Downloader library for executing downloads. This presents some performance overhead when downloading large number files, as is often done when downloading larger modlists. Moved these Downloader classes to the Downloader folder in solution to better align with solution structure. Added service extension for DI Cleaned up impacted files Removed unusued method from ResumableDownloader
Cleaned up code Removed retry login in NonResumableDownload as DownloadService already retries
Remove deprecated nexus auth method
Deduplicated two more DI method across OS Integrated and Launcher
…iles under an arbitrary size NonReusableDownloader leverages the Downloader library for executing downloads. This presents some performance overhead when downloading large number files, as is often done when downloading larger modlists. Moved these Downloader classes to the Downloader folder in solution to better align with solution structure. Added service extension for DI Cleaned up impacted files Removed unusued method from ResumableDownloader
Cleaned up code Removed retry login in NonResumableDownload as DownloadService already retries
Deduplicated two more DI method across OS Integrated and Launcher
…wabbajack into downloadsRefactor
Re-working this to remove usage of Downloader entirely per convo in Discord. |
Reworked this with a new approach. Abandoning and creating new PR |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: When downloading larger modlists Wabbajack can experience periods of low download speed and elevated memory usage. This can significantly slow down the download process.
Cause: The proximate cause of this issue is the usage of the Downloader nuget package for the downloading of files. This package struggles when downloading large quantities of very small files (which many modlists contain thousands of) and it can also lead to elevated memory usage when downloading multiple larger files. The package author indicates that the package is not necessarily suited to downloading large amounts of small files: bezzad/Downloader#158
Solution: Leverage the more modern and supported HttpClient/HttpClientFactory classes to execute these downloads. These are much more performant and are still supported, unlike the .Net Framework classes that are leveraged in the Downloader package.
This adds an additional download client in addition to the current ReusableDownloader. This new client uses the HttpClient to download and stream directly to the disk. Consequently, it is easily able to saturate a 1gb/s connection and because the contents of the files are not persisted memory usage is not a meaningful factor.
This new client does not have the various recovery/resume features of the current approach and as such may be less desirable for those with slower or less reliable connections.
The usage of this path is controlled by a value that can be set in the settings menu that defines the minimum file size at which the ResumableDownloader will be used. It defaults to 0, so anyone who wishes to leverage this new approach will need to increase that setting to a value they feel is appropriate.
The only progress tracking provided with the new approach is when a download starts and finishes, so no progress bars are displayed in the UI but the downloaded files count is updated as expected. Adding static progress bars for downloads should be straightforward. Getting live progress updates would require some non-trivial work. For most downloads this is negligible as they complete so fast but it can be slightly annoying for larger downloads as their status is unknown.
This PR has the ancillary effect of reducing the frequency of UI freezing, though I do not know if that is because of the elimination of progress reporting or if it is related to the downloads themselves.
This PR also creates a new project, Wabbajack.Downloaders.Services, and moves several classes that were previously in Wabbajack.Networking.Http, as to better align these classes to their purpose.
Performance Comparison
Below shows the CPU (red), Memory (blue), and Bytes Received (blue) during the download stage of an installation of LoreRim 3.0 using the approach added here. Memory usage by Wabbajack specifically reached around 5gb at its highest but stabilized around 3gb. Bytes received maintained at ~980Mb/s, decreasing at the end as the number of download streams began to decrease.
For comparison, the below shows the same modlist being downloaded in 3.7.5.1 using the default settings. Download speeds fall up and down, reaching a few hundred mb/s periodically but sustained speeds are quite low. Memory usage reached as high as 17gb and was fluctuating around 10-12gb on average. It would need to run for several hours to complete the full download.
Below shows the added UI element to control the size at which the download client switches to ResumableDownloader
I adjusted the way the Performance settings were being injected in order to make the DI a little cleaner and to deduplicate some code that was being used in two places.
I cleaned up any unnecessary usings and whitespace irregularities in the files I edited as well.