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

Restore Benchmark Functionality & Improve FileTree GetAllDescendantFiles #23

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

Sewer56
Copy link
Member

@Sewer56 Sewer56 commented Sep 19, 2023

Associated details in the following Issues:

Benchmarks:

(On 110476 files)

| Method                 | Mean      | Error     | StdDev    | Gen0     | Gen1    | Gen2    | Allocated |
|----------------------- |----------:|----------:|----------:|---------:|--------:|--------:|----------:|
| GetDescendants         |  3.532 ms | 0.0678 ms | 0.0635 ms |  74.2188 | 31.2500 | 31.2500 |   2.57 MB |
| OriginalGetDescendants | 16.972 ms | 0.3383 ms | 0.4154 ms | 562.5000 | 31.2500 | 31.2500 |  10.47 MB |

The improved GetAllDescendentFiles is a stripped down version of some code I wrote for Advanced Installer backend; figured I'd backport it.

Note: The benchmark code is copied 1:1 verbatim from the App repo, no review is needed there.

@Sewer56 Sewer56 requested a review from a team September 19, 2023 13:23
@Sewer56 Sewer56 self-assigned this Sep 19, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage is 50.00% of modified lines.

Files Changed Coverage
src/NexusMods.Paths/FileTree/FileTreeNode.cs 50.00%

📢 Thoughts on this report? Let us know!.

@halgari
Copy link
Contributor

halgari commented Sep 19, 2023

This change makes the code non-lazy, and removes the ability to early terminate. This means on large file systes we're allocating a list of 300,000 items instead of streaming them into the processing function one at a time

@Al12rs
Copy link
Contributor

Al12rs commented Sep 25, 2023

The branch needs updating after main changes, apart from that should Seweryn add a lazy version back in or can this be merged?

@Sewer56
Copy link
Member Author

Sewer56 commented Sep 25, 2023

We discussed about this last week during standup. This should be fine for merging with the API change as-is. If we ever run into a need to add a lazy method, we'll just restore it (but faster).

@halgari halgari merged commit 7b21f5a into main Oct 12, 2023
5 checks passed
@halgari halgari deleted the restore-benchmark-functionality branch October 12, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants