add some basic parallelism to improve performance #799
Conversation
There was a problem hiding this comment.
I won't block the code, but there are a few areas in which we could use the BCL concurrent classes to make things simpler to write.
We are also using Threads for IO bound work, while indeed will make the code async, that is not the proper ideal since we are using CPU threads for stuff that the IO can deal with better.
Doing some extra work and making the IO async, while is A LOT more effort, will probably have a much better payoff in the long run.
Maybe investing on the lower level classes to be truly async might be a good investment for the future.
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| namespace SwiftReflector { | |||
| public class ErrorHandling { | |||
| object messagesLock = new object (); | |||
There was a problem hiding this comment.
I believe you can make your life easier by using https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentbag-1?view=net-8.0
You will need to make a decision, but looking at your code you can move from a List to a Bag since:
- You are not using an indexer.
- Looks like you do not care about order.
- You access the collections via Linq and the ConcurrentBag does provide the required interfaces to continue doing it.
- Provides a nice thread-safe property "IsEmpty" which you can use in some of the properties.
There is a few draw backs, one is that you won't have an AddRange method: dotnet/runtime#61291
The second one I can't think, by definition all LINQ operations are thread-safe because they only read, the diff is that the concurrent bag provides a snapshot of the collection while iterating, that will change the behavior and probably, if that must be kept, is best to use a lock.
You'll have to probably write that the class must be thread-safe for the future team.
There was a problem hiding this comment.
opened an issue for it #801
| if (!values.TryGetValue (className, out contents)) { | ||
| contents = new ClassContents (formalName, sizeofMachinePointer); | ||
| values.Add (className, contents); | ||
| lock (valuesLock) { |
There was a problem hiding this comment.
Same as with the ErrorHandler, we could use https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentdictionary-2?view=net-8.0
Looking at the code on which you are locking, you could remove the need to the TryGetValue, use TryAdd which will add only if not present. If you were to update, TryUpdate would be the method you want.
I don't know the code base, but you might be in a situation in which some Invetories are thread-safe and others are not.
In this case, I cannot think of a drawback looking at your code AND will ensure that all subclasses are threadsafe.
There was a problem hiding this comment.
opened an issue for it. #800
| lock (valuesLock) { | ||
| OverloadInventory overloads = null; | ||
| if (!values.TryGetValue (tlf.Name, out overloads)) { | ||
| overloads = new OverloadInventory (tlf.Name, sizeofMachinePointer); | ||
| values.Add (tlf.Name, overloads); | ||
| } | ||
| overloads.Add (tlf, srcStm); | ||
| } |
There was a problem hiding this comment.
Same, which also means that changing the dictionary in the base type will make all of your subclasses thread-safe.
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| namespace SwiftReflector.Inventory { | |||
| public abstract class Inventory<T> { | |||
| protected object valuesLock = new object (); | |||
| protected Dictionary<SwiftName, T> values = new Dictionary<SwiftName, T> (); | |||
There was a problem hiding this comment.
A ConcurrentDict will make your life easier.
| lock (valuesLock) { | ||
| ModuleContents module = null; | ||
| if (!values.TryGetValue (tld.Module, out module)) { | ||
| module = new ModuleContents (tld.Module, SizeofMachinePointer); | ||
| values.Add (tld.Module, module); | ||
| } | ||
| module.Add (tld, srcStm); |
There was a problem hiding this comment.
Same, all this methods are yy most of the time.
| foreach (var stm in streams) { | ||
| stm.Dispose (); | ||
| } |
There was a problem hiding this comment.
The Reactive Extensions (https://github.com/dotnet/reactive) provides a nice way to deal with a collection of IDisposables: https://learn.microsoft.com/en-us/previous-versions/dotnet/reactive-extensions/hh229918(v=vs.103)
I don't know how you feel about adding it as a dependency "System.Reactive" but will probably help.
First cut on making aspects of BTfS parallelizable.
ErrorHandling and the *Inventory stack needed to be made thread-safe.
I made reflection and demangling async and changed how they're being invoked to run them in parallel.
Across the entire suit of unit tests, this improves performance by about 7%.
I'm fairly certain that I can make the C# code generation parallel as well - but I'll do that in a separate PR.