Introduce local block factory #102901
Introduce local block factory #102901
Conversation
dnhatn
force-pushed
the
local-breaker
branch
2 times, most recently
from
7b04494 to
9dd0956
Compare
| public static final String LOCAL_BREAKER_OVER_RESERVED_SIZE_SETTING = "esql.block_factory.local_breaker.over_reserved"; | ||
| public static final ByteSizeValue LOCAL_BREAKER_OVER_RESERVED_DEFAULT_SIZE = ByteSizeValue.ofKb(4); | ||
|
|
||
| public static final String LOCAL_BREAKER_OVER_RESERVED_MAX_SIZE_SETTING = "esql.block_factory.local_breaker.max_over_reserved"; |
There was a problem hiding this comment.
These are internal settings now; we can promote them later.
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
Hi @dnhatn, I've created a changelog YAML for you. |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
| /** | ||
| * This method must be called before passing this Block to another Driver. | ||
| */ | ||
| void allowPassingToDifferentDriver(); |
There was a problem hiding this comment.
I see why you called this allowPassingToDifferentDriver, since the block can still be used in the current context, just with its parent breaker. It's kind of a restricted setBlockFactory.
Would you mind locating this method to beside blockFactory(), and also adding a small sentence to the javadoc, say, "Sets this block's blockFactory to the factory's parent" or similar.
| return parent != null ? parent : this; | ||
| } | ||
|
|
||
| public BlockFactory newChildFactory(LocalCircuitBreaker childBreaker) { |
There was a problem hiding this comment.
I like the hierarchy through the block factory. 👍
| * @see BlockFactory#newChildFactory(LocalCircuitBreaker) | ||
| * @see Block#allowPassingToDifferentDriver() | ||
| */ | ||
| public final class LocalCircuitBreaker implements CircuitBreaker, Releasable { |
| @Override | ||
| public final void addInput(Page page) { | ||
| // We need to change the ownership of the blocks of the input page before passing them to another driver. | ||
| page.allowPassingToDifferentDriver(); |
There was a problem hiding this comment.
++ yes, it's important to transfer ownership in the executing thread before the page leaves.
|
Thank you, Chris! |
This optimization is added for enrich lookups, which are likely to match a single document. The change decreases the latency of the enrich operation in the nyc_taxis benchmark from 100ms to 70ms. When combined with #102901, it further reduces the latency to below 40ms, better than the previous performance before the regression. Relates #102625
Requesting and returning memory from a CircuitBreaker can be costly due to the involvement of read/write on one or several atomic longs. To address this issue, the local breaker adopts a strategy of over-requesting memory, utilizing the reserved amount for subsequent memory requests without direct access to the actual breaker.
Before passing a Block to another Driver, it is necessary to switch the owning block factory to its parent, which is associated with the global breaker. This is done to bypass the local breaker when releasing memory, as the releasing thread can be any thread, not necessarily the one executing the Driver.
There are two specific operators that need to change the owning block factory: SinkOperator (superset of ExchangeSinkOperator), which is the last operator of a Driver, and AsyncOperator, which can be responded by any thread in response.
The optimization reduces the latency of the enrich operation in the nyc_taxis benchmark from 100ms to 50ms. When combined with #102902, it further reduces the latency to below 40ms, better than the previous performance before the regression.
Relates #102625