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

fix members of a multibar always rendering #630

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

remi-dupre
Copy link

Hi!

I noticed a rather inconvenient performance issue with MultiBar, contrary to regular progress bar no rate limiter will prevent computing the string that will displayed for a given member ProgressBar. The cost of this "hidden rendering" can quickly add up if unicode+ANSI decoding features are enabled.

My implementation relies on actually rejecting calls to DrawTarget::drawable for multi-bar targets but marking the member as "delayed" for later redraw. When a call for drawable is actually accepted, all delayed member or first redrawn.

Here is an example flamegraph from the application I'm building, using the main branch :

flamegraph-main

A recurring call to set_length takes 30% of the running time in the example above. With my patch it is down to <4% :

flamegraph-fork

@remi-dupre remi-dupre marked this pull request as draft February 9, 2024 15:19
@remi-dupre remi-dupre force-pushed the fix-multibar-performance branch from 5f28ab1 to 53b4628 Compare February 9, 2024 15:51
@remi-dupre remi-dupre marked this pull request as ready for review February 9, 2024 15:52
@djc
Copy link
Member

djc commented Feb 9, 2024

This seems like a pretty roundabout way of doing it. Could we instead keep the Arc<Mutex<BarState>> in MultiStateMember and let all the members draw only when the MultiState::draw_target rate limit allows it?

@remi-dupre remi-dupre force-pushed the fix-multibar-performance branch from 53b4628 to 90df9b6 Compare February 9, 2024 19:02
@remi-dupre
Copy link
Author

Think I get what you mean, I'll try something 👍

@remi-dupre
Copy link
Author

Actually i'm not so sure I get it 😞

Do you mean removing the "delayed" logic altogether? If I do that, a low frequency bar might never redraw if a high frequency bar gets all the allowances 😕

@djc
Copy link
Member

djc commented Feb 10, 2024

In my mind the ideal way of doing this is that we should only draw all of the multistate members together into a single drawable, relying on the MultiState::draw_target's rate limiter to decide when to draw. I think that this solution should not suffer from the problem of low frequency bars never redrawing because all bars would always get drawn at the same time.

It might be tricky to make this work out, though.

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