Repository navigation
Share immutable type display metadata through the mesh-scoped registry chain (no static table) - #5925
Conversation
… mesh-scoped registry chain Every hub builds its own TypeRegistry, and every registration built a fresh TypeDefinition that re-derived from the CLR type alone its display attribute, wordified name, icon and deferred XML-description closure, and every registry re-seeded the 27 basic types although its parent resolved them. A retained-hub heap census counted 3,662,876 definitions for 378 CLR types (#5555, #1186). #5828 shared part of that through a private static ConditionalWeakTable, which AGENTS.md forbids (process-wide state outlives the mesh and bleeds across meshes). - TypeDisplayMetadata: the hub-independent part (display declaration, fallback name, icon, lazy XML summary). - TypeDisplayMetadataCache: an INSTANCE created by the root registry of a chain (the mesh-level one MeshBuilder registers) and handed down to every child registry, so one description per CLR type per mesh; weak-keyed for collectible NodeType assemblies. The static table is removed. - A registry whose parent is a TypeRegistry no longer seeds the basic types; it resolves them through the parent. - Per-hub registration state (collection name, key-function builder, address, overrides) is unchanged: each hub still owns its TypeDefinition. TypeDisplayMetadataSharedAcrossHubsTest: 32 hubs -> 32 definitions over ONE description. Negative controls measured: a per-registry store fails it (32 descriptions); a process-wide static store fails the independent-mesh control. Refs #5555, #1186 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 0) 1 files 1 suites 3m 10s ⏱️ Results for commit d1987b9. |
Test Results (shard 1)563 tests 563 ✅ 1m 39s ⏱️ Results for commit d1987b9. |
Test Results (shard 4) 3 files 3 suites 6m 50s ⏱️ Results for commit d1987b9. |
Test Results (shard 2)580 tests 580 ✅ 7m 16s ⏱️ Results for commit d1987b9. |
Test Results (shard 5)1 706 tests 1 706 ✅ 7m 11s ⏱️ Results for commit d1987b9. |
Test Results (shard 3) 3 files 3 suites 11m 58s ⏱️ Results for commit d1987b9. |
Test Results 14 files 14 suites 38m 8s ⏱️ Results for commit d1987b9. |
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
**Change.** Replaces #5828's process-wide static `ConditionalWeakTable` with a mesh-scoped store: the root `TypeRegistry` of a chain creates a `TypeDisplayMetadataCache` (weak-keyed `ConditionalWeakTable`), child registries inherit it through the parent, giving one `TypeDisplayMetadata` (display attribute, wordified fallback name, icon, lazy XML summary) per CLR type per mesh; registries parented to a `TypeRegistry` no longer re-seed the 27 basic types; `TypeDefinition` exposes the shared instance through a new public `DisplayMetadata` property; the #5828 allocation-budget test is replaced by reference-identity tests; the PortalHeapIsHubs doc page is updated to match.
**Checked from the diff.** The static table is fully removed; the XML-docs read stays lazy and off the hub-construction path (a shared `Lazy<string>` in the default thread-safe mode, safe under concurrent first reads from many hubs); the weak key keeps collectible-assembly types unloadable; per-registry collection names and key functions stay pinned by tests; the basic-type seeding's old `null!` key-function builder argument is replaced with the registry's own builder; no async/await, no new `!`/pragma, and the new tests execute real hosted hubs.
**Not verifiable from the diff.** The remainder of `TypeRegistry.cs` (the declaration of `keyFunctionBuilder`, `CopyOwnRegistrationsTo`, and the lookup fallthrough that now resolves basic types for child registries), whether `MessageHubExtensions.CreateTypeRegistry` is a test-only helper, and whether any non-`TypeRegistry` `ITypeRegistry` implementation exists — filed as one question. The build and test results quoted in the PR description were not reproduced; this review is from the diff alone.
Findings: 0 blocking · 0 should-fix · 1 question · 0 nit
Internal review of d1987b996f77d6924a48b28795ea27c33755303c — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| public TypeRegistry(ITypeRegistry? parent) | ||
| { | ||
| this.parent = parent; | ||
| descriptions = (parent as TypeRegistry)?.descriptions ?? new TypeDisplayMetadataCache(); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
Both the description-store inheritance (`descriptions = (parent as TypeRegistry)?.descriptions ?? new TypeDisplayMetadataCache();`) and the basic-type suppression (`var seeded = parent is TypeRegistry ? [] : BasicTypes;`) key on the parent being the concrete `TypeRegistry` class rather than the `ITypeRegistry` interface the `parent` field is declared as. A child registry whose parent is any other `ITypeRegistry` implementation silently creates its own `TypeDisplayMetadataCache` and re-seeds the 27 basic types — the exact per-hub duplication this change removes — with no error or warning. Every registry constructed in this diff (and every test) builds `TypeRegistry`-only chains, so no such parent appears here; whether a non-`TypeRegistry` `ITypeRegistry` (wrapper, decorator or test double) can appear as a parent in a live chain cannot be determined from the diff. If it can, the sharing silently does not apply on that branch of the chain.
There was a problem hiding this comment.
Keying on the concrete type is deliberate. The two branches serve different purposes.
Basic-type seeding. A registry may skip seeding the basic types only when it knows its parent resolves them. TypeRegistry guarantees that, because every chain rooted in one either seeds them or inherits them. A foreign ITypeRegistry gives no such guarantee: the existing implementations are test doubles such as test/MeshWeaver.Graph.Test/TestHelpers.cs TestTypeRegistry and a Plugins PathResolution test double, and they need not resolve Int32 or Address. Skipping the seed under such a parent would make lookups fail. Re-seeding is the correct behaviour there, not a silent loss.
Description store. The store is an internal instance and is not part of ITypeRegistry. A foreign parent therefore has none to hand down, so the child starts its own store and becomes the root of the sharing below it.
Where this can happen in production. I measured the implementations:
- In
src/,TypeRegistryis the onlyITypeRegistry. - Every production chain is built by
MessageHubConfiguration(new TypeRegistry(parentSp.GetService<ITypeRegistry>())) and rooted atMeshBuilder'sCreateTypeRegistry(), so every chain isTypeRegistryall the way down. WithTypeRegistryswaps the registry itself; it does not add a foreign parent.
So the non-sharing branch is reachable only from test doubles, and there the fallback is the safe choice. No change.
Refs #5555, Refs #1186. This is part of the fix, not the whole of it. It removes one measured source of per-hub memory multiplication. It does not close the OOM or activation-timeout investigations.
Root cause
Every hub builds its own
TypeRegistry.MessageHubConfigurationchains it to the parent's registry, and that chain ends at the mesh-level registryMeshBuilderregisters. Two things were duplicated per hub:WithType/GetOrAddType/GetTypeDefinitioncall built a newTypeDefinition. Each one re-derived, from the CLR type alone, the same set of values: theDisplayAttribute, the wordified name, the icon and a deferred XML-description closure. None of these depend on the hub, yet they were rebuilt once per hub per type.The private native census reported on both issues showed the result: 3,662,876
TypeDefinitions for 378 CLR types over 40,908 hubs, with 3,652,612 separate description graphs. #5828 shared part of this, but it did so through aprivate static readonly ConditionalWeakTable. That is a static collection, which AGENTS.md forbids ("No static collections — ever"): it outlives the mesh and is shared across meshes and tests.Fix
TypeDisplayMetadataholds the part of a type description that does not depend on the hub: the display declaration, the fallback name, the icon and the lazy XML summary. Resource-backed names are still resolved per definition, and the XML read still stays off the construction path.TypeDisplayMetadataCacheis an instance, not a static. The root registry of a chain (the mesh-level registry) creates it, and each child registry receives it through the parent chain. The result is one description per CLR type per mesh. It is a weak-keyedConditionalWeakTable, so types from collectible NodeType assemblies can still be unloaded with their load context. The static table from Reduce repeated type metadata allocation across hub registries #5828 is removed.TypeRegistryno longer seeds the basic types. It resolves them through the parent.TypeDefinitionfor every type it registers, and the collection name, key-function builder, address and explicit overrides all stay per registry.Evidence
TypeDisplayMetadataSharedAcrossHubsTest(new):IndependentMeshes_DoNotShareDescriptionsfails. Two independent meshes must not share a description.Stringto the mesh registry's own definition. An independent root seeds its own basic types.Local results:
dotnet build -c Release -warnaserroris clean (0 warnings, 0 errors) for MeshWeaver.Messaging.Hub, MeshWeaver.Messaging.Hub.Test, MeshWeaver.Data.Test and MeshWeaver.Documentation.The allocation-budget test from #5828 is replaced. It exercised the standalone public constructor, which now deliberately owns a private description. Sharing is now pinned by reference identity through the registries, not by a byte budget. The doc page
Doc/Architecture/PortalHeapIsHubsis updated.Surface
TypeDisplayMetadataand a new public propertyTypeDefinition.DisplayMetadata.🤖 Generated with Claude Code