perf(cache): Reduce allocations on the normalize, read, and SQLite paths - #383
Open
AlexanderGH wants to merge 9 commits into
Open
perf(cache): Reduce allocations on the normalize, read, and SQLite paths#383AlexanderGH wants to merge 9 commits into
AlexanderGH wants to merge 9 commits into
Conversation
…out of the db Writing a record larger than the 1 MiB chunk size went through `asIterable().chunked()`, boxing every one of its bytes into a `List<Byte>` and materializing every chunk before the first insert, only for `toByteArray()` to unbox them again. Slice the array instead. Reading went the other way: the row bytes were copied into a Buffer, copied out of it with `readByteArray()`, then copied into another Buffer by the deserializer. Have the deserializer read the Buffer directly. Also size records without serializing them into a ByteArray that is only measured and dropped, size the maps the deserializer fills so they don't rehash on the way to their known size, and build the set of changed keys in one pass rather than through a list holding every field key of every record.
Every write returns the field keys it changed, and each of the places
that assembles them was doing so through intermediate collections:
`flatMap {}.toSet()` materializes every key of every record in a list
before the set that is actually returned, and `Record.changedKeys` went
through an intersection, two differences, a filtered list and a mapped
list to compare two records field by field.
`writeOperation` also collected the normalized records into a `Set`
before handing them to `merge`, which takes a `Collection`. The values of
a map keyed by cache key are already distinct, so that only deep-hashed
every record of the response — `Record.hashCode` recurses through
`fields` and `metadata`. `writeFragment` already passed `.values` bare.
Also hoist the per-field date map in `withDates` out of the loop that was
rebuilding the same two entries for every field of the record.
…once `CacheControlCacheResolver` asked for the field key five times while resolving a single field. The default generator formats the field's arguments as JSON through an okio Buffer to build that key, so each of those calls re-encoded the same string. The resolvers and the record mergers then paired every `containsKey` with a `get` for the same key. Only a null value is ambiguous between "absent" and "present and null", so the second lookup is only needed then rather than for every field. Metadata is empty unless a MetadataGenerator is configured, which is the default, so `mergedWith` no longer copies a map to merge nothing into it.
`processErrors` resolved one response name at a time, and each resolution re-flattened the field's whole selection set with `filterIsInstance` and `flatMap` before filtering it down to a single name. That is quadratic in the number of fields selected on the object. Build the index once and reuse it for every key. `firstError` also allocated an ArrayDeque to walk each value it was given, where the overwhelming majority are scalars holding no error at all.
…sult Both the write and read paths merge the fields sharing a response name into one, and both rebuild the merged field unconditionally. A group of one - the common shape - merges to a field equal to the original, so the copy, its builder and its concatenated selection list are pure waste, once per field per object. Normalizing also scanned the flat list of collected fields once per key of the object being normalized; the fields are now indexed by response name as they are collected, which costs no more than the list did. Reusing the field instance is also what makes its key worth memoizing: it is then the same instance for every object normalized against those selections, and computing a key encodes the field's arguments to JSON. Merged fields stay uncached, so the memo cannot retain the copies.
The reader keys the objects it reads by their response path, and derives one path per field of every object - twice, since a field's value is both registered and, on a cache miss, reported - by copying the parent's path into a longer list. Assembling the result then walks the data and rebuilds every one of those lists again to look the objects up. The paths all share prefixes, so they are now interned into a tree. Appending a segment is a lookup on the parent node rather than a copy of the whole path, the second and later asks for a path hand back the node built for the first, and keying the map on one hashes an identity rather than walking its segments. Only the errors reported for cache misses still need a path as a list.
…y field the same one A field's max age is asked for once per field per object, and asking builds the path of the field as a list of max age fields, each of which walks its type's interface hierarchy. A GlobalMaxAgeProvider ignores that path entirely, and DefaultMaxAgeProvider is one, so the usual case pays for a path that is thrown away unread. Both callers now resolve the max age of a global provider once, up front, and build the path only for a provider that reads it.
The changes on this branch rewrite hot paths without changing what they produce, so most of what was missing is characterization cover: tests that fail if the behaviour moves, not just if the code does. - RecordChangedKeysTest: Record.changedKeys over the one-pass rewrite, with the null-versus-absent distinction its containsKey guard depends on. - DefaultRecordMergerTest: eleven cases where one was, including the one that matters most - a cached null is not replaced by an incoming error unless ERRORS_REPLACE_CACHED_VALUES says so - plus FieldMerger delegation. - SqlNormalizedCacheTest: records that span, shrink across, and are loaded alongside others at the 1 MiB blob chunk boundary. - NormalizerFieldMergingTest: the normalizer clears the condition off a field whether it merged several selections or reused a lone one, and generates a field key once per field of a parent type rather than once per object normalized against it. That last one is the actual regression test: with the previous Normalizer in place it reports 21 field keys for ten users where one user asks for 3. - MaxAgeFieldPathTest: a non-global MaxAgeProvider still receives the whole field path from both callers - deepest-first when normalizing, root-first when reading - and the global short-circuit answers what the provider would have, at both Duration.ZERO and Duration.INFINITE. - ResponsePathTest: objects of nested lists are read at their own path, and a cache miss inside a list is reported at its own index, which is where interned response paths would go wrong. Two asymmetries turned up while writing these. Both predate this branch and are pinned rather than changed: the reader keeps a condition on the field it hands a FieldKeyGenerator where the normalizer clears it, and the normalizer asks for a field's max age after recursing into its value where the resolver asks on the way down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Summary
An allocation and complexity pass over the three hot paths of the normalized cache: normalizing a response (write), CacheBatchReader (read), and record serialization to/from SQLite. Everything here is behavior-preserving — no public API change, apiCheck passes with no dump regenerated.
The theme is work repeated per field per object. Several of these paths did something once per field of once per object in a list, where the result was either identical every time or reachable without building an intermediate collection at all.
Reviewers: Please review one commit at a time. Each commit message contains useful context about the smaller sets of changes.
Write path (Normalizer)
The last two compose: memoization is only possible because the field instance is now stable across the objects of a list. Only the shared (non-rebuilt) fields are memoized — a merged field is a fresh instance per object, so caching its key would only retain the copy.
Read path (CacheBatchReader)
Max age
MaxAgeContext(fieldPath.map { it.toMaxAgeField() }) was built per field per object, and toMaxAgeField() recursively rebuilds a MaxAgeContext.Type tree for every interface the field's type implements. GlobalMaxAgeProvider ignores its context entirely, and DefaultMaxAgeProvider is one — so the whole tree was being built and discarded for everyone on the default configuration. An is GlobalMaxAgeProvider check hoisted to construction skips it, in both Normalizer and CacheControlCacheResolver.
Providers that do read the path are unaffected and still get the full context.
SQLite
Elsewhere
Correctness notes for reviewers
The one subtlety worth a close look: the Normalizer's rebuild does .condition(emptyList()), so a lone field is only reused when its condition is already empty. Reusing a field that has a condition would hand downstream consumers — a custom FieldKeyGenerator, MetadataGenerator, EmbeddedFieldsProvider, or CacheKeyGenerator — a field that differs from what they used to receive.
CacheBatchReader's rebuild does not clear the condition, so its singleton skip is unconditionally safe, and it falls back to the original groupBy when duplicate response names do exist, h preserves ordering exactly.