-
Notifications
You must be signed in to change notification settings - Fork 901
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
nvCOMP GZIP integration #16770
nvCOMP GZIP integration #16770
Conversation
…fea-nvcomp-gzip
Unit tests are passing when the integration is enabled. Keeping it disabled by default for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks straightforward.
@@ -447,6 +454,7 @@ std::optional<std::string> is_decompression_disabled(compression_type compressio | |||
size_t required_alignment(compression_type compression) | |||
{ | |||
switch (compression) { | |||
case compression_type::GZIP: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvCOMP omitted nvcompGzipRequiredAlignment
, so I could not follow the pattern we use for other formats.
We can return the same value as deflate because it's the same compression format; the only difference is an additional header in GZIP.
The alternative would be to use a hard-coded number.
@@ -447,6 +454,7 @@ std::optional<std::string> is_decompression_disabled(compression_type compressio | |||
size_t required_alignment(compression_type compression) | |||
{ | |||
switch (compression) { | |||
case compression_type::GZIP: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this require the same alignment as DEFLATE
instead of its own alignment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#16770 (comment)
we posted at the same time :)
updated that comment to provide context
/merge |
This reverts commit 750adca.
Description
nvCOMP GZIP integration. Opt-in for now.
Checklist