Increase speed for create and unpack bundle operations - #26
Conversation
Now can support bundles split on uncompressed size. This allows for compression to be deferred and run asynchronously. This method is not appropriate for all data types. Split on compressed size remains the default. Memory operations that were causing speed degredation have been mitigated. Processing bottleneck is now md5 calculations and compression. Thread options have been added. Chunk processing is now conducted asynchronously. With deferred compression this is a considerable speed increase. Memory operations that were causing speed degredation have been mitigated. This is aided by caching more in RAM and the cache value is exposed to the user. Processing bottleneck is now md5 checksum and IO. Tests functionally remain the same, changes were made to make them work with the changes in internal interfaces. Further tests to cover new options will follow. Options added to CLI contain appropriate documentation through help interface. Auto generated c++ api covers all additions to the interface. Supporting bundle documentation is now out of date, an update will follow.
1. Added option to fail if patch size exceeds certain size 2. create-patch will now also produce a resourcegroup of new files added 3. apply-patch can now be set to skip new files to allow for them to be delivered in an alternate manner. Ie through bundling which works better if the new files added are large.
apply-patch now exposes a vector that will hold all the relative paths of the resources to remove between next and prev supplied. create-patch size limit now defaults to 0 meaning no limit.
* create-bundle: skip compression, split on uncompressed, threading * create-patch: creation of new files resource group, fail max patch size exceeded
|
|
||
| DownloadSettings downloadSettings; | ||
|
|
||
| uintmax_t chunkReadCacheSize = 1073741824; |
There was a problem hiding this comment.
That's a lot of bytes. Any chance we could present this in a more human readable format?
There was a problem hiding this comment.
Yep :D
How do you mean here specifically?
Have it be in GB? my only concern there would be that everything else is in bytes in the API. Or are we talking when the value is presented in the CLI? With this being intended to be used through lib and the CLI only exposes this operation in the dev build it might not make sense to bother.
There was a problem hiding this comment.
It doesn't have to be any particular specific way. I'd consider any of the following to be an improvement
I'd be fine with any of the following for example:
uintmax_t chunkReadCacheSize = 1073741824; // 1 GB
uintmax_t chunkReadCacheSize = 1 << 29
uintmax_t chunkReadCacheSize = 1024 * 1024 * 1024
But if I were to pick a favorite way to handle this sort of thing, I'd have to go with User-defined literals.
| /// @see ResourceGroup::CreatePatch for information regarding patch creation. | ||
| /// @return Result see CarbonResources::Result for more details. | ||
| Result Apply( const PatchApplyParams& params ); | ||
| Result Apply( PatchApplyParams& params ); |
There was a problem hiding this comment.
Why take away the const qualifier here?
| int numThreads = params.asyncSettings.numberOfThreads + 1; | ||
|
|
||
| chunkFile.clearCache = false; | ||
| ThreadPool<std::shared_ptr<ProcessChunkPoolArguments>, std::shared_ptr<ProcessChunkThreadArguments>> threadPool( numThreads - 1 ); |
There was a problem hiding this comment.
Why initialize numThreads as params.asyncSettings.numberOfThreads + 1, only to subtract the 1 again when the variable is used?
| endif() | ||
|
|
||
| project(resources VERSION 4.3.2) | ||
| project(resources VERSION 4.4.0) |
There was a problem hiding this comment.
Wouldn't this change be breaking ABI, and therefore would warrant a major revision bump?
|
|
||
| if( bundleCreateParams.calculateCompressions ) | ||
| { | ||
| std::cout << "Calculate Compression: Off" << std::endl; |
There was a problem hiding this comment.
Shouldn't this be "Calculate Compression: On" ?
|
|
||
| if( patchApplyParams.skipNewFiles ) | ||
| { | ||
| std::cout << "Skip New Files: Off" << std::endl; |
There was a problem hiding this comment.
Shouldn't this be the "Skip New Files: On" case?
| } | ||
| if( m_splitOnCompressedSize ) | ||
| { | ||
| m_compressionStream->operator<<( &data ); |
There was a problem hiding this comment.
This no longer checks the return values of the << operator. Is this intentional?
| if( addResourceResult.type != ResultType::SUCCESS ) | ||
| { | ||
| delete chunkResource; | ||
| commonArguments->bundleResources[threadArguments->chunkIndex] = chunkResource; |
There was a problem hiding this comment.
If we don't make it to this point, then we leak chunkResource.
| commonArguments->RunStatusUpdate(); | ||
| } | ||
|
|
||
| return; |
|
|
||
| if( !ResourceTools::GetLocalFileData( sourceFile, data ) ) | ||
| { | ||
| commonArguments->status = Result{ ResultType::FAILED_TO_OPEN_FILE }; |
There was a problem hiding this comment.
Shouldn't we acquire the mutex before we update the status here? Also, what do we want to do if the status is already in a failure state? Wouldn't it be better to retain the original error status?
|
|
||
| if( !data.compressedChunkIn->StartRead( compressedDir ) ) | ||
| { | ||
| if( index > m_chunkInfos.size() ) |
There was a problem hiding this comment.
Shouldn't this be index >= m_chunkInfos.size()?
| m_nextResourceGroupPathArgumentId( "next-resourcegroup-path" ), | ||
| m_resourceGroupRelativePathArgumentId( "--resourcegroup-relative-path" ), | ||
| m_patchResourceGroupRelativePathArgumentId( "--patchResourcegroup-relative-path" ), | ||
| m_newFilesResourceGroupRelativePathArgumentId( "--new-files-Resourcegroup-relative-path" ), |
There was a problem hiding this comment.
That uppercase "R" looks out of place.
|
|
||
| AddArgumentFlag( m_splitOnUncompressedSize, "Set to split on uncompressed size rather than the default compressed size. This will likely lead to more inefficient chunking but may run faster." ); | ||
|
|
||
| AddArgument( m_numberOfThreadsId, "Nnumber of threads to use for async processes.", false, false, SizeToString( defaultParams.asyncSettings.numberOfThreads ) ); |
There was a problem hiding this comment.
There's an extra n in "Number".
The focus of this change is to increase both creation and unpacking of bundles so it can be used for delivery of large datasets.
Especially focused on datasets which contain very large files.
Local speed test results running on large data set
create-bundle
before: ~38mins
after: ~2mins
unpack-bundle
before: ~8mins
after: ~2mins
create-bundle operation
Now can support bundles split on uncompressed size. This allows for compression to be deferred and run asynchronously. This method is not appropriate for all data types. Split on compressed size remains the default.
Memory operations that were causing speed degredation have been mitigated. Processing bottleneck is now md5 calculations and compression.
Thread options have been added. Chunk processing is now conducted asynchronously. With deferred compression this is a considerable speed increase.
unpack-bundle operation
Memory operations that were causing speed degredation have been mitigated. This is aided by caching more in RAM and the cache value is exposed to the user. Processing bottleneck is now md5 checksum and IO.
apply-patch operation
Apply patch now exposes the resource relative paths that have been removed between previous and next. This is exposed in input parameters as a vector.
Tests
Tests functionally remain the same, changes were made to make them work with the changes in internal interfaces.
Further tests to cover new options will follow.
Documentation
Options added to CLI contain appropriate documentation through help interface. Auto generated c++ api covers all additions to the interface. Supporting bundle documentation is now out of date, an update will follow.