Skip to content

[opt](point-query) reduce short circuit lookup cache usage - #67228

Open
HonestManXin wants to merge 1 commit into
apache:masterfrom
HonestManXin:opt_lookup_cache_usage
Open

[opt](point-query) reduce short circuit lookup cache usage#67228
HonestManXin wants to merge 1 commit into
apache:masterfrom
HonestManXin:opt_lookup_cache_usage

Conversation

@HonestManXin

Copy link
Copy Markdown
Contributor

In an online point query workload, a wide table with about 80 columns was queried through nearly 10,000 client connections. The Backend LookupConnectionCache usage on a single BE could grow to more than 10 GB.
The root cause is that short circuit point query contexts used random UUIDs as Backend lookup cache keys. Even when different connections executed the same query shape, each context generated a different cache ID. As a result, the Backend could not reuse the pre-calculated lookup cache entries across
connections, and many duplicate cache entries were created. This problem becomes much more obvious on wide tables because serialized descriptors and output expressions are larger.
This PR derives the short circuit cache ID from the serialized query context, including descriptor table, output expressions, and query options. Therefore, identical query contexts are mapped to a bounded set of cache IDs instead of always generating random IDs. To avoid concentrating a hot query on a
single Backend cache shard, a round-robin bucket is also mixed into the hash, so hot identical queries can still be spread across multiple LookupConnectionCache shards and reduce lock contention.
In the online workload above, the lookup cache usage on a single BE dropped from more than 10 GB to about 270 MB.

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@HonestManXin

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot 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.

Review status: capped/incomplete. This review reached the three-round limit, and the final FE round still found one additional accepted performance path. All current candidates have nevertheless been independently verified and deduplicated. Nine accepted issues are represented by eight inline comments (the two unsent-ID paths share one comment): three P1 correctness issues and six P2 performance issues.

Critical checkpoint conclusions

  • Goal and proof: The change can bound one byte-identical prepared-query shape to 128 steady-state UUIDs, but it does not safely achieve the memory/contention goal. Cross-connection reuse introduces wrong-result races, stale schema-derived state, nondeterministic function-state carryover, cold/warm contention, and avoidable FE work. No changed test proves the new identity or ownership contract.
  • Scope and clarity: The Java diff is compact, but it changes BE cache ownership from one prepared context to unrelated connections and FE processes. That cross-stack lifecycle expansion is not safely contained by this one-file edit.
  • Concurrency and locks: AtomicLong allocation and signed wrap are locally safe. BE cache hits, however, share an unprotected RuntimeState and original VExprContextSPtrs; the one protected member performs wide allocation/clearing/destruction under _block_mutex. No additional lock-order/deadlock defect was found.
  • Lifecycle and memory safety: Cache handles, active shared_ptr lifetimes, tracker switching, and Java static initialization are balanced, with no ownership cycle or SIOF found. Lifecycle correctness still fails because hot entries retain schema-derived mappings and already-open function state across independent statements. Transient allocation issues are called out inline.
  • Error handling and nullability: This diff adds no new status/exception or nullable-column path; no separate defect was found on those surfaces.
  • Configuration and compatibility: No configuration, persisted format, transaction, storage write, or wire field changes. Existing UUID decoding and lightweight miss resend remain wire-compatible, but rolling FE/BE operation does not fence mutable or stale cached state.
  • Parallel paths and conditions: Full/lightweight prepared execution, miss resend, replica retry, cloud snapshot reads, binary/text results, COM_QUERY, and nondeterministic prepared execution were traced. The latter two contain unsent hash work, and the hasNondeterministic() FE fence is bypassed by BE content-key reuse.
  • Data correctness: Shared timezone/expression state, omitted schema generation, and sequential seeded-random state carryover are blocking wrong-result paths. Visible-version handling and read-only tablet/version ownership otherwise remain unchanged.
  • Tests and results: The changed-file set contains no test. Existing FE tests do not inspect cacheID; BE concurrency tests do not execute one shared Reusable; current timezone/nondeterministic regressions are sequential or too short to repeat a bucket. No local build/test was run because the authoritative review prompt forbids builds. Current CI has CheckStyle, formatter, license, and secrets checks passing; the PR title checker is failing, and automated code-review is pending.
  • Observability: Existing cache/profile metrics remain available, but they do not prove same-key safety. No distinct missing-observability issue was found.
  • FE/BE state passing and performance: No new field is introduced, but the UUID's semantics now omit required schema/lifecycle dimensions. Performance issues include global stride collapse, non-single-flight cold initialization, three full ByteString copies, two unsent-ID paths, and serialized warm block-pool work.
  • User focus: No additional user-provided review focus was supplied.

Please address all inline findings and add identity, same-key concurrent hit/miss, warm schema-transition, nondeterministic-state, and wide-block contention coverage before re-review.

serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
this.cacheID = UUID.randomUUID();
this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr, serializedQueryOptions);

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.

[P1] Do not share this mutable BE context across connections. The first and 129th identical prepared contexts on one FE (or bucket 0 on two FEs) now use the same UUID, so concurrent BE requests receive the same Reusable. Only its block pool is locked: each request writes runtime_state()->set_timezone(...) and both execute the same original VExprContextSPtrs, whose execution mutates context/function state. For example, A can set UTC, B overwrite Asia/Tokyo, and A's supported from_unixtime point query formats with B's timezone; concurrent string/timezone access is also a C++ data race. Make the cached value immutable and clone/lease request-local runtime and expression state (or otherwise serialize the whole use) before collapsing IDs; adding timezone to the hash alone does not make same-timezone executions thread-safe.

// high sys CPU, while still bounding the number of cache entries.
private static UUID genCacheID(ByteString serializedDescTable, ByteString serializedOutputExpr,
ByteString serializedQueryOptions) {
int bucket = (int) Math.floorMod(CACHE_ID_BUCKET_COUNTER.getAndIncrement(), CACHE_ID_BUCKET_NUM);

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.

[P2] Rotate buckets per query identity, not from one JVM-global ordinal. With a fixed set of N prepared contexts created in the same order on every connection, query j gets (N * connection + j) mod 128 and reaches only 128 / gcd(N, 128) buckets. For 128 statements, every instance of each hot query uses one UUID and one BE shard, recreating the single-shard contention this code claims to avoid even though the global bucket histogram is even. Scope the sequence to the unhashed query identity (or use another proven per-query spread) and test repeated multi-statement connection initialization.

private static UUID genCacheID(ByteString serializedDescTable, ByteString serializedOutputExpr,
ByteString serializedQueryOptions) {
int bucket = (int) Math.floorMod(CACHE_ID_BUCKET_COUNTER.getAndIncrement(), CACHE_ID_BUCKET_NUM);
Hasher hasher = Hashing.murmur3_128().newHasher();

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.

[P2] Generate this ID only when a request can actually send it. StmtExecutor creates this context for every text-protocol short-circuit query, but PointQueryExecutor.buildLookupRequest sends cacheID only for COM_STMT_EXECUTE, so every COM_QUERY copies and scans the full plan for no BE benefit. There is a second dead path for nondeterministic prepared statements: execution builds the context it sends, then ExecuteCommand stores another context after execution even though its hasNondeterministic() guard prevents that retained context from ever taking the direct path. Both replace fixed-size random-UUID work with full-payload copies and hashing. Make ID generation lazy/request-driven and test both unsent paths.

ByteString serializedQueryOptions) {
int bucket = (int) Math.floorMod(CACHE_ID_BUCKET_COUNTER.getAndIncrement(), CACHE_ID_BUCKET_NUM);
Hasher hasher = Hashing.murmur3_128().newHasher();
hasher.putBytes(serializedDescTable.toByteArray());

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.

[P2] Hash the existing ByteString views instead of allocating three full copies. Each toByteArray() duplicates a payload that this context already owns, so 10,000 wide-table contexts create three large transient arrays apiece solely for Murmur input and add avoidable GC pressure to the workload being optimized. Guava's hasher can consume ByteBuffer; use serialized...asReadOnlyByteBuffer() (or another zero-copy view) for contexts that actually need an ID.

int bucket = (int) Math.floorMod(CACHE_ID_BUCKET_COUNTER.getAndIncrement(), CACHE_ID_BUCKET_NUM);
Hasher hasher = Hashing.murmur3_128().newHasher();
hasher.putBytes(serializedDescTable.toByteArray());
hasher.putBytes(serializedOutputExpr.toByteArray());

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.

[P1] Preserve the nondeterministic-plan no-reuse boundary in this ID. ExecuteCommand deliberately avoids reusing its retained short-circuit context when hasNondeterministic() is true, but a fresh execution still hashes the same BE TExpr: FE's volatile identity is not serialized. Two FEs starting at bucket 0 can therefore send the same UUID for random(7). The first miss opens and seeds Random's cached THREAD_LOCAL mt19937_64; the later sequential hit reuses that already-open function context and advances the first statement's generator instead of reseeding to 7. This needs no race, and serializing the shared context would not fix it. Keep nondeterministic contexts uniquely keyed or clone/open request-local expression/function state, with a sequential warm-cache seeded-random test across matching buckets.

Hasher hasher = Hashing.murmur3_128().newHasher();
hasher.putBytes(serializedDescTable.toByteArray());
hasher.putBytes(serializedOutputExpr.toByteArray());
hasher.putBytes(serializedQueryOptions.toByteArray());

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.

[P1] Include the schema generation in this cache identity. isReusable deliberately invalidates an FE context when baseSchemaVersion changes, but the new UUID omits that value. A row_store_columns schema change can keep the serialized descriptor/output/options byte-identical while BE's Reusable::init derives different include_col_uids/missing_col_uids from the new TabletSchema. If the 128 old hot IDs are resident, refreshed contexts immediately hit those stale objects; a column removed from row storage is then not fetched from column storage and can be returned as a default/wrong value. Hash a complete schema/version token and/or validate cached state against the request tablet schema, with a warm-cache row-store schema-change test.

hasher.putBytes(serializedDescTable.toByteArray());
hasher.putBytes(serializedOutputExpr.toByteArray());
hasher.putBytes(serializedQueryOptions.toByteArray());
hasher.putInt(bucket);

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.

[P2] Keep wide-block work out of the newly shared per-key mutex. Once independent connections converge on one of these 128 IDs, every warm hit uses the same Reusable::_block_mutex. Each pool has only 32 blocks; when it empties, get_block() allocates a wide block while holding the lock, and return_block() clears every column and may destroy an excess block while still locked. In the stated 10,000-request burst, about 78 borrowers share each ID, so at least 46 allocations serialize per pool and the excess blocks are destroyed before the next burst repeats that work. Move only the vector pop/push under the lock, doing allocation, clearing, and over-capacity destruction after unlocking (or use request-local/striped pools), with a >32-borrower warm-cache contention test.

hasher.putBytes(serializedQueryOptions.toByteArray());
hasher.putInt(bucket);
ByteBuffer buffer = ByteBuffer.wrap(hasher.hash().asBytes());
return new UUID(buffer.getLong(), buffer.getLong());

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.

[P2] Coalesce cold initialization for the new shared keys. BE currently does get(uuid), deserializes and runs Reusable::init(..., 32), then add(uuid) with no per-key single-flight; duplicate insertion is last-writer-wins. During a 10,000-connection cold start, roughly 78 requests per new key can all miss and allocate the wide descriptors, expression state, and 32 blocks before the cache converges, preserving an O(connection-count) transient memory/CPU spike. Add lookup-or-create/single-flight coordination with a barrier-based same-key miss test.

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