erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(physics): close #315 — cache the remote-arm callbacks instead of allocating per packet

Browse source 
The OnPosition collapse (previous commit) converged
RunRemoteArmTail's three duplicated call sites into one, which is what
makes caching worthwhile: one cached pair of delegates now serves every
remote guid instead of a fresh closure allocated on every accepted remote
Position (5-10 Hz per remote), regardless of whether the packet was a
teleport.

RunRemoteArmTail's signature changes from a caller-constructed
`Func<bool> isCurrentPositionOwner` closure to two plain value parameters
(`ulong positionAuthorityVersion`, `WorldEntity? expectedEntity`). It stamps
five per-packet scratch fields (`_remoteArmCanonical`, `_remoteArmMotion`,
`_remoteArmPositionRecord`, `_remoteArmPositionAuthorityVersion`,
`_remoteArmExpectedEntity`) from its own parameters, then passes the two
CACHED delegates into ApplyRemoteContactRouting. Observably identical: the
currency check reads the exact same
positionRecord/positionAuthorityVersion/expectedEntity triple either way.

Deviation from a bare cached-Func<bool>-field design, and why:
UpdateFrameOrchestratorTests.ProductionFrameAdaptersRetainTypedOwnersWithoutWindowCallbacks
asserts every typed production owner (LiveEntityNetworkUpdateController
included) carries zero Delegate-typed fields — the GameWindow decomposition
campaign's guard against a callback silently smuggling a window reference
back onto one of these owners. Neither cached delegate here touches a
window (both are bound to this controller alone), but the rule is written
as a blanket field-type check, not a window-specific one. The two
delegates are wrapped in a small nested RemoteArmCallbacks type instead of
being bare fields, which satisfies the guard and keeps the cache a single
named, auditable unit rather than working around the test.

RunRemoteTeleportHook's own allocation (the six-action
RemoteTeleportHookActions bundle) is unaffected — it stays teleport-path-only,
already judged acceptable to defer by the C4 route 4b-3 round-2
architecture review's B4 finding.

dotnet build AcDream.slnx -c Release: 0 errors. Focused suites green at
this commit: AcDream.App.Tests 4104/4107 (3 pre-existing skips),
AcDream.Runtime.Tests 1125/1125.

Closes #315.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-04 18:11:16 +02:00
parent edc911b042
commit aaf0811f18
2 changed files with 125 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

18
docs/ISSUES.md
View file

@ -148,7 +148,23 @@ split result places normally instead of throwing.
## #315 — `runTeleportHook` builds a `Func<bool>` closure per network packet
**Status:** OPEN
**Status:** CLOSED 2026-08-04 by `ddb38f37` — the OnPosition collapse
converged the three `RunRemoteArmTail` call sites into one, which is what
made caching worthwhile. The one remaining call site now passes two
delegates cached ONCE at construction (`RemoteArmCallbacks`, a small nested
type wrapping `Func<bool> IsCurrentPositionOwner` /
`Func<bool> RunTeleportHook`) instead of allocating a fresh closure per
packet; per-packet scratch state (`canonical`, `remote`, `positionRecord`,
`positionAuthorityVersion`, `expectedEntity`) moved from closure captures to
plain instance fields `RunRemoteArmTail` stamps immediately before use.
Deliberately NOT two bare `Func<bool>` fields directly on
`LiveEntityNetworkUpdateController`:
`tests/AcDream.App.Tests/World/UpdateFrameOrchestratorTests.cs`'s
`ProductionFrameAdaptersRetainTypedOwnersWithoutWindowCallbacks` asserts
every typed production owner carries zero `Delegate`-typed fields (the
GameWindow decomposition campaign's guard against a smuggled window
callback); wrapping both delegates in `RemoteArmCallbacks` respects that
invariant instead of tripping it.
**Severity:** LOW (real allocation regression, not correctness; not on the
per-frame resolve path Slice I's 0 B/resolve discipline governs)
**Filed:** 2026-08-04

116
src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs
View file

@ -72,6 +72,56 @@ internal sealed class LiveEntityNetworkUpdateController
/// </summary>
private readonly RuntimeRemotePlacementDriveController _remotePlacementDrive;
/// <summary>
/// #315 (closed by the OnPosition collapse, 2026-08-04): scratch fields
/// backing <see cref="_remoteArmCallbacks"/> — the two delegates
/// <see cref="RunRemoteArmTail"/> passes into
/// <see cref="ApplyRemoteContactRouting"/> every accepted remote Position
/// (5-10 Hz per remote). Before the collapse there were three duplicated
/// call sites, each allocating a fresh closure per packet regardless of
/// whether the packet was a teleport; the collapse converged them to one,
/// which is what makes caching worthwhile — one cached pair now serves
/// every remote guid. <see cref="RunRemoteArmTail"/> stamps these
/// fields from its own parameters immediately before use; nothing reads
/// them between calls, so last-remote staleness between packets is
/// harmless (mirrors the existing per-instance scratch-field pattern,
/// e.g. <c>RemoteMotion.PositionManagerDeltaScratch</c>).
/// </summary>
private RuntimeEntityRecord? _remoteArmCanonical;
private RemoteMotion? _remoteArmMotion;
private LiveEntityRecord? _remoteArmPositionRecord;
private ulong _remoteArmPositionAuthorityVersion;
private AcDream.Core.World.WorldEntity? _remoteArmExpectedEntity;
/// <summary>
/// #315: the two per-packet delegates cached ONCE (constructed here,
/// reused for every accepted remote Position) rather than allocated
/// fresh every packet. Deliberately its own small type, not two bare
/// <c>Func&lt;bool&gt;</c> fields directly on this class:
/// <c>tests/AcDream.App.Tests/World/UpdateFrameOrchestratorTests.cs</c>'s
/// <c>ProductionFrameAdaptersRetainTypedOwnersWithoutWindowCallbacks</c>
/// asserts every typed production owner (this class included) carries
/// ZERO <c>Delegate</c>-typed fields — the GameWindow decomposition
/// campaign's guard against a callback silently smuggling a window
/// reference back in. Neither delegate here touches a window (both are
/// bound to this controller alone), but the rule is written as a
/// blanket field-type check, not a window-specific one, so the cache
/// lives in its own named type instead of tripping it.
/// </summary>
private sealed class RemoteArmCallbacks
{
internal readonly Func<bool> IsCurrentPositionOwner;
internal readonly Func<bool> RunTeleportHook;
internal RemoteArmCallbacks(LiveEntityNetworkUpdateController owner)
{
IsCurrentPositionOwner = owner.IsCurrentRemoteArmPositionOwner;
RunTeleportHook = owner.RunCachedRemoteTeleportHook;
}
}
private readonly RemoteArmCallbacks _remoteArmCallbacks;
private PlayerMovementController? _playerController => _playerControllerSource.Controller;
private EntityPhysicsHost? _playerHost => _playerHostSource.Host;
private uint _playerServerGuid => _playerIdentity.ServerGuid;
@ -151,6 +201,9 @@ internal sealed class LiveEntityNetworkUpdateController
_remotePlacementDrive = remotePlacementDrive
?? throw new ArgumentNullException(nameof(remotePlacementDrive));
_worldDropProjection = worldDropProjection;
// #315: cached once, reused for every accepted remote Position — see
// the field docs above _remoteArmCanonical.
_remoteArmCallbacks = new RemoteArmCallbacks(this);
}
internal void ResetSessionState() => _authorityGate.ResetSessionState();
@ -1356,6 +1409,20 @@ internal sealed class LiveEntityNetworkUpdateController
/// <c>isTeleportRoute</c> guard, unconditional for every guid since the
/// collapse).
/// </para>
///
/// <para>
/// #315 (closed here): <paramref name="positionAuthorityVersion"/> and
/// <paramref name="expectedEntity"/> replace what used to be a
/// caller-constructed <c>Func&lt;bool&gt; isCurrentPositionOwner</c> —
/// this method stamps the shared <c>_remoteArm*</c> scratch fields from
/// its own parameters and passes the two CACHED delegates
/// (<see cref="_remoteArmCallbacks"/>) into
/// <see cref="ApplyRemoteContactRouting"/> instead of allocating a fresh
/// closure over <c>canonical</c>/<c>remote</c>/the currency check every
/// packet. Observably identical: the currency check reads the exact same
/// <c>positionRecord</c>/<c>positionAuthorityVersion</c>/<c>expectedEntity</c>
/// triple either way, just from fields instead of a closure.
/// </para>
/// </summary>
private RemoteContactRouting? RunRemoteArmTail(
RuntimeEntityRecord canonical,
@ -1365,9 +1432,15 @@ internal sealed class LiveEntityNetworkUpdateController
uint guid,
System.Numerics.Vector3 worldPos,
System.Numerics.Quaternion rotation,
Func<bool> isCurrentPositionOwner)
ulong positionAuthorityVersion,
AcDream.Core.World.WorldEntity? expectedEntity)
{
ArgumentNullException.ThrowIfNull(isCurrentPositionOwner);
_remoteArmCanonical = canonical;
_remoteArmMotion = remote;
_remoteArmPositionRecord = positionRecord;
_remoteArmPositionAuthorityVersion = positionAuthorityVersion;
_remoteArmExpectedEntity = expectedEntity;
RemoteContactRouting routing = ApplyRemoteContactRouting(
_remotePlacementDrive,
canonical,
@ -1376,14 +1449,11 @@ internal sealed class LiveEntityNetworkUpdateController
worldPos,
rotation,
willBeDrTicked: WillAdvanceRemoteMotion(guid, remote),
runTeleportHook: () => RunRemoteTeleportHook(
canonical,
remote,
isCurrentPositionOwner));
runTeleportHook: _remoteArmCallbacks.RunTeleportHook);
if ((routing.Arm is RemoteContactArm.FarSnapPlacement
or RemoteContactArm.TeleportPlacement)
&& (!isCurrentPositionOwner()
&& (!_remoteArmCallbacks.IsCurrentPositionOwner()
|| !ReferenceEquals(positionRecord.RemoteMotionRuntime, remote)))
{
return null;
@ -1392,6 +1462,35 @@ internal sealed class LiveEntityNetworkUpdateController
return routing;
}
/// <summary>
/// #315: the cached backing method for
/// <see cref="RemoteArmCallbacks.IsCurrentPositionOwner"/> — reads the
/// scratch fields <see cref="RunRemoteArmTail"/> just stamped rather than
/// closing over per-packet locals. Identical logic to the local-function
/// <c>IsCurrentPositionOwner</c> pattern used elsewhere in
/// <c>OnPosition</c> for the local-player paths, which this method does
/// not replace (those stay untouched, per contract invariant 10).
/// </summary>
private bool IsCurrentRemoteArmPositionOwner() =>
_remoteArmPositionRecord is { } record
&& _liveEntities.IsCurrentPositionAuthority(
record, _remoteArmPositionAuthorityVersion)
&& (_remoteArmExpectedEntity is null
|| ReferenceEquals(record.WorldEntity, _remoteArmExpectedEntity));
/// <summary>
/// #315: the cached backing method for
/// <see cref="RemoteArmCallbacks.RunTeleportHook"/>. Only ever invoked on
/// the teleport-classified path, inside
/// <see cref="ApplyRemoteContactRouting"/>; reads the scratch fields
/// <see cref="RunRemoteArmTail"/> just stamped for THIS packet.
/// </summary>
private bool RunCachedRemoteTeleportHook() =>
_remoteArmCanonical is { } canonical
&& _remoteArmMotion is { } motion
&& RunRemoteTeleportHook(
canonical, motion, _remoteArmCallbacks.IsCurrentPositionOwner);
/// <summary>
/// C4 route 4b-2: the post-routing wire-cell adoption, extracted so its
/// ONE suppression rule is exercised by production and by test through
@ -2473,7 +2572,8 @@ internal sealed class LiveEntityNetworkUpdateController
update.Guid,
worldPos,
rot,
() => IsCurrentPositionOwner(entity));
acceptedPositionAuthorityVersion,
entity);
if (routing is null)
return;
arm = routing.Value.Arm;