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

Improve progress bar flickering. #6615

Merged
merged 2 commits into from Feb 1, 2019

Conversation

Projects
None yet
5 participants
@ehuss
Copy link
Contributor

ehuss commented Feb 1, 2019

This attempts to reduce the amount of flickering primarily by reducing the number of times the progress bar is updated. The outline of changes:

  • Don't display the progress bar for all the initial "Fresh" messages (if -v is not given).
  • Don't redisplay the progress bar if it hasn't changed.
  • Don't clear the progress bar if it is not displayed.
  • Don't clear the progress bar for Message events that don't print anything.
  • Drain messages in batches to avoid frequently updating the progress bar.
  • Only display progress bar if the queue is blocked.

This significantly improves the initial "fresh" updates, but I still see some flickering during normal updates. I wasn't able to figure out why. Logging to a file and doing screen captures I see cargo is printing the progress bar <1ms after it is cleared. I'm guessing that it's just bad timing where the terminal renders just before the progress bar is displayed, and it has to wait an entire rendering cycle until it gets displayed.

I tested on a variety of different terminals and OS's, but the more testing this can get the better.

This unfortunately adds some brittleness of carefully clearing the progress bar before printing new messages. I don't really see an easy way to make that better since there is such a wide variety of ways a Message can be printed.

Fixes #6574

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Feb 1, 2019

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

alexcrichton left a comment

This looks great to me, thanks! All those strategies sound spot on to me, and the code all looks great.

The only thing I'd wonder is if we could improve the situation with removing the need for self.progress.clear(). I think we basically funnel all output through config.shell(), so I wonder if we could add hooks there to clear progress and such? It'd be great if we could do something like register a current progress bar and then any prints would automatically clear the progress bar, and then the progress bar would auto-update next time it gets an update or something like that.

I suspect though that the form of auto-clearning like would be nice is actually pretty difficult to implement and not easy to set up. Perhaps worth giving it a small stab though? If it doesn't work out I'm fine with manual clearning and vigilant reviews

cx.bcx
.config
.shell()
.verbose(|c| c.status("Running", &cmd))?;

This comment has been minimized.

@alexcrichton

alexcrichton Feb 1, 2019

Member

Since the verbosity check is already happening above, could this c.status(...) move into the if above?

This comment has been minimized.

@alexcrichton

alexcrichton Feb 1, 2019

Member

Similar with "Fresh" below

@ehuss ehuss force-pushed the ehuss:progress-updates branch from 0932af1 to 2932713 Feb 1, 2019

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Feb 1, 2019

I've updated with a flag in Shell to track the status. I think it is marginally better, but still a little brittle.

The only other approach I can think of is to merge Progress and Shell together, but that doesn't seem great.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 1, 2019

@bors: r+

Ok I think I'm personally a bit more in favor of this current flag, so let's stick with that and see how it goes!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2019

📌 Commit 2932713 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2019

⌛️ Testing commit 2932713 with merge 129e762...

bors added a commit that referenced this pull request Feb 1, 2019

Auto merge of #6615 - ehuss:progress-updates, r=alexcrichton
Improve progress bar flickering.

This attempts to reduce the amount of flickering primarily by reducing the number of times the progress bar is updated. The outline of changes:

- Don't display the progress bar for all the initial "Fresh" messages (if -v is not given).
- Don't redisplay the progress bar if it hasn't changed.
- Don't clear the progress bar if it is not displayed.
- Don't clear the progress bar for `Message` events that don't print anything.
- Drain messages in batches to avoid frequently updating the progress bar.
- Only display progress bar if the queue is blocked.

This significantly improves the initial "fresh" updates, but I still see some flickering during normal updates. I wasn't able to figure out why. Logging to a file and doing screen captures I see cargo is printing the progress bar <1ms after it is cleared. I'm guessing that it's just bad timing where the terminal renders just before the progress bar is displayed, and it has to wait an entire rendering cycle until it gets displayed.

I tested on a variety of different terminals and OS's, but the more testing this can get the better.

This unfortunately adds some brittleness of carefully clearing the progress bar before printing new messages. I don't really see an easy way to make that better since there is such a wide variety of ways a `Message` can be printed.

Fixes #6574
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 129e762 to master...

@bors bors merged commit 2932713 into rust-lang:master Feb 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

bors added a commit that referenced this pull request Feb 2, 2019

Auto merge of #6618 - ehuss:fix-progress-stdout, r=Eh2406
Fix overlapping progress with stdout.

Minor issue introduced in #6615. The progress bar was not being cleared for stdout output (because Shell only does stderr).

@ehuss ehuss referenced this pull request Feb 3, 2019

Merged

Update cargo #58131

bors added a commit to rust-lang/rust that referenced this pull request Feb 5, 2019

Auto merge of #58131 - ehuss:update-cargo, r=alexcrichton
Update cargo

7 commits in 245818076052dd7178f5bb7585f5aec5b6c1e03e..4e74e2fc0908524d17735c768067117d3e84ee9c
2019-01-27 15:17:26 +0000 to 2019-02-02 17:48:44 +0000
- Fix overlapping progress with stdout. (rust-lang/cargo#6618)
- Improve progress bar flickering. (rust-lang/cargo#6615)
- Add detail to multiple rename deps (rust-lang/cargo#6603)
- Fix race condition in local registry crate unpacking (rust-lang/cargo#6591)
- Revert "Make incremental compilation the default for all profiles." (rust-lang/cargo#6610)
- Fixup the docs on crate-type (rust-lang/cargo#6606)
- Document that owner --add now just invites (rust-lang/cargo#6604)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment