Skip to content

Commit a70de81

Browse files
CopilotT-Gro
andcommitted
Complete DList migration - replace all QueueList usage with CachedDList
Systematically replaced QueueList with CachedDList across entire codebase: - TypedTree.fs/fsi: Core ModuleOrNamespaceType now uses CachedDList - TypedTreeOps.fs: CombineModuleOrNamespaceTypes uses O(1) append - TypedTreePickle.fs: Added p_cached_dlist/u_cached_dlist functions - CheckDeclarations.fs, NameResolution.fs, NicePrint.fs, fsi.fs, Optimizer.fs, Symbols.fs Build: ✅ Success (0 errors, 0 warnings) Tests: Some failures in FSharp.Core metadata reading (pickle format compatibility) Next: Investigate pickle format issues and run performance validation Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
1 parent 8a7d479 commit a70de81

11 files changed

Lines changed: 166 additions & 44 deletions

File tree

DECISIONS.md

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
# DList Migration Decisions
2+
3+
## Migration Strategy
4+
5+
### 1. Naming Convention
6+
- Using `CachedDList` instead of `DList` in public APIs for clarity
7+
- Module functions follow same naming as `QueueList` for easy replacement
8+
9+
### 2. API Compatibility Decisions
10+
11+
#### QueueList.appendOne vs CachedDList.appendOne
12+
- **QueueList**: `QueueList<'T> -> 'T -> QueueList<'T>` (curried)
13+
- **CachedDList**: Both member (`x.AppendOne(y)`) and module function (`appendOne x y`)
14+
- **Decision**: Use module function `CachedDList.appendOne` for compatibility
15+
- **Perf Impact**: None - O(1) for both
16+
17+
#### QueueList.append vs CachedDList.append
18+
- **QueueList**: `QueueList<'T> -> QueueList<'T> -> QueueList<'T>` - O(n) operation
19+
- **CachedDList**: `CachedDList<'T> -> CachedDList<'T> -> CachedDList<'T>` - **O(1) operation**
20+
- **Decision**: Direct replacement - this is the KEY OPTIMIZATION
21+
- **Perf Impact**: **Massive improvement** - O(1) vs O(n) for main hot path
22+
23+
#### QueueList.foldBack
24+
- **QueueList**: Custom implementation with reversed tail handling
25+
- **CachedDList**: Delegates to `List.foldBack` on materialized (cached) list
26+
- **Decision**: Direct replacement via cached list
27+
- **Perf Impact**: Neutral to positive (caching amortizes cost across multiple foldBack calls)
28+
29+
#### QueueList.ofList
30+
- **QueueList**: Creates front/back split
31+
- **CachedDList**: Stores list directly, creates DList wrapper
32+
- **Decision**: Direct replacement
33+
- **Perf Impact**: Slightly better (less splitting)
34+
35+
### 3. Migration Order
36+
37+
1. **Phase 1: Core Types** (TypedTree.fs/fsi)
38+
- Change `ModuleOrNamespaceType` constructor to use `CachedDList`
39+
- Update cache invalidation in mutation methods
40+
- Update all property implementations using foldBack
41+
42+
2. **Phase 2: Serialization** (TypedTreePickle.fs)
43+
- Add `p_cached_dlist` and `u_cached_dlist` functions
44+
- Replace `p_qlist`/`u_qlist` usage for `ModuleOrNamespaceType`
45+
46+
3. **Phase 3: Hot Paths** (TypedTreeOps.fs)
47+
- **CombineModuleOrNamespaceTypes** - CRITICAL: O(1) append instead of O(n)
48+
- Update all `QueueList.foldBack` calls to `CachedDList.foldBack`
49+
50+
4. **Phase 4: Remaining Usage Sites**
51+
- Symbols.fs, Optimizer.fs, fsi.fs, etc.
52+
- Replace as needed for compilation
53+
54+
### 4. Backward Compatibility
55+
56+
#### Pickle Format
57+
- **Decision**: Keep pickle format compatible by converting CachedDList to/from list
58+
- **Implementation**: `p_cached_dlist = p_wrap CachedDList.toList (p_list pv)`
59+
- **Rationale**: Avoids breaking binary compatibility
60+
61+
#### FirstElements/LastElements Properties
62+
- **QueueList**: Has separate front and reversed back lists
63+
- **CachedDList**: Single materialized list
64+
- **Decision**: `FirstElements` returns full materialized list, `LastElements` returns empty list
65+
- **Rationale**: These are rarely used except in debugging; compatibility maintained
66+
- **Perf Impact**: None for actual usage
67+
68+
### 5. Performance Expectations
69+
70+
Based on benchmarks (V5 - DList with cached iteration):
71+
72+
| Metric | QueueList | CachedDList | Improvement |
73+
|--------|-----------|-------------|-------------|
74+
| Append (2 DLists) | O(n) | **O(1)** | **Massive** |
75+
| AppendOne | O(1) | O(1) | Same |
76+
| foldBack (first call) | O(n) | O(n) | Same |
77+
| foldBack (subsequent) | O(n) | O(1) (cached) | Better |
78+
| Memory overhead | 1x | 1.6x | Acceptable |
79+
| Combined scenario (5000 appends) | 19.7ms | 4.8ms | **4.1x faster** |
80+
81+
Expected impact on compilation (5000 files, same namespace):
82+
- **Typecheck phase**: 171s → ~40-50s (4x improvement)
83+
- **Total time**: 8:43 → ~2-3 min
84+
- **Memory**: 11.69 GB → ~12-14 GB (small increase acceptable)
85+
86+
### 6. Known Limitations
87+
88+
1. **LastElements always empty**: CachedDList doesn't maintain separate front/back
89+
- **Impact**: Minimal - only used in debug views
90+
- **Alternative**: Could track but adds complexity with no benefit
91+
92+
2. **Lazy materialization**: First iteration/foldBack forces full materialization
93+
- **Impact**: Positive - amortizes cost across multiple operations
94+
- **Benchmark confirmed**: Still 4.1x faster overall
95+
96+
3. **Memory overhead 1.6x**: Stores both DList function and cached list
97+
- **Impact**: Acceptable trade-off for 4x speedup
98+
- **Mitigation**: Lazy evaluation means cache only created when needed
99+
100+
### 7. Rollback Plan
101+
102+
If issues arise:
103+
1. All changes localized to TypedTree* files and utilities
104+
2. Can revert by changing imports back to QueueList
105+
3. DList code can remain for future use
106+
4. Benchmark results preserved for reference
107+
108+
### 8. Testing Strategy
109+
110+
1. **Unit Tests**: Existing TypedTree tests should pass unchanged
111+
2. **Integration**: Full compiler test suite
112+
3. **Performance**: 5000 file scenario with --times flag
113+
4. **Validation**: Compare against baseline results in investigation/
114+
115+
## Status
116+
117+
- [x] DList implementation complete (DList.fs/fsi)
118+
- [x] Benchmarks confirm 4.1x improvement
119+
- [ ] TypedTree migration
120+
- [ ] Build validation
121+
- [ ] Test suite validation
122+
- [ ] Performance measurements

src/Compiler/Checking/CheckDeclarations.fs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5641,7 +5641,7 @@ let CombineTopAttrs topAttrs1 topAttrs2 =
56415641
assemblyAttrs = topAttrs1.assemblyAttrs @ topAttrs2.assemblyAttrs }
56425642

56435643
let rec IterTyconsOfModuleOrNamespaceType f (mty: ModuleOrNamespaceType) =
5644-
mty.AllEntities |> QueueList.iter f
5644+
mty.AllEntities |> CachedDList.iter f
56455645
mty.ModuleAndNamespaceDefinitions |> List.iter (fun v ->
56465646
IterTyconsOfModuleOrNamespaceType f v.ModuleOrNamespaceType)
56475647

src/Compiler/Checking/NameResolution.fs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,12 @@ let UnionCaseRefsInModuleOrNamespace (modref: ModuleOrNamespaceRef) =
7676
/// Try to find a type with a union case of the given name
7777
let TryFindTypeWithUnionCase (modref: ModuleOrNamespaceRef) (id: Ident) =
7878
modref.ModuleOrNamespaceType.AllEntities
79-
|> QueueList.tryFind (fun tycon -> tycon.GetUnionCaseByName id.idText |> Option.isSome)
79+
|> CachedDList.tryFind (fun tycon -> tycon.GetUnionCaseByName id.idText |> Option.isSome)
8080

8181
/// Try to find a type with a record field of the given name
8282
let TryFindTypeWithRecdField (modref: ModuleOrNamespaceRef) (id: Ident) =
8383
modref.ModuleOrNamespaceType.AllEntities
84-
|> QueueList.tryFind (fun tycon -> tycon.GetFieldByName id.idText |> Option.isSome)
84+
|> CachedDList.tryFind (fun tycon -> tycon.GetFieldByName id.idText |> Option.isSome)
8585

8686
/// Get the active pattern elements defined by a given value, if any
8787
let ActivePatternElemsOfValRef g (vref: ValRef) =
@@ -4666,7 +4666,7 @@ let rec private EntityRefContainsSomethingAccessible (ncenv: NameResolver) m ad
46664666

46674667
// Search the types in the namespace/module for an accessible tycon
46684668
(mty.AllEntities
4669-
|> QueueList.exists (fun tc ->
4669+
|> CachedDList.exists (fun tc ->
46704670
not tc.IsModuleOrNamespace &&
46714671
not (IsTyconUnseen ad g ncenv.amap m allowObsolete (modref.NestedTyconRef tc)))) ||
46724672

src/Compiler/Checking/NicePrint.fs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2479,14 +2479,14 @@ module TastDefinitionPrinting =
24792479
if mspec.IsNamespace then []
24802480
else
24812481
mspec.ModuleOrNamespaceType.AllEntities
2482-
|> QueueList.toList
2482+
|> CachedDList.toList
24832483
|> List.map (fun entity -> layoutEntityDefn denv infoReader ad m (mkLocalEntityRef entity))
24842484

24852485
let valLs =
24862486
if mspec.IsNamespace then []
24872487
else
24882488
mspec.ModuleOrNamespaceType.AllValsAndMembers
2489-
|> QueueList.toList
2489+
|> CachedDList.toList
24902490
|> List.filter shouldShow
24912491
|> List.sortBy (fun v -> v.DisplayNameCore)
24922492
|> List.map (mkLocalValRef >> PrintTastMemberOrVals.prettyLayoutOfValOrMemberNoInst denv infoReader)

src/Compiler/Interactive/fsi.fs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1663,7 +1663,7 @@ let internal mkBoundValueTypedImpl tcGlobals m moduleName name ty =
16631663
Parent(TypedTreeBasics.ERefLocal entity)
16641664
)
16651665

1666-
mty <- ModuleOrNamespaceType(ModuleOrNamespaceKind.ModuleOrType, QueueList.one v, QueueList.empty)
1666+
mty <- ModuleOrNamespaceType(ModuleOrNamespaceKind.ModuleOrType, CachedDList.one v, CachedDList.empty)
16671667

16681668
let bindExpr = mkCallDefaultOf tcGlobals range0 ty
16691669
let binding = Binding.TBind(v, bindExpr, DebugPointAtBinding.NoneAtLet)

src/Compiler/Optimize/Optimizer.fs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4269,7 +4269,7 @@ and OptimizeModuleExprWithSig cenv env mty def =
42694269
let rec elimModTy (mtyp: ModuleOrNamespaceType) =
42704270
let mty =
42714271
ModuleOrNamespaceType(kind=mtyp.ModuleOrNamespaceKind,
4272-
vals= (mtyp.AllValsAndMembers |> QueueList.filter (Zset.memberOf deadSet >> not)),
4272+
vals= (mtyp.AllValsAndMembers |> CachedDList.filter (Zset.memberOf deadSet >> not)),
42734273
entities= mtyp.AllEntities)
42744274
mtyp.ModuleAndNamespaceDefinitions |> List.iter elimModSpec
42754275
mty

src/Compiler/Symbols/Symbols.fs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,7 @@ type FSharpEntity(cenv: SymbolEnv, entity: EntityRef, tyargs: TType list) =
730730
member _.NestedEntities =
731731
if isUnresolved() then makeReadOnlyCollection [] else
732732
entity.ModuleOrNamespaceType.AllEntities
733-
|> QueueList.toList
733+
|> CachedDList.toList
734734
|> List.map (fun x -> FSharpEntity(cenv, entity.NestedTyconRef x, tyargs))
735735
|> makeReadOnlyCollection
736736

src/Compiler/TypedTree/TypedTree.fs

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1984,7 +1984,7 @@ type ExceptionInfo =
19841984

19851985
/// Represents the contents of a module or namespace
19861986
[<Sealed; StructuredFormatDisplay("{DebugText}")>]
1987-
type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, entities: QueueList<Entity>) =
1987+
type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: CachedDList<Val>, entities: CachedDList<Entity>) =
19881988

19891989
/// Mutation used during compilation of FSharp.Core.dll
19901990
let mutable entities = entities
@@ -2030,15 +2030,15 @@ type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, en
20302030

20312031
/// Mutation used during compilation of FSharp.Core.dll
20322032
member _.AddModuleOrNamespaceByMutation(modul: ModuleOrNamespace) =
2033-
entities <- QueueList.appendOne entities modul
2033+
entities <- CachedDList.appendOne entities modul
20342034
modulesByDemangledNameCache <- None
20352035
allEntitiesByMangledNameCache <- None
20362036
allEntitiesByLogicalMangledNameCache <- None
20372037

20382038
#if !NO_TYPEPROVIDERS
20392039
/// Mutation used in hosting scenarios to hold the hosted types in this module or namespace
20402040
member mtyp.AddProvidedTypeEntity(entity: Entity) =
2041-
entities <- QueueList.appendOne entities entity
2041+
entities <- CachedDList.appendOne entities entity
20422042
tyconsByMangledNameCache <- None
20432043
tyconsByDemangledNameAndArityCache <- None
20442044
tyconsByAccessNamesCache <- None
@@ -2098,13 +2098,13 @@ type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, en
20982098
else NameMap.add name2 x tab
20992099

21002100
cacheOptByref &allEntitiesByMangledNameCache (fun () ->
2101-
QueueList.foldBack addEntityByMangledName entities Map.empty)
2101+
CachedDList.foldBack addEntityByMangledName entities Map.empty)
21022102

21032103
/// Get a table of entities indexed by logical name
21042104
member _.AllEntitiesByLogicalMangledName: NameMap<Entity> =
21052105
let addEntityByMangledName (x: Entity) tab = NameMap.add x.LogicalName x tab
21062106
cacheOptByref &allEntitiesByLogicalMangledNameCache (fun () ->
2107-
QueueList.foldBack addEntityByMangledName entities Map.empty)
2107+
CachedDList.foldBack addEntityByMangledName entities Map.empty)
21082108

21092109
/// Get a table of values and members indexed by partial linkage key, which includes name, the mangled name of the parent type (if any),
21102110
/// and the method argument count (if any).
@@ -2116,7 +2116,7 @@ type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, en
21162116
else
21172117
tab
21182118
cacheOptByref &allValsAndMembersByPartialLinkageKeyCache (fun () ->
2119-
QueueList.foldBack addValByMangledName vals MultiMap.empty)
2119+
CachedDList.foldBack addValByMangledName vals MultiMap.empty)
21202120

21212121
/// Try to find the member with the given linkage key in the given module.
21222122
member mtyp.TryLinkVal(ccu: CcuThunk, key: ValLinkageFullKey) =
@@ -2137,7 +2137,7 @@ type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, en
21372137
else
21382138
tab
21392139
cacheOptByref &allValsByLogicalNameCache (fun () ->
2140-
QueueList.foldBack addValByName vals Map.empty)
2140+
CachedDList.foldBack addValByName vals Map.empty)
21412141

21422142
/// Compute a table of values and members indexed by logical name.
21432143
member _.AllValsAndMembersByLogicalNameUncached =
@@ -2146,7 +2146,7 @@ type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, en
21462146
MultiMap.add x.LogicalName x tab
21472147
else
21482148
tab
2149-
QueueList.foldBack addValByName vals MultiMap.empty
2149+
CachedDList.foldBack addValByName vals MultiMap.empty
21502150

21512151
/// Get a table of F# exception definitions indexed by demangled name, so 'FailureException' is indexed by 'Failure'
21522152
member mtyp.ExceptionDefinitionsByDemangledName =
@@ -2161,7 +2161,7 @@ type ModuleOrNamespaceType(kind: ModuleOrNamespaceKind, vals: QueueList<Val>, en
21612161
NameMap.add entity.DemangledModuleOrNamespaceName entity acc
21622162
else acc
21632163
cacheOptByref &modulesByDemangledNameCache (fun () ->
2164-
QueueList.foldBack add entities Map.empty)
2164+
CachedDList.foldBack add entities Map.empty)
21652165

21662166
[<DebuggerBrowsable(DebuggerBrowsableState.Never)>]
21672167
member mtyp.DebugText = mtyp.ToString()
@@ -6041,7 +6041,7 @@ type Construct() =
60416041

60426042
/// Create a new node for the contents of a module or namespace
60436043
static member NewModuleOrNamespaceType mkind tycons vals =
6044-
ModuleOrNamespaceType(mkind, QueueList.ofList vals, QueueList.ofList tycons)
6044+
ModuleOrNamespaceType(mkind, CachedDList.ofList vals, CachedDList.ofList tycons)
60456045

60466046
/// Create a new node for an empty module or namespace contents
60476047
static member NewEmptyModuleOrNamespaceType mkind =
@@ -6129,7 +6129,7 @@ type Construct() =
61296129
entity_typars= LazyWithContext.NotLazy []
61306130
entity_tycon_repr = repr
61316131
entity_tycon_tcaug=TyconAugmentation.Create()
6132-
entity_modul_type = MaybeLazy.Lazy(InterruptibleLazy(fun _ -> ModuleOrNamespaceType(Namespace true, QueueList.ofList [], QueueList.ofList [])))
6132+
entity_modul_type = MaybeLazy.Lazy(InterruptibleLazy(fun _ -> ModuleOrNamespaceType(Namespace true, CachedDList.ofList [], CachedDList.ofList [])))
61336133
// Generated types get internal accessibility
61346134
entity_pubpath = Some pubpath
61356135
entity_cpath = Some cpath

src/Compiler/TypedTree/TypedTree.fsi

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1359,7 +1359,7 @@ type ExceptionInfo =
13591359
[<Sealed; StructuredFormatDisplay("{DebugText}")>]
13601360
type ModuleOrNamespaceType =
13611361

1362-
new: kind: ModuleOrNamespaceKind * vals: QueueList<Val> * entities: QueueList<Entity> -> ModuleOrNamespaceType
1362+
new: kind: ModuleOrNamespaceKind * vals: CachedDList<Val> * entities: CachedDList<Entity> -> ModuleOrNamespaceType
13631363

13641364
/// Return a new module or namespace type with an entity added.
13651365
member AddEntity: tycon: Tycon -> ModuleOrNamespaceType
@@ -1384,7 +1384,7 @@ type ModuleOrNamespaceType =
13841384
member ActivePatternElemRefLookupTable: NameMap<ActivePatternElemRef> option ref
13851385

13861386
/// Type, mapping mangled name to Tycon, e.g.
1387-
member AllEntities: QueueList<Entity>
1387+
member AllEntities: CachedDList<Entity>
13881388

13891389
/// Get a table of entities indexed by both logical type compiled names
13901390
member AllEntitiesByCompiledAndLogicalMangledNames: NameMap<Entity>
@@ -1393,7 +1393,7 @@ type ModuleOrNamespaceType =
13931393
member AllEntitiesByLogicalMangledName: NameMap<Entity>
13941394

13951395
/// Values, including members in F# types in this module-or-namespace-fragment.
1396-
member AllValsAndMembers: QueueList<Val>
1396+
member AllValsAndMembers: CachedDList<Val>
13971397

13981398
/// Compute a table of values type members indexed by logical name.
13991399
member AllValsAndMembersByLogicalNameUncached: MultiMap<string, Val>

0 commit comments

Comments
 (0)