perf(events): encode events with a hand-written JSON writer (tier 1) - #530
abelonogov-ld wants to merge 24 commits into
Conversation
…igest ContextSummarizer keyed its per-context trackers by LDContext.contextHash(), which encodes the context to JSON and takes a SHA-256 of the result. That is the right price for the flag cache, which persists the digest and compares it across launches, but a bucketing key does not need to cost it — and an application that reads a flag on every redraw paid it on every redraw. Measured on the recording benchmark, an evaluation that only summarizes falls from 24.92 µs to 3.29 µs. The key is now the context itself: it hashes fullyQualifiedKey(), a string the context already holds, and lets Equatable break ties. Contexts sharing a canonical key but differing elsewhere collide in the hash and separate in ==, so the Hashable contract holds. It also clears redactAnonymousAttributes when building the key. That flag says how a context is encoded for output rather than which context it is, but Swift synthesizes == across every stored property, so leaving it set would let an encoding concern split one context into two summary buckets. The Java SDK's LDContext.equals has no such field, so clearing it is also what keeps Apple and Android agreeing on which evaluations belong to the same summary. This is more correct than what it replaces. Keying by a digest meant a collision would silently merge two contexts' summaries; comparing the contexts themselves cannot. Co-authored-by: Cursor <cursoragent@cursor.com>
The event path is the one place the SDK encodes often enough for Codable's reflection to show up in a profile. This writes the wire form directly into a byte buffer instead, which is what the Android SDK already does, and caches the encoded context so that a run of evaluations -- nearly always the same context over and over -- encodes it once. Getting there needed one change in the model. A feature event used to redact its context by deep-copying LDContext and setting a flag on the copy, which cost an allocation per event and defeated the buffer-identity comparison that makes a context-keyed cache affordable. Redaction is a property of the event rather than of the context -- one context redacts on a feature event and does not on the debug event beside it -- so it now lives on Event, and the context is held as the caller gave it. JSONEncoder is kept as the oracle: EventJSONWriterTests asserts the two produce identical bytes for every event shape, and where they disagree Codable is right. The reporter's encoder is also built once rather than per delivery, and one JSONWriter covers a whole run so its buffer is reused across the events in it. Co-authored-by: Cursor <cursoragent@cursor.com>
The redaction test built a fresh event per encoding and compared the whole JSON, so two events either side of a millisecond boundary disagreed on creationDate and the assertion failed on a field it has nothing to say about. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the reverse-digit integer path; String(value).utf8 is 3× slower. Co-authored-by: Cursor <cursoragent@cursor.com>
The counterpart of the Android benchmark of the same name, and deliberately so: same context shapes, same privacy settings, same run length, same warm-then-fastest-of-five aggregation. Run this in the Simulator and that one in an emulator and both execute on the host Mac's cores, which is the only way the two platforms' encoders have been compared without a difference in silicon sitting in the middle of the answer. It also prints the byte count beside every row, because only the shapes whose output matches on both platforms can be quoted across them, and that is worth checking rather than assuming. Opt-in, and awkwardly so: the environment variable has to come from the scheme's Test action, which xcodebuild's own environment does not reach. The doc comment says how, rather than leaving it set in the shared scheme where it would add twenty-five seconds to every CI run. Co-authored-by: Cursor <cursoragent@cursor.com>
| hooksLock.lock() | ||
| defer { hooksLock.unlock() } | ||
| return storedHooks | ||
| return Array(storedHooks) |
There was a problem hiding this comment.
Aren't there existing JSON writer libraries that can be used to get the performance of direct writing, but that don't require us to re-implement JSON syntax handling?
There was a problem hiding this comment.
I reused unit tests from those libraries, not libraries itself
| } | ||
| } | ||
|
|
||
| private func appendEscape(_ byte: UInt8) { |
There was a problem hiding this comment.
What spec is this following?
| while magnitude > 0 { | ||
| bytes.append(UInt8(ascii: "0") + UInt8(magnitude % 10)) | ||
| magnitude /= 10 | ||
| } |
There was a problem hiding this comment.
Is this more performant than a community swift library for json writing? This feels like re-inventing the wheel.
And to be more clear, I'm referring to a library that lets you do json object writing piece by piece and not serializing whole classes via annotations/reflection etc...
There was a problem hiding this comment.
I reused unit tests from those libraries, not libraries itself. All edge cases should be covered
| self.canonicalizedKey = canonicalizedKey | ||
| } | ||
|
|
||
| internal init(copyFrom: LDContext) { |
There was a problem hiding this comment.
replaced by real struct copy, manual copying is slow
| /// which is what makes a context-keyed cache affordable. | ||
| /// | ||
| /// Only the top-level flag is ever read. `encode(to:)` passes the parent's value down to every sub-context and | ||
| /// ignores whatever theirs holds, so the sub-contexts do not need rewriting here. |
There was a problem hiding this comment.
Why does this comment talk about encode(to:) passes the parent's value down to every sub-context ? That seems not relevant to this function and is relevant in the encode commenting.
There was a problem hiding this comment.
I removed this method and it only applies to Codable which now plays only in unit tests
| /// The hand-written counterpart to `LDContext.encode(to:)`, for the encoding experiment behind `EventJSONWriter`. | ||
| /// | ||
| /// The field set, the omissions, and the redaction rules are deliberately identical to the `Codable` path; where the | ||
| /// two disagree, the `Codable` path is right and this is wrong. `EventJSONWriterTests` asserts they agree. |
There was a problem hiding this comment.
This AI comment needs to be improved. It shouldn't talk about development history and such.
There was a problem hiding this comment.
I think there are a good number of context serialization contract tests in the sdk test harness. It may be worth pointing AI at the sdk-test-harness to find those cases to get the same amount or more of case coverage.
|
|
||
| /// Integral values are written without a fractional part, which is what `JSONEncoder` does and what the service | ||
| /// already receives. Anything else takes Swift's shortest round-trip description, which is valid JSON. | ||
| func write(_ value: Double) { |
There was a problem hiding this comment.
Should we really be implementing this when community libs have this? Same for the other primitive types.
There was a problem hiding this comment.
Should be handled by reusing unit tests from other libraries
There was a problem hiding this comment.
Attribute references tends to be a tricky aspect of this sort of logic. How are you confirming you have proper attribute reference handling?
tanderson-ld
left a comment
There was a problem hiding this comment.
Ran the multi-agent review battery over this branch (~195k differential-fuzz comparisons against the JSONEncoder oracle, plus TSan stress runs). Redaction parity, cache slot correctness, escaping, and flush concurrency all came back clean. High/Medium findings below, summarized — I have failing proof tests for the two Highs and a longer Low/informational list if useful.
High
- Duplicate
_metamember. An attribute literally named_metais accepted bytrySetValue(java-core rejects it as reserved) and emitted by the attribute loop, then the redaction block emits the SDK's_metaagain — two members in one object.Codablemasked this (repeat keys replace); first-wins parsers now read the app's value and dropredactedAttributes, and an app-supplied_metathat never left the device now ships. Suggest rejecting_metain the builder + adding it to the test corpus. - Summarizer key hashes only the canonical key (
ContextSummarizer.ContextKey), so same-key/different-attribute contexts pile into one probe chain of deepLDContext ==walks — synchronous on thevariation()caller's thread, growing until a successful flush. Measured 57x per-insert at N=2000 (linear growth, crossover ≈ N=30) via the mundane re-identify-with-a-changing-attribute pattern. Count-based hash discriminators won't fix it; needs attribute values in the hash or a bounded/digest fallback. - The
fix crashcommit (e70cb42a) looks like three inert edits — theData/Arrayaliasing its comments describe doesn't reproduce, theLDClient.hooksArray()copy fixes nothing (and adds a per-evaluation allocation on the hottest path), and thetakeStageschange is identical semantics. If so, the actual crash is still unexplained — can you share the crash log and whether it reproduces with theJSONWriterchange alone?
Medium
- Cache can serve wrong
_meta.redactedAttributesspellings for multi-contexts —spellings(of:)flattens+sorts across sub-contexts, so per-part spelling permutations collide. No value leak; fix is sorting per context and concatenating incontextsorder. - Non-finite doubles become
null;"metricValue": nullviolates the v4.0 custom-event schema (and diverges from Android, which drops). Omit the key for non-finite metrics; haveunserializableEventSpecassert payload bytes. - Integral doubles in
[1e17, 9.22e18]diverge fromJSONEncodereven after canonicalization — adding1e17totestNumberFormattingfails today. Widen the fast-path guard tomagnitude < 1e17or document it (and restate the "identical bytes" claim as JSON-equality;/-escaping and-0.0also differ at byte level). - No runtime escape hatch back to
.codable— the default wire path has no config switch, so the field remedy for any divergence is a release. - The shipping configuration (
.handWrittenCachingContext+ anonymous context + privacy config) is never exercised at reporter level — the reporter-level encoder comparison uses.codablevs.handWritten, and reporter specs use a non-anonymous stub. One mixed-batch reporter test on the default encoding would close it.
| private let encoder: JSONEncoder | ||
|
|
||
| let encoding: Encoding | ||
| private let handWrittenEncoder: EventJSONWriter |
There was a problem hiding this comment.
| private let handWrittenEncoder: EventJSONWriter | |
| private let jsonWriter: EventJSONWriter |
| } else if value.isFinite { | ||
| bytes.append(contentsOf: String(value).utf8) | ||
| } else { | ||
| bytes.append(contentsOf: JSONWriter.nullBytes) |
There was a problem hiding this comment.
NaN/±Inf silently become null here, where the Codable path throws (which today drops the whole event batch via try? encode). The parity tests can't cover this case since JSONEncoder refuses these values — worth an explicit decision either way.
There was a problem hiding this comment.
Matching with Codable for now.
| // Matches `Meta.isEmpty` and `Meta.encode`: `_meta` is written whenever either list is non-empty, but | ||
| // `privateAttributes` is only included when the caller asked for it, which the event path never does. A context | ||
| // with private attributes and nothing redacted therefore writes an empty `_meta`, as it does today. | ||
| if !privateAttributes.isEmpty || !redactedAttributes.isEmpty { |
There was a problem hiding this comment.
trySetValue allows a custom attribute literally named _meta, which the loop above already wrote — this block then emits a second _meta key in the same object (RFC 8259 says receiver behavior is unpredictable). The Codable path collapses to one key, last write wins.
| /// contexts that differ only in how a private attribute was spelled are `==` yet encode differently. `==` alone is | ||
| /// therefore not a sound key, and the spellings are compared separately. | ||
| /// | ||
| /// Contexts with no private attributes -- the common case -- settle this without allocating. |
There was a problem hiding this comment.
Isn't there a way to do this without allocating at all?
There was a problem hiding this comment.
Concretely: by the time this runs, entry.context == context has already passed, and == covers privateAttributes — so the two sets are equal as parsed references, and the only open question is spelling drift between equal members. Set.firstIndex(of:) returns the stored member, so the check can probe the cached set directly:
for ref in context.privateAttributes {
guard let i = entry.context.privateAttributes.firstIndex(of: ref),
entry.context.privateAttributes[i].raw() == ref.raw()
else { return false }
}
// then recurse pairwise over zip(entry.context.contexts, context.contexts)Hashed probe plus stored-string compare: no arrays, no sorted(), and Entry.spellings / spellings(of:) / the empty-set fast path all go away — store stops allocating too. This leans on Reference hashing by parsed components rather than rawPath, which its Hashable contract already requires given its ==.
There was a problem hiding this comment.
removed that logic, which I consider redundant and kept simple like in Android
|
|
||
| /// Counted only so the experiment can report a hit rate rather than assume one. | ||
| private(set) var hits = 0 | ||
| private(set) var misses = 0 |
There was a problem hiding this comment.
Were these just for debugging inspection?
Only the benchmark on andrey/event-store-sqlite reads them. On this branch they are dead state updated under the cache's lock on every lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
A single unserializable event -- a custom event with a non-finite metric value, say -- made the whole flush return nil, so every event queued with it was lost and the next flush hit the same wall. Encode event by event and skip the ones that fail, logging each at error level. Failures are dropped rather than put back: the writer cannot fail transiently, so a retried event would fail every later flush too.
The cache was a long-lived object behind a configuration flag and its own
lock. None of those three earned their keep:
- Encoding happens in one place, EventReporter.encode(_:), which already
builds a JSONWriter per batch. Giving the EventJSONWriter the same lifetime
means the cache is reached from one thread by construction, so the
UnfairLock goes. It was also not buying what it looked like: publish runs on
a global queue, so two flushes in flight shared one two-entry cache and
evicted each other.
- A batch is where the reuse is -- a run of evaluations on one context -- so
nothing is lost by not outliving one.
- With no lifetime to choose, there is no mode to choose either, so
cachingContexts and Encoding.handWrittenCachingContext go with it.
Redaction now writes _meta.redactedAttributes from Reference.canonical()
rather than the spelling the caller used. Reference equality is on parsed
components, so Reference("name") == Reference("/name"); writing the spelling
out meant two contexts could be equal and still encode differently, which the
cache had to close with a separate spellings comparison. Writing one spelling
closes it at the source instead, and both spellings are explicitly allowed in
redactedAttributes by the contract test harness.
The benchmark that measures the cache moves to andrey/event-store-sqlite with
the other benchmarks.
The writer and the JSONWriter it wrote into had to be created, scoped and kept off other threads together, but the caller was the one keeping them in step. Now that EventJSONWriter has a lifetime of its own -- a batch -- the buffer has the same one, so it lives inside and encode(_:) is the only entry point. encode(_:into:) goes, and write(_:into:) becomes private.
The private helpers took the JSONWriter as a parameter from when the caller supplied it. It is a stored property now, so the parameter only shadowed it.
Above 2^53 JSONEncoder switches to exponent form; the writer kept plain integers up to Int64 range, which parsed to the same value but broke the equal-JSON comparison from 1e17 up. Tests now assert equal JSON rather than identical bytes, and pin the known byte-level differences (/ and -0). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- Reject events holding NaN or infinite numbers, as JSONEncoder does, and never cache a context that failed to encode. - Write _meta in events only when something was redacted, as Android does, and reject _meta in the context builder. - Pass anonymous redaction as an encoding argument instead of a stored flag. - Serve equal but separately built contexts from the encoding cache. - Verify LDContextJSONWriter against the LDContextCodableSpec fixtures. - Rewrite docs to describe behavior rather than history. Co-authored-by: Cursor <cursoragent@cursor.com>
… is off Co-authored-by: Cursor <cursoragent@cursor.com>
tanderson-ld
left a comment
There was a problem hiding this comment.
Informally approving. Looking for Tier 1 spec approval.
| case ("anonymous", _): | ||
| return false | ||
| case ("_meta", _): | ||
| return false |
There was a problem hiding this comment.
You asked about it before and it mathches Java/Android
|
|
||
| if redactAll { | ||
| redaction.redactedAttributes.append(reference.raw()) | ||
| redaction.redactedAttributes.append(reference.canonical()) |
There was a problem hiding this comment.
Answering my own question after digging: this line itself is fine — canonical() here matches the Codable path exactly (both encoders changed together in 9af5db7), so they agree.
But there is an adjacent pre-existing bug in this loop, shared by both encoders: Reference(name) parses attribute names as attribute references. A name without a leading slash is safe (it parses as a single literal component), but an attribute literally named with a leading slash — trySetValue("/foo", …) stores one, and context JSON decoding can produce one too — parses as the path foo, so getValue finds nothing and the attribute is silently dropped from event output: not written, and not listed in redactedAttributes either. java-core doesn't have this bug (LDContext.getValue(String) is a literal-name lookup; only redaction matching goes through refs), so fixing it is also a parity gain.
Since it predates this PR and both encoders share it, a follow-up ticket is fine. That said, the fix looks bounded if you'd rather close it here: fetch the value by literal name in this loop instead of getValue(Reference(name)), emit the escaped literal spelling (/foo → /~1foo) in redactedAttributes under redact-all, mirror the same in encodeSingleContext, and add /foo- and a~b-named attributes to the parity corpus. Note it's a behavior change either way — attributes that previously vanished will start shipping (same class as the _meta fix already in this PR). If you'd rather defer, I'll file the ticket.
| let context: LDContext | ||
|
|
||
| func hash(into hasher: inout Hasher) { | ||
| hasher.combine(context.fullyQualifiedKey()) |
There was a problem hiding this comment.
This keying is the one High finding still open from my review pass. Hashing only fullyQualifiedKey() puts every distinct context that shares a key into one collision chain, and each probe through that chain is a full deep LDContext ==. Since trackRequest runs synchronously inside every variation() call (eventQueue.sync), and trackers only drain on a successful flush — never while offline — the per-evaluation cost grows linearly with the number of distinct same-key contexts seen. The trigger is mundane: re-identify the same key with one changing attribute (session id, screen name) and evaluate flags in between.
Measured: the old digest key was flat ~16µs/insert at any N; this is ~935µs/insert at N=2000 (57x, growing linearly; crossover ≈ N=30). I have a failing proof test that pins the shape if you want it.
To be fair on the spec: CSSE Req 1.1.1.3 explicitly allows "a hash code + equality system like commonly used with hash maps", so this is compliant — and this keying actually fixes the old contextHash() NaN fallback, which merged distinct contexts and violated the spec's MUST. The concern is purely the hot-path cost shape, not correctness.
Suggested fix: include the attribute values in the hash — Java's LDContext.hashCode() does exactly this (and notes the cost in a comment), so Android already pays it; that's both the flat-cost fix and cross-platform parity. Compute it once per trackRequest and store it in ContextKey (this path currently re-hashes the key for up to four separate dictionary operations). A sha256-style digest is also spec-blessed if you'd rather keep hash(into:) trivial. One caution: a bounded/capped tracker set is not a compliant alternative — CSSE Req 1.1.2 requires a summary per context that evaluated.
There was a problem hiding this comment.
It is a special key class for summarizatio not related to == of LDContext, summarizer doesn't use a full context
Using sha256 is overkill for hashing here, there is no
cryptography concerns here
There was a problem hiding this comment.
The key class is special, agreed — but it isn't key-only, and per the spec it can't be. CSSE Req 2.1.1 requires the accumulator to store the LDContext, and Req 2.1.3.1 requires the summary event to carry the event-filtered context — which this code does (TrackerWithContext.context). And Req 1.1.1.3 says the context-as-key mechanism "MUST account for all attributes of the context" — which your new == also does: equalsIgnoringWhichAttributesArePrivate walks the full attribute dictionary. So the summarizer very much uses the full context; the only things the key ignores are the two carve-outs:
- Ignoring which attributes are private: fine by the spec's letter (private markers live in
_meta, not attributes) and it's faithful v11 grouping. - The both-non-finite branch: this one does breach 1.1.1.3's MUST — two same-key contexts with entirely different attributes, each holding one NaN anywhere, merge into one accumulator. A NaN special case is genuinely needed for reflexivity (
.nan != .nan), but it can be narrower — NaN-aware value equality — rather than merging everything non-finite per key.
On sha256: agreed it's overkill, and that was never the ask. The spec names sha256 for the digest-only variant (collision probability, not secrecy) — since we keep == disambiguation, no digest is needed at all. The ask is just hash distribution: include the attribute values in hash(into:) (java-core's LDContext.hashCode() does exactly this), so same-key contexts stop forming a single probe chain. That cost shape is unchanged by the new == — the hash is still key-only, and a failing probe now does up to three attribute walks (the ignoring-private compare plus containsNonFiniteNumber() on both sides).
Separate note on maxContexts: Reqs 1.1.1.2/1.1.2 are unconditional — an accumulator per unique context, a summary per context that evaluated — so dropping evaluations at the cap is non-compliant. Forcing an early flush when the map grows would bound memory while still delivering the data; if we want a hard offline bound, that seems like an sdk-specs conversation first.
…g optimizations Redacted attributes are written as the application spelled them again, and the context encoding cache misses when two otherwise equal contexts spell a private attribute differently, rather than serving one spelling's bytes for the other. Summaries are grouped as keying by contextHash() did: of the private attributes only whether there are any counts, and contexts holding NaN or an infinity are grouped by fully qualified key. Previously a context holding NaN never matched its own tracker, so each evaluation added a summary until the next flush. Co-authored-by: Cursor <cursoragent@cursor.com>
…arizer.ContextKey == Co-authored-by: Cursor <cursoragent@cursor.com>
The per-context summarizer retains a context and its counters for every distinct context, and nothing clears it while the client is offline, so its memory grew for as long as the outage lasted. eventCapacity now bounds the number of contexts counted between deliveries, as on Android. Only a context not yet counted is turned away; the refused evaluation is counted into the diagnostics dropped total, and its full event is still decided by the event capacity. Co-authored-by: Cursor <cursoragent@cursor.com>
Group contexts by ==, which includes which attributes are private, as Android does. The full hash is stored on the key and the last key is reused for an equal context, and trackRequest does one dictionary lookup instead of three. Co-authored-by: Cursor <cursoragent@cursor.com>
Tier 1 of the mobile event durability work. This tier is the event buffer and nothing else: no persistence, no bounded flush. Those are tiers 2 and 3, which stack on this branch.
What this changes
The event path is the one place the SDK encodes often enough for
Codable's reflection to show up in a profile. This writes the wire form directly into a byte buffer instead — the same approach the Android SDK takes — and caches the encoded context, because a run of evaluations is nearly always the same context over and over.Getting there needed one change in the model. A feature event used to redact its context by deep-copying
LDContextand setting a flag on the copy. That cost an allocation per event, and it defeated the buffer-identity comparison that makes a context-keyed cache affordable:DictionaryandArrayequality check buffer identity before contents, so two contexts sharing storage compare equal in nanoseconds instead of by walking every attribute. Redaction is really a property of the event rather than of the context — one context redacts on a feature event and does not on the debug event beside it — so it now lives onEvent, and the context is held as the caller gave it.The summarizer keys its trackers by context rather than by a digest of one, for the same reason.
Why this is safe
JSONEncoderis kept as the oracle.EventJSONWriterTestsasserts the two encoders produce identical bytes for every event shape the SDK emits, and where they disagreeCodableis right. The reporter still exposes.codableso that comparison is a real switch rather than a claim.Notes for review
EventReporter's encoder is built once rather than per delivery, and oneJSONWritercovers a whole run so its buffer is reused across the events in it.LDContextgains a small internal seam (redactingAnonymousAttributes(),redactionDecision(...)) so the hand-written writer reuses the existing redaction logic rather than reimplementing it. The two encoders must differ in how bytes are produced, not in what gets redacted.Spec: Event Durability §3.1, §5.
Made with Cursor