Add support for adding projects concurrently in DependencyGraphSpec using builtIn concurrent type - #3593
Add support for adding projects concurrently in DependencyGraphSpec using builtIn concurrent type#3593erdembayar wants to merge 7 commits into
Conversation
…multithread safety. There are no native framework concurrent SortedSet nor concurrent SortedDictionary, I just decided use ConcurrentDictionary for both of them. Then we sort it before consume it. Also using ConcurrentDictionary in place of SortedSet may be extra space overhead since we only care about dictionary key not values. Please note ConcurrentBag not suitable for this because it doesn't check for duplicate item and checking it costs O(n) time complexity.\ Overall I believe replacing SortedSet by ConcurrentDictionary degrade performance, because it's pure C# code usually doesn't tax heavy for small/medium size sets. Here insert time O(logN) for SortedSet but much cheaper O(1) for Dictionary. Iterate through values then O(n) for SortedSet but expensive O(n* logN)for Dictionary before consumption.
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
There's a TODO for this issue in the code:
Can you please resolve that?
Ready for approval after that I think.
zivkan
left a comment
zivkan
left a comment
There was a problem hiding this comment.
This PR is less effort than my preferred option, but I'd prefer a more "functional" (less side-effects) approach. In my opinion, DependencyGraphSpec should be immutable, and the parallel foreach should not mutate the object for each package, but should do create the PackageSpec in parallel, then use the orchestrating task/thread to accumulate the results in a lock-free way. Very roughly:
var tasks = new List<Task<PackageSpec>>();
var packageSpecs = List<PackageSpec>();
IEnumerable<Project> enumerator = projects.GetEnumerable();
for (int i = 0; i < Environment.ProcessorCount; i++)
{
if (!enumerator.MoveNext())
{
break;
}
tasks.Add(Task.Run(() => GetProjectSpec(enumerator.Current)));
}
while (tasks.Count > 0)
{
var finished = await Task.WhenAny(tasks);
tasks.Remove(finished);
packageSpecs.Add(finished.Result);
if (enumerator.Current != null && enumerator.MoveNext())
{
tasks.Add(() => GetPackageSpec(enumerator.Current));
}
}
var dgSpec = new DependencyGraphSpec(packageSpecs);Even better would be to use TPL dataflow, rather than building our own concurrency limiter. In one of Martin's PRs I already provided sample code on how the same thing is achieveable with TPL dataflow. I remember that NuGet.Build.Tasks.Console already uses TPL dataflow, so it should be very easy to use there. Making the DGSpec immutable would be much less easy.
…we made .AddProject support concurrency.
Ok. Resolved it. |
zivkan
left a comment
zivkan
left a comment
There was a problem hiding this comment.
Trying to reason about multi-threaded correctness is difficult. It seems to me that _restore and _projects are never both accessed in the same method, so it seems ok to me. And at the moment is looks like only NuGet.Build.Tasks.Console was trying to use it in parallel, and only to add.
Having said that, I've read enumerating ConcurrentDictionary is expensive, so if this class' Restore and Projects properties are accessed more often than the methods that add things to the dictionary, this PR might reduce performance, not improve it.
| } | ||
|
|
||
| _projects.Add(projectUniqueName, projectToRestore); | ||
| _projects.TryAdd(projectUniqueName, projectToRestore); |
There was a problem hiding this comment.
The semantics here are probably changing. I'm not sure about SortedDictionary<TKey, TValue>, but Dictionary<TKey, TValue> throws when you try to call .Add on a key that already exists. The new code will silently ignore the new value.
| PackageSpec packageSpec = JsonPackageSpecReader.GetPackageSpec(jsonReader, path); | ||
|
|
||
| dgspec._projects.Add(projectsPropertyName, packageSpec); | ||
| dgspec._projects.TryAdd(projectsPropertyName, packageSpec); |
There was a problem hiding this comment.
The semantics here are probably changing. I'm not sure about SortedDictionary<TKey, TValue>, but Dictionary<TKey, TValue> throws when you try to call .Add on a key that already exists. The new code will silently ignore the new value.
There was a problem hiding this comment.
Changed the syntax.
There was a problem hiding this comment.
I added wrapper function check duplicate key value before add value.
This PR is less effort than my preferred option, but I'd prefer a more "functional" (less side-effects) approach. In my opinion, DependencyGraphSpec should be immutable, and the parallel foreach should not mutate the object for each package, but should do create the PackageSpec in parallel, then use the orchestrating task/thread to accumulate the results in a lock-free way. Very roughly:
var tasks = new List<Task<PackageSpec>>(); var packageSpecs = List<PackageSpec>(); IEnumerable<Project> enumerator = projects.GetEnumerable(); for (int i = 0; i < Environment.ProcessorCount; i++) { if (!enumerator.MoveNext()) { break; } tasks.Add(Task.Run(() => GetProjectSpec(enumerator.Current))); } while (tasks.Count > 0) { var finished = await Task.WhenAny(tasks); tasks.Remove(finished); packageSpecs.Add(finished.Result); if (enumerator.Current != null && enumerator.MoveNext()) { tasks.Add(() => GetPackageSpec(enumerator.Current)); } } var dgSpec = new DependencyGraphSpec(packageSpecs);Even better would be to use TPL dataflow, rather than building our own concurrency limiter. In one of Martin's PRs I already provided sample code on how the same thing is achieveable with TPL dataflow. I remember that NuGet.Build.Tasks.Console already uses TPL dataflow, so it should be very easy to use there. Making the DGSpec immutable would be much less easy.
I'll take into my other PR.
| var spec = JsonPackageSpecReader.GetPackageSpec(specJson); | ||
| #pragma warning restore CS0618 | ||
| _projects.Add(prop.Name, spec); | ||
| _projects.TryAdd(prop.Name, spec); |
There was a problem hiding this comment.
The semantics here are probably changing. I'm not sure about SortedDictionary<TKey, TValue>, but Dictionary<TKey, TValue> throws when you try to call .Add on a key that already exists. The new code will silently ignore the new value.
There was a problem hiding this comment.
Changed the syntax.
There was a problem hiding this comment.
using dict[key] = value does not reproduce the semantics of Dictionary<T1, T2>()'s .Add method which throws an exception when the key already exists.
I have no idea if any of our code depends on that behaviour, throwing if the key already exists, so maybe it's not a problem. But the changed syntax doesn't maintain semantics.
There was a problem hiding this comment.
Ok. I added wrapper function ConcurrentDictAddCheckDuplicateKey which check for duplicate key before add value and throw exception if it's duplicate key to keep existing behaviour. Please check.
76c9809 to
a6d2898
Compare
This reverts commit a6d2898.
I don't see that approach as much more effort. The only mutation that we do right now is for CPVM. Do you have any suggestion where to move this mutation? I'm not a fan, but I don't have a suggestion that's not mutating the package spec before adding it to the DGSpec.
I'm not sure how expensive it'd have to be for this to be true. |
You know the code better than I do, so I want to believe you. But
Well, the theoretical way to do it is for the PackageSpec to also be immutable. When CPVM needs to "modify" a PackageSpec, it creates a new DGSpec with the "to-be-modified" PackageSpec removed, and in its place a new PackageSpec with the required changes. I don't know if it will be as easy to implement as it was for me to write that though. We should look into how Roslyn implemented their immutable data types, and see if there's an easy way to create a new instance of a class which is a clone of another instance with just 1 property modified. |
|
I assume the "CPVM mutates" thing you're referring to is NuGet.Client/src/NuGet.Core/NuGet.ProjectModel/DependencyGraphSpec.cs Lines 260 to 275 in d4d80b1 Line 262 already does Is there anywhere else that you believe CPVM mutates a DGSpec or PackageSpec? |
|
I did a real quick benchmark, creating dictionaries with 300 entries to simulate a large solution, and enumerating to only count the number of entries:
Note, that with this PR Isn't this code used to determine no-op, so this can impact no-op performance? Or is it only "full" restore? |
|
Changing my benchmark program a little to compare enumerating SortedSet, SortedDictionary and ConcurrentDictiory (with LINQ sort), I get these results:
so, each time it's about 0.24 ms slower to read. Since DGSpec has two properties |
…re because it was SortedSet. It doesn't throw exception for duplicate value and after converting to ConcurrentDictionary we only care key not value in ConcurrentDictionary. But for _projects we need to check for duplicate key also care for value in it.
|
Without digging, the enumeration will be called at least 3*n number of times where n is the number of projects. (original generation, project specific generation, no-op hash). We start with a solution dg spec and then we move to a project level dg specs and those are enumerated once or twice during an actual no-op restore. |
|
so the current experience is somehow worse than the old one. @erdembayar had a different proposal where he was using a lock object, so the lock object is likely faster than this. Finally, the lockless approach is likely the cleanest if possible. |
For all sets less than 1k (Solution with 1k projects) usually it's just difference of some nano seconds and even cost of sorting is negligible. Even for SortedSet we used to pay insertion O(LogN) fee but now it's O(1). But paying higher fee before consume by sorting O(N*LogN) instead of O(N). Much of it cancel/compensate each other. |
I can just use synchronization lock. @zivkan please check this PR#3594. |
| if (dict.ContainsKey(key)) | ||
| { | ||
| throw new ArgumentException("An element with the same key already exists in the System.Collections.Generic.Dictionary: " + nameof(dict) + " key:" + key); | ||
| } | ||
|
|
||
| dict.TryAdd(key, value); |
There was a problem hiding this comment.
This looks up the key twice, making the add twice as slow, plus introducing a multi-threaded timing bug (think about when two threads try to add the same key at the same time)
| if (dict.ContainsKey(key)) | |
| { | |
| throw new ArgumentException("An element with the same key already exists in the System.Collections.Generic.Dictionary: " + nameof(dict) + " key:" + key); | |
| } | |
| dict.TryAdd(key, value); | |
| if (!dict.TryAdd(key, value)) | |
| { | |
| throw new ArgumentException("An element with the same key already exists in the System.Collections.Generic.Dictionary: " + nameof(dict) + " key:" + key); | |
| } |
There was a problem hiding this comment.
It seems we can't avoid sync lock, to fix this I need to add sync lock. This PR getting bit complicated maybe just other one is fine. PR#3594.
There was a problem hiding this comment.
In this method, you can avoid a lock by using the suggestion I made. The dictionary method is TryAdd, hence it already tells you if it's successful or not. You don't need a lock.
However, I 100% agree that this PR is problematic. It's trading off read performance for concurrent add. I don't think that's a good trade off.
| { | ||
| if (dict.ContainsKey(key)) | ||
| { | ||
| throw new ArgumentException("An element with the same key already exists in the System.Collections.Generic.Dictionary: " + nameof(dict) + " key:" + key); |
There was a problem hiding this comment.
dict is the parameter name of the private method, so putting that name in the exception string doesn't help callers of the public API.
Can you share your snippet? |
I can just use synchronization lock. @zivkan please check this PR#3594. |
public class DirectoryEnumerationBenchmarks
{
private SortedDictionary<string, string> _sortedDict;
private ConcurrentDictionary<string, string> _concurrentDict;
private SortedSet<string> _sortedSet;
public DirectoryEnumerationBenchmarks()
{
_sortedDict = new SortedDictionary<string, string>();
_concurrentDict = new ConcurrentDictionary<string, string>();
_sortedSet = new SortedSet<string>();
for (int i = 300; i > 0; i--)
{
var val = $"Project{i}";
_sortedDict.Add(val, val);
_concurrentDict.TryAdd(val, val);
_sortedSet.Add(val);
}
}
[Benchmark(Baseline = true)]
public int EnumerateSortedDictionary()
{
int count = 0;
foreach (var kvp in _sortedDict)
{
count++;
}
return count;
}
[Benchmark]
public int EnumerateConcurrentDictionary()
{
int count = 0;
foreach (var kvp in _concurrentDict.OrderBy(k => k.Key))
{
count++;
}
return count;
}
[Benchmark]
public int EnumerateSortedSet()
{
int count = 0;
foreach (var key in _sortedSet)
{
count++;
}
return count;
}
} |
Although this PR seemed nicer at first, as we get to better understand how it all works, it seems that the |
4 participants
Bug
Fixes: Nuget/Home#9002
Regression: No
Fix
Details:
The DependencyGraphSpec object uses a standard collection object as a backing collection. This makes it impossible to use it in a parallel loop or multiple threads. It should use a concurrent collection instead.
I have 2 competing idea for this task.
Here is 2nd one which doesn't use synchronization lock. For one which uses synchronization lock please check PR#3594.
There are no native framework concurrent SortedSet nor concurrent SortedDictionary, I just decided use ConcurrentDictionary for both of them. Then we sort it before consume it.
Also using ConcurrentDictionary in place of SortedSet may be extra space overhead since we only care about dictionary key not values. Please note ConcurrentBag not suitable for this because it doesn't check for duplicate item and checking it costs O(n) time complexity.
Overall I don't believe replacing SortedSet by ConcurrentDictionary degrade performance, because it's pure C# code usually doesn't tax heavy for small/medium size sets.
Here insert time O(logN) for SortedSet but much cheaper O(1) for Dictionary.
Iterate through values then O(n) for SortedSet but expensive O(n* logN)for Dictionary before consumption.
Testing/Validation
Tests Added: No
Reason for not adding tests: There are already many unit tests covering this code paths.
Validation: Manual testing.