computablee / DotMP

A collection of powerful abstractions for parallel programming in .NET with an OpenMP-like API.
https://computablee.github.io/DotMP/
GNU Lesser General Public License v2.1
29 stars 7 forks source link

Full rewrite of core scheduler, support custom schedulers #101

Closed computablee closed 1 year ago

computablee commented 1 year ago

Which issue are you addressing?

Closes #99

How have you addressed the issue?

There has been a full rewrite of the core worksharing parallel-for scheduling code. The entire codebase was stripped down and rethought. A new IScheduler interface has been added, which, when implemented, creates scheduler classes. Instantiations of those classes can be passed to all of the worksharing parallel-for methods, which will then use the custom scheduler.

The builtin schedulers of static, dynamic, and guided have been implemented using this API (albeit, by deriving from the Schedule abstract class to facilitate source compatibility). Lots of now unused fields have been removed, and the code is substantially more modular as a result. There was a ton of decoupling! Better OOP practice has been incorporated, too-- there is now a much greater reliance on polymorphism rather than switch/case. This is excellent for scalability, modularity, and maintainability.

Some further organization would probably fare well. Several classes are just thrown into a file without much thought. I would appreciate feedback on how to organize the new codebase.

How have you tested your patch?

All unit tests pass, without needing any changes. All examples build, also without needing any changes. This proves that, to the best of my testing, source compatibility is maintained. I have added another unit test which uses a custom scheduler which schedules a loop in serial on thread 0. This test passes. I've also made sure to provide error handling where necessary, and update certain unit tests to make sure that the error handling works.

Code coverage is not perfect, I am waiting on the codecov bot to let me know where I have untested code, and I will work on that before merging the PR.

Finally, performance. I have not done extensive testing, but a basic test using the HeatTransfer benchmark proves that, at least for dynamic scheduling, the new code is actually faster than the old code!

codecov[bot] commented 1 year ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (1f90708) 99.54% compared to head (0abe017) 99.01%. Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #101 +/- ## ========================================== - Coverage 99.54% 99.01% -0.53% ========================================== Files 12 12 Lines 1096 1120 +24 Branches 108 110 +2 ========================================== + Hits 1091 1109 +18 - Misses 0 5 +5 - Partials 5 6 +1 ``` | [Files](https://app.codecov.io/gh/computablee/DotMP/pull/101?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Phillip+Allen+Lane) | Coverage Δ | | |---|---|---| | [DotMP/Init.cs](https://app.codecov.io/gh/computablee/DotMP/pull/101?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Phillip+Allen+Lane#diff-RG90TVAvSW5pdC5jcw==) | `98.83% <100.00%> (-1.17%)` | :arrow_down: | | [DotMP/Parallel.cs](https://app.codecov.io/gh/computablee/DotMP/pull/101?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Phillip+Allen+Lane#diff-RG90TVAvUGFyYWxsZWwuY3M=) | `98.77% <88.88%> (-0.20%)` | :arrow_down: | | [DotMP/Iter.cs](https://app.codecov.io/gh/computablee/DotMP/pull/101?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Phillip+Allen+Lane#diff-RG90TVAvSXRlci5jcw==) | `94.59% <95.31%> (-5.41%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

computablee commented 1 year ago

Codecov issues are largely irrelevant. Everything that should be tested has been testing. Merging...