From fc94b876341dde05c1cff42de274fb4fd04d874c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:31:32 -0400 Subject: [PATCH] perf(abcip): cache the declaration-only UDT layout per member list P11: AbCipUdtReadPlanner called AbCipUdtMemberLayout.TryBuild(parent.Members) for every member reference on every poll - O(members^2) per parent per tick, a dictionary allocation each time. GetOrBuild caches the layout (including a "not groupable" verdict) in a ConditionalWeakTable keyed by the member list's identity, which is immutable for the life of the tag table; a re-init's new lists replace the entries and the old ones are collected with them. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipUdtMemberLayout.cs | 24 +++++++++++++++++++ .../AbCipUdtReadPlanner.cs | 2 +- .../AbCipUdtMemberLayoutTests.cs | 22 +++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs index b4b271f3..3c44f80f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtMemberLayout.cs @@ -25,6 +25,30 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// public static class AbCipUdtMemberLayout { + /// Layouts already computed, keyed by the parent definition's member list (reference identity). + private static readonly System.Runtime.CompilerServices.ConditionalWeakTable, LayoutBox> Cache = new(); + + /// Holds a computed layout, including a "not groupable" (null) verdict. + private sealed class LayoutBox(IReadOnlyDictionary? offsets) + { + public IReadOnlyDictionary? Offsets { get; } = offsets; + } + + /// + /// , computed once per member list (review 07 P11: the read planner rebuilt the layout — + /// a dictionary allocation and an O(members) walk — for every member reference on every poll, O(members²) per + /// parent per tick). A parent definition's member list is immutable for the life of the tag table that holds + /// it, so caching by list identity is exact; a re-init builds new lists and the old entries are collected with + /// them (). + /// + /// The parent definition's declared members. + /// The member offsets, or null when the members cannot be laid out by declaration. + public static IReadOnlyDictionary? GetOrBuild(IReadOnlyList members) + { + ArgumentNullException.ThrowIfNull(members); + return Cache.GetValue(members, m => new LayoutBox(TryBuild(m))).Offsets; + } + /// /// Try to compute member offsets for the supplied declared members. Returns null /// if any member type is unsupported for declaration-only layout. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs index 28bc181f..338d905e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipUdtReadPlanner.cs @@ -67,7 +67,7 @@ public static class AbCipUdtReadPlanner continue; } - var offsets = AbCipUdtMemberLayout.TryBuild(parent.Members); + var offsets = AbCipUdtMemberLayout.GetOrBuild(parent.Members); if (offsets is null || !offsets.TryGetValue(memberName, out var offset)) { fallback.Add(new AbCipUdtReadFallback(i, name)); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs index b895b2cc..4841c4f6 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipUdtMemberLayoutTests.cs @@ -99,4 +99,26 @@ public sealed class AbCipUdtMemberLayoutTests new AbCipStructureMember("Setpoints", AbCipDataType.Real, ElementCount: 4), }).ShouldBeNull(); } + + /// Review 07 P11: the layout is computed once per member list, not once per member per poll. + [Fact] + public void GetOrBuild_computes_a_layout_once_per_member_list() + { + IReadOnlyList members = + [ + new AbCipStructureMember("A", AbCipDataType.DInt), + new AbCipStructureMember("B", AbCipDataType.Real), + ]; + var first = AbCipUdtMemberLayout.GetOrBuild(members); + first.ShouldNotBeNull(); + first["B"].ShouldBe(4); + AbCipUdtMemberLayout.GetOrBuild(members).ShouldBeSameAs(first); + + // Positive control: a different (equal-content) list is its own entry, and a not-groupable verdict caches too. + IReadOnlyList copy = [.. members]; + AbCipUdtMemberLayout.GetOrBuild(copy).ShouldNotBeSameAs(first); + IReadOnlyList withBool = [new AbCipStructureMember("F", AbCipDataType.Bool)]; + AbCipUdtMemberLayout.GetOrBuild(withBool).ShouldBeNull(); + AbCipUdtMemberLayout.GetOrBuild(withBool).ShouldBeNull(); + } }