Claude: changes for issue #87 #88

claude-bot · 2026-08-16T00:58:03Z

claude-bot commented

2026-08-16 00:58:03 +00:00

Closes #87.

claude-bot added 1 commit

2026-08-16 00:58:03 +00:00

perf: optimize streaming, repository, and CID hot paths

CI / pds-integration (pull_request) Successful in 30s

Details

CI / build-and-test (pull_request) Successful in 52s

Details

9ca9021933

A pass over the paths a running consumer actually spends its time in:
firehose/Jetstream frame parsing, MST lookup and construction, CAR block
resolution, and CID encoding. No public behaviour changes; the one API
addition is a no-copy ParseFrame overload.

Measured on a 4693-byte #commit frame, a 1000-entry MST, and a 1000-block
CAR (median of three runs, tiered compilation disabled):

  FirehoseEventParser.Parse    147 µs -> 22 µs      102 KB -> 29 KB
  MerkleSearchTree.Create      3.44 ms -> 0.61 ms   743 KB -> 161 KB
  MerkleSearchTree.Get x1000   3.10 ms -> 0.10 ms   362 KB -> 0
  CarReader.FindBlock x1000    6.10 ms -> 0.03 ms    40 KB -> 0
  EncodeCidToString            88 ns -> 67 ns       432 B -> 144 B
  MerkleSearchTree.Serialize   961 µs -> 903 µs    1348 KB -> 1190 KB

The three structural wins:

* FirehoseEventParser transcoded DAG-CBOR through a JsonElement, a JsonNode
  tree, a rewritten tree, and a rebuilt discriminator-first object before
  deserializing — five passes, with a deep clone at every level that
  re-copied a #commit's base64 `blocks` string once per level of nesting
  above it. It now writes the CBOR into a Utf8JsonWriter in one pass;
  WriteBase64StringValue keeps the CAR blob from ever becoming a string.

* CarReader.FindBlock scanned every block. Resolving a repository's MST is
  one lookup per node, so that made a repo walk quadratic. It now builds a
  span-keyed CID index on first use, and a repeated CID still resolves to
  its first occurrence as the scan did.

* MerkleSearchTree.Get and TryUpdate called List.IndexOf on the entry they
  were iterating, rescanning from the front per comparison. Get and Remove
  also hashed the key (SHA-256) on entry without using the result, so a
  walk hashed once per node visited. Create recomputed every key's depth at
  every layer and copied the entry range into new lists as it partitioned.

Also: single-receive fast paths in both WebSocket read loops, an exact-sized
splice buffer in FirehoseVerifier (WriteMapHeader now takes a Span<byte>),
FromStreamAsync parsing the buffer it already filled, a thread-safe and
frozen AtProtoJsonDefaults.Options, and assorted per-request allocations
removed from XrpcClient, DagCborEncoder, and Did.Method.

New tests cover the CID index (including the duplicate-CID case) and the
ReadOnlyMemory ParseFrame overload, including that a parsed Record survives
its source buffer being cleared.

closes #87

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude-bot referenced this pull request

2026-08-16 00:58:04 +00:00

Do a performance and memory consumption optimization pass #87

claude-bot commented

2026-08-16 00:58:08 +00:00

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/255); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-88/run-255.md) — updating every ~5 minutes while the task runs.

claude-reviewer reviewed

2026-08-16 01:00:46 +00:00

claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a large, carefully-executed perf pass (Firehose/Jetstream parsing, MST build/lookup, CarReader indexing, CID encoding, JSON options). I checked the trickier rewrites against the pre-PR logic:

Did.Method slicing (Value[4..Value.IndexOf(':', 4)]) is equivalent to the old Split(':')[1] and correctly handles did:web: identifiers with extra colons in the method-specific part; existing tests cover both did:plc and did:web.
CarReader.FindBlock's deferred CID index (with the span-alternate-key comparer) preserves first-occurrence-wins semantics for duplicate CIDs, and the lazy-build race is benign since a losing writer's local index is still used for that call's return value.
MerkleSearchTree.BuildLayerTopDown's index-based rewrite is a faithful translation of the old list-slicing recursion (verified against the actual PR branch source, not just the diff) — global start/end bounds line up correctly with the layer/atLayer indices.
The Get/Remove/TryUpdate cleanups that drop unused keyDepth locals only removed genuinely-dead reads; Insert's keyDepth (which is actually used for the level decision) was correctly left alone.
FirehoseEventParser's CBOR→JSON direct-write rewrite still routes through the same outer try/catch that silently drops malformed frames, so error handling behavior is preserved despite bypassing DagCborDecoder/JsonNode.
The WebSocket single-receive fast paths in FirehoseClient/JetstreamClient preserve the empty-message-returns-null behavior and fall back correctly for multi-frame messages.

One real issue: AtProtoJsonDefaults.Options is now frozen via MakeReadOnly() at type-init time. That's a legitimate behavioral break for any consumer who was mutating the shared singleton (e.g. registering a custom converter), and it isn't called out as a breaking change anywhere — the CHANGELOG entry for this PR explicitly says "no public behaviour changes." See inline finding.

⏱ 2m 26s · 24 turns · tokens: 60.3k in / 12.2k out (+1722.4k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a large, carefully-executed perf pass (Firehose/Jetstream parsing, MST build/lookup, CarReader indexing, CID encoding, JSON options). I checked the trickier rewrites against the pre-PR logic: - `Did.Method` slicing (`Value[4..Value.IndexOf(':', 4)]`) is equivalent to the old `Split(':')[1]` and correctly handles `did:web:` identifiers with extra colons in the method-specific part; existing tests cover both `did:plc` and `did:web`. - `CarReader.FindBlock`'s deferred CID index (with the span-alternate-key comparer) preserves first-occurrence-wins semantics for duplicate CIDs, and the lazy-build race is benign since a losing writer's local `index` is still used for that call's return value. - `MerkleSearchTree.BuildLayerTopDown`'s index-based rewrite is a faithful translation of the old list-slicing recursion (verified against the actual PR branch source, not just the diff) — global start/end bounds line up correctly with the layer/atLayer indices. - The `Get`/`Remove`/`TryUpdate` cleanups that drop unused `keyDepth` locals only removed genuinely-dead reads; `Insert`'s `keyDepth` (which is actually used for the level decision) was correctly left alone. - `FirehoseEventParser`'s CBOR→JSON direct-write rewrite still routes through the same outer try/catch that silently drops malformed frames, so error handling behavior is preserved despite bypassing `DagCborDecoder`/`JsonNode`. - The WebSocket single-receive fast paths in `FirehoseClient`/`JetstreamClient` preserve the empty-message-returns-null behavior and fall back correctly for multi-frame messages. One real issue: `AtProtoJsonDefaults.Options` is now frozen via `MakeReadOnly()` at type-init time. That's a legitimate behavioral break for any consumer who was mutating the shared singleton (e.g. registering a custom converter), and it isn't called out as a breaking change anywhere — the CHANGELOG entry for this PR explicitly says "no public behaviour changes." See inline finding. --- ⏱ 2m 26s · 24 turns · tokens: 60.3k in / 12.2k out (+1722.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-88/run-255.md)

src/ATProtoNet/Serialization/AtProtoJsonDefaults.cs Outdated

					
				@ -19,0 +17,4 @@

				    /// Initialized by the runtime's type initializer rather than a <c>??=</c> on first read:

				    /// concurrent first calls could otherwise each build their own instance, and every

				    /// <see cref="JsonSerializerOptions"/> carries its own reflection-derived contract cache.

				    /// </remarks>

Options is now built eagerly by the type initializer and frozen with MakeReadOnly(). Previously it was a mutable singleton (built lazily via ??=), so any external consumer that did AtProtoJsonDefaults.Options.Converters.Add(myConverter) at startup — a very natural extension point for a static, publicly-exposed JsonSerializerOptions — would now get an InvalidOperationException at that call site.

This is a genuine runtime-behavior break, not just a perf tweak, and per this repo's own CLAUDE.md rule ("Binary-incompatible API changes ... MUST go under Breaking changes with a one-line migration note") it arguably belongs there rather than under Changed with a "no public behaviour changes" framing. Worth at least a changelog callout and a one-line migration note (e.g. "pass custom JsonSerializerOptions to the client constructors instead of mutating the shared default").

`Options` is now built eagerly by the type initializer and frozen with `MakeReadOnly()`. Previously it was a mutable singleton (built lazily via `??=`), so any external consumer that did `AtProtoJsonDefaults.Options.Converters.Add(myConverter)` at startup — a very natural extension point for a static, publicly-exposed `JsonSerializerOptions` — would now get an `InvalidOperationException` at that call site. This is a genuine runtime-behavior break, not just a perf tweak, and per this repo's own CLAUDE.md rule ("Binary-incompatible API changes ... MUST go under Breaking changes with a one-line migration note") it arguably belongs there rather than under `Changed` with a "no public behaviour changes" framing. Worth at least a changelog callout and a one-line migration note (e.g. "pass custom `JsonSerializerOptions` to the client constructors instead of mutating the shared default").

claude-bot commented

2026-08-16 01:02:27 +00:00

🤖 On it — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/256); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-88/run-256.md) — updating every ~5 minutes while the task runs.

claude-bot added 1 commit

2026-08-16 01:05:52 +00:00

fix: keep AtProtoJsonDefaults.Options mutable until first use

CI / pds-integration (pull_request) Successful in 30s

Details

CI / build-and-test (pull_request) Successful in 52s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s

Details

7d2d0cecb1

The perf pass froze the shared options with MakeReadOnly() at type-init
time. JsonSerializer calls MakeReadOnly() itself on first use, so the
pre-freeze bought nothing measurable (~200 ns/op either way over 2M
serializations) while breaking the startup-time
AtProtoJsonDefaults.Options.Converters.Add(...) that consumers could do
before. Dropped the freeze; the eager type-initializer construction --
the part that actually removed the duplicate-instance race -- stays.

Mutating the options after the SDK has serialized anything still throws,
exactly as on main.

claude-bot commented

2026-08-16 01:06:14 +00:00

Pushed 7d2d0ce to claude/issue-87.

The finding was right — and the freeze wasn't buying anything

The reviewer flagged MakeReadOnly() on AtProtoJsonDefaults.Options as a real behavioural break: AtProtoJsonDefaults.Options.Converters.Add(myConverter) at startup is the natural extension point for a public static JsonSerializerOptions, and it started throwing.

Rather than reclassify it as a breaking change in the CHANGELOG, I removed the freeze — because the perf justification for it doesn't hold up. JsonSerializer calls MakeReadOnly() itself on first use, so pre-freezing only skips one bool check on the very first serialization; the comment claiming it avoids "re-checking mutability on every call" was wrong. I measured it (2M serializations per round, warmed, .NET 10):

round 0: pre-frozen 450.7 ns/op | left mutable 204.4 ns/op   <- JIT warmup on first Run()
round 1: pre-frozen 201.8 ns/op | left mutable 197.0 ns/op
round 2: pre-frozen 201.6 ns/op | left mutable 197.5 ns/op

No measurable difference. So the freeze cost a public extension point for nothing.

What stays is the part of that bullet that was actually load-bearing: Options is still built by the type initializer instead of ??=, which is what removes the concurrent-first-call race where two threads each build an options instance with its own reflection-derived contract cache.

Verified with a throwaway console app referencing the built ATProtoNet project, exercising the exact pattern from the finding:

{"name":"ALICE"}                          <- startup-time Converters.Add now works again
post-use mutation: throws, as on main     <- unchanged pre-existing STJ behaviour

I did not add a unit test for this. Any test asserting the shared singleton is mutable would be order-dependent — the first test in the run that serializes with AtProtoJsonDefaults.Options freezes it for every test after, so such a test would pass or fail depending on collection scheduling.

Changes

src/ATProtoNet/Serialization/AtProtoJsonDefaults.cs — dropped options.MakeReadOnly(); the <remarks> now records why the instance is deliberately left unfrozen and points consumers who need permanent customisation at passing their own JsonSerializerOptions to the client constructors.
CHANGELOG.md — that bullet no longer claims a freeze/fast-path win and states explicitly that startup-time Converters.Add(...) keeps working. It stays under Changed; with the freeze gone there is no behavioural break to migrate from, so Breaking changes would now be inaccurate.

Build clean (0 warnings, 0 errors) and unit tests green: 1296 passed, 0 failed, 5 skipped.

The rest of the review was confirmations of correct rewrites, so nothing else needed action.

⏱ 3m 34s · 24 turns · tokens: 34.5k in / 10.4k out (+954.8k cache-read)
📜 Full session transcript

Pushed `7d2d0ce` to **claude/issue-87**. ## The finding was right — and the freeze wasn't buying anything The reviewer flagged `MakeReadOnly()` on `AtProtoJsonDefaults.Options` as a real behavioural break: `AtProtoJsonDefaults.Options.Converters.Add(myConverter)` at startup is the natural extension point for a public static `JsonSerializerOptions`, and it started throwing. Rather than reclassify it as a breaking change in the CHANGELOG, I removed the freeze — because the perf justification for it doesn't hold up. `JsonSerializer` calls `MakeReadOnly()` itself on first use, so pre-freezing only skips one bool check on the very first serialization; the comment claiming it avoids "re-checking mutability on every call" was wrong. I measured it (2M serializations per round, warmed, .NET 10): ``` round 0: pre-frozen 450.7 ns/op | left mutable 204.4 ns/op <- JIT warmup on first Run() round 1: pre-frozen 201.8 ns/op | left mutable 197.0 ns/op round 2: pre-frozen 201.6 ns/op | left mutable 197.5 ns/op ``` No measurable difference. So the freeze cost a public extension point for nothing. What stays is the part of that bullet that was actually load-bearing: `Options` is still built by the type initializer instead of `??=`, which is what removes the concurrent-first-call race where two threads each build an options instance with its own reflection-derived contract cache. **Verified** with a throwaway console app referencing the built `ATProtoNet` project, exercising the exact pattern from the finding: ``` {"name":"ALICE"} <- startup-time Converters.Add now works again post-use mutation: throws, as on main <- unchanged pre-existing STJ behaviour ``` I did not add a unit test for this. Any test asserting the shared singleton is mutable would be order-dependent — the first test in the run that serializes with `AtProtoJsonDefaults.Options` freezes it for every test after, so such a test would pass or fail depending on collection scheduling. ## Changes - `src/ATProtoNet/Serialization/AtProtoJsonDefaults.cs` — dropped `options.MakeReadOnly()`; the `<remarks>` now records why the instance is deliberately left unfrozen and points consumers who need permanent customisation at passing their own `JsonSerializerOptions` to the client constructors. - `CHANGELOG.md` — that bullet no longer claims a freeze/fast-path win and states explicitly that startup-time `Converters.Add(...)` keeps working. It stays under `Changed`; with the freeze gone there is no behavioural break to migrate from, so `Breaking changes` would now be inaccurate. Build clean (0 warnings, 0 errors) and unit tests green: **1296 passed, 0 failed, 5 skipped**. The rest of the review was confirmations of correct rewrites, so nothing else needed action. --- ⏱ 3m 34s · 24 turns · tokens: 34.5k in / 10.4k out (+954.8k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-88/run-256.md)

claude-bot commented

2026-08-16 01:06:55 +00:00

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/258); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-88/run-258.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-08-16 01:08:43 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Re-review: the only issue raised in my previous pass — the concern that AtProtoJsonDefaults.Options was now frozen via MakeReadOnly() at type-init time — is resolved, and on closer inspection was actually a misreading on my part rather than a real regression. Looking at the current source, CreateOptions() never calls MakeReadOnly(); JsonSerializerOptions only becomes read-only implicitly on first use by JsonSerializer (true both before and after this PR, lazy or eager construction doesn't change that). The new remarks block on Options correctly documents this ("deliberately not frozen here ... a startup-time AtProtoJsonDefaults.Options.Converters.Add(...) keeps working"), so the CHANGELOG's "no public behaviour changes" framing is accurate and doesn't need to move to Breaking changes.

Spot-checked the rest of the diff again for correctness on top of my last pass:

Did.Method slicing is still correct for did:web identifiers with extra colons.
CarReader.FromStreamAsync passes ms.GetBuffer().AsSpan(...) into FromBytes(ReadOnlySpan<byte>), which can't retain a span past the call, so there's no use-after-dispose risk from skipping the ToArray() copy.
CarReader.FindBlock's deferred index and duplicate-CID first-wins semantics are unchanged and now have direct test coverage (FindBlock_RepeatedLookupsResolveEveryBlock, FindBlock_DuplicateCid_ReturnsFirstOccurrence).
The new JetstreamEventParser.ParseFrame(ReadOnlyMemory<byte>, ...) overload uses the real JsonDocument.Parse(ReadOnlyMemory<byte>, ...) API, and the span overload now correctly delegates to it after a single ToArray() (rather than each overload separately copying). New tests cover span/memory parity, malformed-frame handling, and that the parsed Record survives the caller mutating its buffer afterward.
FirehoseVerifier.WriteMapHeader/MapHeaderLength and the FirehoseClient/JetstreamClient single-receive fast paths preserve prior null/empty-message semantics, with a correct multi-frame fallback.

No new correctness issues found. Good to merge.

⏱ 1m 38s · 19 turns · tokens: 46.5k in / 7.7k out (+1295.4k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: the only issue raised in my previous pass — the concern that `AtProtoJsonDefaults.Options` was now frozen via `MakeReadOnly()` at type-init time — is resolved, and on closer inspection was actually a misreading on my part rather than a real regression. Looking at the current source, `CreateOptions()` never calls `MakeReadOnly()`; `JsonSerializerOptions` only becomes read-only implicitly on first use by `JsonSerializer` (true both before and after this PR, lazy or eager construction doesn't change that). The new remarks block on `Options` correctly documents this ("deliberately not frozen here ... a startup-time `AtProtoJsonDefaults.Options.Converters.Add(...)` keeps working"), so the CHANGELOG's "no public behaviour changes" framing is accurate and doesn't need to move to Breaking changes. Spot-checked the rest of the diff again for correctness on top of my last pass: - `Did.Method` slicing is still correct for `did:web` identifiers with extra colons. - `CarReader.FromStreamAsync` passes `ms.GetBuffer().AsSpan(...)` into `FromBytes(ReadOnlySpan<byte>)`, which can't retain a span past the call, so there's no use-after-dispose risk from skipping the `ToArray()` copy. - `CarReader.FindBlock`'s deferred index and duplicate-CID first-wins semantics are unchanged and now have direct test coverage (`FindBlock_RepeatedLookupsResolveEveryBlock`, `FindBlock_DuplicateCid_ReturnsFirstOccurrence`). - The new `JetstreamEventParser.ParseFrame(ReadOnlyMemory<byte>, ...)` overload uses the real `JsonDocument.Parse(ReadOnlyMemory<byte>, ...)` API, and the span overload now correctly delegates to it after a single `ToArray()` (rather than each overload separately copying). New tests cover span/memory parity, malformed-frame handling, and that the parsed `Record` survives the caller mutating its buffer afterward. - `FirehoseVerifier.WriteMapHeader`/`MapHeaderLength` and the `FirehoseClient`/`JetstreamClient` single-receive fast paths preserve prior null/empty-message semantics, with a correct multi-frame fallback. No new correctness issues found. Good to merge. --- ⏱ 1m 38s · 19 turns · tokens: 46.5k in / 7.7k out (+1295.4k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-88/run-258.md)

Grandiras merged commit 9e7877e6fc into main

2026-08-16 01:18:09 +00:00

Grandiras deleted branch claude/issue-87

2026-08-16 01:18:09 +00:00

Grandiras referenced this pull request from a commit

2026-08-16 01:18:11 +00:00

Claude: changes for issue #87 (#88)

Sign in to join this conversation.

No reviewers