yugan95 commented on PR #7621:
URL: https://github.com/apache/paimon/pull/7621#issuecomment-4533242443
@LsomeYeah Thanks for the thoughtful review! All four points are addressed
in the
updated commit:
**1. 4MB threshold basis**
The threshold is derived from the production scenario that surfaced this
bug: 256
buckets × 4MB = 1GB baseline, which leaves reasonable headroom in a
typical 4–8GB
TaskManager heap. 4MB is high enough to avoid interfering with normal
workloads (most
records are well under 1MB) while preventing the pathological accumulation
we saw in
heap dumps (256 × 100MB+ = tens of GB).
**2. Thrashing near the threshold — fixed with hysteresis**
Good catch — this was a real gap. I've updated both
`RowHelper.resetIfTooLarge()` and
`BinaryRowSerializer.deserialize(reuse)` with a hysteresis guard:
- `RowHelper`: only releases when `buffer > threshold && lastRecord <
threshold`
- `BinaryRowSerializer`: only shrinks when `buffer > threshold &&
currentRecordLength <
threshold`
This means:
- **Sustained large records (5–10MB):** buffer is retained, no thrashing
- **Occasional large record → back to small records:** buffer is released,
OOM
protection kicks in
- **Normal small records (< 4MB):** buffer never exceeds threshold, the
check is a
complete no-op
Tests updated to verify the hysteresis behavior in both classes.
**3. Hot-path overhead**
With hysteresis, the overhead on the hot path is 1 null check + 2 int
comparisons — all
on fields already in L1 cache from the preceding serialize(). For
normal-sized records
the buffer never reaches 4MB, so `resetIfTooLarge()` short-circuits at the
size check.
The JIT will inline this. No branch is taken and no allocation occurs.
**4. Prior art**
The hysteresis approach here is inspired by the same principle behind
Spark's
`UnsafeExternalSorter` — retain capacity when the workload demands it,
release when it
doesn't. A full memory-pressure-based release (integrating with Paimon's
managed memory
pool) would be a larger scope change; the simple hysteresis guard covers
the production
failure mode while keeping the change minimal and safe.
Also, the Parquet config pass-through has been moved to #7956.
--
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]
