Change the ILC DependencyFramework to use sinks instead of lists - #132955
Open
agocke wants to merge 4 commits into
Open
Change the ILC DependencyFramework to use sinks instead of lists#132955agocke wants to merge 4 commits into
agocke wants to merge 4 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 07268f0e-f7cc-4a0a-87cf-fcb906277823
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs — dependencyList ??= new DependencyList(); no longer has the intended effect after switching away… |
 |
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs — AddStaticDependencies now re-scans all virtual slots and interfaces each time it is called. This… |
What changed in this PR
This PR refactors the ILCompiler dependency analysis pipeline (ILC/ILTrim/ReadyToRun tooling) from “produce intermediate dependency lists/enumerables” to a sink-based model where nodes push dependencies directly into a provided sink, aiming to reduce allocations and expensive intermediate enumeration.
Changes:
- Introduces
IDependencySink/DependencySinkand updates the dependency framework contracts (IDependencyNode) toAddStaticDependencies/AddConditionalDependencies/SearchDynamicDependenciessink-based APIs. - Migrates many dependency nodes and helper algorithms from
IEnumerable<...>/DependencyListbuilding to directly writing to sinks. - Removes
ComputedStaticDependencyNodeand updates a variety of call sites (ILTrim + ReadyToRun + AOT compiler) to the new pattern.
| File | Description |
|---|---|
| src/coreclr/tools/ILTrim.Core/RootingHelpers.cs | Switch reflected-member rooting helpers to sink-based collection. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/VirtualMethodUseNode.cs | Convert static/conditional/dynamic dependency reporting to sink-based overrides. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenWriterNode.cs | Convert conditional/dynamic dependency APIs to sink-based no-op implementations. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/TypeSpecificationNode.cs | Push analyzed TypeSpec signature dependencies directly into the sink. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/TypeReferenceNode.cs | Replace yielded dependencies with sink adds. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/TypeDefinitionNode.cs | Convert static + conditional dependencies to sink-based adds. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/StandaloneSignatureNode.cs | Route standalone signature analysis dependencies into the sink. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/PropertyDefinitionNode.cs | Convert CA + owning-type dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ParameterNode.cs | Convert CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ModuleReferenceNode.cs | Convert “no deps” implementation to sink-based no-op. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ModuleDefinitionNode.cs | Convert module-level dependencies + CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/MethodSpecificationNode.cs | Convert signature + instantiated-method dependency reporting to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/MethodImplementationNode.cs | Convert yielded token dependencies to sink adds. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/MemberReferenceNode.cs | Convert signature/token analysis dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ManifestResourceNode.cs | Convert descriptor/implementation/CA dependencies to sink usage and update asserts. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/GenericParameterNode.cs | Convert constraint/CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/GenericParameterConstraintNode.cs | Convert constraint/CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/FieldDefinitionNode.cs | Convert signature/constant/CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/EventDefinitionNode.cs | Convert event + accessor + CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ConstantNode.cs | Convert “no deps” implementation to sink-based no-op. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/AssemblyReferenceNode.cs | Convert “no deps” implementation to sink-based no-op. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/AssemblyDefinitionNode.cs | Convert CA dependencies to sink usage. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/NodeFactory.cs | Update NullDependencyNode to sink-based dependency APIs. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/MethodBodyNode.cs | Convert cached dependency list emission to sink iteration. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/InterfaceUseNode.cs | Convert “no deps” implementations to sink-based no-ops. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/ConstructedTypeNode.cs | Convert static + conditional dependencies to sink iteration. |
| src/coreclr/tools/ILTrim.Core/DependencyAnalysis/AssemblyRootNode.cs | Convert rooting deps to sink adds and update empty conditional/dynamic implementations. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/VirtualMethodUseNode.cs | Convert dependencies and metadata manager calls to sink usage. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/ObjectNode.cs | Refactor relocation + wasm signature deps to be written directly to sink. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/INodeWithRuntimeDeterminedDependencies.cs | Replace “return deps enumerable” with sink-based AddDependencies signature. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/ExternSymbolNode.cs | Convert “no deps” implementations to sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs | Route virtual method slot-use tracking deps to the shared additional-dependency sink. |
| src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/DependencyAnalysis/MethodCodeNode.cs | Convert conditional + non-relocation dependencies to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Route additional dependencies through the shared additional-dependency sink. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/TypeMapAssemblyTargetsNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunProxyTypeMapNode.cs | Convert deps to sink usage and replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunExternalTypeMapNode.cs | Convert deps to sink usage and replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/FileLayoutOptimizer.cs | Update dependency traversal to use reusable per-depth sinks/lists. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Update helper to accept sink rather than ref-list allocation. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmR2RToInterpreterThunkNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmInterpreterToR2RThunkNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeFixupSignature.cs | Convert non-relocation deps + helper signatures to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SymbolNodeRange.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/StringDiscoverableAssemblyStubNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/SignatureEmbeddedPointerIndirectionNode.cs | Convert reloc dependency to sink add. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/PrecodeMethodImport.cs | Convert base+local deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/NewArrayFixupSignature.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodWithGCInfo.cs | Convert GC info/fixup deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodGCInfoNode.cs | Convert “no deps” implementation to sink-based no-op. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodFixupSignature.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/InstrumentationDataTableNode.cs | Convert embedded PGO dependency emission to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ImportThunk.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ImportSectionNode.cs | Convert static deps to sink adds. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Import.cs | Convert static deps to sink add. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/GenericLookupSignature.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ExceptionInfoLookupTableNode.cs | Convert non-relocation deps to sink add. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs | Convert non-relocation deps to sink add. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelayLoadMethodImport.cs | Convert base+local deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelayLoadHelperMethodImport.cs | Convert base deps + canonical method dep to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelayLoadHelperImport.cs | Convert conditional “helper present” deps to sink adds. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DeferredTillPhaseNode.cs | Convert static deps to sink adds and update dynamic signature. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DebugDirectoryEntryNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/InheritedVirtualMethodsNode.cs | Convert conditional deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/EmbeddedPointerIndirectionNode.cs | Update abstract contract to sink-based AddStaticDependencies. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/EmbeddedObjectNode.cs | Convert empty conditional/dynamic deps to sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ArrayOfEmbeddedPointersNode.cs | Convert deps to sink AddRange pattern. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ArrayInterfaceMethodsNode.cs | Convert conditional deps to sink usage and early-return pattern. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/AllMethodsOnTypeNode.cs | Convert method enumeration deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ILCompiler.DependencyAnalysisFramework.csproj | Add new sink source files; remove ComputedStaticDependencyNode compile include. |
| src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/IDependencySink.cs | Introduce public sink interfaces for static/conditional dependency emission. |
| src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/IDependencyNode.cs | Update dependency node interface to sink-based methods. |
| src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ComputedStaticDependencyNode.cs | Remove unused list-based computed-dependency node type. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedTypeMapManager.cs | Convert conditional deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedInteropStubManager.cs | Convert interop dependency hooks to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs | Convert conditional dependency callback to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/RuntimeConstructableTypeDependencies.cs | Convert conditional dependency list parameter to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ReachabilityInstrumentationProvider.cs | Update explicit IDependencyNode implementations to sink-based APIs. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/InteropStubManager.cs | Update abstract interop dependency hooks to sink-based APIs. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/EmptyInteropStubManager.cs | Update empty interop stub manager overrides to sink-based APIs. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DescriptorMarker.cs | Update RootingHelpers call sites to pass sink/list directly. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/VTableSliceNode.cs | Convert static + conditional deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/VariantInterfaceMethodUseNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeThreadStaticIndexNode.cs | Convert non-relocation deps to sink add. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs | Convert deps to sink usage and remove cached dependency list. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TrimmingDescriptorNode.cs | Convert descriptor-derived deps to sink enumeration adds. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TentativeMethodNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TentativeInstanceMethodNode.cs | Convert conditional dep array to sink add. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StructMarshallingDataNode.cs | Convert deps to sink adds and replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StringAllocatorMethodNode.cs | Convert deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/StaticsInfoHashtableNode.cs | Convert helper signature to accept sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/SerializedFrozenObjectNode.cs | Convert conditional deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/SealedVTableNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ScannedMethodNode.cs | Convert cached deps emission to sink iteration. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/RuntimeMethodHandleNode.cs | Convert non-relocation deps + metadata manager calls to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/RuntimeFieldHandleNode.cs | Convert ldtoken deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionVirtualInvokeMapNode.cs | Convert helper signature to accept sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectionInvokeSupportDependencyAlgorithm.cs | Convert params-array dependency helper to accept sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedTypeNode.cs | Convert deps to sink adds + update empty enumerables to no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedMethodNode.cs | Convert reflectability deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedFieldNode.cs | Convert reflectability + signature deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ReflectedDelegateNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ProxyTypeMapRequestNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ProxyTypeMapNode.cs | Convert conditional deps to sink usage + update empty enumerables to no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/PropertyMetadataNode.cs | Convert CA-based conditional deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectGetTypeFlowDependenciesNode.cs | Route dataflow-derived deps into sink and update empty enumerables to no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectGetTypeCalledNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NotReadOnlyFieldNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NonGCStaticsNode.cs | Convert conditional + non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs | Convert module-use deps helper to sink-based signature (and should remove leftover lazy-init). |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleMetadataNode.cs | Convert static + conditional CA deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/MethodParameterMetadataNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/MetadataEETypeNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InvalidProxyTypeMapNode.cs | Convert static deps to sink add; replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InvalidExternalTypeMapNode.cs | Convert static deps to sink add; replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceUseNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceGenericVirtualMethodTableNode.cs | Convert helper signature + metadata deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchMapNode.cs | Convert non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InterfaceDispatchCellInfoSectionNode.cs | Convert “cell deps” helper from returning list to sink-based emitter. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/InlineableStringsResourceNode.cs | Convert helper signature to accept sink; replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GvmDispatchCellInfoSectionNode.cs | Convert “cell deps” helper from returning list to sink-based emitter. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodTableNode.cs | Convert helper signature + metadata deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericVirtualMethodImplNode.cs | Convert deps to sink usage + update empty enumerables to no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericTypesTemplateMap.cs | Convert helper signature to accept sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericStaticBaseInfoNode.cs | Convert deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericMethodsTemplateMap.cs | Convert helper signature to accept sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericMethodsHashtableNode.cs | Convert helper signature + metadata deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericMethodsHashtableEntryNode.cs | Convert deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs | Convert non-relocation deps + metadata manager calls to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GCStaticsNode.cs | Convert conditional + non-relocation deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/FunctionPointerMapNode.cs | Convert helper signature to accept sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/FrozenObjectNode.cs | Convert relocation dependency discovery to sink adds. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternalTypeMapRequestNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternalTypeMapNode.cs | Convert conditional/static deps to sink usage + update dynamic signature. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExactMethodInstantiationsNode.cs | Convert helper signature + metadata deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExactMethodInstantiationsEntryNode.cs | Convert deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EventMetadataNode.cs | Convert CA-based conditional deps to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EmbeddedTrimmingDescriptorNode.cs | Convert descriptor-derived deps to sink adds. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EmbeddedPointerIndirectionNode.cs | Update abstract contract to sink-based AddStaticDependencies. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EmbeddedObjectNode.cs | Convert empty conditional/dynamic deps to sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DispatchCellNode.cs | Convert cell dependency helpers to sink-based emitters. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DictionaryLayoutNode.cs | Convert static + conditional deps to sink usage and update dynamic signature. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DelegateTargetVirtualMethodNode.cs | Convert static/conditional deps to sink-based no-ops / adds. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DelegateMarshallingDataNode.cs | Convert static deps to sink AddRange and update empty enumerables to no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/DataflowAnalyzedTypeDefinitionNode.cs | Convert dataflow dependency plumbing to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CustomAttributeMetadataNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs | Convert non-relocation deps + interop hook call to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CodeBasedDependencyAlgorithm.cs | Convert static + conditional dependency helpers to sink interfaces. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CanonicalDefinitionEETypeNode.cs | Convert non-relocation deps to sink-based no-op. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ArrayOfEmbeddedPointersNode.cs | Convert deps to sink AddRange pattern. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/AnalyzedProxyTypeMapNode.cs | Convert static deps to sink adds and replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/AnalyzedExternalTypeMapNode.cs | Convert static deps to sink adds and replace empty enumerables with no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/AnalysisCharacteristicNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/AddressTakenMethodNode.cs | Replace empty enumerables with sink-based no-ops. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs | Route Object.GetType dataflow dependencies into an injected sink. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/GenericArgumentDataFlow.cs | Convert generic argument dataflow dependency plumbing to sink usage. |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/AnalysisBasedInteropStubManager.cs | Update interop manager overrides to sink-based signatures. |
Comment on lines
+61
to
65
| public override void AddStaticDependencies(DependencySink<NodeFactory> sink, NodeFactory context) | ||
| { | ||
| if (_staticDependencies == null) | ||
| { | ||
| _staticDependencies = new DependencyList(); | ||
|
|
||
| foreach (var entry in ScanForGenericVirtualMethodEntries()) | ||
| GenericVirtualMethodTableNode.GetGenericVirtualMethodImplementationDependencies(ref _staticDependencies, context, entry.CallingMethod, entry.ImplementationMethod); | ||
|
|
||
| foreach (var entry in ScanForInterfaceGenericVirtualMethodEntries()) | ||
| InterfaceGenericVirtualMethodTableNode.GetGenericVirtualMethodImplementationDependencies(ref _staticDependencies, context, entry.CallingMethod, entry.ImplementationType, entry.ImplementationMethod); | ||
| } | ||
| foreach (var entry in ScanForGenericVirtualMethodEntries()) | ||
| GenericVirtualMethodTableNode.GetGenericVirtualMethodImplementationDependencies(sink, context, entry.CallingMethod, entry.ImplementationMethod); | ||
|
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs — dependencyList ??= new DependencyList(); no longer has the intended effect after switching away… View comment |
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs — AddStaticDependencies now re-scans all virtual slots and interfaces each time it is called. This… View comment |
Suppressed comments (1)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs:17
dependencyList ??= new DependencyList();no longer propagates a newly-created list back to the caller (the parameter is no longerref). IfdependencyListwere ever null, this would silently drop dependencies into a temporary list, and it also keeps a now-unnecessaryDependencyListalias. Since all current callers pass a non-null sink, remove the null-coalescing allocation and the unused alias import to avoid misleading behavior and unnecessary allocations.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b693437d-4563-4edc-9bf7-9901adaade97
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs — dependencyList ??= new DependencyList(); no longer has the intended effect after switching away… View comment |
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs — AddStaticDependencies now re-scans all virtual slots and interfaces each time it is called. This… View comment |
Suppressed comments (1)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs:16
dependencyList ??= new DependencyList();no longer behaves like the oldref DependencyListpattern. If a caller passesnull, this only reassigns the local parameter and any dependencies added here are dropped on return (and theDependencyListalias becomes the only use of that using). Since callers are expected to provide a sink, it’s better to require non-null and avoid silently losing dependencies.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b693437d-4563-4edc-9bf7-9901adaade97
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencySink.cs — DependencySink allocates a List<Dependency> unconditionally, even when constructed in the “wrapper”… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs — AddStaticDependencies now re-scans all virtual slots and interfaces each time it is called. This… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ModuleUseBasedDependencyAlgorithm.cs — dependencyList ??= new DependencyList(); no longer has the intended effect after switching away… View resolved comment |
Comment on lines
+46
to
+48
| private readonly List<Dependency> _dependencies = new List<Dependency>(); | ||
| private readonly DependencyNodeCore<DependencyContextType>.DependencyList? _staticDependencies; | ||
| private readonly DependencyNodeCore<DependencyContextType>.CombinedDependencyList? _conditionalDependencies; |
MichalStrehovsky
left a comment
MichalStrehovsky
left a comment
Member
There was a problem hiding this comment.
We have unit tests for the framework in #130849. We haven't really done any changes to the framework ever since it was added in dotnet/corert#42 so there wasn't much of a need. I thought I might need to do a change this summer so I ported the tests, but then ended up not making the change anyway so the test PR wasn't a priority.
Comment on lines
+55
to
63
| protected override void ComputeNonRelocationBasedDependencies(DependencySink<NodeFactory> sink, NodeFactory factory) | ||
| { | ||
| DependencyList dependencies = base.ComputeNonRelocationBasedDependencies(factory); | ||
| DependencySink<NodeFactory> dependencies = sink; | ||
| base.ComputeNonRelocationBasedDependencies(sink, factory); | ||
|
|
||
| factory.AddVirtualMethodDiscoveryDependencies(ref dependencies, _arrayType); | ||
| factory.AddVirtualMethodDiscoveryDependencies(dependencies, _arrayType); | ||
|
|
||
| return dependencies; | ||
| return; | ||
| } |
Member
There was a problem hiding this comment.
Patterns like this look sloppy (not going to comment on all).
Member
Author
There was a problem hiding this comment.
Fair -- I thought the same thing but I thought it might make the review easier. I'll have copilot clean it up
| } | ||
|
|
||
| public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFactory factory) | ||
| public override void AddStaticDependencies(DependencySink<NodeFactory> sink, NodeFactory factory) |
Member
There was a problem hiding this comment.
DependencyListEntry is a nested class so it doesn't need to repeat the NodeFactory part and we don't need a using for the framework. It would definitely look better here for DependencySink to be like that too, not sure if there's any drawbacks though.
| } | ||
|
|
||
| [Fact] | ||
| public void DependencyAnalyzerComputesDeferredStaticDependenciesBeforeDynamicDependencies() |
Member
There was a problem hiding this comment.
I do not understand what this is testing.
Comment on lines
+44
to
+52
| public override void AddDependenciesDueToMethodCodePresence(IDependencySink<NodeFactory> dependencies, NodeFactory factory, MethodDesc method) | ||
| { | ||
| } | ||
|
|
||
| public override void AddInterestingInteropConstructedTypeDependencies(ref DependencyList dependencies, NodeFactory factory, TypeDesc type) | ||
| public override void AddInterestingInteropConstructedTypeDependencies(DependencySink<NodeFactory> dependencies, NodeFactory factory, TypeDesc type) | ||
| { | ||
| } | ||
|
|
||
| public override void AddMarshalAPIsGenericDependencies(ref DependencyList dependencies, NodeFactory factory, MethodDesc method) | ||
| public override void AddMarshalAPIsGenericDependencies(IDependencySink<NodeFactory> dependencies, NodeFactory factory, MethodDesc method) |
Member
There was a problem hiding this comment.
What are the rules for when this should be IDependencySink interface and when it should use DependencySink class?
Member
Author
There was a problem hiding this comment.
In short, IDependencySink should only be used when at least one of the callers uses a DependencyList to construct a temporary list and scan the results. This isn't very common -- I believe there are only two actual roots and I tried to move every place possible over to a DependencySink.
The biggest reason in general to use IDependencySink is when you want to review and filter results after receiving them. If you don't care, you might as well use the main sink, which doesn't guarantee those semantics (and can be kind of heavyweight in that case).
I thought about adding a comment here, but it wasn't quite clear where I should add it. Let me take another shot.
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.
The current DependencyFramework algorithm is based on creating intermediate lists of dependencies that are processed into the whole dependency graph. This intermediate list production is very expensive. By moving to a series of worker-owned sinks that are threaded down through the dependency framework we can avoid intermediate container allocation and enumeration.
Rough dependency analysis numbers, measured on the Azure MCP server (an extremely large AOT application):
The largest savings were directly from the sink conversion: about 517 MiB from conditional dependency arrays, 163 MiB from static dependency arrays, 125 MiB from conditional iterators, 121 MiB from static enumerators, and 95 MiB from DependencyList objects.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com