Improve module loading performance using List<T> instead of ConcurrentBag<T> #17477
Conversation
|
Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request. If you like Orchard Core, please star our repo and join our community channels. |
|
|
||
| Parallel.ForEach(names, new ParallelOptions { MaxDegreeOfParallelism = 8 }, (name) => | ||
| { | ||
| modules.Add(new Module(name, false)); | ||
| var module = new Module(name, false); | ||
| lock (lockObj) // Ensures thread-safe addition |
There was a problem hiding this comment.
There is nothing to optimize in the class by changing the structure. This parallel loop is completely useless, it should be a straight loop or LINQ query.
If there was something to be done in parallel it would be the calls to GetModuleNames(), but even that is useless because of how it's currently implemented in the class, the costly logic being in the constructor.
So one thing to try is to move the logic of loading assemblies into GetModuleNames() and then parallelizing these calls, not the code adding to the list.
Finally add some log at the beginning and end of this method, with and without the code changes to show how much is gained. No need to parallelize if there are no gains.
|
Thanks. |
This PR improves the performance of module loading in Orchard CMS by replacing ConcurrentBag with a List and a lock. This change was made because:
ConcurrentBag is inefficient for iteration and removals.
List with a lock provides better performance for one-time additions.
Benchmark results show a significant improvement in execution time.
Changes Include:
Replaced ConcurrentBag with List and a lock.
Updated GetModules() method to use Parallel.ForEach() safely.