[opt](paimon) Reduce JNI read object allocations - #66244
Open
Gabriel39 wants to merge 1 commit into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
marked this pull request as ready for review
July 29, 2026 14:10
Contributor
TPC-H: Total hot run time: 29357 ms |
Contributor
TPC-DS: Total hot run time: 177045 ms |
Contributor
ClickBench: Total hot run time: 25 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes.
Two blocking issues remain:
- The eagerly cached
ZoneIdrejects Doris-supported short session-zone aliases before any value is read, including scalar-only Paimon scans. - Recursive per-position caches retain the historical union of nested shapes for the scanner lifetime, so valid shifting nested data can require multiplicatively more heap than any individual row.
Critical checkpoint conclusions:
- Goal and proof: Reusing wrappers and caching timestamp conversion state reduces allocations for stable shapes, and the new unit assertions cover identity reuse and selected temporal boundaries. The two findings above show the optimization is not yet safe over the full production input/session domain.
- Scope and clarity: The PR is focused on two Paimon scanner files and uses one shared reuse helper. No unrelated changes were found.
- Concurrency: Each Java scanner is driven serially by its owning JNI reader, and
VectorColumnrecursively consumes unpacked values synchronously before rebinding. No race, lock, or deadlock issue was found. - Lifecycle and memory: Active wrappers are rebound correctly, but inactive recursive caches survive shrink/null transitions and
releaseBatch, producing the reported scanner-lifetime historical-shape retention. - Compatibility and error behavior: No persisted, storage-format, RPC, or JNI layout change is involved, so no rolling-upgrade shim is needed. The eager one-argument zone parse is a behavior compatibility regression for valid Doris sessions; JNI error propagation otherwise remains intact.
- Parallel and conditional paths: Arrays, maps, structs, sparse struct indexes, nesting, nulls, grow/shrink emission, mixed top-level schemas, both Paimon timestamp mappings, and both JNI reader generations were traced. The timestamp fast paths are algebraically equivalent to the previous UTC conversions; no separate timestamp-value bug was found.
- Tests and results: The added expected values are correct, but coverage misses a Doris short-zone scanner construction and shifting nested high-water marks/retained heap. No builds or tests were run in this review-only environment, as required by the review instructions.
- Performance and observability: Stable-shape allocation reductions are credible, but the benchmark's fixed 16-element shape does not bound historical recursive retention. Existing scanner/JVM metrics add no distinct blocker beyond fixing that retention.
- Configuration, transactions, persistence, data writes, FE-BE variable additions, static initialization, and master failover are not applicable.
User focus: no additional focus was provided; the complete PR was reviewed.
|
|
||
| public void setTimeZone(String timeZone) { | ||
| this.timeZone = timeZone; | ||
| this.timeZone = ZoneId.of(timeZone); |
Contributor
There was a problem hiding this comment.
Doris accepts short session-zone IDs through TimeUtils.timeZoneAliasMap (for example, EST) and forwards them unchanged to JniReader; plain ZoneId.of("EST") throws ZoneRulesException. Because PaimonJniScanner calls this setter unconditionally, this change makes even an INT-only Paimon scan fail during scanner construction, whereas the previous setter did not resolve the zone unless an LTZ value was read. Please resolve with Doris-equivalent alias semantics (including its CST handling) and cover a supported short ID on a non-temporal scan.
| List<PaimonColumnValue> cache, int cacheIndex, DataGetters childRecord, int childIndex, | ||
| ColumnType childDorisType, DataType childPaimonType) { | ||
| while (cache.size() <= cacheIndex) { | ||
| cache.add(null); |
Contributor
There was a problem hiding this comment.
These recursive per-position caches keep the historical union of nested shapes until the scanner is closed. For ARRAY<ARRAY<INT>> with outer size N, successive rows can put an M-element inner array at a different outer position while the others are empty; every row has O(N+M) elements, but each outer wrapper permanently keeps its own M-entry child cache, leaving O(N*M) wrappers (and their last Paimon container references) reachable from the scanner. Shrink/null paths never prune them, and the fixed-size benchmark does not exercise this. Please bound/prune nested caches or use a scanner-owned scratch/pool design whose retained size follows the maximum live row/batch shape, and add a shifting-nested-shape retention test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: #66223
Problem Summary:
The Paimon JNI read path created a new
PaimonColumnValuefor every array element, map key/value, and struct field on every row. Timestamp conversions also resolved the session time zone per value and built redundant intermediate temporal objects. On wide nested data or large scan batches, these short-lived objects increase allocation rate and GC pressure.For example, consider two consecutive
ARRAY<INT>rows,[10, 20]and[30, 40]. Previously, reading the second row allocated two new wrappers even though the first row's wrappers had already been consumed. This PR keeps two position-specific wrappers and resets their record, index, Doris type, Paimon type, and time zone before reading the second row. This is safe becauseVectorColumnconsumes the unpacked list synchronously before the scanner advances to another value.The same invariant is applied separately to array elements, map keys, map values, and struct fields. Caches are lazy, grow only when needed, and only the current collection size or projected struct indexes are emitted, so shrinking collections cannot expose stale entries. Struct wrappers are cached by the original field index, preserving sparse/non-contiguous projection behavior fixed by #66223.
For temporal values, the session time-zone string is now parsed once into a
ZoneId. A PaimonTIMESTAMP_LTZepoch instant is converted directly into that zone instead of constructing UTC and target-zone intermediates.TIMESTAMPTZuses Paimon's equivalent local representation directly instead of converting throughInstantand UTC.What changed?
PaimonColumnValuewrappers across rows for arrays, maps, structs, and recursively nested values.ZoneIdinstead of resolving it for every value.Local allocation and latency microbenchmark
Environment: Linux x86_64, Oracle JDK 17.0.16, Paimon 1.3.1. The benchmark ran latest
masterand this commit in separate worktrees on the same machine. Each case used 100,000 warmup iterations and 5 measured trials; the table reports medians. Collection cases ran 300,000 operations per trial, timestamp cases ran 1,000,000. Allocation was measured withThreadMXBean; GC count/time was measured withGarbageCollectorMXBean. A full GC was requested before each measured trial and excluded from the reported GC delta.The collection output lists were preallocated and cleared between operations to isolate allocation inside the Paimon value conversion path. Therefore these numbers are a focused Java microbenchmark, not an end-to-end query throughput claim.
Benchmark data shape and values:
ARRAY<INT>[16]: oneGenericRowcontaining oneGenericArraywith[0, 1, ..., 15].MAP<INT,BIGINT>[16]: oneGenericRowcontaining one insertion-orderedGenericMapwith{0: 0, 1: 1, ..., 15: 15}.STRUCT<INT,STRING,BIGINT>: one binary row containing(10, "x", 100); all three fields are projected.TIMESTAMP_LTZ(9): one fixed instant,2024-03-10T10:30:00.123456789Z, read in session zoneAmerica/Los_Angeles. This date exercises the US DST transition day.TIMESTAMPTZ(9): one fixed pre-epoch value,1969-12-31T23:59:59.999999999Z, read in UTC.Every benchmark operation rereads the same Paimon row/value. This intentionally removes file I/O, row construction, and table scan variability, and isolates the allocation/latency of
PaimonColumnValue.unpack*()or timestamp conversion. The core collection loop is equivalent to:On
master, eachunpackArraycall above creates 16 wrapper objects. With this PR, the warmup creates the 16 position-specific wrappers once and measured iterations only reset them. Map and struct use the same loop shape with separate key/value or projected-field output lists.ARRAY<INT>[16]MAP<INT,BIGINT>[16]STRUCT<INT,STRING,BIGINT>TIMESTAMP_LTZ(9)TIMESTAMPTZ(9)The remaining map allocation comes primarily from Paimon's
GenericMap.keyArray()/valueArray()materialization, which is unchanged by this PR. Struct latency is effectively unchanged while allocation drops by 75%.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Validation command:
Result: 20 tests, 0 failures, 0 errors, 0 skipped.
Check List (For Reviewer who merge this PR)