-
Notifications
You must be signed in to change notification settings - Fork 7
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 ZStandard byte compressor #20
Conversation
registry.addDirectory( "../../install/lib" ); | ||
registry.addDirectory( "images" ); | ||
registry.addDirectory( | ||
"/nfs4/bbp.epfl.ch/visualization/resources/meshes/mediumPly/" ); |
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.
really? what happens if this directory does not exist?
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.
The no file is found.
It would be good to write a few words to motivate each of the two individual commits. On what basis where the compression parameters changed? When should one choose to use the new ZStandard over the other alternatives? |
They are relative to the RLE compressor and the data compressed. Since both change over time with new compilers, CPUs and tested data, they are a somewhat arbitrary snapshot at creation time. They are determined by running the compressor benchmark which calculates them. |
There are two ways: Let Pression decide by the magic in Compressor::choose (https://github.com/Eyescale/Pression/blob/master/pression/compressor.h#L78), or pick one manually for your use case. As you can see from the speed/ratio, it's about 2x slower then the current "standard" Snappy but compresses 30% better. I'm still benchmarking what that means for Collage-based data distribution. |
NB: I'm working on compression plugin API v2 which should be usable for normal people, so that we may start using it in Deflect and ZeroEQ eventually. |
OK thanks, the important part for me is to add this information in the changelog or commit messages for future reference. Something like:
|
Added point 1 as FAQ in README |
The profile of ZStandard is part of it's spec, imo it would be redundant to mention it again. |
Retest this please. |
Well, these things are somewhat subjective, but I think it would have helped me as a reviewer to see this information in the commit messages, and by extension I though it could be useful for users and future reference. But +1 then. |
This compressor is 2x slower then the current "standard" Snappy but compresses 30% better. Users can select it manually or let Compressor::choose pick it for the appropriate speed/ratio.
Retest this please. |
Ping - needs approval (now) |
) | ||
|
||
include_directories( | ||
${CMAKE_CURRENT_SOURCE_DIR}/compressor/zstd/lib |
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.
absolute path necessary?
_results[0]->reserve( size ); | ||
|
||
size = ZSTD_compress( _results[0]->getData(), size, inData, nPixels, 2 ); | ||
assert( size != 0 ); |
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.
throw and assert?
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.
+1 otherwise
Add ZStandard byte compressor, update byte compressor profiles This compressor is 2x slower then the current "standard" Snappy but compresses 30% better. Users can select it manually or let Compressor::choose pick it for the appropriate speed/ratio.
No description provided.