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 --no-content-size to the cli #2032

Merged
merged 7 commits into from
Mar 9, 2020

Conversation

bimbashrestha
Copy link
Contributor

#1926

Ran

#!/bin/bash
while true
do
echo "line" >> /tmp/log.txt
done

and

./zstd --compress --no-content-size --stdout /tmp/log.txt > /tmp/log.txt.zst

Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to playTests.sh. You can use zstd -l or zstd -lv to check if the frame content size is written into the frame.

programs/zstdcli.c Outdated Show resolved Hide resolved
@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 9, 2020

Consider adding a line about this new command in programs/zstd.1.md.

Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ about the use case

When using --no-content-size, but the input file size is known, do we still want to adjust our compression parameters for the input file size? Should we mention that behavior in the man page?

@Cyan4973
Copy link
Contributor

Cyan4973 commented Mar 9, 2020

I think the only explicit use case is about compressing a file which is being appended to at the same time, thus changing its size during the compression process.

Setting the size to UNKNOWN disable any parameter adaptation, and make the behavior similar to what providing the input through a pipe would have done.

In contrast, just disabling the content size field, using ZSTD_CCtx_setParameter(cctx, ZSTD_c_contentSizeFlag, 0); would have given a chance to adjust the compression parameters to the first detected size of the file. This could make a small difference for smaller files.

My own preference would be for the second case, though I have no "strong" reason for that, just some "gut feeling", essentially because it feels closer to the expressed command : "no-content-size", rather than "no-content-size-and-assume-large-file". But anyway, I suspect the difference between these 2 options is pretty small and won't matter much.

@terrelln
Copy link
Contributor

terrelln commented Mar 9, 2020

I'm in favor of keeping parameter adjustment as well.

@bimbashrestha
Copy link
Contributor Author

Ah okay then! Haden't thought about changing parameters honestly but that makes sense to me. I'll change it to unset ZSTD_c_contentSizeFlag instead of setting filesize to unknown.

Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once Travis is passing

@bimbashrestha
Copy link
Contributor Author

I thought I noticed Travis failing but it's okay now. Guess it was flakey:/

@bimbashrestha bimbashrestha merged commit 10f915f into facebook:dev Mar 9, 2020
@terrelln
Copy link
Contributor

I thought I noticed Travis failing but it's okay now. Guess it was flakey:/

Yeah, I re-ran it and it passed. Seemed to be some infra flakiness, because it was a super weird issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants