Switch Project data structures from ImmutableDictionary => Dictionary and lock #78287
Conversation
…ictionary and lock Will add more information if speedometer run provides compelling results
ghost
added
Area-IDE
untriaged
ToddGrun
changed the title
| private ImmutableDictionary<DocumentId, AnalyzerConfigDocument?> _idToAnalyzerConfigDocumentMap = ImmutableDictionary<DocumentId, AnalyzerConfigDocument?>.Empty; | ||
| private readonly object _gate = new(); | ||
|
|
||
| // Only access these dictionaries when holding _gate |
There was a problem hiding this comment.
alternatively, they could be their own locks. but i don't care that much. this would also enable concurrent access to all of them, while each one was threadsafe. up to you.
There was a problem hiding this comment.
oh. nevermind. you use null. ignore this.
There was a problem hiding this comment.
Went ahead and switched the access of the dictionaries to be protected by using themselves as locks. Don't love it, but it does give better lock granularity.
|
|
||
| // Quick check first: if we already have created a SourceGeneratedDocument wrapper, we're good | ||
| if (_idToSourceGeneratedDocumentMap.TryGetValue(documentId, out var sourceGeneratedDocument)) | ||
| if (_idToSourceGeneratedDocumentMap != null && _idToSourceGeneratedDocumentMap.TryGetValue(documentId, out var sourceGeneratedDocument)) |
There was a problem hiding this comment.
accessing out of lock. not safe.
There was a problem hiding this comment.
Bah, this one was the ugly stepchild, and I only looked at the GetOrAdd usages, but yes, the TryGetValue calls need to be protected too. Added methods for both GetOrAdd and TryGetValue that encapsulate the locking.
| _idToAnalyzerConfigDocumentMap ??= []; | ||
| return _idToAnalyzerConfigDocumentMap.GetOrAdd(documentId, s_tryCreateAnalyzerConfigDocumentFunction, this); | ||
| } | ||
| } |
There was a problem hiding this comment.
small nit. consider extracting helper for this pattern. we often have triplicate methods for docs/analyzerdocs/additionaldocs. i lke it when they all call into a single helper.
There was a problem hiding this comment.
Extracted both GetOrAdd and TryGetValue calls into their own methods that can be called for any of the dictionaries.
| // Values for all these are created on demand. | ||
| private ImmutableDictionary<ProjectId, Project> _projectIdToProjectMap; | ||
| // Values for all these are created on demand. Only access when holding _gate. | ||
| private readonly Dictionary<ProjectId, Project> _projectIdToProjectMap = []; |
There was a problem hiding this comment.
since you do initialize this, consider having hte dict be its own lock.
There was a problem hiding this comment.
are you auditing all usages of _projectIdToProjectMap to make sure it is locked?
There was a problem hiding this comment.
Yes, I did check all five dictionaries converted, just didn't notice the TryGetValue calls on _idToSourceGeneratedDocumentMap
…Todd! 2) Create common shared methods for each of the dictionaries to get/add items that handles the lock. 3) Don't love it, but made the dictionaries be their own lock for accessing them.
3 participants
Solution/Project classes keep several immutable dictionaries for mapping (Document/Project)Id to various data. ImmutableDictionaries are a fine (but slow) data structure when data between versions is to be shared, but none of the values in these ImmutableDictionaries persist between solution/project transformations. In fact, the only operation performed on these dictionaries is GetOrAdd calls. As immutable dictionaries are quite poor performing wrt lookups/adds, I thought I would try out two approaches to see if they showed improvements:
I created test insertions for each of these PRs to gather speedometer numbers, particularly during the solution load of the c# editing speedometer test. The measurements below are from GC Heap Allocs and CPU samples for those tests under (Solution/Project).GetDocument calls
*** Baseline ***
Alloc: 120 MB (1.0%)
CPU: 560 ms (0.3%)
*** Concurrent Dictionary ***
Alloc: 59 MB
CPU: 258 ms (0.1%)
*** Dictionary with lock ***
Alloc: 28 MB
CPU: 172 ms (< 0.1%)
So, it looks like the best performing of the bunch is this PR, which uses simple dictionaries and locks, thus why I'm elevating this PR out of draft mode. The ConcurrentDictionary PR is interesting as the code changes are simple and do give a nice perf improvement.