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

Create closure to do sorting. #281

Merged
merged 4 commits into from Sep 12, 2020
Merged

Create closure to do sorting. #281

merged 4 commits into from Sep 12, 2020

Conversation

aldhsu
Copy link
Contributor

@aldhsu aldhsu commented Sep 13, 2019

Was hacking on #277 and realised that splitting the directories out wasn't necessary so jumped to the sorting with a created closure.

Future architecture for sorting:

  1. Take flags and "compose"(not sure how this works in Rust, so I just went with a loop), sorting functions together to create the sorting function
  2. Cache sorting function to reuse for the recursive calls to Core#sort

The test pass but I would like to do some performance testing and complete the second step.

@aldhsu
Copy link
Contributor Author

aldhsu commented Sep 13, 2019

Performance testing

Using the closure:

~/Code/lsd/target/release/lsd -R --group-dirs=last  1.93s user 1.47s system 89% cpu 3.802 total

Without the closure:

lsd -R --group-dirs=last  1.91s user 1.49s system 89% cpu 3.821 total

Doesn't look like much has changed.

@aldhsu aldhsu marked this pull request as ready for review September 13, 2019 08:40
@aldhsu
Copy link
Contributor Author

aldhsu commented Sep 20, 2019

@Peltoche this is "ready" now. Let me know what I can do to get it merged.

@Peltoche
Copy link
Collaborator

Hi @aldhsu ,

A big PR with a lot of change have been merge. I think we are ready now to merge your PR if you succeed to rebase it on top of the current master

@aldhsu aldhsu force-pushed the use-closure-sort branch 4 times, most recently from c4b6c84 to a651851 Compare December 16, 2019 23:57
@aldhsu
Copy link
Contributor Author

aldhsu commented Dec 17, 2019

Branch is up to date now.
After the refactor, it should be simpler to add new sort criteria, add support for specifying sort direction on individual fields and add support for multisorting.

Possibly minor effect on performance, recursive sorting around 100k files:
Before

Benchmark #1: lsd --recursive -r --sizesort
  Time (mean ± σ):      2.171 s ±  0.015 s    [User: 1.108 s, System: 1.037 s]
  Range (min … max):    2.149 s …  2.187 s    10 runs

After, branch built with --release

Benchmark #1: /Users/allen.hsu/Code/lsd/target/release/lsd --recursive -r --sizesort
  Time (mean ± σ):      2.261 s ±  0.032 s    [User: 1.187 s, System: 1.042 s]
  Range (min … max):    2.218 s …  2.328 s    10 runs

Copy link
Collaborator

@Peltoche Peltoche left a comment

Choose a reason for hiding this comment

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

This is way easier to read, thanks!

I think we can improve a little the readability with an enum for the sort order

src/sort.rs Outdated
let mut sorters: Vec<(bool, Sorter)> = vec![];
match flags.directory_order {
DirOrderFlag::First => {
sorters.push((false, Box::new(with_dirs_first)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be great to replace the boolean with a more explicite enum like:

enum SortOrder {
  Default,
  Reverse,
}

It would give something like:

sorters.push((SortOrder::Reverse, Box::new(with_dirs_first)));

Copy link
Contributor Author

@aldhsu aldhsu Jan 8, 2020

Choose a reason for hiding this comment

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

Oh yeah that totally makes sense and the enum already exists. Addressed in bab0f0e.

@zwpaper
Copy link
Member

zwpaper commented Aug 13, 2020

@aldhsu this change looks great, can u help to fix the conflict and make this PR happen?

@aldhsu aldhsu force-pushed the use-closure-sort branch 2 times, most recently from 4ad510b to e3bb742 Compare August 17, 2020 09:40
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2020

Codecov Report

Merging #281 into master will increase coverage by 5.95%.
The diff coverage is 82.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   66.05%   72.01%   +5.95%     
==========================================
  Files          19       19              
  Lines        1803     1865      +62     
==========================================
+ Hits         1191     1343     +152     
+ Misses        612      522      -90     
Impacted Files Coverage Δ
src/app.rs 73.33% <ø> (+36.49%) ⬆️
src/core.rs 0.00% <0.00%> (ø)
src/display.rs 26.39% <19.04%> (-0.28%) ⬇️
src/meta/mod.rs 18.62% <62.50%> (+1.51%) ⬆️
src/icon.rs 97.85% <80.95%> (+1.34%) ⬆️
src/meta/name.rs 93.04% <89.07%> (-3.23%) ⬇️
src/meta/size.rs 89.38% <91.66%> (+22.39%) ⬆️
src/meta/filetype.rs 82.10% <92.00%> (+4.99%) ⬆️
src/sort.rs 96.11% <92.45%> (+27.12%) ⬆️
src/color.rs 79.41% <100.00%> (+6.43%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e1662d...05a6998. Read the comment docs.

@aldhsu aldhsu force-pushed the use-closure-sort branch 2 times, most recently from 098185d to 5ccfee9 Compare August 17, 2020 12:04
@aldhsu
Copy link
Contributor Author

aldhsu commented Aug 17, 2020

@zwpaper I've updated the PR. I have a bit more knowledge about Rust now and I realized the closure is actually not necessary since closures are just implicit structs. I switched it to store a vec of function pointers on the Core struct instead of a closure, I think this is a bit easier to work with.

Before

Benchmark #1: lsd -R --group-dirs=last
  Time (mean ± σ):     885.4 ms ±  26.4 ms    [User: 551.8 ms, System: 318.1 ms]
  Range (min … max):   854.0 ms … 938.9 ms    10 runs

After

Benchmark #1: ~/Code/lsd/target/release/lsd -R --group-dirs=last
  Time (mean ± σ):     923.2 ms ±  23.5 ms    [User: 596.3 ms, System: 318.9 ms]
  Range (min … max):   909.3 ms … 988.2 ms    10 runs

This still definitely makes sorting a bit slower.

@zwpaper
Copy link
Member

zwpaper commented Aug 31, 2020

@meain @Peltoche could u help to review this PR, this could help developing sorting functionality a lot

@meain
Copy link
Member

meain commented Sep 1, 2020

I will try to take a look at it some time this week.

@meain
Copy link
Member

meain commented Sep 12, 2020

Hey, thanks a lot. LGTM.
Sorry for the delay.

@meain meain merged commit 85bee8b into lsd-rs:master Sep 12, 2020
@aldhsu aldhsu deleted the use-closure-sort branch September 14, 2020 06:04
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

5 participants