Build the signing path's JsonSerializerOptions once - #147
Merged
Conversation
ObjectToJsonNode constructed a fresh JsonSerializerOptions on every call, and
every signing operation goes through it - Encode, EncodeForSigning,
EncodeForSigningClaim and EncodeForMultiSigning all route there. LOVault.ToHex
did the same, on a much colder path.
Measured end to end on EncodeForSigning, 50 000 calls, best of five rounds:
before 1075.8 ms 14458 B/op
after 621.8 ms 13601 B/op
1.73x, and 857 fewer bytes per call. The output is unchanged: the encoded blob
hashes to E9DE857C47B2CDDD0845344D0A9C4892C91A1A41051FD7A692EFFA6C4A46B47D
before and after, and the 28 existing assertions against known-good hex across
six BinaryCodec test files still pass.
Worth stating what this is not, because the usual telling of this bug oversells
it. Since .NET 8 System.Text.Json shares a caching context between structurally
equal options instances, so type metadata was not being rebuilt per call - had
it been, the gap would be orders of magnitude rather than 1.7x. What was paid
was an allocation and a structural-equality lookup in that shared pool, which
is capped at 64 contexts and no longer leaned on here.
The instances are only ever read, and System.Text.Json freezes an options
object on first use, so sharing them across threads is safe.
Self-review of the previous commit. <see cref="Encode"/> matched two overloads, which the compiler reported as CS0419 - a warning this change introduced. Pinned to Encode(object), the one ObjectToJsonNode is actually reached from. The remarks quoted the isolated benchmark - 1.61x on a serialize call in a scratch project - where the number that characterises this code is the end-to- end one on EncodeForSigning: 1.73x and 857 bytes. A comment outlives the pull request it was written alongside, so it should carry the figure that describes the path it sits on. Also split a sentence that ran "since ... so ..." into two, and recorded why the modest size of the gap is itself the evidence that metadata was not being rebuilt.
Review finding, checked against the source rather than taken on trust: JsonSerializerOptions.Caching.cs on release/7.0 already carries TrackedCachingContexts with MaxTrackedContexts = 64 and an EqualityComparer over structural equality. The later PR rewrote that mechanism, it did not introduce it. Nothing about the argument changes - metadata still was not being rebuilt per call, and the modest size of the measured gap remains the evidence for that. Only the version is wrong, and it is wrong in a comment, which is where a wrong fact does the most quiet damage. Corrected in the remarks added by this branch and in JsonSerializerOptionsCache, which has carried the same claim since it was written. Same sentence, same error, no reason to leave one of them standing.
Platonenkov
added a commit
that referenced
this pull request
Aug 27, 2026
…ed it Release preparation for 27/08, found by checking what actually changed since 11.0.0.0 rather than by looking at this branch alone. Xrpl.BinaryCodec/XrplBinaryCodec.cs changed in #147 and the package version did not. Promoting that way publishes nothing: dotnet nuget push runs with --skip-duplicate, so a package whose version already exists on the feed is passed over in silence, and the fix reaches no consumer while the run stays green. Moved to 11.0.1.0 - a performance fix with no contract change, so patch. The same PR left no CHANGES.md entry. A 1.73x change on the path every signing operation takes is not a silent one, so it has one now, with the measurement and with why the usual telling of that bug oversells it. Xrpl stays at 11.1.0.0: this release carries a contract change, since code that read an out-of-range amount used to get a number and now gets an exception. AddressCodec, Keypairs and both X402 packages are untouched and keep their versions - they are consumed by ProjectReference, so a package built at a newer version keeps depending on the published ones. CHANGES.md still opens with "## Unreleased". Stamping it belongs to the promotion, when the date is known.
This was referenced Aug 27, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ObjectToJsonNodebuilt a freshJsonSerializerOptionson every call, and every signing operation goes through it —Encode,EncodeForSigning,EncodeForSigningClaimandEncodeForMultiSigningall route there.LOVault.ToHexdid the same on a much colder path.This is what
CA1869warns about; the analyzer is not enabled here, so nothing caught it.Measured, end to end
EncodeForSigningon a payment, 50 000 calls, best of five rounds, against the real code path rather than an isolated snippet:1.73x, and 857 fewer bytes per call.
Output is unchanged
The point of a signing path is that its bytes are exact, so that is what was checked rather than assumed. The encoded blob hashes identically before and after:
The 28 existing assertions against known-good hex across six
Xrpl.BinaryCodec.Testfiles pass unchanged, and they are what would catch a serialization difference.What this is not
The usual telling of this bug is that each new options instance starts with an empty cache and forces System.Text.Json to rebuild type metadata. Since .NET 8 that is not what happens: structurally equal options instances share a caching context, so metadata was not being rebuilt — had it been, the gap would be orders of magnitude rather than 1.7x, and the measurement above is itself the evidence.
What was actually paid per call is an allocation plus a structural-equality lookup in that shared pool. The pool is also capped at 64 contexts, which this no longer leans on.
The same reasoning is already written down in
JsonSerializerOptionsCache, which solved the harder version of this problem — polymorphic converters re-entering the serializer with their own converter stripped, once per converted value.Thread safety
Both instances are only ever read, and System.Text.Json freezes an options object on first use, so sharing them across threads is safe.
Not in scope
CA1869could be enabled to stop a third instance of this appearing. Left out deliberately — it is a repo-wide analyzer setting rather than part of this fix, and worth deciding on its own.