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

More simplification (with a bonus performance boost) #5

Merged
merged 8 commits into from
Sep 24, 2023

Conversation

tertsdiepraam
Copy link
Member

@tertsdiepraam tertsdiepraam commented Nov 20, 2022

  • Extract snippets like this into a div_ceil function:
let mut num_lines = self.cells.len() / num_columns;
if self.cells.len() % num_columns != 0 {
    num_lines += 1;
}
  • Simplify the Display impl, by precalculating the separator and using the padding functionality of format!.
  • Simplify theoretical_max_num_lines by using enumerate and only deal with the widths instead of the the whole Cell.
  • Remove a clone in width_dimensions.
  • Add an example that takes a few seconds for benchmarking.

Once I had the example, I benchmarked this against main and already got a >2.58x improvement!

Benchmark 1: target/bench/old
  Time (mean ± σ):      4.412 s ±  0.048 s    [User: 3.738 s, System: 0.670 s]
  Range (min … max):    4.354 s …  4.507 s    10 runs
 
Benchmark 2: target/bench/new
  Time (mean ± σ):      1.709 s ±  0.014 s    [User: 1.085 s, System: 0.624 s]
  Range (min … max):    1.694 s …  1.739 s    10 runs
 
Summary
  'target/bench/new' ran
    2.58 ± 0.03 times faster than 'target/bench/old'

EDIT: Got another performance boost from 2.1x to 2.5x faster

This yields another 1.20 performance improvement from 2.0 seconds to 1.7 seconds
@tertsdiepraam tertsdiepraam marked this pull request as draft November 20, 2022 18:56
@tertsdiepraam
Copy link
Member Author

Note entirely sure what's going on but there's some weird things happening in this branch when I try to run all the firefox files.

@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@bfea0a5). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #5   +/-   ##
=======================================
  Coverage        ?   75.71%           
=======================================
  Files           ?        4           
  Lines           ?      387           
  Branches        ?       58           
=======================================
  Hits            ?      293           
  Misses          ?       78           
  Partials        ?       16           
Flag Coverage Δ
macos_latest 75.71% <0.00%> (?)
ubuntu_latest 75.71% <0.00%> (?)
windows_latest 75.71% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@tertsdiepraam tertsdiepraam marked this pull request as ready for review August 12, 2023 17:02
@tertsdiepraam
Copy link
Member Author

Alright, I'm putting this up for review anyway. The reason is that I want to have these cleanups before I start working on more fixes. It doesn't have to be perfect and we can fix more along the way.

@sylvestre sylvestre merged commit b2b5ed5 into uutils:main Sep 24, 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.

2 participants