generated from Nexus-Mods/NexusMods.App.Template
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add Trait Driven Composable Tree System #30
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is to provide an alternative for FindSubPaths which has inherent performance issues with large trees. And NMA uses that for installers.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 77.07% 85.64% +8.57%
==========================================
Files 35 44 +9
Lines 1823 2939 +1116
Branches 311 507 +196
==========================================
+ Hits 1405 2517 +1112
+ Misses 361 357 -4
- Partials 57 65 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
erri120
changed the title
Introducing: State of the Art, Trait Driven Composable Tree System (fixes #26)
Add Trait Driven Composable Tree System (fixes #26)
Nov 15, 2023
erri120
changed the title
Add Trait Driven Composable Tree System (fixes #26)
Add Trait Driven Composable Tree System
Nov 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #26
Details
Read the original issue and/or documentation (Introduction) for details.
The system should be fairly well documented.
Test Coverage
100% line coverage across 1358 code statements, and 158 tests total.
Misc. Notes
Source Generation
This tree system is prime candidate for source generation (as mentioned during end of last week in Retrospective).
It might be possible to dedupe some of the
Array
&ObservableCollection
code (where there are no API differences). That would be done by wrapping the collections in a struct constrained toIEnumerable<T>
and/or other interfaces as needed.Problem is, C# compiler can 'lower' usage of
IEnumerable
(such asforeach
) into more performantfor
loop, when it encounters types like array etc. I don't know if that would be preserved if using this constraint. This is currently unproven.Alternatively, T4 templates may be used, since the API differences between
Array
&ObservableCollection
are not that huge. I actually think that may be a decent approach to this. Might put up an issue for this if we ever need to add more APIs here.Performance
Although no new benchmarks join the fray, the code is guaranteed zero overhead (source:
trust me bro
).All trait methods use zero cost abstractions (or none at all, if necessary) to get the job done. They're as fast as if they were manually written into the class by a human; and faster than anything in our existing
FileTree
.There are some things that might be worth benchmarking, e.g. whether
GetChildrenRecursiveUnsafe
is fasterbreadth first
ordepth first
. For now I haven't had a chance to find out.General Outlook
Fairly happy with what you can achieve using this system.
Although the implementation is a bit funny, with respects to de-virtualization and all, I think the zero cost trait based system emulates Rust quite well.
If you're reviewing this, bear in mind the
ObservableCollection
&Array
stuff is almost entirely duplicated, so if you see one, you can skim/skip the other.