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

test(block): add benchmarks #368

Merged
merged 1 commit into from
Aug 5, 2023

Conversation

Valentin271
Copy link
Member

Description

I added benchmarks to the block widget to uncover eventual performance issues.
I tried to cover common (creation, render) or expensive (inner area) calculations. I also ran the benchmark with three different common buffer sizes.
From what I can see it doesn't seem to have any bottleneck here.

What is covered by the benches :

  • Block creation
  • Render with different parameters (empty, titles, borders, padding)
  • Computing inner area

Details

  • This PR adds benchmarks only for block. I think it's a better idea to split the work on benchmarks to add them incrementally.

  • I'm not sure the black_box is really necessary for the block creation bench. Otherwise the compiler advises to put the new function reference as parameter (see below). I'm afraid this is less legible and might not work as expected.

group.bench_function("new", |b| b.iter(Block::new));
  • Lastly, I have a small concern about my benches reports not looking the same as the paragraph ones :
    image
    I based myself off of this bench (the paragraph one), so I'm not sure this is the expected behavior (works in the terminal and results are correct once clicked on though).

This PR relates to #137.

Thank you for your review and your work.

@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Merging #368 (2887861) into main (e82521e) will not change coverage.
Report is 3 commits behind head on main.
The diff coverage is n/a.

❗ Current head 2887861 differs from pull request most recent head a61bf39. Consider uploading reports for the commit a61bf39 to get more accurate results

@@           Coverage Diff           @@
##             main     #368   +/-   ##
=======================================
  Coverage   84.99%   84.99%           
=======================================
  Files          40       40           
  Lines        8686     8686           
=======================================
  Hits         7383     7383           
  Misses       1303     1303           

@joshka
Copy link
Member

joshka commented Aug 5, 2023

I think you probably need to have a numeric value to get the output like the paragraph (could use Rect::area() for this value)

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

LGTM. I'd be surprised if this really ever has significant perf issues.

To keep the benchmark times down, it's worth consolidating some of the benches. E.g. forget about the bare new() bench. This was only necessary in the paragraph benches as we're testing some fairly large strings (~200*65535 chars) to understand the limits of scrolling and wrapping.

I suspect (without running these) that the render times are likely fairly similar (or the absolute value is small enough that the relative difference is irrelevant). In which case, it's probably sufficient to bench an empty block, and one with all the features (titles, borders, styles, padding etc.)

Can you paste some of the output metrics into the PR? It doesn't hurt to have them recorded for posterity later.

benches/block.rs Outdated Show resolved Hide resolved
@Valentin271
Copy link
Member Author

Valentin271 commented Aug 5, 2023

I think you probably need to have a numeric value to get the output like the paragraph (could use Rect::area() for this value)

Yes, you're right. I used area as suggested. Too bad criterion cannot use text as a table entry :/

LGTM. I'd be surprised if this really ever has significant perf issues.

Same here, just thought I'd benchmark the main building blocks starting with the easiest for me.

I suspect (without running these) that the render times are likely fairly similar (or the absolute value is small enough that the relative difference is irrelevant). In which case, it's probably sufficient to bench an empty block, and one with all the features (titles, borders, styles, padding etc.)

After modifying the benchmark according to your review and looking more in depth, render times are similar : only 10 to 50 µs between empty and with all features for the biggest area (depending on the run, on my machine of course).

Can you paste some of the output metrics into the PR? It doesn't hurt to have them recorded for posterity later.

Sure, here are some comparisons I found interesting (keep in mind this was recorded on my machine, you might not get the same results) :

Figure 1: Time to render all features relative to the buffer area
image

Figure 2: Time to render an empty block relative to the buffer area
image

Figure 3: Time to render an empty block and one with all features on a maxed ou area
image

Figure 4: Time to render an empty block and one with all features on a common area (200x50)
image

The last two graphs are interesting because we can see a difference when rendering all features on figure 3 (which is to be expected) but when used in a common context there's actually no visible difference (figure 4).


  • I removed the smallest area (20x10) because it was always about 1µs, I don't those times are really of any use
  • I also removed the inner computation as it was taking 5 ns constantly

@joshka
Copy link
Member

joshka commented Aug 5, 2023

Looks good - I think Figure 4 is the one that matters the most here. And it's fast enough.
Thanks for doing this - it's another good base test to add to.

Added benchmarks to the block widget to uncover eventual performance issues
@orhun orhun force-pushed the refactor/block-benchmarks branch from 2887861 to a61bf39 Compare August 5, 2023 14:46
@orhun orhun changed the title test(block): Added benchmarks test(block): add benchmarks Aug 5, 2023
@orhun orhun added this pull request to the merge queue Aug 5, 2023
Merged via the queue into ratatui-org:main with commit e18393d Aug 5, 2023
28 checks passed
@Valentin271 Valentin271 deleted the refactor/block-benchmarks branch August 5, 2023 15:18
@joshka joshka added this to the v0.23.0 milestone Aug 21, 2023
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

3 participants