perf: parse DID document entries once per document #203

Merged

Grandiras merged 1 commit from issue-200-identity-perf into main 2026-10-05 12:31:14 +00:00

Conversation 8 Commits 1 Files changed 5 +54 -55

Grandiras commented 2026-10-05 02:31:44 +00:00

Owner

Copy link

Closes #200.

DidDocument lookups now parse and decode each entry once per (immutable, shared) document. Every property they read is init-only, so the caches cannot go stale.

• DidDocumentService caches its parsed http(s) endpoint, and VerificationMethod its did:key. The strict TryGetVerificationKey remembers that it decoded the key, so ParseDidKey runs once per method; failures are not cached, so errors behave as before.
• The lookups use indexed loops, so no enumerator is boxed per call.
• GetHandle validates once via Handle.TryCreate and still rejects at://@….
• Dead null branches are removed from the two document converters, along with the unused Handle.IsValidSyntax. PlcClient drops the invariant-culture formatting of non-negative integers.

Benchmark	Baseline	Candidate
GetPdsEndpoint	83.9 ns, 96 B	4.0 ns, 0 B
GetSigningKey	17.4 ns, 176 B	7.2 ns, 0 B
TryGetVerificationKey("#atproto")	53.8 µs, 984 B	6.6 ns, 0 B
GetHandle	38.7 ns, 112 B	20.3 ns, 72 B
Parse document	1125 ns, 1736 B	1134 ns, 1760 B

TryGetVerificationKey sits on every signature check against a cached document (service auth, firehose commit verification).

Net src: −14 lines. Build has 0 warnings; tests pass.

🤖 Generated with Claude Code

Closes #200. `DidDocument` lookups now parse and decode each entry once per (immutable, shared) document. Every property they read is `init`-only, so the caches cannot go stale. - `DidDocumentService` caches its parsed http(s) endpoint, and `VerificationMethod` its `did:key`. The strict `TryGetVerificationKey` remembers that it decoded the key, so `ParseDidKey` runs once per method; failures are not cached, so errors behave as before. - The lookups use indexed loops, so no enumerator is boxed per call. - `GetHandle` validates once via `Handle.TryCreate` and still rejects `at://@…`. - Dead null branches are removed from the two document converters, along with the unused `Handle.IsValidSyntax`. `PlcClient` drops the invariant-culture formatting of non-negative integers. | Benchmark | Baseline | Candidate | |---|---|---| | `GetPdsEndpoint` | 83.9 ns, 96 B | 4.0 ns, 0 B | | `GetSigningKey` | 17.4 ns, 176 B | 7.2 ns, 0 B | | `TryGetVerificationKey("#atproto")` | 53.8 µs, 984 B | 6.6 ns, 0 B | | `GetHandle` | 38.7 ns, 112 B | 20.3 ns, 72 B | | Parse document | 1125 ns, 1736 B | 1134 ns, 1760 B | `TryGetVerificationKey` sits on every signature check against a cached document (service auth, firehose commit verification). Net `src`: −14 lines. Build has 0 warnings; tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-10-05 02:31:44 +00:00

perf: DID document lookups parse and decode once per document (closes #200) ... 11fecd366f

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

opencode-bot commented 2026-10-05 02:31:45 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — the review will link the full session transcript.

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

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-203/run-2026-10-05T02-31-45-046Z.md) — updating every ~5 minutes while the review runs.

opencode-reviewer approved these changes 2026-10-05 02:52:33 +00:00

Dismissed

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

The caches are safe and behavior is preserved. I verified the two externally-checkable claims: Utf8JsonWriter.WriteStringValue(null) writes the JSON null value, so the rewritten StringOrNullConverter.Write is output-identical; and NumberFormatInfo.NativeDigits has no effect on formatting (only Basic Latin digits are used, and default "G" integer formatting emits no group separators), so the "non-negative integers format the same under every culture" comment in PlcClient is correct — and the values are validated non-negative anyway, so the culture-dependent negative sign can't appear.

Other checks: the at://@ pre-check in GetHandle reproduces exactly what the old IsValidSyntax-then-TryParse ordering rejected (single-@ stripping vs. syntax check), and the first-at://-entry-decides rule is unchanged. The removed JsonTokenType.Null case in StringListConverter.Read was genuinely dead code: without HandleNull => true the serializer handles null tokens itself, as the new comment states; NonNullListConverter correctly keeps HandleNull. TryGetVerificationKey still distinguishes absent/malformed, failures are still not cached (exceptions in MultibaseToBytes/ParseDidKey leave nothing cached, so errors repeat as before), and the _didKey/HttpEndpoint/KeyChecked caches derive only from init-only properties, so they cannot go stale. Null entries in hand-built lists are still skipped.

Minor note, no action needed: the lazy caches introduce a benign data race when a document is shared across threads — concurrent first calls may each compute the value — but reference/bool writes are atomic and the values are deterministic (idempotent lazy init), so this is fine as a perf cache.

---

⏱ 20m 48s · 12 steps · $0.0423 · tokens: 158.2k in / 1.3k out (+239.2k cache-read) · Go: 5h 0% · wk 0% · mo 91%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. The caches are safe and behavior is preserved. I verified the two externally-checkable claims: `Utf8JsonWriter.WriteStringValue(null)` writes the JSON `null` value, so the rewritten `StringOrNullConverter.Write` is output-identical; and `NumberFormatInfo.NativeDigits` has no effect on formatting (only Basic Latin digits are used, and default "G" integer formatting emits no group separators), so the "non-negative integers format the same under every culture" comment in `PlcClient` is correct — and the values are validated non-negative anyway, so the culture-dependent negative sign can't appear. Other checks: the `at://@` pre-check in `GetHandle` reproduces exactly what the old `IsValidSyntax`-then-`TryParse` ordering rejected (single-`@` stripping vs. syntax check), and the first-`at://`-entry-decides rule is unchanged. The removed `JsonTokenType.Null` case in `StringListConverter.Read` was genuinely dead code: without `HandleNull => true` the serializer handles null tokens itself, as the new comment states; `NonNullListConverter` correctly keeps `HandleNull`. `TryGetVerificationKey` still distinguishes absent/malformed, failures are still not cached (exceptions in `MultibaseToBytes`/`ParseDidKey` leave nothing cached, so errors repeat as before), and the `_didKey`/`HttpEndpoint`/`KeyChecked` caches derive only from `init`-only properties, so they cannot go stale. Null entries in hand-built lists are still skipped. Minor note, no action needed: the lazy caches introduce a benign data race when a document is shared across threads — concurrent first calls may each compute the value — but reference/bool writes are atomic and the values are deterministic (idempotent lazy init), so this is fine as a perf cache. --- ⏱ 20m 48s · 12 steps · $0.0423 · tokens: 158.2k in / 1.3k out (+239.2k cache-read) · Go: 5h 0% · wk 0% · mo 91% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-203/run-2026-10-05T02-31-45-046Z.md) <!-- opencode-reviewed-head:11fecd366f03b2c391fe3e54c783517614fc3ea9 --> <!-- opencode-verdict:approve -->

Grandiras force-pushed issue-200-identity-perf from 11fecd366f to 5b47094bb9 2026-10-05 02:58:17 +00:00 Compare

Grandiras force-pushed issue-200-identity-perf from 5b47094bb9 to 6c1d0ef9bc 2026-10-05 04:14:23 +00:00 Compare

opencode-bot commented 2026-10-05 04:15:24 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — the review will link the full session transcript.

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

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-203/run-2026-10-05T04-15-23-786Z.md) — updating every ~5 minutes while the review runs.

opencode-reviewer approved these changes 2026-10-05 04:16:08 +00:00

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Re-review: no blocking issues; the approved state stands unchanged. Re-verified against the current sources:

• The KeyChecked flag is set only after ParseDidKey succeeds, so failed decodes stay uncached and repeat exactly as before; statuses (Absent/Malformed/Found) and the first-entry-decides rule for both services and verification methods are unchanged (the type mismatch still returns Malformed before the endpoint is consulted).
• The _didKey and HttpEndpoint caches derive only from init-only properties, so they cannot go stale on the immutable, shared document; a ToDidKey() result of null is not cached by ??=, which is the correct no-value semantics.
• GetHandle's at://@ pre-check reproduces the old IsValidSyntax rejection of an @-prefixed claim, and the existing theory matrix covers it; the new repeated-lookup test (Assert.Same on the memoized Uri, repeated TryGetVerificationKey, agreement with GetSigningKey) directly exercises the caches.
• The converter cleanups are output-identical: WriteStringValue(null) emits JSON null; the null token case in StringListConverter.Read was genuinely dead (HandleNull unset means the serializer handles the nullable type itself), while NonNullListConverter correctly keeps HandleNull => true and its malformed-entry exception.
• The PlcClient interpolation change is behavior-preserving: non-negative longs (all three are validated non-negative) format identically across cultures.

Minor non-blocking note carried over from the previous pass: the lazy caches are idempotent non-atomic — concurrent first reads on a shared document may each compute the value once — but the writes are atomic and the values deterministic, so this is benign for a perf cache.

---

⏱ 0m 44s · 23 steps · $0.0157 · tokens: 33.4k in / 2.2k out (+261.0k cache-read) · Go: 5h 1% · wk 0% · mo 91%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review: no blocking issues; the approved state stands unchanged. Re-verified against the current sources: - The `KeyChecked` flag is set only after `ParseDidKey` succeeds, so failed decodes stay uncached and repeat exactly as before; statuses (`Absent`/`Malformed`/`Found`) and the first-entry-decides rule for both services and verification methods are unchanged (the `type` mismatch still returns `Malformed` before the endpoint is consulted). - The `_didKey` and `HttpEndpoint` caches derive only from `init`-only properties, so they cannot go stale on the immutable, shared document; a `ToDidKey()` result of `null` is not cached by `??=`, which is the correct no-value semantics. - `GetHandle`'s `at://@` pre-check reproduces the old `IsValidSyntax` rejection of an `@`-prefixed claim, and the existing theory matrix covers it; the new repeated-lookup test (`Assert.Same` on the memoized `Uri`, repeated `TryGetVerificationKey`, agreement with `GetSigningKey`) directly exercises the caches. - The converter cleanups are output-identical: `WriteStringValue(null)` emits JSON `null`; the `null` token case in `StringListConverter.Read` was genuinely dead (`HandleNull` unset means the serializer handles the nullable type itself), while `NonNullListConverter` correctly keeps `HandleNull => true` and its malformed-entry exception. - The `PlcClient` interpolation change is behavior-preserving: non-negative `long`s (all three are validated non-negative) format identically across cultures. Minor non-blocking note carried over from the previous pass: the lazy caches are idempotent non-atomic — concurrent first reads on a shared document may each compute the value once — but the writes are atomic and the values deterministic, so this is benign for a perf cache. --- ⏱ 0m 44s · 23 steps · $0.0157 · tokens: 33.4k in / 2.2k out (+261.0k cache-read) · Go: 5h 1% · wk 0% · mo 91% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-203/run-2026-10-05T04-15-23-786Z.md) <!-- opencode-reviewed-head:6c1d0ef9bc65ebabefee805cb08a9cd79d8110cb --> <!-- opencode-verdict:approve -->

Grandiras commented 2026-10-05 11:49:29 +00:00

Author

Owner

Copy link

@claude /review

@claude /review

claude-bot commented 2026-10-05 11:49:36 +00:00

Collaborator

Copy link

🤖 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/730); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-203/run-730.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-10-05 11:50:01 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

The change looks correct and I'd merge it.

• Caches: _didKey, _endpoint and KeyChecked all derive from init-only properties. Concurrent first calls can race, but they all compute the same value, so the race is benign. Failures are not cached, so errors behave as before.
• GetHandle: I checked Handle.TryCreate. It strips a leading @, so the explicit at://@ rejection preserves the old behaviour. The text argument of null makes it take the string from the span.
• Converter cleanup: System.Text.Json handles null tokens itself unless HandleNull is set, so removing the null branches and HandleNull is safe. The value! in the list converter's Write is fine for the same reason.
• PlcClient: after is a long checked non-negative and count is an int?, so culture-sensitive formatting cannot change the output.
• Tests and changelog: The new test covers repeated lookups on a shared document, and the CHANGELOG entry is in place.

Minor nit: the CHANGELOG entry doesn't list the GetHandle allocation change as a separate bullet, but it is mentioned in the same line.

---

🧠 claude-sonnet-5-5 · ⏱ 0m 13s · 4 turns · tokens: 18.8k in / 1.3k out (+70.7k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. The change looks correct and I'd merge it. - **Caches:** `_didKey`, `_endpoint` and `KeyChecked` all derive from `init`-only properties. Concurrent first calls can race, but they all compute the same value, so the race is benign. Failures are not cached, so errors behave as before. - **`GetHandle`:** I checked `Handle.TryCreate`. It strips a leading `@`, so the explicit `at://@` rejection preserves the old behaviour. The `text` argument of `null` makes it take the string from the span. - **Converter cleanup:** System.Text.Json handles null tokens itself unless `HandleNull` is set, so removing the null branches and `HandleNull` is safe. The `value!` in the list converter's `Write` is fine for the same reason. - **`PlcClient`:** `after` is a `long` checked non-negative and `count` is an `int?`, so culture-sensitive formatting cannot change the output. - **Tests and changelog:** The new test covers repeated lookups on a shared document, and the CHANGELOG entry is in place. Minor nit: the CHANGELOG entry doesn't list the `GetHandle` allocation change as a separate bullet, but it is mentioned in the same line. --- 🧠 claude-sonnet-5-5 · ⏱ 0m 13s · 4 turns · tokens: 18.8k in / 1.3k out (+70.7k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-203/run-730.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:6c1d0ef9bc65ebabefee805cb08a9cd79d8110cb -->

Grandiras force-pushed issue-200-identity-perf from 6c1d0ef9bc to 661d4a0371 2026-10-05 12:09:12 +00:00 Compare

Grandiras commented 2026-10-05 12:31:13 +00:00

Author

Owner

Copy link

Approval carry-over: this head (661d4a0) is a CHANGELOG-only rebase of the approved 6c1d0ef. The PR's patch against its merge base, excluding CHANGELOG.md, has the same git patch-id (bd21783c), and CI is green on the new head. opencode-reviewer and claude-reviewer both approved 6c1d0ef.

Approval carry-over: this head (661d4a0) is a CHANGELOG-only rebase of the approved 6c1d0ef. The PR's patch against its merge base, excluding `CHANGELOG.md`, has the same `git patch-id` (bd21783c), and CI is green on the new head. opencode-reviewer and claude-reviewer both approved 6c1d0ef.

Grandiras merged commit ce4906dbbf into main 2026-10-05 12:31:14 +00:00

Grandiras deleted branch issue-200-identity-perf 2026-10-05 12:31:15 +00:00

Grandiras referenced this pull request from a commit 2026-10-05 12:31:16 +00:00

perf: parse DID document entries once per document (#203)

Sign in to join this conversation.

Reviewers

No reviewers

opencode-reviewer

claude-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

5 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!203

No description provided.