Skip to content

perf(events): encode events with a hand-written JSON writer (tier 1) - #530

Open
abelonogov-ld wants to merge 24 commits into
v11from
andrey/event-durability-tier1-buffer
Open

abelonogov-ld wants to merge 24 commits into
v11from
andrey/event-durability-tier1-buffer

Conversation

@abelonogov-ld

Copy link
Copy Markdown
Contributor

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 LDContext and 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: Dictionary and Array equality 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 on Event, 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

JSONEncoder is kept as the oracle. EventJSONWriterTests asserts the two encoders produce identical bytes for every event shape the SDK emits, and where they disagree Codable is right. The reporter still exposes .codable so that comparison is a real switch rather than a claim.

Notes for review

  • EventReporter's encoder is built once rather than per delivery, and one JSONWriter covers a whole run so its buffer is reused across the events in it.
  • LDContext gains 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

abelonogov-ld and others added 2 commits September 18, 2026 17:31
…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>
@abelonogov-ld
abelonogov-ld requested a review from a team as a code owner September 19, 2026 01:07
@abelonogov-ld
abelonogov-ld marked this pull request as draft September 19, 2026 01:23
abelonogov-ld and others added 3 commits September 18, 2026 18:38
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>
@abelonogov-ld
abelonogov-ld marked this pull request as ready for review September 21, 2026 14:43
hooksLock.lock()
defer { hooksLock.unlock() }
return storedHooks
return Array(storedHooks)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reused unit tests from those libraries, not libraries itself

}
}

private func appendEscape(_ byte: UInt8) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What spec is this following?

while magnitude > 0 {
bytes.append(UInt8(ascii: "0") + UInt8(magnitude % 10))
magnitude /= 10
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reused unit tests from those libraries, not libraries itself. All edge cases should be covered

self.canonicalizedKey = canonicalizedKey
}

internal init(copyFrom: LDContext) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This AI comment needs to be improved. It shouldn't talk about development history and such.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really be implementing this when community libs have this? Same for the other primitive types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be handled by reusing unit tests from other libraries

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tanderson-ld left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Duplicate _meta member. An attribute literally named _meta is accepted by trySetValue (java-core rejects it as reserved) and emitted by the attribute loop, then the redaction block emits the SDK's _meta again — two members in one object. Codable masked this (repeat keys replace); first-wins parsers now read the app's value and drop redactedAttributes, and an app-supplied _meta that never left the device now ships. Suggest rejecting _meta in the builder + adding it to the test corpus.
  2. Summarizer key hashes only the canonical key (ContextSummarizer.ContextKey), so same-key/different-attribute contexts pile into one probe chain of deep LDContext == walks — synchronous on the variation() 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.
  3. The fix crash commit (e70cb42a) looks like three inert edits — the Data/Array aliasing its comments describe doesn't reproduce, the LDClient.hooks Array() copy fixes nothing (and adds a per-evaluation allocation on the hottest path), and the takeStages change is identical semantics. If so, the actual crash is still unexplained — can you share the crash log and whether it reproduces with the JSONWriter change alone?

Medium

  1. Cache can serve wrong _meta.redactedAttributes spellings 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 in contexts order.
  2. Non-finite doubles become null; "metricValue": null violates the v4.0 custom-event schema (and diverges from Android, which drops). Omit the key for non-finite metrics; have unserializableEventSpec assert payload bytes.
  3. Integral doubles in [1e17, 9.22e18] diverge from JSONEncoder even after canonicalization — adding 1e17 to testNumberFormatting fails today. Widen the fast-path guard to magnitude < 1e17 or document it (and restate the "identical bytes" claim as JSON-equality; /-escaping and -0.0 also differ at byte level).
  4. 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.
  5. The shipping configuration (.handWrittenCachingContext + anonymous context + privacy config) is never exercised at reporter level — the reporter-level encoder comparison uses .codable vs .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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private let handWrittenEncoder: EventJSONWriter
private let jsonWriter: EventJSONWriter

} else if value.isFinite {
bytes.append(contentsOf: String(value).utf8)
} else {
bytes.append(contentsOf: JSONWriter.nullBytes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

/// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a way to do this without allocating at all?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ==.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these just for debugging inspection?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

abelonogov-ld and others added 12 commits September 24, 2026 11:48
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>
- 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 tanderson-ld left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informally approving. Looking for Tier 1 spec approval.

case ("anonymous", _):
return false
case ("_meta", _):
return false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing bug?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You asked about it before and it mathches Java/Android


if redactAll {
redaction.redactedAttributes.append(reference.raw())
redaction.redactedAttributes.append(reference.canonical())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing bug?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#534 separate pr as it is preexisting

Comment thread LaunchDarkly/LaunchDarkly/Models/Context/LDContextJSONWriter.swift
let context: LDContext

func hash(into hasher: inout Hasher) {
hasher.combine(context.fullyQualifiedKey())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@abelonogov-ld abelonogov-ld Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

abelonogov-ld and others added 2 commits September 29, 2026 09:22
…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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants