Adam
/
AyCode.Core
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity
AyCode.Core/AyCode.Core/docs/BINARY/BINARY_TODO.md

25 KiB
Raw Blame History

AcBinarySerializer — TODO

Priority legend

  • P0 blocker · P1 important · P2 nice-to-have · P3 idea

ACCORE-BIN-T-S8P4: Replace JSON-in-Binary request parameters

Priority: P1 · Type: Refactor · Status: Closed (2026-04-26, landed in commits cdd54d3 2026-04-05 + 3b70070 2026-04-06) · Related: ../XCUT/XCUT_ISSUES.md#accore-xcut-i-x8q1 (canonical), AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md

Migrate client→server request parameters from JSON-in-Binary envelope to direct Binary serialization (matching response path). Coordinated change across client, server, and all consuming projects. Do NOT attempt as side-effect of unrelated work.

Acceptance: SignalPostJsonDataMessage<T> replaced by a SignalPostBinaryDataMessage<T> (or equivalent); no JSON round-trip on the wire for request params; benchmarks confirm no regression.

Resolution

  • What: Length-prefixed, per-parameter binary format introduced via SignalRSerializationHelper.SerializeParametersToBinary / DeserializeParametersFromBinary; further unified into SignalParams (single byte[] carrying packed method parameters with SetParameterValues / GetParameterValues).
  • Where: AyCode.Services/SignalRs/AcSignalRClientBase.cs, AcWebSignalRHubBase.cs, ISignalParams.cs (server + client dispatch); IAcSignalRHubClient.cs (legacy wrappers).
  • Equivalent (not literal SignalPostBinaryDataMessage<T>): SignalParams was chosen over a 1:1 binary wrapper class — fewer indirections on the hot path, type-safe pack/unpack, and DataSerializerType field on SignalReceiveParams for response format indication.
  • Wire impact: No JSON round-trip on the wire for request params; this is a breaking change vs. previous JSON-in-Binary clients/servers (see commit message).
  • Legacy types: SignalPostJsonMessage, SignalPostJsonDataMessage<T>, SignalPostMessage<T>, ISignalPostMessage<T> all marked [Obsolete] in IAcSignalRHubClient.cs; deletion tracked separately in AyCode.Services/docs/SIGNALR/SIGNALR_TODO.md#accore-sig-t-s3n8 (gated on consumer migration).

ACCORE-BIN-T-Q2N7: Re-evaluate DiscountProductMapping SGen exclusion

Priority: P3 · Type: Investigation · Related: BINARY_ISSUES.md#accore-bin-i-f1w8

Investigate whether the new int Id shadowing pattern can be handled by SGen (via base-class introspection, property-setter lookup on the base) to eliminate the runtime compiled-expression fallback for this entity class.

ACCORE-BIN-T-W9F1: Generate BinarySerializeTypeMetadata / BinaryDeserializeTypeMetadata at compile time

Priority: P1 · Type: Performance · Related: BINARY_ISSUES.md#accore-bin-i-n6q3

Eliminate the dominant first-call cost (reflection + Expression.Compile in metadata ctor) for SGen types by emitting pre-built metadata from the source generator.

Design outline:

  • TypeMetadataBase / BinarySerializeTypeMetadata / BinaryDeserializeTypeMetadata get a second constructor that accepts pre-computed values (hashes, MinWriteSize, ComplexPropertyCount, flags, IsIId, IdAccessorType, etc.). No reflection executes in this ctor.
  • Source generator keeps its existing s_typeNameHash / s_propertyHashes static fields (hot-path access stays static, zero indirection) and passes the same references to the metadata — single source of truth, no duplicate computation.
  • ModuleInit registers both the writer/reader and the pre-built metadata into a GeneratedMetadataRegistry. GetWrapperSlow consults this registry first, falling back to the reflection-based MetadataFactory for runtime-only types.
  • Lazy RuntimeInit() pattern for Expression.Compile property accessors:
    • TypeMetadataBase gets volatile bool _runtimeInitialized + internal void RuntimeInit() (idempotent, no lock needed).
    • GetWrapperSlow calls metadata.RuntimeInit() only when wrapper.GeneratedWriter == null || !Options.UseGeneratedCode — SGen types skip it entirely (they never touch runtime accessors on their own metadata; non-SGen child types have their own metadata and run the factory path normally).
    • Hybrid mode stays correct: an SGen type on the SGen path never uses its own property accessors; a non-SGen child type's metadata runs the reflection ctor as today.
  • volatile guards the flag; multiple contexts may race into RuntimeInit, second run is a no-op.

Thread safety: GlobalMetadataCache is ConcurrentDictionary; generated metadata is registered once at ModuleInit; wrapper construction is per-context and unchanged.

Acceptance:

  • Cold benchmark: first Serialize<T> of a fresh SGen type shows no reflection / Expression.Compile on the call stack.
  • Runtime fallback (UseGeneratedCode=false) still produces identical wire output and uses the full metadata accessors.
  • Deserialize side has parity (same approach for BinaryDeserializeTypeMetadata).
  • Existing tests pass; wire format unchanged.

ACCORE-BIN-T-T5J8: JIT Tier 1 warmup for generated hot methods

Priority: P2 · Type: Performance · Related: BINARY_ISSUES.md#accore-bin-i-n6q3

After ACCORE-BIN-T-W9F1 lands, JIT of generated WriteProperties / ScanObject / ScanForDuplicates becomes the dominant residual first-call cost for SGen types. Options to evaluate (benchmark before committing):

  • [MethodImpl(MethodImplOptions.AggressiveOptimization)] on the generated hot methods — skips Tier 0, compiles directly at Tier 1. Simple generator change. Trade-off: larger one-time JIT cost in exchange for eliminating the Tier 0→1 recompile step.
  • Background prewarm from ModuleInit: Task.Run(() => RuntimeHelpers.PrepareMethod(handle)) for each registered writer/reader method. Parallelizes JIT with app startup. Keep it opt-in (option flag) to avoid surprising consumers with extra startup threads.
  • ReadyToRun (R2R) in consuming projects' publish config — pre-compiles IL to native at publish time. External to SGen, complementary. Document as a recommended publish setting.
  • Code chunking (split generated methods exceeding a property threshold into sub-methods, e.g. WriteProperties_Part1 / _Part2) — measure first. Only beneficial for unusually large types (20+ properties / nested collections). Call overhead can offset gains; JIT inliner may already handle reasonably-sized methods well.
  • try / finally audit on hot path — On .NET 9 (project's minimum target), JIT silently refuses to inline any method containing an EH region (AggressiveInlining is ignored). [.NET 10 partially lifts this for same-module try-finally — see dotnet/runtime#112998, merged 2025-03-20 — but catch, cross-module, and P/Invoke-stub cases stay blocked. Until project's minimum runtime moves to .NET 10, treat EH as an absolute inlining barrier; even after the upgrade, several sub-cases keep the rule.] Audit scope:
    • Hand-written bridges: WriteValueGenerated / WriteObjectGenerated / WriteStringGenerated / ScanValueGenerated and any helper called from generated WriteProperties for accidental try/finally / using blocks.
    • SGen output template (AcBinarySourceGenerator.cs): generated WriteProperties / ScanObject / ScanForDuplicates / ReadObject / ReadProperties MUST stay straight-line. Future feature additions ([CustomSerializer] / [CustomDeserializer] hooks, OnSerializing / OnDeserialized callbacks, validation attributes, rented-buffer using blocks) are tempting candidates for try/catch/finally — emit them in separate cold helpers, never inline into the generated hot method. A single accidental try block in WriteProperties makes the whole generated method non-inlinable, killing the SGen Root Fast Path benefit.
    • Resource cleanup (Pool/ArrayPool/Dispose) belongs in Serialize<T> entry-frame only, not in per-property helpers or generated hot methods. See BINARY_IMPLEMENTATION.md Rule #3 (Inlining barriers) and BINARY_SGEN.md (SGen Output Constraints).
  • stackalloc size discipline on hot path — On .NET 9, methods containing localloc (any C# stackalloc) historically blocked inlining. Modern .NET allows inlining only for fixed-size stackalloc ≤ 32 bytes outside loops (see dotnet/runtime#7113) — anything larger or loop-nested still blocks. Our typical scratch-buffer patterns (UTF-8 encoding scratch, ArrayPool fallbacks) sit far above 32 bytes (256+), so any helper containing such a stackalloc is non-inlinable. Combined with try/finally for ArrayPool.Return cleanup, the method is doubly non-inlinable on .NET 9. Plan accordingly: keep stackalloc-using helpers as deliberate cold call-frames, not as AggressiveInlining candidates.
  • Native AOT — out of scope for this TODO; separate architectural decision with deployment-model implications.

Acceptance:

  • Benchmark a realistic entity graph (≥ 3 referenced child types) and show first-call time within ~10% of steady-state after ACCORE-BIN-T-W9F1 + chosen mitigation(s).
  • Document which combination is recommended for SignalR hot-path workloads vs. batch serialization.

ACCORE-BIN-T-Z3K8: Replace IId<T> interface dependency with convention/attribute-based Id detection

Priority: P1 · Type: Refactor

The binary serializer currently detects Id-tracking properties via the IId<T> interface (AyCode.Interfaces). This couples the serializer to a framework-specific abstraction and forces consumer types to implement the interface for tracking participation. Move to a POCO-friendly detection scheme:

  • IdDetectionMode.Convention (default) — convention-based; any property named Id is treated as the tracking key. Zero-friction onboarding.
  • IdDetectionMode.Attribute — explicit; only properties marked with a serializer-native [Id] (or similar) attribute are tracked.
  • [IgnoreId] attribute — escape hatch in Convention mode to exclude an Id-named property from tracking when the developer wants explicit opt-out.

Implicit contract for Convention mode: within a single class, the Id property must be type-level unique. Whether it semantically represents a primary key or a sequence number is irrelevant — the tracker keys by (Type, Id), so per-type uniqueness is the only requirement. Violating this invariant typically signals a domain-modelling problem, not a serializer bug. Design rationale discussed in conversation 2026-04-27.

Acceptance:

  • Binary serializer no longer references IId<T> in any execution path (no interface checks, no where T : IId<TKey> constraints in the serializer surface).
  • Wire format unchanged.
  • Existing consumers using IId<T>-implementing types still work transparently in Convention mode (their Id property is detected via convention).
  • New consumers can use plain POCOs with no AyCode.Interfaces dependency.
  • IdDetectionMode exposed on AcBinaryOptions (or successor options class post-rebrand).
  • Default mode = Convention.

ACCORE-BIN-T-N7V1: Replace [JsonIgnore] dependency with serializer-native ignore attribute

Priority: P2 · Type: Refactor

Property exclusion from binary serialization currently relies on [JsonIgnore] (Newtonsoft.Json). This couples the binary serializer to a third-party JSON library's attribute and is conceptually wrong — a binary serializer should not consult a JSON-specific marker for its exclusion semantics.

Define a serializer-native ignore attribute (working name [BinaryIgnore]; final name TBD pending broader rebrand). For backward compatibility during transition, also continue recognizing [JsonIgnore] with a deprecation note.

Possible cross-cutting consideration: if Toon and other future serializers also need property-exclusion, a single shared attribute (e.g., [SerializerIgnore] in a common abstractions package) may be cleaner than per-serializer attributes. Decide before naming finalizes — this may belong in XCUT_TODO.md rather than purely BINARY scope.

Acceptance:

  • Native ignore attribute defined in the binary serializer's namespace (or shared abstractions package, pending the cross-cutting decision above).
  • Both native attribute and [JsonIgnore] recognized during a transitional period; native attribute takes precedence on conflict.
  • [JsonIgnore] recognition flagged for removal in a future major version (track in a follow-up cleanup TODO once consumer projects have migrated).
  • No new code dependency on Newtonsoft.Json for property-exclusion logic.

ACCORE-BIN-T-Y6R2: Implement projection serialization phase 1 (runtime path)

Priority: P1 · Type: Feature · Related: ../adr/0001-binary-projection-serialization.md (canonical)

Implement the phase 1 runtime path of source→target projection serialization per ADR 0001. See the ADR for full context, decision rationale, alternatives, consequences, and acceptance criteria.

Sibling rebrand-prep TODOs: ACCORE-BIN-T-Z3K8 (IId migration), ACCORE-BIN-T-N7V1 (JsonIgnore replacement).

ACCORE-BIN-T-D6H4: Create AsyncPipeReaderInput class (Step 1 of ADR-0003)

Priority: P1 · Type: Refactor · Related ADR: docs/adr/0003-acbinary-streaming-receive-architecture.md Step 1

Add new sealed class AsyncPipeReaderInput : IBinaryInputBase, IDisposable in AyCode.Core/Serializers/Binaries/AsyncPipeReaderInput.cs. Self-contained sliding-window buffer (byte[] + _writePos + _readPos + _completed + ManualResetEventSlim) with reset-to-0 cycling preserved verbatim from today's SegmentBufferReader. Producer API: Feed(ReadOnlySpan<byte>), Complete(). Consumer API (IBinaryInputBase): Initialize / TryAdvanceSegment / Release.

Existing SegmentBufferReader.cs and SegmentBufferReaderInput.cs remain unchanged in this step — they keep serving the SignalR AcBinaryHubProtocol.TryParseChunkData path. Migration to the new class is in Step 6 (ACCORE-SBP-T-G7T2).

Naming rationale: AsyncPipeReaderInput mirrors the existing send-side AsyncPipeWriterOutput. The Async prefix follows .NET BCL convention for type-level naming (AsyncEnumerable, IAsyncDisposable, AsyncLocal<T>).

Acceptance:

  • New class compiles; isolated unit tests cover Feed / TryAdvanceSegment / Complete / Dispose contracts (incl. producer-consumer concurrency, missed-signal double-check, grow-buffer handoff race).
  • Existing SignalR tests continue to pass on the unchanged SegmentBufferReader path (no behavioral regression).

ACCORE-BIN-T-M2K1: Add AsyncPipeReaderInput.DrainFromAsync extension (Step 2 of ADR-0003)

Priority: P1 · Type: Feature · Related ADR: docs/adr/0003-acbinary-streaming-receive-architecture.md Step 2 · Depends on: ACCORE-BIN-T-D6H4

Add public static async Task DrainFromAsync(this AsyncPipeReaderInput input, PipeReader reader, CancellationToken ct) in AyCode.Core/Serializers/Binaries/AsyncPipeReaderInputExtensions.cs (NEW file). Pulls from a System.IO.Pipelines.PipeReader and feeds the input via repeated Feed(span) calls; calls Complete() at end-of-stream.

Separate file (not a method on the class) so the core AsyncPipeReaderInput.cs does not import System.IO.Pipelines in its primary contract — the pull-mode is opt-in at use-sites.

Acceptance:

  • Extension drains an in-memory Pipe end-to-end in a unit test (write some bytes → DrainFromAsync → assert AsyncPipeReaderInput state).
  • Complete() correctly invoked at end-of-stream (result.IsCompleted).

ACCORE-BIN-T-V7C9: Replace misleading parallel test with real parallel pipeline test (Step 3 of ADR-0003)

Priority: P1 · Type: Test · Related ADR: docs/adr/0003-acbinary-streaming-receive-architecture.md Step 3 · Depends on: ACCORE-BIN-T-M2K1

The current AcBinarySerializerPipeParallelTests.cs is misleading — it does not actually exercise serializer↔deserializer parallelism (single-threaded in practice). Rewrite to drive a producer thread (serializer) and a consumer thread (deserializer) through an in-memory Pipe, with AsyncPipeReaderInput.DrainFromAsync on the receive side. Measure ser+deser overlap and verify the ADR-0003 claimed ~1 µs / MB perf delta vs today's struct-based path.

Acceptance:

  • Test passes consistently on Windows + Linux CI.
  • Measured perf delta documented in test output / commit message.
  • Test serves as regression guard for future receive-side changes (no silent perf-cliff regression goes undetected).

ACCORE-BIN-T-A3T8: Add NamedPipe helpers — SerializeToNamedPipeAsync / DeserializeFromNamedPipeAsync (Step 4 of ADR-0003)

Priority: P1 · Type: Feature · Related ADR: docs/adr/0003-acbinary-streaming-receive-architecture.md Step 4 · Depends on: ACCORE-BIN-T-V7C9

Add static extension methods on AcBinarySerializerOptions for full NamedPipe IPC lifecycle (one-shot send / receive). New file AyCode.Core/Serializers/Binaries/AcBinarySerializerNamedPipeExtensions.cs. Send: NamedPipeServerStream → PipeWriter.Create(stream) → AsyncPipeWriterOutput. Receive: NamedPipeClientStream → PipeReader.Create(stream) → AsyncPipeReaderInput.DrainFromAsync.

Cross-platform: Windows + Linux (Unix-domain-socket via NamedPipe BCL API). WASM throws PlatformNotSupportedException per BCL contract.

Acceptance:

  • Cross-platform integration test: roundtrip a complex object graph through a NamedPipe; assert structural equality.
  • WASM build does not link these helpers (or throws clear PNS at runtime if invoked).

ACCORE-BIN-T-B5Y6: Add FileStream helpers — SerializeToFileStreamAsync / DeserializeFromFileStreamAsync (Step 5 of ADR-0003)

Priority: P1 · Type: Feature · Related ADR: docs/adr/0003-acbinary-streaming-receive-architecture.md Step 5 · Depends on: ACCORE-BIN-T-A3T8

Add static extension methods on AcBinarySerializerOptions for streaming file I/O. New file AyCode.Core/Serializers/Binaries/AcBinarySerializerFileStreamExtensions.cs. Send: FileStream.Create(path) → PipeWriter.Create(fileStream) → AsyncPipeWriterOutput. Receive: FileStream.OpenRead(path) → PipeReader.Create(fileStream) → AsyncPipeReaderInput.DrainFromAsync.

Critical streaming-doctrine invariant: peak buffer memory bounded by BufferWriterChunkSize × 2 (~8 KB at default), regardless of file size. NOT file-size-aware — do not pre-allocate to file size (would defeat streaming and break zerocopy / zeroalloc).

Acceptance:

  • Large-file roundtrip test (≥ 100 MB) passes with memory profiler showing peak buffer ≤ 16 KB throughout.
  • Full structural equality of round-tripped object.

ACCORE-BIN-T-K3W7: Rename BufferWriterChunkSize to reflect actual semantics

Priority: P3 · Type: Refactor · Breaking: Yes (public option API)

The property name BufferWriterChunkSize is misleading: across the three output paths it does NOT consistently represent a "chunk".

Output path What BufferWriterChunkSize actually controls Wire-format chunk?
ArrayBinaryOutput (Byte[] API) Initial buffer capacity of the internal byte[] No
BufferWriterBinaryOutput (IBufferWriter overload) Internal buffer size — how much data accumulates before Advance() + new GetMemory() on the underlying writer No
AsyncPipeWriterOutput (streaming) Both internal buffer and wire-format chunk frame size for chunked framing Yes (only here)
Receive side (AsyncPipeReaderInput, SegmentBufferReader[Input]) Initial receive buffer = BufferWriterChunkSize × 2 No (just sizing hint)

Only the streaming AsyncPipeWriterOutput path has a wire-format "chunk" concept (chunked framing for length-prefixed segments). On the other 75% of paths the property name reads as if the serializer were segmenting the payload, which is not what happens.

Possible directions (decide before implementing):

  1. Single rename, semantic-neutral — BufferWriterChunkSize → BufferWriterBufferSize or BufferWriterPageSize. Minimal API surface change, single-property semantics preserved. Downside: still slightly off for the streaming path where there IS chunked framing.
  2. Two-property split — InternalBufferSize (universal: how much data accumulates before Advance/Grow) + StreamingChunkSize (only meaningful for AsyncPipeWriterOutput; separate knob, defaults to InternalBufferSize). Cleanest semantics, most ceremony, slightly more options to document.
  3. Single rename, streaming-honest — Keep as BufferWriterChunkSize but document explicitly that on non-streaming paths the value is repurposed as buffer size. Cheapest change (docs only). Downside: doesn't fix the underlying confusion the field name causes.

Pick one before touching code. Option 2 is the most correct but adds API surface; Option 1 is the pragmatic middle.

Affected callers / docs to update on rename:

  • AcBinarySerializerOptions.cs (definition)
  • AcBinarySerializer.cs × 3 sites (ArrayBinaryOutput ctor, BufferWriterBinaryOutput ctor, AsyncPipeWriterOutput ctor)
  • AcBinaryDeserializer.cs × 1 site (receive-side initial capacity derivation)
  • AsyncPipeReaderInput.cs, SegmentBufferReader.cs, SegmentBufferReaderInput.cs — XML doc cross-refs
  • BINARY_WRITERS.md, BINARY_TODO.md (this entry, plus the streaming-doctrine invariant in ACCORE-BIN-T-B5Y6), BINARY_ISSUES.md (line 151 — already lists BufferWriterChunkSize among the struct-mutation issue's affected setters)
  • Consumer-side: AyCode.Services/SignalRs/AcBinaryHubProtocol.cs ctor mutates _options.BufferWriterChunkSize = options.BufferSize; — see BINARY_ISSUES.md#accore-bin-i-... (struct-mutation context). Coordinate the rename with the struct-mutation fix to avoid two cross-cutting churn waves on the same property.

Acceptance:

  • Property renamed (or split) per the chosen direction; all internal references updated.
  • XML docs reflect the actual semantics on each output path (initial capacity / advance threshold / chunk frame size — whichever applies).
  • Consumer-side usage in AcBinaryHubProtocol updated; if Option 2 is chosen, the protocol uses StreamingChunkSize (the streaming knob), not the universal one.
  • Wire format unchanged. Default values unchanged (65535 / equivalent).
  • Migration note in CHANGELOG / release notes since this is a breaking change to AcBinarySerializerOptions.

ACCORE-BIN-T-R5K2: Multi-message reuse for AsyncPipeReaderInput

Priority: P3 · Type: Feature · Related: BINARY_ISSUES.md#accore-bin-i-q4t8 — full Symptom / Root cause / Workarounds documented there; do not duplicate here.

Add a "next message" cursor / reset semantics so a long-lived AsyncPipeReaderInput can be reused across multiple Deserialize<T>(input, opts) calls without setting up a fresh instance per message. Removes the per-message ArrayPool rent + ManualResetEventSlim allocation + two Task.Run calls that the canonical pattern (DeserializeFromPipeReaderAsync) requires today, opening a true zero-alloc-per-message path on long-lived raw IPC transports (NamedPipe, FileStream, NetworkStream).

Design candidates (pick one — prototype first, measure the small-message zero-alloc claim before committing):

  • A. Initialize emits _readPos as starting position (instead of always 0), and the sliding-window reset becomes "anytime _readPos > 0 after a Deserialize completes, reset both _writePos and _readPos to 0". Smallest API change, no public surface added. Caveat: requires the deserializer to call TryAdvanceSegment at least once during message read so _readPos reflects the consumed boundary — small fully-buffered messages currently skip it entirely.

  • B. New SetReadCursor(int position) / AdvanceReadTo(int position) method: caller (deserializer or wrapper) reports the consumed offset after each Deserialize. Sliding-window reset triggers explicitly. Cleaner separation of concerns (consumer knows where it stopped), but adds a public API surface.

  • C. ResetCompletion() for framed mode: orthogonal to A/B — needed for framed multi-message reuse where the [202] CHUNK_END marker currently makes _completed = true irreversible. Combine with whichever cursor design is chosen.

Acceptance:

  • New tests exercise N consecutive Deserialize<T>(sharedInput, opts) calls on the same instance, both raw and framed modes, with payload sizes both above and below the initial buffer capacity. All N results match their respective inputs (no buffer-position aliasing, no message-#1-duplicate-on-#2 regression).
  • Existing DeserializeFromPipeReaderAsync unit tests continue to pass (single-message path unchanged).
  • Wire format unchanged (this is consumer-side reader plumbing, not a wire-level change).
  • Allocation profile of N consecutive reads on the shared input: 0 bytes per call after warmup (ArrayPool rent reused across calls, no MRES per call, no Task.Run per call). The deserialized object graph allocations stay (those are user-visible).