Simplify mkCacheInt32/mkCacheGeneric to use ConcurrentDictionary#486
Conversation
Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com> Agent-Logs-Url: https://github.com/fsprojects/FSharp.TypeProviders.SDK/sessions/2136140c-e701-4fb6-b32e-c8152a78d29c
c7c8212
into
repo-assist/fix-thread-safety-lazy-caches-481-a00dbff9b73cb3e8
There was a problem hiding this comment.
Pull request overview
This PR updates the AssemblyReader caching helpers to reduce contention during metadata decoding by replacing Dictionary + lock with ConcurrentDictionary, enabling concurrent cache access.
Changes:
- Replaced the lock-protected
Dictionarycaches inmkCacheInt32andmkCacheGenericwithConcurrentDictionary. - Implemented cache lookup/insert via
TryGetValue+TryAddto avoidGetOrAddoverload-resolution issues in this context.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cache = ConcurrentDictionary<int32, _>() | ||
| fun f (idx:int32) -> | ||
| lock syncObj (fun () -> | ||
| let mutable res = Unchecked.defaultof<_> | ||
| if cache.TryGetValue(idx, &res) then res | ||
| else | ||
| let v = f idx | ||
| cache.[idx] <- v | ||
| v) | ||
| match cache.TryGetValue idx with | ||
| | true, v -> v | ||
| | false, _ -> | ||
| let v = f idx | ||
| cache.TryAdd(idx, v) |> ignore | ||
| cache.[idx] |
There was a problem hiding this comment.
cache.TryGetValue idx uses the F# out-arg-to-tuple translation, which allocates a tuple on every cache access (including hits). Since this cache is likely hot, consider using the byref overload (let mutable res = Unchecked.defaultof<_>; if cache.TryGetValue(idx, &res) then res else ...) to avoid per-call allocations. Also, after TryAdd you can return v directly when the add succeeds to avoid a second dictionary lookup.
| match cache.TryGetValue idx with | ||
| | true, v -> v | ||
| | false, _ -> | ||
| let v = f idx | ||
| cache.TryAdd(idx, v) |> ignore | ||
| cache.[idx] |
There was a problem hiding this comment.
After computing v on a miss, the code does TryAdd and then unconditionally reads cache.[idx], which forces another lookup. Consider returning v when TryAdd succeeds (and only falling back to reading from the dictionary when it fails due to a concurrent add). This keeps the concurrent behavior but avoids extra work on every miss.
mkCacheInt32andmkCacheGenericwere usingDictionary+lock syncObj, serializing all cache access including expensive metadata decode work.Changes
mkCacheInt32/mkCacheGeneric: replacedDictionary+lockwithConcurrentDictionaryusingTryGetValue/TryAdd. No locks, nosyncObj.GetOrAdd(key, factory)was not usable here — F# type inference cannot disambiguate it fromGetOrAdd(key, value)when the value type is an unconstrained generic.TryAddhas no overloads and sidesteps the issue. On a concurrent cache miss,f idxmay be computed twice (bothGetOrAddwith factory and this pattern share that behaviour per the .NET docs), which is acceptable since these factories are pure metadata reads.⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.