dwsmith1983 commented on code in PR #5613:
URL: https://github.com/apache/datafusion-comet/pull/5613#discussion_r4362158058
##########
native/core/src/execution/memory_pools/fair_pool.rs:
##########
@@ -152,45 +196,77 @@ impl MemoryPool for CometFairMemoryPool {
additional: usize,
) -> Result<(), DataFusionError> {
if additional > 0 {
- let mut state = self.state.lock();
- let num = state.consumers.len();
- let limit = self
- .pool_size
- .checked_div(num)
- .expect("overflow in checked_div");
- let consumer_used = *state.consumer_used(reservation);
- if limit < consumer_used.saturating_add(additional) {
- return resources_err!(
- "Failed to acquire {additional} bytes where this consumer
already holds {consumer_used} bytes and the fair limit is {limit} bytes, {num}
registered ({} bytes overcommitted)",
- self.spark.overcommit()
- );
- }
- // The shares alone do not bound the pool's total, because a
consumer keeps what it
- // reserved before another consumer registered.
- let used = state.used;
- if self.pool_size < used.saturating_add(additional) {
- return resources_err!(
- "Failed to acquire {additional} bytes where {used} bytes
already reserved ({} bytes overcommitted) and the pool limit is {} bytes",
- self.spark.overcommit(),
- self.pool_size
- );
+ let consumer = reservation.consumer().id();
+ // Check and charge under one lock so concurrent grows cannot
jointly exceed a share
+ // or pool_size.
Review Comment:
> What do you think about splitting the pool's lock in two instead, the way
Spark's `TaskMemoryManager` does?
Done in 9dd4c4cb0. `grow` and `try_grow` hold a new `acquire` mutex across
the Spark call, `state` is held only for the arithmetic, and `acquire` is
always taken before `state`. `try_grow` follows your sketch. The provisional
charge, `settle`, `try_acquire_leaving_a_short_grant` and the ledger are gone,
and the guide's two-windows section is now a short description of the two locks.
The one addition is `lock_acquire`: a contended `acquire` is waited on in
`block_in_place`, because its holder can be parked in Spark and a plain lock
would stall the Tokio worker's other tasks. The three fixes for main are out of
this PR: #7174, #7175, #7176.
--
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]
