github-actions[bot] commented on code in PR #67228:
URL: https://github.com/apache/doris/pull/67228#discussion_r3872318766
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
+ Hasher hasher = Hashing.murmur3_128().newHasher();
+ hasher.putBytes(serializedDescTable.toByteArray());
+ hasher.putBytes(serializedOutputExpr.toByteArray());
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
+ Hasher hasher = Hashing.murmur3_128().newHasher();
+ hasher.putBytes(serializedDescTable.toByteArray());
+ hasher.putBytes(serializedOutputExpr.toByteArray());
+ hasher.putBytes(serializedQueryOptions.toByteArray());
+ hasher.putInt(bucket);
+ ByteBuffer buffer = ByteBuffer.wrap(hasher.hash().asBytes());
+ return new UUID(buffer.getLong(), buffer.getLong());
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
+ Hasher hasher = Hashing.murmur3_128().newHasher();
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
+ Hasher hasher = Hashing.murmur3_128().newHasher();
+ hasher.putBytes(serializedDescTable.toByteArray());
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
+ Hasher hasher = Hashing.murmur3_128().newHasher();
+ hasher.putBytes(serializedDescTable.toByteArray());
+ hasher.putBytes(serializedOutputExpr.toByteArray());
+ hasher.putBytes(serializedQueryOptions.toByteArray());
Review Comment:
[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.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShortCircuitQueryContext.java:
##########
@@ -110,14 +122,30 @@ public ShortCircuitQueryContext(Planner planner,
Queriable analzyedQuery) throws
TExprList exprList = new TExprList(exprs);
serializedOutputExpr = ByteString.copyFrom(
new TSerializer().serialize(exprList));
- this.cacheID = UUID.randomUUID();
+ this.cacheID = genCacheID(serializedDescTable, serializedOutputExpr,
serializedQueryOptions);
this.scanNode = olapScanNode;
this.tbl = this.scanNode.getOlapTable();
this.tableName = this.scanNode.getTableNameInPlan();
this.schemaVersion = this.tbl.getBaseSchemaVersion();
this.analzyedQuery = analzyedQuery;
}
+ // Build a 128-bit cache identifier from serialized query structures and a
+ // round-robin bucket. Identical hot queries are intentionally spread
across
+ // multiple Backend LookupConnectionCache shards to reduce lock contention
and
+ // 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);
+ Hasher hasher = Hashing.murmur3_128().newHasher();
+ hasher.putBytes(serializedDescTable.toByteArray());
+ hasher.putBytes(serializedOutputExpr.toByteArray());
+ hasher.putBytes(serializedQueryOptions.toByteArray());
+ hasher.putInt(bucket);
Review Comment:
[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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
