Make the CLI summary data use a human readable format instead of raw bytes #2696

scottchiefbaker · 2021-06-05T16:52:33Z

This PR changes the CLI output summary from bytes to a more human readable string format. This should address issue #2694.

Before:

:zstd -f /tmp/foo.bin 
/tmp/foo.bin         :  2.94%   (280708256 => 8264707 bytes, /tmp/foo.bin.zst)

After:

:./zstd -f /tmp/foo.bin 
/tmp/foo.bin         :  2.94%   (267.7M => 7.9M, /tmp/foo.bin.zst)

This PR adds a humanSize() function to util.c. I wasn't sure where best to put it, but that seemed like a good place. Then it modifies the sprintf() calls for outputting the summary after a file is compressed. It also modifies the sprintf() benchmark code.

Things I checked before landing this code:

Code is space indented not tabs
Functions and variables are camelCase
Variable declarations are grouped with other variables
Code should be C89 compatible
Make test runs without errors on x86_64
Code compiles and outputs the correct format for summary, benchmark, and multiple files

I will probably need some help modifying this to work on ARM or other platforms.

felixhandte

Hey @scottchiefbaker,

Thanks for this PR.

This makes sense to me as a nicer way to present this information. I guess there's a question of whether changing this format will break anyone trying to parse this output, but I can't really imagine why anyone would be doing that in the first place.

I included a few suggestions. Let me know what you think.

programs/util.c

felixhandte · 2021-06-07T15:53:33Z

programs/util.c

@@ -121,6 +121,27 @@ int UTIL_requireUserConfirmation(const char* prompt, const char* abortMsg,
 *  Functions
 ***************************************/

+char* humanSize(unsigned long long size, char* str) {
+    if (size > 1125899906842624L) {
+        snprintf(str, 7, "%.1fP", (float)size / 1125899906842624L);


snprintf was standardized in C99, so it's not guaranteed to be available in all the environments we want Zstd to build.

You could consider rewriting this to not require an intermediate buffer if you just output a rescaled float value and a (static) suffix string that are then passed to the DISPLAYLEVEL() call with "%.1f%s". But maybe that would lead to poor results for small inputs ("you compressed 123.0B" feels weird).

A worst case alternative is that you just wrap this in an #if POSIX_PLATFORM_VERSION >= 200112L and provide a fallback #else implementation.

I see references to fprintf()all over the code base so I just assumed snprintf() was valid. util.c. includes <stdio.h> for fprintf(), can we rely on that, but not snprintf()?

C is not my forte, so I may need some guidance on this one.

As C has evolved, they've introduced new functions into the same header. C90 has fprintf() but not necessarily snprintf(). snprintf() was only standardized (and guaranteed to be present) in C99. Zstd restricts itself to the features guaranteed to be present in C90 (or at least has fallbacks to handle that strict case), so that we can be sure it will compile on all C90-conforming platforms.

programs/util.c

felixhandte · 2021-06-07T15:57:40Z

programs/util.h

@@ -122,6 +122,8 @@ int UTIL_requireUserConfirmation(const char* prompt, const char* abortMsg, const
 #define STRDUP(s) strdup(s)
 #endif

+char* humanSize(unsigned long long size, char* str);


Switch to a more descriptive name? Maybe add a little comment here describing what it does?

I'm open to naming suggestions. I will definitely add a description though, that's easy.

Maybe UTIL_renderHumanReadableByteSize()?

scottchiefbaker · 2021-06-07T16:38:13Z

I'm not a C expert, so I will need some help resolving the one failing test.

programs/util.c

felixhandte

Followed up on a couple notes.

If at any point you want to hand this off, I'm happy to merge what you have into a working branch and I can take over and polish this up. I don't want you to feel like you have to put in any more work, especially if, by your admission, you are not all that comfortable working in C. Let me know what you wanna do!

felixhandte · 2021-06-08T17:50:39Z

programs/util.c

@@ -121,6 +121,27 @@ int UTIL_requireUserConfirmation(const char* prompt, const char* abortMsg,
 *  Functions
 ***************************************/

+char* humanSize(unsigned long long size, char* str) {
+    if (size > 1125899906842624L) {
+        snprintf(str, 7, "%.1fP", (float)size / 1125899906842624L);


As C has evolved, they've introduced new functions into the same header. C90 has fprintf() but not necessarily snprintf(). snprintf() was only standardized (and guaranteed to be present) in C99. Zstd restricts itself to the features guaranteed to be present in C90 (or at least has fallbacks to handle that strict case), so that we can be sure it will compile on all C90-conforming platforms.

felixhandte · 2021-06-08T17:56:41Z

programs/util.h

@@ -122,6 +122,8 @@ int UTIL_requireUserConfirmation(const char* prompt, const char* abortMsg, const
 #define STRDUP(s) strdup(s)
 #endif

+char* humanSize(unsigned long long size, char* str);


Maybe UTIL_renderHumanReadableByteSize()?

scottchiefbaker · 2021-06-08T18:38:54Z

@felixhandte if you want to take this and run with it that would be totally fine with me. I just wanted to get the ball rolling. I think most of the hard work is done?

scottchiefbaker · 2021-06-08T20:20:50Z

@felixhandte when you get things massaged let me know. I'd like to see what it looks like when it's all done. I'm sure I'll have questions for you as I'd like to learn more C.

scottchiefbaker added 9 commits June 4, 2021 20:25

Make the CLI output the file sizes in human readable format

49dd77b

Put the human_size() function in util.c

e93fdd3

Convert names to CamelCase

4792532

Make the variable types match

9da459f

Move the variable declarations to the top

2342122

Use human_size() in the benchmark output also

e4e7358

Use human_size() on the "multiple files compressed" output also

dcfaf26

Convert tabs to spaces

b63b497

human_size() should use size_t

dbb3cd2

facebook-github-bot added the CLA Signed label Jun 5, 2021

Use unsigned long instead to help with some tests

91840eb

scottchiefbaker force-pushed the human_size_output branch from 6e2d800 to 91840eb Compare June 5, 2021 17:09

Update humanSize() to skip the big numbers (it requires 64 bit)

d67d965

scottchiefbaker force-pushed the human_size_output branch from c051875 to d67d965 Compare June 5, 2021 17:33

scottchiefbaker added 2 commits June 5, 2021 11:11

Try unsigned long long

fdcfc18

Try enabling the BIG strings now the unsigned long long is in effect

a1d128a

scottchiefbaker force-pushed the human_size_output branch from f954f5b to a1d128a Compare June 5, 2021 18:13

felixhandte requested changes Jun 7, 2021

View reviewed changes

felixhandte self-assigned this Jun 7, 2021

felixhandte added the enhancement label Jun 7, 2021

Some fixes to address things @felixhandte found

10748cb

aqrit reviewed Jun 7, 2021

View reviewed changes

programs/util.c Outdated Show resolved Hide resolved

Attempt to fix a failing test with help from @aqrit

23b6c67

felixhandte reviewed Jun 8, 2021

View reviewed changes

felixhandte changed the base branch from dev to human_size_output June 8, 2021 19:48

felixhandte merged commit fa57d25 into facebook:human_size_output Jun 8, 2021

felixhandte mentioned this pull request Jun 9, 2021

Format File Sizes Human-Readable in the CLI #2702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the CLI summary data use a human readable format instead of raw bytes #2696

Make the CLI summary data use a human readable format instead of raw bytes #2696

scottchiefbaker commented Jun 5, 2021 •

edited

Loading

felixhandte left a comment

felixhandte Jun 7, 2021

scottchiefbaker Jun 7, 2021 •

edited

Loading

felixhandte Jun 8, 2021

felixhandte Jun 7, 2021

scottchiefbaker Jun 7, 2021

felixhandte Jun 8, 2021

scottchiefbaker commented Jun 7, 2021

felixhandte left a comment

felixhandte Jun 8, 2021

felixhandte Jun 8, 2021

scottchiefbaker commented Jun 8, 2021

scottchiefbaker commented Jun 8, 2021

Make the CLI summary data use a human readable format instead of raw bytes #2696

Make the CLI summary data use a human readable format instead of raw bytes #2696

Conversation

scottchiefbaker commented Jun 5, 2021 • edited Loading

felixhandte left a comment

Choose a reason for hiding this comment

felixhandte Jun 7, 2021

Choose a reason for hiding this comment

scottchiefbaker Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

felixhandte Jun 8, 2021

Choose a reason for hiding this comment

felixhandte Jun 7, 2021

Choose a reason for hiding this comment

scottchiefbaker Jun 7, 2021

Choose a reason for hiding this comment

felixhandte Jun 8, 2021

Choose a reason for hiding this comment

scottchiefbaker commented Jun 7, 2021

felixhandte left a comment

Choose a reason for hiding this comment

felixhandte Jun 8, 2021

Choose a reason for hiding this comment

felixhandte Jun 8, 2021

Choose a reason for hiding this comment

scottchiefbaker commented Jun 8, 2021

scottchiefbaker commented Jun 8, 2021

scottchiefbaker commented Jun 5, 2021 •

edited

Loading

scottchiefbaker Jun 7, 2021 •

edited

Loading