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

Only get draw_target-width when we actually draw #683

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

jaheba
Copy link
Contributor

@jaheba jaheba commented Jan 13, 2025

When calling BarState::draw, there is a call to self.draw_target.width().

However, that operation can be expensive and the width is not needed, if there is no draw-target.

Thus, this PR delays the calling .width() if there is not target to draw to.

For comparison, I've used this minibenchmark:

    let bar = ProgressBar::new_spinner();

    for n in 0..1_000_000 {
        bar.set_message(format!("{}", n));
    }
    bar.finish();

Using the latest release it runs in about 400ms, whilst the fixed version uses about 100ms.

The issue surfaced with uv, when resolving many packages: astral-sh/uv#10384

src/state.rs Outdated Show resolved Hide resolved
@jaheba
Copy link
Contributor Author

jaheba commented Jan 13, 2025

@djc Tests pass locally now.

@jaheba
Copy link
Contributor Author

jaheba commented Jan 13, 2025

Actually, this is not working, since self.draw_target.drawable(...) will set the time of latest update, and then second call to .drawable can return None.

@jaheba
Copy link
Contributor Author

jaheba commented Jan 13, 2025

Should (hopefully) work now.

The contrast in speed is actually bigger when using hyperfine --show-output. The fixed version takes ~120ms, while the current version takes ~700ms.

@jaheba
Copy link
Contributor Author

jaheba commented Jan 13, 2025

@djc Let me know if you need any else for this PR.

@jaheba jaheba mentioned this pull request Jan 13, 2025
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

LGTM modulo some small nits.

Would be nice if you can squash your commits.

Thanks for these performance wins. Do you have more in the pipeline? Would you like a release once these are merged?

src/state.rs Outdated
// `|= self.is_finished()` should not be needed here, but we used to always draw for
// finished progress bars, so it's kept as to not cause compatibility issues in weird cases.
force_draw |= self.state.is_finished();

Copy link
Member

Choose a reason for hiding this comment

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

Nit: please avoid unrelated cosmetic changes.


pub(crate) fn width(&self) -> Option<u16> {
match self {
Drawable::Term { term, .. } => Some(term.size().1),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please use Self instead of Drawable here.

@jaheba jaheba force-pushed the fix-draw_target-width branch from accfa17 to 9f0fb9c Compare January 15, 2025 15:42
@jaheba
Copy link
Contributor Author

jaheba commented Jan 15, 2025

Thanks for the review!

I've addressed your comments.

I don't have any more changes (besides #684) planned. I wanted to look into some basic benchmarking to see if there are any other possible performance improvements.

@djc djc merged commit 33a7843 into console-rs:main Jan 15, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants