Skip to content

[Repo Assist] perf/refactor: convert ILFieldDefs/ILEventDefs/ILPropertyDefs to concrete classes with lazy name-lookup caches - #502

Merged
dsyme merged 2 commits into
masterfrom
repo-assist/improve-il-defs-caches-20260413-74713908d4881b11
Apr 13, 2026
Merged

[Repo Assist] perf/refactor: convert ILFieldDefs/ILEventDefs/ILPropertyDefs to concrete classes with lazy name-lookup caches#502
dsyme merged 2 commits into
masterfrom
repo-assist/improve-il-defs-caches-20260413-74713908d4881b11

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

This PR converts ILFieldDefs, ILEventDefs, and ILPropertyDefs from abstract interfaces to concrete classes, mirroring the existing ILMethodDefs pattern. It adds lazy O(1) name-lookup caches throughout and makes entries truly lazy (computed at most once per type).

Background

Previously these three types were abstract interfaces:

// Before — computed on every .Entries access; linear O(n) name lookups everywhere
type ILFieldDefs =
    abstract Entries: ILFieldDef[]

Every name-based lookup (GetField, GetPropertyImpl, GetEvent, GetEnumUnderlyingType) called Array.tryFind — an O(n) linear scan. Additionally, seekReadEvents and seekReadProperties recomputed the entries array on every .Entries access (the binary reader range scan ran again each time).

Changes

Concrete classes with lazy dict caches — mirroring ILMethodDefs:

type ILFieldDefs(larr: Lazy<ILFieldDef[]>) =
    let lmap = lazy (
        let d = Dictionary<string, ILFieldDef>()
        for f in larr.Force() do d.[f.Name] <- f
        d)
    member __.Entries = larr.Force()
    member __.TryFindByName nm =
        let scc, v = lmap.Value.TryGetValue(nm) in if scc then Some v else None

Similarly for ILEventDefs and ILPropertyDefs.

Entries are now truly lazyseekReadEvents/seekReadProperties previously re-ran the range scan on every .Entries call. Now the lazy constructor caches after first access.

O(1) lookups in TypeSymbol and TargetTypeDefinition:

  • TypeSymbol.GetField/GetPropertyImpl/GetEvent (generic instantiations): now use TryFindByName instead of Array.tryFind
  • TargetTypeDefinition.GetField/GetPropertyImpl/GetEvent: new lazy fieldDefsMap/propDefsMap/eventDefsMap dictionaries over the wrapped reflection objects
  • TargetTypeDefinition.GetEnumUnderlyingType: uses TryFindByName for the "value__" field

All creation sites updated: empty constants, seekReadFields/seekReadEvents/seekReadProperties, and ProvidedTypeBuilder.

Trade-offs

  • Slightly higher memory use per loaded ILTypeDef (dictionary overhead), but only allocated on first name lookup
  • No API surface changes — Entries still works identically; TryFindByName is a new additive method

Relation to issue #500

This PR implements the code improvements from the blocked issue #500 (which was blocked by the inclusion of .github/dependabot.yml). This PR contains only the code changes, without the Dependabot config.

Test Status

126/126 tests pass (dotnet test tests/FSharp.TypeProviders.SDK.Tests.fsproj -c Release --framework net8.0)

Build: dotnet build src/FSharp.TypeProviders.SDK.fsproj -c Release — succeeded, 0 warnings, 0 errors.


Generated by 🌈 Repo Assist, see workflow run. Learn more.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@97143ac59cb3a13ef2a77581f929f06719c7402a

…rete classes with lazy name-lookup caches

- Replace abstract interface ILFieldDefs/ILEventDefs/ILPropertyDefs with concrete
  classes mirroring the existing ILMethodDefs pattern
- Each class holds a Lazy<T[]> for entries and a lazy Dictionary<string,T> for O(1)
  name lookups via TryFindByName
- seekReadFields/seekReadEvents/seekReadProperties now pass lazy computations to
  the constructors, so entries are computed at most once (previously every .Entries
  access on events/properties re-ran the entire range scan)
- TargetTypeDefinition.GetField/GetPropertyImpl/GetEvent now O(1) via lazy
  fieldDefsMap/propDefsMap/eventDefsMap dictionaries over the wrapped reflection
  objects
- TypeSymbol.GetField/GetPropertyImpl/GetEvent for TargetGeneric now use
  TryFindByName instead of Array.tryFind
- TargetTypeDefinition.GetEnumUnderlyingType uses TryFindByName instead of
  Array.tryFind for the 'value__' field lookup
- All creation sites updated (empty constants, binary reader, ProvidedTypeBuilder)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dsyme
dsyme marked this pull request as ready for review April 13, 2026 12:03
@dsyme
dsyme merged commit 75ac611 into master Apr 13, 2026
2 checks passed
github-actions Bot added a commit that referenced this pull request Apr 15, 2026
- Add GenerativePropertiesTests.fs: 5 new tests covering instance read-only,
  read-write, static, multi-property name-lookup (exercises ILPropertyDefs
  lazy FindByName dictionary from #502), and property count on a generative type
- 136/136 tests pass
- Prepare RELEASE_NOTES.md for 8.6.0

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dsyme pushed a commit that referenced this pull request Apr 17, 2026
….6.0 (#504)

🤖 *This is an automated PR from Repo Assist.*

## Summary

**Task 9 – Testing Improvements**: Add `GenerativePropertiesTests.fs`
with 5 new property tests for generative types.

**Task 10 – Take the Repository Forward**: Update `RELEASE_NOTES.md` to
prepare release 8.6.0.

---

## Changes

### New: `tests/GenerativePropertiesTests.fs`

Five tests covering properties in generative type providers:

1. **Instance read-only property** – presence, type,
`CanRead`/`CanWrite`, non-static getter
2. **Instance read-write property** – getter and setter present, correct
method names
3. **Static read-only property** – presence, static getter
4. **Name-lookup for all properties** – calls `GetProperty` by name for
each of the 4 properties on the generated type; directly exercises the
`ILPropertyDefs` lazy `FindByName` dictionary added in #502
5. **Property count** – verifies the correct number of properties is
emitted

### Updated: `RELEASE_NOTES.md`

Added 8.6.0 entry documenting:
- Bug fix: `ProvidedTypeDefinition.Logger` creating a new delegate
reference on each call (#501)
- Refactor/Performance: `ILFieldDefs`/`ILEventDefs`/`ILPropertyDefs`
concrete classes with lazy O(1) name-lookup caches (#502)
- These new tests

---

## Test Status

✅ **136/136 tests pass** (baseline was 131; 5 new tests added, all
green)

```
Passed!  - Failed: 0, Passed: 136, Skipped: 0, Total: 136
```




> Generated by 🌈 Repo Assist, see [workflow
run](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/24430249769).
[Learn
more](https://github.com/githubnext/agentics/blob/main/docs/repo-assist.md).
>
> To install this [agentic
workflow](https://github.com/githubnext/agentics/blob/97143ac59cb3a13ef2a77581f929f06719c7402a/workflows/repo-assist.md),
run
> ```
> gh aw add
githubnext/agentics@97143ac
> ```

<!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto,
id: 24430249769, workflow_id: repo-assist, run:
https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/24430249769
-->

<!-- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant