docs: add optimization planning documents
This commit is contained in:
Joseph Doherty
244
docs/plans/2026-03-13-optimizations_parser-plan.md
Normal file
244
docs/plans/2026-03-13-optimizations_parser-plan.md
Normal file
@@ -0,0 +1,244 @@
|
||||
# Parser Span Retention Implementation Plan
|
||||
|
||||
> **For Codex:** REQUIRED SUB-SKILLS: Use `using-git-worktrees` to create an isolated worktree before making changes, `executeplan` to implement this plan task-by-task, and `finishing-a-development-branch` to merge the verified work back to `main` when implementation is complete.
|
||||
|
||||
**Goal:** Reduce parser hot-path allocations by keeping protocol fields and payloads in byte-oriented views until a caller explicitly needs materialized `string` or copied `byte[]` values.
|
||||
|
||||
**Architecture:** Introduce a byte-first parser representation alongside the current `ParsedCommand` contract, then migrate `NatsClient` and adjacent hot paths to consume the new representation without changing wire behavior. Preserve compatibility through an adapter layer so functional parity stays stable while allocation-heavy paths move to spans, pooled buffers, and sequence slices.
|
||||
|
||||
**Tech Stack:** .NET 10, C#, `System.Buffers`, `System.IO.Pipelines`, `ReadOnlySequence<byte>`, `SequenceReader<byte>`, xUnit, existing benchmark test harness.
|
||||
|
||||
---
|
||||
|
||||
## Scope Anchors
|
||||
- Primary source: `src/NATS.Server/Protocol/NatsParser.cs`
|
||||
- Primary consumer: `src/NATS.Server/NatsClient.cs`
|
||||
- Existing parser tests: `tests/NATS.Server.Core.Tests/ParserTests.cs`
|
||||
- Existing snippet/parity tests: `tests/NATS.Server.Core.Tests/Protocol/ProtocolParserSnippetGapParityTests.cs`
|
||||
- Documentation to update: `Documentation/Protocol/Parser.md`
|
||||
- Benchmark project: `tests/NATS.Server.Benchmark.Tests/NATS.Server.Benchmark.Tests.csproj`
|
||||
- Benchmark run instructions: `tests/NATS.Server.Benchmark.Tests/README.md`
|
||||
- Benchmark comparison report: `benchmarks_comparison.md`
|
||||
|
||||
## Task 0: Create an isolated git worktree for the parser optimization work
|
||||
|
||||
**Files:**
|
||||
- Verify: `.worktrees/`
|
||||
- Modify if needed: `.gitignore`
|
||||
|
||||
**Step 1: Verify the preferred worktree directory is available and ignored**
|
||||
- Check that `.worktrees/` exists and is ignored by git before creating a project-local worktree.
|
||||
- If `.worktrees/` is not ignored, add it to `.gitignore`, then commit that repository hygiene fix before continuing.
|
||||
|
||||
**Step 2: Create the feature worktree on a `codex/` branch**
|
||||
- Run:
|
||||
```bash
|
||||
git worktree add .worktrees/codex-parser-span-retention -b codex/parser-span-retention
|
||||
cd .worktrees/codex-parser-span-retention
|
||||
```
|
||||
- Expected: a new isolated worktree exists at `.worktrees/codex-parser-span-retention` on branch `codex/parser-span-retention`.
|
||||
|
||||
**Step 3: Verify the worktree starts from a clean, passing baseline**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj -c Release`
|
||||
- Expected: PASS. If this fails, stop and resolve or explicitly confirm whether to proceed from a failing baseline.
|
||||
|
||||
**Step 4: Commit any required worktree setup fix**
|
||||
- Only if `.gitignore` changed, run:
|
||||
```bash
|
||||
git add .gitignore
|
||||
git commit -m "chore: ignore local worktree directories"
|
||||
```
|
||||
|
||||
## Task 1: Freeze parser behavior and add allocation-focused tests
|
||||
|
||||
**Files:**
|
||||
- Modify: `tests/NATS.Server.Core.Tests/ParserTests.cs`
|
||||
- Modify: `tests/NATS.Server.Core.Tests/Protocol/ProtocolParserSnippetGapParityTests.cs`
|
||||
- Create: `tests/NATS.Server.Core.Tests/Protocol/ParserSpanRetentionTests.cs`
|
||||
|
||||
**Step 1: Add failing tests for byte-first parser behavior**
|
||||
- Cover `PUB`, `HPUB`, `CONNECT`, and `INFO` with assertions that the new parser path can expose field data without forcing immediate `string` materialization.
|
||||
- Add split-payload cases to prove the parser preserves pending payload state across reads.
|
||||
|
||||
**Step 2: Add compatibility tests for existing `ParsedCommand` behavior**
|
||||
- Keep current semantics for `Type`, `Subject`, `ReplyTo`, `Queue`, `Sid`, `HeaderSize`, and `Payload`.
|
||||
- Ensure malformed protocol inputs still throw `ProtocolViolationException` with existing snippets/messages.
|
||||
|
||||
**Step 3: Run targeted tests to verify the new tests fail first**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj --filter "FullyQualifiedName~ParserTests|FullyQualifiedName~ParserSpanRetentionTests|FullyQualifiedName~ProtocolParserSnippetGapParityTests" -c Release`
|
||||
- Expected: FAIL in the newly added parser span-retention tests only.
|
||||
|
||||
**Step 4: Commit the failing-test baseline**
|
||||
- Run:
|
||||
```bash
|
||||
git add tests/NATS.Server.Core.Tests/ParserTests.cs tests/NATS.Server.Core.Tests/Protocol/ProtocolParserSnippetGapParityTests.cs tests/NATS.Server.Core.Tests/Protocol/ParserSpanRetentionTests.cs
|
||||
git commit -m "test: lock parser span-retention behavior"
|
||||
```
|
||||
|
||||
## Task 2: Introduce byte-oriented parser view types
|
||||
|
||||
**Files:**
|
||||
- Create: `src/NATS.Server/Protocol/ParsedCommandView.cs`
|
||||
- Modify: `src/NATS.Server/Protocol/NatsParser.cs`
|
||||
|
||||
**Step 1: Add a hot-path parser view contract**
|
||||
- Create a `ref struct` or small `readonly struct` representation for command views that can carry:
|
||||
- operation kind
|
||||
- subject/reply/queue/SID as spans or sequence-backed views
|
||||
- payload as `ReadOnlySequence<byte>` or `ReadOnlyMemory<byte>` when contiguous
|
||||
- header size and max-messages metadata
|
||||
|
||||
**Step 2: Add an adapter to the current `ParsedCommand` shape**
|
||||
- Keep the public/internal `ParsedCommand` entry point usable for existing consumers and tests.
|
||||
- Centralize materialization so `Encoding.ASCII.GetString(...)` and `ToArray()` happen in one adapter layer instead of inside every parse branch.
|
||||
|
||||
**Step 3: Re-run parser tests**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj --filter "FullyQualifiedName~Parser" -c Release`
|
||||
- Expected: FAIL only in branches not yet migrated to the new view path.
|
||||
|
||||
**Step 4: Commit the parser-view scaffolding**
|
||||
- Run:
|
||||
```bash
|
||||
git add src/NATS.Server/Protocol/ParsedCommandView.cs src/NATS.Server/Protocol/NatsParser.cs
|
||||
git commit -m "feat: add byte-oriented parser view contract"
|
||||
```
|
||||
|
||||
## Task 3: Rework control-line parsing and pending payload state
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/NATS.Server/Protocol/NatsParser.cs`
|
||||
|
||||
**Step 1: Remove early string materialization from control-line parsing**
|
||||
- Change `ParsePub`, `ParseHPub`, `ParseSub`, `ParseUnsub`, `ParseConnect`, and `ParseInfo` to keep raw byte slices in the hot parser path.
|
||||
- Replace `_pendingSubject` and `_pendingReplyTo` string fields with byte-oriented pending state.
|
||||
|
||||
**Step 2: Avoid unconditional payload copies**
|
||||
- Update `TryReadPayload()` so single-segment payloads can flow through as borrowed memory/slices.
|
||||
- Copy only when the payload is multi-segment or when the compatibility adapter explicitly requires a standalone buffer.
|
||||
|
||||
**Step 3: Replace repeated tiny literal allocations**
|
||||
- Stop using per-call `u8.ToArray()`-style buffers for CRLF and other fixed protocol tokens inside this parser path.
|
||||
- Add shared static buffers where appropriate.
|
||||
|
||||
**Step 4: Run targeted regression tests**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj --filter "FullyQualifiedName~ParserTests|FullyQualifiedName~ProtocolParserSnippetGapParityTests" -c Release`
|
||||
- Expected: PASS.
|
||||
|
||||
**Step 5: Commit the parser hot-path rewrite**
|
||||
- Run:
|
||||
```bash
|
||||
git add src/NATS.Server/Protocol/NatsParser.cs
|
||||
git commit -m "perf: keep parser state in bytes until materialization"
|
||||
```
|
||||
|
||||
## Task 4: Migrate `NatsClient` to the new parser path without changing behavior
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/NATS.Server/NatsClient.cs`
|
||||
- Modify: `tests/NATS.Server.Core.Tests/ParserTests.cs`
|
||||
- Modify: `tests/NATS.Server.Core.Tests/Protocol/ClientProtocolGoParityTests.cs`
|
||||
|
||||
**Step 1: Consume parser views first, materialize only at command handling boundaries**
|
||||
- Update `ProcessCommandsAsync` and any parser call sites so hot `PUB`/`HPUB` handling can read subject, reply, and payload from the byte-oriented representation.
|
||||
- Keep logging/tracing behavior intact, but ensure tracing is the only reason strings are created on trace-enabled paths.
|
||||
|
||||
**Step 2: Preserve feature parity**
|
||||
- Verify header parsing, payload size checks, connect/info handling, and slow-consumer/error behavior still match current tests.
|
||||
|
||||
**Step 3: Run consumer-facing protocol tests**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj --filter "FullyQualifiedName~ClientProtocolGoParityTests|FullyQualifiedName~ParserTests" -c Release`
|
||||
- Expected: PASS.
|
||||
|
||||
**Step 4: Commit the consumer migration**
|
||||
- Run:
|
||||
```bash
|
||||
git add src/NATS.Server/NatsClient.cs tests/NATS.Server.Core.Tests/ParserTests.cs tests/NATS.Server.Core.Tests/Protocol/ClientProtocolGoParityTests.cs
|
||||
git commit -m "perf: consume parser command views in client hot path"
|
||||
```
|
||||
|
||||
## Task 5: Add benchmarks, document the change, run full verification, and refresh the benchmark comparison report
|
||||
|
||||
**Files:**
|
||||
- Create: `tests/NATS.Server.Benchmark.Tests/Protocol/ParserHotPathBenchmarks.cs`
|
||||
- Modify: `Documentation/Protocol/Parser.md`
|
||||
- Modify: `benchmarks_comparison.md`
|
||||
|
||||
**Step 1: Add parser-focused benchmark coverage**
|
||||
- Add microbenchmarks for:
|
||||
- `PING` / `PONG`
|
||||
- `PUB`
|
||||
- `HPUB`
|
||||
- split payload reads
|
||||
- Capture throughput and allocation deltas before/after.
|
||||
|
||||
**Step 2: Update protocol documentation**
|
||||
- Document the new parser view + adapter split, why strings are deferred, and where payload copying is still intentionally required.
|
||||
|
||||
**Step 3: Run full verification**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj -c Release`
|
||||
- Run: `dotnet test tests/NATS.Server.Benchmark.Tests/NATS.Server.Benchmark.Tests.csproj --filter "FullyQualifiedName~Parser" -c Release`
|
||||
- Expected: PASS; benchmark output shows reduced allocations relative to the baseline run.
|
||||
|
||||
**Step 4: Run the full benchmark suite per the benchmark project README**
|
||||
- Run:
|
||||
```bash
|
||||
dotnet test tests/NATS.Server.Benchmark.Tests \
|
||||
--filter "Category=Benchmark" \
|
||||
-v normal \
|
||||
--logger "console;verbosity=detailed" 2>&1 | tee /tmp/bench-output.txt
|
||||
```
|
||||
- Expected: benchmark comparison output is captured in `/tmp/bench-output.txt`, including the "Standard Output Messages" blocks described in `tests/NATS.Server.Benchmark.Tests/README.md`.
|
||||
|
||||
**Step 5: Update `benchmarks_comparison.md` with the new benchmark results**
|
||||
- Extract the comparison blocks from `/tmp/bench-output.txt`.
|
||||
- Update `benchmarks_comparison.md` with the latest msg/s, MB/s, ratio, and latency values.
|
||||
- Update the benchmark date, environment description, Summary table, and Key Observations so they match the new run.
|
||||
|
||||
**Step 6: Commit the verification, docs, and benchmark report refresh**
|
||||
- Run:
|
||||
```bash
|
||||
git add tests/NATS.Server.Benchmark.Tests/Protocol/ParserHotPathBenchmarks.cs Documentation/Protocol/Parser.md benchmarks_comparison.md
|
||||
git commit -m "docs: record parser hot-path allocation strategy"
|
||||
```
|
||||
|
||||
## Task 6: Merge the verified parser work back to `main` and clean up the worktree
|
||||
|
||||
**Files:**
|
||||
- No source changes expected
|
||||
|
||||
**Step 1: Confirm the feature branch is fully verified before merge**
|
||||
- Reuse the verification from Task 5. Do not merge if the core tests, parser benchmark tests, or full benchmark suite run did not complete successfully.
|
||||
|
||||
**Step 2: Merge `codex/parser-span-retention` back into `main`**
|
||||
- Return to the primary repository worktree and run:
|
||||
```bash
|
||||
git checkout main
|
||||
git pull
|
||||
git merge codex/parser-span-retention
|
||||
```
|
||||
- Expected: the parser optimization commits merge cleanly into `main`.
|
||||
|
||||
**Step 3: Re-run verification on the merged `main` branch**
|
||||
- Run: `dotnet test tests/NATS.Server.Core.Tests/NATS.Server.Core.Tests.csproj -c Release`
|
||||
- Run: `dotnet test tests/NATS.Server.Benchmark.Tests/NATS.Server.Benchmark.Tests.csproj --filter "FullyQualifiedName~Parser" -c Release`
|
||||
- Confirm `benchmarks_comparison.md` reflects the results captured from the Task 5 full benchmark suite.
|
||||
- Expected: PASS on merged `main`.
|
||||
|
||||
**Step 4: Delete the feature branch and remove the worktree**
|
||||
- Run:
|
||||
```bash
|
||||
git branch -d codex/parser-span-retention
|
||||
git worktree remove .worktrees/codex-parser-span-retention
|
||||
```
|
||||
- Expected: only `main` remains checked out in the primary workspace, and the temporary parser worktree is removed.
|
||||
|
||||
## Completion Checklist
|
||||
- [ ] Parser optimization work was implemented in an isolated `.worktrees/codex-parser-span-retention` worktree on branch `codex/parser-span-retention`.
|
||||
- [ ] `NatsParser` no longer materializes hot-path `string` values during parse unless the compatibility adapter requests them.
|
||||
- [ ] Single-segment payloads can pass through without an unconditional `byte[]` copy.
|
||||
- [ ] Existing parser and protocol behavior remains green in core tests.
|
||||
- [ ] Parser-focused benchmark coverage exists in `tests/NATS.Server.Benchmark.Tests/Protocol/`.
|
||||
- [ ] The full benchmark suite was run using the workflow from `tests/NATS.Server.Benchmark.Tests/README.md`.
|
||||
- [ ] `benchmarks_comparison.md` was updated to reflect the latest benchmark run.
|
||||
- [ ] `Documentation/Protocol/Parser.md` explains the byte-first parser architecture.
|
||||
- [ ] Verified parser optimization commits were merged back into `main`.
|
||||
Reference in New Issue
Block a user