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

Entropy: improve performance by keeping colorgradient static. #32

Merged
merged 3 commits into from Nov 14, 2021

Conversation

siedentop
Copy link
Contributor

Thanks for the repo!

I ran cargo flamegraph on the code when using the Entropy visualization. This showed one very obvious area of improvement. csscolor::parse::parse was parsing hex numbers as part of creating the colograde::magma() color gradient.

I think the easiest solution is just to make colorgrade::magma() a static and re-use it. What do you think?

Aside: Is there any work in making the UI into a separate thread from the calculations? The mouse-over is just sluggish when I change the color scheme.

Thanks for reviewing!

@siedentop
Copy link
Contributor Author

siedentop commented Nov 9, 2021

Update: Faster UI can be found in this PR: #34

PING: @sharkdp

@sharkdp
Copy link
Owner

sharkdp commented Nov 9, 2021

Hi Christoph, thank you for the feedback!

I ran cargo flamegraph on the code when using the Entropy visualization. This showed one very obvious area of improvement. csscolor::parse::parse was parsing hex numbers as part of creating the colograde::magma() color gradient.

I think the easiest solution is just to make colorgrade::magma() a static and re-use it. What do you think?

Maybe we could be even faster by computing (and caching) the actual colors for a set of (discretized) entropy values? We do something very similar in the ColorGradient style:

binocle/src/style.rs

Lines 77 to 81 in 4f995ef

let mut byte_color = [[0, 0, 0, 0]; 256];
for (byte, color) in byte_color.iter_mut().enumerate() {
let gradient_color = gradient.at((byte as f64) / 255.0f64);
*color = rgba_from_color(gradient_color);
}

Aside: Is there any work in making the UI into a separate thread from the calculations? The mouse-over is just sluggish when I change the color scheme.

Not yet, no. I knew I was asking for trouble though 😄. Running rendering and UI in separate threads is definitely the way to go.

@sharkdp
Copy link
Owner

sharkdp commented Nov 9, 2021

By the way: there is a ticket for slow entropy computation here: #7. I have a few ideas on how to speed it up, but most of this comes at the cost of accuracy (like computing only one entropy value per N byte chunk).

@siedentop
Copy link
Contributor Author

Maybe we could be even faster by computing (and caching) the actual colors for a set of (discretized) entropy values? We do something very similar in the ColorGradient style

Ah yes! I'll try that instead. That should be better. Although, performance wise it won't make as much of a difference (because with the current changes, all the compute is happening in the entropy calculation, I believe). But it is the cleaner approach (and probably just a tad faster).


Not yet, no. I knew I was asking for trouble though 😄. Running rendering and UI in separate threads is definitely the way to go.

PR #34 addresses this. While I think it makes the logic a bit more complicated and isn't in the spirit of "immediate mode" in egui, it is the approach suggested by the Pixel library (get_frame method):

Get a mutable byte slice for the pixel buffer. The buffer is not cleared for you; it will
retain the previous frame's contents until you clear it yourself.

In consequence, I would add multiple threads on-top of the "only paint if changed". But just using separate threads would also be appropriate, IMO.

[I will cross-post this on #34.]

@siedentop
Copy link
Contributor Author

I have implemented your suggestion of caching the discretized entropy value.

I was not able to set up a benchmark (without a massive overhaul of the crate), and did not measure performance. Both implementations feel the same to me.

I prefer the original version, since the logic is simpler. But feel free to decide which version you prefer and choose that one.

src/style.rs Outdated Show resolved Hide resolved
@siedentop
Copy link
Contributor Author

siedentop commented Nov 11, 2021

Some performance measurements with tracing.

All data is with the test file as supplied and Zoom set to 1x. And all other settings left at default. Of course, Entropy was selected. This is done on an M1 Macbook Air connected to a 5K monitor.

NOTE: First measurements done running in debug 🤦🏼‍♂️. Left for posterity, but should not be considered.

I am looking at the time of the Binocle::draw annotated function. The RedrawRequested part will always take 16ms (for a consistent frame rate). There is virtually no jitter.

Version Commit Debug @ 1x Release @ 1x Release @ 4x
master 4f995ef 219 ms 9.2 ms 60 ms (!)
Static Gradient 8ecfe64 160 ms 6.7 ms 13 ms
Discretized + Cached Color 6624c03 155 ms 6.6 ms 11.8 ms

This is only possible due to the tracing support: #35

@sharkdp
Copy link
Owner

sharkdp commented Nov 14, 2021

Thank you for the benchmark results and the update. Still need to figure out why it's slower when zoomed in.

@sharkdp sharkdp merged commit 83779cc into sharkdp:master Nov 14, 2021
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.

None yet

2 participants