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.
🤖 *This PR was created by [Repo Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23774627661), an automated AI assistant.* ## Summary Release notes for version **8.4.0**, covering all changes merged since 8.3.0 (February 26, 2026). ## Changes included in 8.4.0 **Bug fixes:** - `GetEnumUnderlyingType()` now correctly handles non-Int32 enum underlying types (#470) - `decodeILCustomAttribData` reads correct byte-width for non-Int32 enum fixed args (ECMA-335) (#475) - Generative delegate type support fixed; `GetInterface` implemented on `ProvidedTypeDefinition` and `TargetTypeDefinition` (#479) - Thread-safety races in `TargetTypeDefinition` member-wrapper caches fixed (#482) **Performance:** - Member wrapper objects cached in `TargetTypeDefinition` to avoid repeated allocations (#471) - `FullName`, `BaseType` and `GetInterfaces()` cached in `TargetTypeDefinition` (#485) **Refactoring:** - `mkCacheInt32`/`mkCacheGeneric` simplified to use `ConcurrentDictionary` (#486) **CI:** - GitHub Actions updated from v1 to v4 (#476) ## Test Status ✅ All 126 tests pass on the current master (`dotnet test tests/FSharp.TypeProviders.SDK.Tests.fsproj -c Release`) *Please bump the NuGet package version to 8.4.0 in the project file before merging if that step isn't automated.* > Generated by 🌈 Repo Assist at [{run-started}](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23774627661). [Learn more](https://github.com/githubnext/agentics/blob/main/docs/repo-assist.md). > > To install this [agentic workflow](https://github.com/githubnext/agentics/tree/1f672aef974f4246124860fc532f82fe8a93a57e/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@1f672ae > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto, id: 23774627661, workflow_id: repo-assist, run: https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23774627661 --> <!-- gh-aw-workflow-id: repo-assist --> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.