Skip to content
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

Adding a LZ4 Compressor/Decompressor to the Compression Gem #15326

Merged
merged 3 commits into from Mar 29, 2023

Conversation

lemonade-dm
Copy link
Contributor

@lemonade-dm lemonade-dm commented Mar 23, 2023

A lz4 compressor implementation has been added that registers with the
the CompressionRegistrar which is available in tools code.
In client code a lz4 decompressor implementation has been added that
registers with the DecompressionRegistrar.

Added UnitTest to the Compression Gem to validate the
CompressionRegistrar and DecompressionRegistrar works properly.

Updated the Compression Interface with a CompressBound function which
provides an upper bound on potential compressed data size given the
uncompressed data size.

Also added more detailed error result structure which can provide an
error string on errors that occurs during compression/decompression

How was this PR tested?

Ran newly added Googletest for the Compression Gem that validates the (De)CompressorRegistrar system and the (De)CompressorLZ4Impl classes

@lemonade-dm lemonade-dm requested review from a team as code owners March 23, 2023 17:48
@lemonade-dm lemonade-dm requested a review from a team March 23, 2023 17:59
AddComponentDescriptors and AddRequiredComponents to accept
non-allocating structures.

Instead of accepting a vector of parameters, they have been updated to
accept a span or initalizer_list types.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
The CompressionLZ4 gem wraps the lz4 3rdParty library and implements the
Compression Gem ICompressionInterface and IDecompressionInterface.

A lz4 compressor implementation has been added that registers with the
the CompressionRegistrar which is available in tools code.
In client code a lz4 decompressor implementation has been added that
registers with the DecompressionRegistrar.

Added UnitTest to the Compression Gem to validate the
CompressionRegistrar and DecompressionRegistrar works properly.

Updated the Compression Interface with a CompressBound function which
provides an upper bound on potential compressed data size given the
uncompressed data size.

Also added more detailed error result structure which can provide an
error string on errors that occurs during compression/decompression

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
@lemonade-dm lemonade-dm requested review from a team as code owners March 24, 2023 01:43
@lemonade-dm lemonade-dm requested a review from a team March 27, 2023 20:50
@nick-l-o3de
Copy link
Contributor

nick-l-o3de commented Mar 28, 2023

While I can see this was a bunch of work, have we considered just adding this component to the existing compression gem instead of making a new one? Each additional gem adds gigabytes of overhead (due to static lib sizes and pdbs) and many seconds of link time. if its just a part of the engine, does it really have to be its own gem?

It feels like it could be just a handful of files in another gem (liek the compression gem itself) which just handles all the small and low-dependency compression formats that the engine itself offers, in one gem... and thus only one link cost and static compile cost?

@lemonade-dm
Copy link
Contributor Author

While I can see this was a bunch of work, have we considered just adding this component to the existing compression gem instead of making a new one? Each additional gem adds gigabytes of overhead (due to static lib sizes and pdbs) and many seconds of link time. if its just a part of the engine, does it really have to be its own gem?

It feels like it could be just a handful of files in another gem (liek the compression gem itself) which just handles all the small and low-dependency compression formats that the engine itself offers, in one gem... and thus only one link cost and static compile cost?

Yes, this could be added to the existing Compression Gem as part of a builtin list of supported gems.
It would currently only be LZ4 at this time with potential support for Zstd and Zlib coming in the future.

However the Compression Interface gem is meant to be used by other gems to extend the archive system with additional compression algorithms and not necessarily require that compression libraries such as lz4, zstd or zlib be a dependency of the engine.

That being said I think the cost of using those libraries and loading in the few symbols that the compression gem would use, would have negligible cost.

I'll move over the Compressor LZ4 implementation into the Compression gem

@nick-l-o3de
Copy link
Contributor

While I can see this was a bunch of work, have we considered just adding this component to the existing compression gem instead of making a new one? Each additional gem adds gigabytes of overhead (due to static lib sizes and pdbs) and many seconds of link time. if its just a part of the engine, does it really have to be its own gem?
It feels like it could be just a handful of files in another gem (liek the compression gem itself) which just handles all the small and low-dependency compression formats that the engine itself offers, in one gem... and thus only one link cost and static compile cost?

Yes, this could be added to the existing Compression Gem as part of a builtin list of supported gems. It would currently only be LZ4 at this time with potential support for Zstd and Zlib coming in the future.

However the Compression Interface gem is meant to be used by other gems to extend the archive system with additional compression algorithms and not necessarily require that compression libraries such as lz4, zstd or zlib be a dependency of the engine.

That being said I think the cost of using those libraries and loading in the few symbols that the compression gem would use, would have negligible cost.

I'll move over the Compressor LZ4 implementation into the Compression gem

Yes, I'm trying to save compile time here practically. It'd be a different calculus if the gem system added very light weight but each gem currently has a huge minimum memory cost and compile cost. I wish it weren't so... if they were tiny and cheap I'd be all for making lots of them instead of few...

@lemonade-dm
Copy link
Contributor Author

While I can see this was a bunch of work, have we considered just adding this component to the existing compression gem instead of making a new one? Each additional gem adds gigabytes of overhead (due to static lib sizes and pdbs) and many seconds of link time. if its just a part of the engine, does it really have to be its own gem?
It feels like it could be just a handful of files in another gem (liek the compression gem itself) which just handles all the small and low-dependency compression formats that the engine itself offers, in one gem... and thus only one link cost and static compile cost?

Yes, this could be added to the existing Compression Gem as part of a builtin list of supported gems. It would currently only be LZ4 at this time with potential support for Zstd and Zlib coming in the future.
However the Compression Interface gem is meant to be used by other gems to extend the archive system with additional compression algorithms and not necessarily require that compression libraries such as lz4, zstd or zlib be a dependency of the engine.
That being said I think the cost of using those libraries and loading in the few symbols that the compression gem would use, would have negligible cost.
I'll move over the Compressor LZ4 implementation into the Compression gem

Yes, I'm trying to save compile time here practically. It'd be a different calculus if the gem system added very light weight but each gem currently has a huge minimum memory cost and compile cost. I wish it weren't so... if they were tiny and cheap I'd be all for making lots of them instead of few...

The LZ4 Compressor logic and Unit Test have been moved into the Compression Gem and the CompressionLZ4 gem has been removed.

@lemonade-dm lemonade-dm changed the title Adding a CompressionLZ4 sub gem to the Compression Gem Adding a LZ4 Compressor/Decompressor to the Compression Gem Mar 28, 2023
@lemonade-dm lemonade-dm force-pushed the compression-lz4-gem branch 2 times, most recently from 3840ee7 to 02b970c Compare March 28, 2023 19:35
With these changes the CompressionLZ4 gem is being removed.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
@lemonade-dm lemonade-dm merged commit 3909e99 into o3de:development Mar 29, 2023
3 checks passed
@lemonade-dm lemonade-dm deleted the compression-lz4-gem branch March 29, 2023 16:33
tkothadev pushed a commit to aws-lumberyard-dev/o3de that referenced this pull request Mar 29, 2023
* Updating the GemTestEnvironment AddDynamicModules,
AddComponentDescriptors and AddRequiredComponents to accept
non-allocating structures.

Instead of accepting a vector of parameters, they have been updated to
accept a span or initalizer_list types.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

* Adding a CompressionLZ4 sub gem to the Compression Gem

The CompressionLZ4 gem wraps the lz4 3rdParty library and implements the
Compression Gem ICompressionInterface and IDecompressionInterface.

A lz4 compressor implementation has been added that registers with the
the CompressionRegistrar which is available in tools code.
In client code a lz4 decompressor implementation has been added that
registers with the DecompressionRegistrar.

Added UnitTest to the Compression Gem to validate the
CompressionRegistrar and DecompressionRegistrar works properly.

Updated the Compression Interface with a CompressBound function which
provides an upper bound on potential compressed data size given the
uncompressed data size.

Also added more detailed error result structure which can provide an
error string on errors that occurs during compression/decompression

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

* Combined the LZ4 Compression/Decompressor Logic into the Compression Gem

With these changes the CompressionLZ4 gem is being removed.

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>

---------

Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants