IDriver.GetMemoryFootprint / FlushOptionalCachesAsync have no consumer after #522 #525
Assignees
dohertj2 (dohertj2)
Clear assignees
dohertj2 (dohertj2)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dohertj2/lmxopcua#525
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Split out of #522, which deleted the driver-tier recycle machinery. Kept separate on purpose: removing these touches all 12 drivers and every test stub, which would have buried the reviewable part of that change in mechanical churn.
What is true today
IDriver.GetMemoryFootprint()andIDriver.FlushOptionalCachesAsync()have no production call site. Their only consumer wasMemoryTracking, deleted in #522.IDriverinsideDriverHostActor(=> 0/Task.CompletedTask).GetMemoryFootprint's doc-comment previously claimed "Polled every 30s by Core", which was not true in v3 even before #522 — nothing polled it. Both members now carry a doc-comment stating they are unconsumed.The decision
(a) Delete both from
IDriver. Removes ~12 implementations plus test stubs. Purely mechanical, but wide — best done alone, not alongside a behavioural change.(b) Keep and build a consumer. If per-driver memory pressure is wanted again, build it for the architecture as it now is: a process-wide watchdog that can ask individual drivers to flush caches — not a per-driver tier whose only remedy needed an out-of-process supervisor that cannot exist in-process (see #522 for why that model is gone).
FlushOptionalCachesAsyncis the genuinely useful half here — it degrades gracefully, where recycle did not — and the drivers' existing implementations would be reusable as-is.Unlike #522, there is a real argument for (b): a long-running server with symbol-cache-heavy drivers (TwinCAT, AB CIP) does benefit from a flush-under-pressure path, and the driver-side work is already written. What is missing is only the trigger.
⚠️ Correcting this issue's own body, and withdrawing a recommendation made against it.
When filing this I wrote that
FlushOptionalCachesAsyncis "the genuinely useful half" and that "the drivers' existing implementations would be reusable as-is". I then argued the opposite when recommending deletion, on the stated evidence that "1 of 12 actually frees memory". That number was wrong. It came from grepping for two specific cache-clear calls and treating their absence elsewhere as proof of absence — not from reading the twelve implementations.The actual inventory
FlushOptionalCachesAsync(12 implementations):=> Task.CompletedTask)GetMemoryFootprint(12 implementations): 7 return0; 5 compute an estimate (TwinCAT, OpcUaClient, Galaxy, MTConnect, Mqtt) from invented per-entry constants that have never been validated against real memory, because nothing ever consumed them.MTConnect is the one that undermines the delete case: its flush is designed, not incidental — compare-and-swap discipline referenced across ~8 doc-comments, an invariant that a flush can never leave browse permanently disabled, and tests asserting the footprint genuinely decreases afterwards. Deleting it deletes working, tested code.
What survives of the argument
Narrower, but still real: the two halves are coherent in exactly one driver. AbCip and Calculation free real memory while reporting footprint
0; TwinCAT, Galaxy and Mqtt report footprints behind no-op flushes. Only MTConnect has a footprint that means something and a flush that relieves it — which is what a pressure-triggered consumer would actually need.Status
The deletion sweep had already edited 12 drivers,
IDriverand ~15 test files when the recount landed. It was reverted rather than completed, because the justification no longer held. Nothing from #525 is on master.The three options now, with the corrected evidence:
GetMemoryFootprintonly, keepFlushOptionalCachesAsync— removes the half that is genuinely useless (7/12 zeros, unvalidated constants) and keeps the half three drivers implement meaningfully. Leaves one caller-less member.Unresolved pending a decision. The generalisable lesson: a grep for the shapes you expect is not an inventory — enumerate the set and read each member, especially when the count is what your recommendation rests on.
Done — option (a) delete, owner decision. master
b8154469(merged9fa701b1).Removed from
IDriver, all 12 driver implementations, and every test stub — plus the support code that existed only to feed them, which is the part a member-only deletion would have left behind as fresh dead weight:ProbeModelDataItemCount, andCountDataItems— a full device-tree walk run on every cache fill to populate a number with no remaining reader. That is the "wasted work on some paths" this issue anticipated._discoveredNodeCountfield and the discovery-pass write to it.ApproxBytesPerAuthoredTag.Two notes, neither a change in what the software does.
AbCipTemplateCachenow has no production reset path — its only caller was AB CIP's flush, which itself had no caller, so nothing changes except how visible the fact is. Recorded on the type, along with a correction: that class's comment claimed a deviceReinitializeAsyncalso flushed it, and no such call ever existed. A UDT redefined on a live controller keeps its stale shape until the driver instance is rebuilt.MTConnect's
/probecache is still dropped, byInvalidateProbeModel()on an Agent restart — nowinternalrather thanprivateso the tests that used the flush purely to empty the cache drive the real path instead. Those tests now cover a case that actually occurs in production, which they did not before. Several unrelated doc comments citedFlushOptionalCachesAsyncas the canonical compare-and-swap example and are repointed.Tests deleted: 3 OpcUaClient, 3 Galaxy, 2 MQTT, 1 MTConnect, 1 AbCip. One is worth recording as a lesson rather than a line item —
FlushOptionalCachesAsync_resets_footprint_counterasserted a footprint of 0 before the flush and 0 after, with no discovery in between, so it would have passed against a flush that did nothing at all.Full suite green except 7 failures verified identical on master before and after, by name rather than by assumption (4 AbLegacy integration with the compose profile down, 3 Host.IntegrationTests).
Docs corrected where they described this as live:
DriverLifecycle.md(which carried this issue's placeholder),drivers/AbCip.md,drivers/AbLegacy.md, andv2/driver-stability.md— whose "Core polls this every 30 s" and per-driverDriverConfigmemory budget were never built as written, and are now banner-ed as design intent rather than description. That doc is also where the argument for option (b) is preserved: if a flush-under-pressure watchdog is wanted later, the deleted per-driver implementations are in git history and were mostly reusable. What was always missing is the trigger.