8000 Fix --progress flag to properly control progress display and default … by binhdvo · Pull Request #2698 · facebook/zstd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix --progress flag to properly control progress display and default … #2698

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

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

binhdvo
Copy link
Contributor
@binhdvo binhdvo commented Jun 7, 2021

…progress display on when using -v

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.

This looks good to me, once the build warning is fixed!

I think there is a follow up though, for both compression & decompression:

zstd -c --progress /path/to/file > /dev/null
zstd -dc --progress /path/to/file > /dev/null

In both of these cases, no newline is printed at the end of compression. Normally, the progress is replaced with a status line, like these commands:

zstd /path/to/file -o /dev/null
zstd -d /path/to/file -o /dev/null

I think we should either:

  1. Add a \n after the last progress message.
  2. Also print the final status line with --progress.

I don't have a super strong preference, but I would go with (2) unless we have an explicit reason why we chose not to.

@binhdvo binhdvo force-pushed the bootcamp branch 4 times, most recently from dfb852e to 60e2072 Compare June 7, 2021 22:06
@@ -1612,7 +1612,8 @@ FIO_compressFilename_internal(FIO_ctx_t* const fCtx,
U64 const timeLength_ns = UTIL_clockSpanNano(timeStart);
double const timeLength_s = (double)timeLength_ns / 1000000000;
double const cpuLoad_pct = (cpuLoad_s / timeLength_s) * 100;
DISPLAYLEVEL(4, "%-20s : Completed in %.2f sec (cpu load : %.0f%%)\n",
DISPLAYLEVEL((g_display_prefs.progressSetting == FIO_ps_always ? 1 : 4),
Copy link
Contributor
8000

Choose a reason for hiding this comment

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

This seems separate from the --progress notification issue.
This message is more part of the final status.

And I don't believe that this additional information (time spent) belongs to default, which should remain restricted to 1 line.
We may have room for discussion in determining if it's more appropriate for level 3 (-v) or 4 (-vv),
but it should certainly not be part of 2 (default).

Copy link
Contributor Author
@binhdvo binhdvo Jun 8, 2021

Choose a reason for hiding this comment

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

This was added per Nick's comment suggesting we add the final status line when using --progress to overwrite the last progress message; it will still remain at level 4 in absence of the progress flag

Copy link
Contributor

Choose a reason for hiding this comment

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

The final status line is just the line specifying the final size and compression ratio.
Information about timing and speed are considered supplementary (and do constitute an additional line).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it, will update

@Cyan4973 Cyan4973 merged commit 05d7090 into facebook:dev Jun 9, 2021
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
0