Skip to content

Only get draw_target-width when we actually draw#683

Merged
djc merged 1 commit intoconsole-rs:mainfrom
jaheba:fix-draw_target-width
Jan 15, 2025
Merged

Only get draw_target-width when we actually draw#683
djc merged 1 commit intoconsole-rs:mainfrom
jaheba:fix-draw_target-width

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

@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