You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mbutrovich (via GitHub)" <gi...@apache.org> on 2026/03/05 17:35:25 UTC
[I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
mbutrovich opened a new issue, #20728:
URL: https://github.com/apache/datafusion/issues/20728
### Describe the bug
Commit 3f0b3425c ("Improve sort memory resilience", #19494) added per-chunk `reservation.try_grow(total_sorted_size)` calls inside the sort's `unfold` closure (`sort.rs:748-750`). This causes a significant performance regression for projects that implement custom `MemoryPool` backends where `try_grow` is not trivially cheap.
In Apache DataFusion Comet, `try_grow` crosses JNI into Spark's `TaskMemoryManager.acquireExecutionMemory()`, which internally calls `ExecutionMemoryPool.memoryUsed()` — an O(n) HashMap summation. The result is a **32x increase** in `acquireMemory` calls compared to DataFusion
51, where the sort path did not have per-chunk reservations.
### To Reproduce
We ran TPC-H SF100 query 21 using DataFusion Comet (Spark accelerator using DataFusion). Query 21 is SMJ-heavy with multiple sort operators feeding into sort-merge joins.
- **DF 51**: 695 `acquireMemory` JFR samples
- **DF 52.2**: 22,356 `acquireMemory` JFR samples (~32x increase)
JFR profiling shows the hot path:
CometFairMemoryPool::try_grow
→ JNI → CometTaskMemoryManager.acquireMemory
→ TaskMemoryManager.acquireExecutionMemory
→ ExecutionMemoryPool.memoryUsed() // O(n) HashMap sum
The call originates from `sort.rs` line 748-750:
```rust
reservation
.try_grow(total_sorted_size)
.map_err(Self::err_with_oom_context)?;
This runs inside the unfold closure for every chunk of sorted output.
Expected behavior
Memory accounting should not assume try_grow is free. The pre-#19494 sort code managed memory without per-chunk try_grow calls. The new granular tracking is valuable for resilience but the cost should be amortized — *e.g.*, reserving once for the full sorted output rather than
per-chunk, or providing a way to batch reservations.
Additional context
- Workload: TPC-H SF100 Q21, 2 iterations, JFR captured on executor
- Comet memory pool delegates to Spark via JNI, so each try_grow has non-trivial overhead
- This likely affects any MemoryPool implementation that does real work in try_grow (*e.g.*, coordination with external memory managers)
Claude helped me write this issue after investigating. I will open a counterpart issue on Comet.
--
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich closed issue #20728: perf: Sort memory reservation causes performance regression with custom MemoryPool implementations
URL: https://github.com/apache/datafusion/issues/20728
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
Posted by "EmilyMatt (via GitHub)" <gi...@apache.org>.
EmilyMatt commented on issue #20728:
URL: https://github.com/apache/datafusion/issues/20728#issuecomment-4007086665
I actually really like how cleanly it happens there, although I'm still not sure how drop(reservation) differs from reservation.free()
But there's not doubt that if this resolves the issue it's much better(and in general try_resize feels better I think, as it performs less)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on issue #20728:
URL: https://github.com/apache/datafusion/issues/20728#issuecomment-4007043460
Thanks for the quick response @EmilyMatt!
You're right that `try_grow` is called once per `sort_batch_stream` invocation, not per chunk. The issue is that `sort_batch_stream` is called [once per in-memory batch in the multi-batch merge path](https://github.com/apache/datafusion/blob/13cebf8432a3c649ebc3426933753e159b2b7b9a/datafusion/physical-plan/src/sorts/sort.rs#L687), and the old code used `drop(reservation)` — which for most `MemoryPool` implementations is very cheap (just decrementing a counter, no pool-level reacquisition needed). The new code does `reservation.free()` + `reservation.try_grow(total_sorted_size)`, which is two full pool roundtrips where there were previously zero.
In Comet's case each pool call crosses JNI into Spark's `TaskMemoryManager.acquireExecutionMemory()`, which is expensive — but this would affect any `MemoryPool` implementation where `try_grow`/`shrink` do real work (coordination with an external memory manager, logging, etc.).
We confirmed this by replacing `free()` + `try_grow()` with `try_resize()`, which only interacts with the pool for the delta (typically near-zero since sorted output ≈ input size). The `acquireMemory` samples dropped from 22,356 to 451 (back to DF51 levels), and wall-clock time improved. I should have full benchmarks shortly.
In the meantime I have a draft PR up already: #20729
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on issue #20728:
URL: https://github.com/apache/datafusion/issues/20728#issuecomment-4006595761
CC @EmilyMatt
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on issue #20728:
URL: https://github.com/apache/datafusion/issues/20728#issuecomment-4006675147
I am working on a fix that replaces the `free()` and `try_grow()` with a `try_resize()`. I'll report back with benchmarks in an hour or so.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] perf: Sort memory reservation causes performance regression with custom MemoryPool implementations [datafusion]
Posted by "EmilyMatt (via GitHub)" <gi...@apache.org>.
EmilyMatt commented on issue #20728:
URL: https://github.com/apache/datafusion/issues/20728#issuecomment-4006782189
@mbutrovich
Isn't this by definition called once per *fully* sorted batch?
```
reservation
.try_grow(total_sorted_size)
.map_err(Self::err_with_oom_context)?;
```
I.e., before,
drop(reservation) was called, which only reduced it.
Now we do
reservation.free(), and then reservation.try_grow()
So I'd expect one extra call per chunked sort, which seems reasonable.
How many tasks it that machine running? perhaps there is synchronisation happening in Spark making it slower?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org