baibaichen opened a new pull request, #12672:
URL: https://github.com/apache/gluten/pull/12672
## What changes are proposed in this pull request?
Fixes #12671
`SparkMmapAllocator` subclasses `MmapAllocator` and governs the cache
capacity in **two** places,
because reporting it and enforcing it do different jobs:
- **Reporting** is the virtual `capacity()`, which
`AsyncDataCache::canTryAllocate()` measures
headroom against, so lowering it is what makes the cache evict older
entries instead of growing.
It returns `max(governed, allocated)` rather than the governed value
alone: the headroom is an
*unsigned* subtraction, so a capacity below the allocated bytes would wrap
to a huge number and
turn the cap into an unlimited allowance. Clamped, the headroom is exactly
zero.
- **Enforcing** is the three admission points, which take the bound as a
parameter. Reporting alone
would not do: `canTryAllocate()` runs without a lock by design, so readers
passing it together
took more than the cap — one entry over a 32MB grant with 16 threads,
measured.
A periodic checker on the JVM side moves the capacity — halving the room
above the floor when
storage tightens, reclaiming a fixed step when it lifts — and does **all**
of the Spark borrow and
repay. Growing borrows first and then raises the cap; shrinking lowers the
cap first and then repays
what was actually given up. Native code never calls back into Java.
### What this bound does and does not guarantee
It is a **soft bound that settles, rather than one that holds**:
- The checker runs **periodically** (1s by default), so the capacity follows
Spark's pressure with a
lag of up to one interval. It is not synchronous with allocation.
- Lowering the capacity is a *request*. `AsyncDataCache` evicts what it can,
but **pinned entries
survive it**. When a lot is pinned the cache stays above the new target
and there is nothing to do
but wait — only what was **actually** freed is repaid to Spark, so the
reservation is never
under-stated while that lasts.
- Cache usage therefore **converges to below** what is reported to Spark. It
does not step over the
reservation and stay there.
- The one window where usage can exceed the reservation is an allocation
**already in flight** when
the capacity changes: the bound is read once on the way in, so lowering it
cannot reach a call
already past that point. It is bounded by concurrent readers times
`loadQuantum` (forced <= 8MB
when the cache is on), and closes as those allocations finish.
### Why not a hard bound
The original plan was to hook the cache into the memory manager's LRU list,
so eviction could be
driven synchronously and the bound made hard. **Pins are what make that
impossible**: there is no
answer to "how much can you release right now". A synchronous reclaim would
have to either block on
readers or return an unpredictable amount, and the memory manager can act on
neither. Backing off
periodically, and repaying only what was really freed, is what pins actually
permit.
### Dependency — please read before merging
This depends on facebookincubator/velox#18358, which parameterises the three
`MmapAllocator`
admission paths as protected `...WithCapacity` overloads (a pure refactor;
the existing entry points
forward the configured capacity). It is unavoidable: `Allocation::append` is
private to three named
`friend` classes and **friendship is not inherited**, so copying
`MmapAllocator` into Gluten does not
compile.
The last commit is marked `[TMP]` and sets `UPSTREAM_VELOX_PR_ID` in
`get-velox.sh` so that CI can
build against that PR. **It must be reverted before this merges**, once the
PR has landed and the
pinned branch has picked it up.
Opened as a draft for that reason — the design and the Gluten side are ready
for review.
The design, including the invariants, the rejected alternatives and the
known limitations, is in
`docs/superpowers/specs/2026-07-31-velox-cache-backpressure-design.md`.
## How was this patch tested?
New tests:
- `cpp/velox/tests/SparkMmapAllocatorTest.cc` — 17 cases, covering the
invariants, page alignment,
concurrent readers filling the cache together, and the shutdown contract.
- `PeriodicMemoryCheckerSuite` — 28 cases, covering the adjustment
algorithm, a refused borrow, a
shrink blocked by pins, and the startup configuration checks.
- `CacheStorageReservationDynamicSizingSuite` — 1 case, that the reservation
counts towards the
process memory gate under dynamic off-heap sizing.
All passing locally, along with `dev/format-scala-code.sh` and
clang-format-15.
Not yet done, and worth a reviewer's attention: the cross-profile compile
(spark-3.3 / 3.5 /
scala-2.12) has not been run locally, and the defaults for `minCacheSize`,
`cacheRatio` and
`stepSize` have not been calibrated against a real cluster.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI (Claude Opus 5)
--
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]
