[Repo Assist] Perf: cache member wrapper objects in TargetTypeDefinition - #471
Merged
dsyme merged 2 commits intoMar 16, 2026
Conversation
GetConstructors/GetMethods/GetFields/GetEvents/GetProperties/GetNestedTypes on TargetTypeDefinition previously recreated all wrapper objects (anonymous ConstructorInfo/MethodInfo/FieldInfo/PropertyInfo/EventInfo/Type instances) on every invocation. When the F# compiler queries the same target type multiple times during compilation the wrappers were silently discarded and rebuilt each call. This change introduces six lazy caches (ctorDefs, methDefs, fieldDefs, eventDefs, propDefs, nestedDefs) so wrapper objects are created once per TargetTypeDefinition instance. The BindingFlags filter is still applied on every call (cheap flag-testing) while the expensive array traversal and object allocation is amortised. Thread safety is provided by the default Lazy<T> publication mode. For assemblies with many members (Framework types used as reference contexts by generative TPs) this avoids O(n) re-allocation on every member query, improving compilation performance. All 110 existing tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dsyme
approved these changes
Mar 16, 2026
github-actions Bot
added a commit
that referenced
this pull request
Mar 20, 2026
Closes #481 Root cause: PR #471 added lazy caches (ctorDefs/methDefs/fieldDefs/etc.) in TargetTypeDefinition so wrapper objects are allocated once and shared across all GetConstructors/GetMethods/etc. calls. When the F# compiler invokes these from multiple threads concurrently, the lazies can be forced concurrently, and the underlying non-thread-safe caches race: * ILMethodDefs.getmap() / ILTypeDefs.getmap() / ILExportedTypesAndForwarders.getmap() used a mutable null-check pattern without synchronisation. One thread sets lmap to a new Dictionary and starts filling it; a second thread sees the non-null lmap and reads it while the first is still writing -> InvalidOperationException. * mkCacheInt32 / mkCacheGeneric (binary-reader caches) had the same unsynchronised ref-null pattern. * TxTable<T>.Get wrote to Dictionary<int,T> without a lock; concurrent type-resolution calls (txILTypeRef -> txTable.Get) from shared cached MethodInfo/ConstructorInfo objects could collide. Fixes: * ILMethodDefs / ILTypeDefs / ILExportedTypesAndForwarders: build lmap inside lock syncObj so the dictionary is fully populated before any reader can see it. Subsequent calls acquire the lock, check the already-set field and return immediately (single branch). * mkCacheInt32 / mkCacheGeneric: each cache now holds its own lock object and protects every TryGetValue/set_Item pair. * TxTable<T>: backed by ConcurrentDictionary<int, Lazy<T>> so that concurrent GetOrAdd calls for the same token race safely, with Lazy<T> guaranteeing the factory runs exactly once per token. Adds a thread-safety regression test: 8 threads × 50 iterations each calling GetConstructors/GetMethods/GetFields/GetProperties/GetEvents/ GetNestedTypes on the same TargetTypeDefinition simultaneously. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Mar 20, 2026
github-actions Bot
added a commit
that referenced
this pull request
Mar 20, 2026
Closes #481 Root cause: PR #471 added lazy caches (ctorDefs/methDefs/fieldDefs/etc.) in TargetTypeDefinition so wrapper objects are allocated once and shared across all GetConstructors/GetMethods/etc. calls. When the F# compiler invokes these from multiple threads concurrently, the lazies can be forced concurrently, and the underlying non-thread-safe caches race: * ILMethodDefs.getmap() / ILTypeDefs.getmap() / ILExportedTypesAndForwarders.getmap() used a mutable null-check pattern without synchronisation. One thread sets lmap to a new Dictionary and starts filling it; a second thread sees the non-null lmap and reads it while the first is still writing -> InvalidOperationException. * mkCacheInt32 / mkCacheGeneric (binary-reader caches) had the same unsynchronised ref-null pattern. * TxTable<T>.Get wrote to Dictionary<int,T> without a lock; concurrent type-resolution calls (txILTypeRef -> txTable.Get) from shared cached MethodInfo/ConstructorInfo objects could collide. Fixes: * ILMethodDefs / ILTypeDefs / ILExportedTypesAndForwarders: build lmap inside lock syncObj so the dictionary is fully populated before any reader can see it. Subsequent calls acquire the lock, check the already-set field and return immediately (single branch). * mkCacheInt32 / mkCacheGeneric: each cache now holds its own lock object and protects every TryGetValue/set_Item pair. * TxTable<T>: backed by ConcurrentDictionary<int, Lazy<T>> so that concurrent GetOrAdd calls for the same token race safely, with Lazy<T> guaranteeing the factory runs exactly once per token. Adds a thread-safety regression test: 8 threads × 50 iterations each calling GetConstructors/GetMethods/GetFields/GetProperties/GetEvents/ GetNestedTypes on the same TargetTypeDefinition simultaneously. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Mar 20, 2026
sergey-tihon
added a commit
that referenced
this pull request
Mar 22, 2026
…481) (#482) 🤖 *This is an automated pull request from [Repo Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23358988317).* Fixes #481 — `InvalidOperationException` / `NullReferenceException` when the F# compiler accesses type-provider types from multiple parallel compilation threads. ## Root cause PR #471 introduced lazy caches (`ctorDefs`/`methDefs`/`fieldDefs`/`eventDefs`/`propDefs`/`nestedDefs`) in `TargetTypeDefinition` so member-wrapper objects are allocated once and reused. When the compiler invokes `GetConstructors`/`GetMethods`/etc. from multiple threads on the same type, several underlying caches that were never designed for concurrent access are hit simultaneously: | Site | Problem | |---|---| | `ILMethodDefs.getmap()` / `ILTypeDefs.getmap()` / `ILExportedTypesAndForwarders.getmap()` | `mutable lmap = null` checked without a lock. Thread A sets `lmap` to a new `Dictionary` and starts filling it; Thread B sees the non-null value and reads from it while Thread A is writing → `InvalidOperationException`. | | `mkCacheInt32` / `mkCacheGeneric` (binary-reader row caches) | Same unsynchronised `ref null` / `Dictionary` pattern across all 8 per-reader caches. | | `TxTable(T).Get` | `Dictionary(int,T)` written without any lock; concurrent type-resolution calls (via cached `MethodInfo`/`ConstructorInfo` → `txILTypeRef` → `txTable.Get`) from two threads can collide. | ## Fix * **`ILMethodDefs` / `ILTypeDefs` / `ILExportedTypesAndForwarders`** – add a `syncObj` per instance; build `lmap` inside `lock syncObj` so the dictionary is fully populated before any reader sees it. Subsequent calls acquire the lock, see the already-set field, and return immediately. * **`mkCacheInt32` / `mkCacheGeneric`** – each closure now owns a `syncObj` and wraps every `TryGetValue` + write pair in `lock`. * **`TxTable(T)`** – backed by `ConcurrentDictionary(int, Lazy<T)>`. `GetOrAdd` races safely; the `Lazy(T)` wrapper (using F#'s default `ExecutionAndPublication` mode) ensures the factory is called **at most once** per token, preserving the identity-equality guarantee that `TxTable` provides. ## Test status New regression test added: **`TargetTypeDefinition member-wrapper caches are thread-safe under parallel access`** — 8 threads × 50 iterations, each calling all six `GetXxx` methods on the same `TargetTypeDefinition` concurrently. ``` Passed! - Failed: 0, Passed: 118, Skipped: 0, Total: 118 (net8.0) ``` All pre-existing tests continue to pass. > Generated by [Repo Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23358988317) for issue #481 · [◷](https://github.com/search?q=repo%3Afsprojects%2FFSharp.TypeProviders.SDK+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests) > > To install this [agentic workflow](https://github.com/githubnext/agentics/tree/d1d884596e62351dd652ae78465885dd32f0dd7d/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@d1d8845 > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, id: 23358988317, workflow_id: repo-assist, run: https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23358988317 --> <!-- gh-aw-workflow-id: repo-assist --> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com> Co-authored-by: Sergey Tihon <sergey.tihon@gmail.com>
dsyme
pushed a commit
that referenced
this pull request
Mar 23, 2026
…getTypeDefinition (#485) 🤖 *This is an automated PR from Repo Assist, an AI assistant for this repository.* ## Summary `TargetTypeDefinition.FullName`, `BaseType`, and `GetInterfaces()` each compute their result from immutable input data (`inp.Namespace`/`inp.Name`, `inp.Extends`, `inp.Implements`) but were recomputed on every call — allocating new strings/arrays and re-running type resolution each time. For large type providers with many types (e.g. SwaggerProvider), where the F# compiler queries these properties many times per type during type-checking, this saves repeated allocations and type-resolution work. ### Changes | Property | Before | After | |---|---|---| | `FullName` | String concatenation every call | Cached `lazy` — computed once, same `string` returned thereafter | | `BaseType` | `txILType` (type-resolution) every call | Cached `lazy` — resolved once | | `GetInterfaces()` | `Array.map txILType` (allocates new `Type[]`) every call | Cached `lazy` — resolved and allocated once | All three caches use F# `lazy` which defaults to `LazyThreadSafetyMode.ExecutionAndPublication`, so concurrent first-access from multiple F# compiler threads is safe. This is complementary to PR #471 (which cached member-wrapper arrays) and does not touch the thread-safety areas being addressed by PRs #482/#483. ## Test Status ``` Passed! - Failed: 0, Passed: 117, Skipped: 0, Total: 117 (net8.0) ``` All 117 pre-existing tests pass. The `netstandard2.0` build target ran OOM on the CI machine (infrastructure issue, not caused by this change — the same issue affects master); the `net8.0` build and tests both pass cleanly. > Generated by [Repo Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23367946628) · [◷](https://github.com/search?q=repo%3Afsprojects%2FFSharp.TypeProviders.SDK+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests) > > To install this [agentic workflow](https://github.com/githubnext/agentics/tree/d1d884596e62351dd652ae78465885dd32f0dd7d/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@d1d8845 > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, id: 23367946628, workflow_id: repo-assist, run: https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23367946628 --> <!-- gh-aw-workflow-id: repo-assist --> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dsyme
pushed a commit
that referenced
this pull request
Apr 1, 2026
🤖 *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>
dsyme
added a commit
that referenced
this pull request
Jul 29, 2026
…er body (issue #341) (#520) ## Problem Issue #341 reported that generative compilation (`AssemblyCompiler.Compile`) is still very slow for large providers (SwaggerProvider + Stripe schema), even after the memoization work in #443/#457/#471/#493/#497/#502/#509/#513. Profiling the benchmark suite with dotnet-trace showed that with those caches in place, **~80% of `Compile` time was FSharp.Core quotation deserialization** (`PatternsModule.deserialize` → `instMeth` → `GetGenericArguments`/`MakeGenericType`), attributed to the `CodeGenerator` constructor. Root cause: `CodeGenerator` defines ~63 active patterns from quotation literals — `let (|Addition|_|) = (|SpecificCall|_|) <@ (+) @>`, `<@ max @>`, `<@ sqrt @>`, etc. Evaluating a quotation literal deserializes it (reflection-binding every method it mentions), and a `CodeGenerator` is constructed **once per emitted member body**. A provider with 10,000 members therefore deserialized those quotations ~630,000 times. `lessThan` was worse still: it deserialized `<@@ (<) @@>` on every call. ## Fix * New `CodeGenPatterns(convTypeToTgt)` type holding all these patterns as `member val` properties, so they are computed once at construction (a plain property getter would re-evaluate per access). * `AssemblyCompiler.Compile` constructs one instance and passes it to every `CodeGenerator`; the constructor now just re-binds the local active patterns from it, so the large `emitExpr` match code is untouched. * `lessThan` uses a precomputed generic method definition. ## Results Quick benchmark (`benchmarks -- --quick`, ms/iter, before → after): | Scenario | Before | After | Speedup | |---|---|---|---| | Methods 50×20 | 880 | 86 | 10× | | Methods 200×20 | 2893 | 180 | 16× | | Methods 1000×10 | 6892 | 535 | 13× | | Props 500×10 | 8074 | 1259 | 6.4× | | ComplexBody 300×10 | 2528 | 158 | 16× | | Nested 100×4×5 | 1668 | 127 | 13× | End-to-end with the exact repro from #341 (SwaggerProvider built from source against this branch, Stripe `spec3.sdk.json`, `dotnet fsi`): **62–72 s → 19–20 s** wall clock; the remainder is dominated by schema download, OpenApi parsing and fsi startup rather than the TPSDK. Re-profiling after the change confirms the SDK-side deserialization is gone; the remaining `deserialize` samples come from the provider author's own `<@@ ... @@>` literals in `invokeCode` (once per member, inherent to user code). ## Testing * Full test suite passes: 160/160. * Benchmark suite builds and runs (numbers above). * SwaggerProvider builds unmodified against the patched `ProvidedTypes.fs` and the generated Stripe client works (`Stripe.Client()` constructed successfully). Fixes #341 (the remaining slowness reported there after the earlier rounds of caching). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Don Syme <dsyme@github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 This PR was created by Repo Assist, an automated AI assistant.
Problem
TargetTypeDefinition.GetConstructors/GetMethods/GetFields/GetEvents/GetProperties/GetNestedTypesrecreated all wrapper objects (anonymousConstructorInfo,MethodInfo,FieldInfo,PropertyInfo,EventInfo, nestedTypeinstances) on every call:When the F# compiler queries the same target type more than once during a compilation session (which it does for types used as base classes, interface types, generic constraints, etc.), the wrappers were silently discarded and rebuilt each call — O(n) allocation with no benefit.
Fix
Introduce six
lazycaches (ctorDefs,methDefs,fieldDefs,eventDefs,propDefs,nestedDefs) so wrapper construction happens once perTargetTypeDefinitioninstance. TheBindingFlagsfilter is still applied on every call (cheap) while the expensive traversal and object allocation is amortised:All 110 existing tests pass unchanged.