Skip to content

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

Merged
Cyan4973 merged 1 commit intofacebook:devfrom
binhdvo:bootcamp
Jun 9, 2021
Merged

Fix --progress flag to properly control progress display and default …#2698
Cyan4973 merged 1 commit intofacebook:devfrom
binhdvo:bootcamp

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
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

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