Trivial query overhead2 - #765
Closed
adsharma wants to merge 5 commits into
Closed
Conversation
Cache the PhysicalPlan (operator tree template) in CachedPreparedStatement so that mapOperator recursion is skipped on subsequent executions. Key changes: - Added physicalPlanCache + resultSetDescriptor to CachedPreparedStatement - Added prepareForReuse() to PhysicalOperator for resetting mutable state - ResultCollector::prepareForReuse() recreates sharedState with fresh table - TableFunctionCall::prepareForReuse() resets FTableScan scan position - Added resetState() to TableFuncSharedState for scan-position reset - LiteralExpressionEvaluator reads fresh value from ParameterExpression at init time instead of the frozen copy, enabling correct parameter propagation on reused plans - cachePhysicalPlan flag in executeNoLock prevents overhead in queryNoLock Performance: ~7-8% improvement for prepared-statement RETURN $i queries on the implicit-cache path (118 vs 127 us/iter), no regression for the no-params path.
Avoid re-allocating per-iteration resources by keeping them alive: - FactorizedTable::clear() keeps existing DataBlocks and resets their bookkeeping (freeSize, numTuples) instead of freeing + re-allocating. Saves a calloc(256KB) per FactorizedTable.clear() call. - DataChunkState::getSingleValueDataChunkState() returns a function-static singleton instead of heap-allocating a new state + 8KB sel-vector buffer on every call. Safe because the selection-vector internals are immutable after construction. - ResultCollector::prepareForReuse() calls sharedState->getTable()->clear() instead of constructing a fresh FactorizedTable. Combined with the FactorizedTable::clear() change, this keeps the allocation hot and eliminates the malloc/free cycle for the result table's DataBlock.
…ment
Avoid the per-execution DataChunk/ValueVector allocation in
ProcessorTask::run by passing a pre-built ResultSet through the
ExecutionContext. The ResultSet is owned by CachedPreparedStatement
and outlives the cloned PhysicalPlan, so its DataChunks, ValueVectors,
and value buffers survive across the for-loop pattern:
for i in range(n):
conn.execute("RETURN $i", {"i": i})
The cached ResultSet is only used when the ProcessorTask is
single-threaded: a shared ResultSet would race between worker threads
writing to the same ValueVectors concurrently. Multi-threaded tasks
fall back to allocating a fresh ResultSet per call, preserving the
old behaviour.
The ResultSet::resetForReuse() helper clears per-execution state
(multiplicity, null mask, auxiliary buffer) so the next execution
starts from a clean slate.
For a loop such as
for i in range(n):
conn.execute("RETURN $i", {"i": i})
each iteration was paying a 16KB+ calloc for the trivial-ResultSet's
ValueVector value buffer (plus a fresh NullMask), because
Sink::getResultSet() allocated a new ResultSet on every
ProcessorTask::run() call. Reuse the allocation across calls.
Ownership and thread safety
---------------------------
* The worker thread OWNS the ResultSet outright via a thread_local
shared_ptr. No aliasing, no borrowing from a longer-lived cache.
Lifetime is well-defined: dropped when the thread exits, or when a
different prepared statement runs on this thread and the descriptor
pointer no longer matches (we rebuild).
* A fresh descriptor pointer is a strong-enough cache key. The
ResultSetDescriptor lives in the Sink, which lives in the PhysicalPlan
built by executeNoLock. The descriptor pointer is stable across
iterations of conn.execute() on the same prepared statement and
changes for a different one. We always read the descriptor through
the freshly-built Sink inside this run() call, before any thread_local
lookup, so use-after-free on the descriptor is impossible by
construction.
* Reset between runs is bounded: ResultSet::resetForReuse() clears
multiplicity, null mask, and auxiliary buffers; value buffers are
overwritten in place by the operators during executeInternal.
* The 'no descriptor' branch (e.g. OrderByMerge) keeps the old
per-call allocation via sink->getResultSet().
* The drop on descriptor change is safe even with a live QueryResult
in the C-API. The ResultSet is purely an inter-operator scratchpad
(DataChunks / ValueVectors / value buffers) that the ResultCollector
copies into its FactorizedTable inside executeInternal. The
QueryResult returned to the user only holds a shared_ptr to that
FactorizedTable; it never references the ResultSet. Once the merge
into sharedState->getTable() is complete, the ResultSet is dead
weight from the result path's point of view and can be freed
without touching the live QueryResult.
Why TLS over an arena / pointer-bumping allocator
------------------------------------------------
The alternative is a thread-private bump allocator (bump ptr over a
slab, return a new region each call). It would also avoid the per-call
calloc, and in the hot path it would be slightly cheaper than TLS
because there is no shared_ptr indirection on the fast path and no
deserializer work for the 'descriptor matches' check. We did not take
that path for three reasons:
1. Reset cost. The arena would have to be rewindable or the descriptors
would have to know which slab to free from. ResultSet::resetForReuse
walks every DataChunk / ValueVector and toggles a few state bits;
the per-iteration cost is O(numValueVectors) and stays bounded by
the descriptor. A bump allocator resets in O(1) by moving the bump
pointer, but it must then either keep the old slabs around (memory
grows unbounded) or unmap them (slower than resetForReuse at the
scale we care about, which is the trivial-query case where the
descriptor has 1-2 ValueVectors).
2. Where the win actually lives. The hot path is the trivial
prepared-statement loop:
for i in range(n): conn.execute('RETURN $i', {'i': i})
Here the descriptor is the same on every iteration and the
number of distinct descriptors on a given thread is small (often 1).
A TLS map keyed by descriptor pointer is a 1-entry cache; the
hash/compare cost is negligible compared to the calloc we are
avoiding. The arena's O(1) reset doesn't help because we would
have already paid the allocation cost for the slab.
3. Code surface. TLS is local to ProcessorTask::run() and ResultSet;
an arena-based design would need a new Allocator abstraction,
integration with the MemoryManager (which is not a bump allocator),
and a plan for what happens when the slab is full (fall back to
malloc? allocate a fresh slab? per-descriptor? per-thread?).
The data we are keeping alive -- DataChunk, ValueVector, value
buffer, null mask -- is heterogeneous and the lifetimes do not
nest cleanly, which is what makes a bump allocator awkward here.
The TLS design pays a small price on the descriptor miss (one
make_shared<ResultSet>), which only happens on the first call per
(thread, descriptor) pair. For the trivial-query loop that is
one allocation total per worker thread; the remaining n-1 calls
reuse the slot.
Contributor
Author
|
This will land as #764 |
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
1 participant
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.
temp pr to run ci for #764