andygrove commented on PR #6048:
URL:
https://github.com/apache/datafusion-comet/pull/6048#issuecomment-5743763058
Thanks for the review. The P2 is right, and I reproduced all three
mechanisms against the Arrow
18.3 sources before changing anything:
- `ArrayImporter.java:58` allocates the owning `ArrowArray` with
`ArrowArray.allocateNew(allocator)`,
so every import charges a JVM-created struct to the import allocator.
- `BitVectorHelper.loadValidityBuffer:328` does
`allocator.buffer(getValidityBufferSize(valueCount))`
when the source validity buffer is absent and the field node is all-valid
or all-null. For 4096
rows that is 512 bytes, which with the struct matches the 640 bytes you
measured.
- `Data.exportVector`'s allocator is documented as being "for allocating C
data interface fields"
and only constructs an `ArrayExporter`; it never re-owns the vector's
buffers. So a UDF result
allocated from `inputs.head.getAllocator()` stays charged to the import
allocator for as long as
the export holds it.
So the docs claim that the difference is the Arrow memory the JVM allocated
itself was too strong.
Two changes in response.
**Narrowed the claim.** `jvm_arrow_imported` is now described as what the
FFI import path holds,
which is mostly imported buffers plus a little JVM-allocated import
overhead, making
`jvm_arrow_allocated - jvm_arrow_imported` a close lower bound rather than
an exact split. The
tracing guide names both contaminants with their magnitudes, and states that
the two counters are
separate reads of process-wide state, so they are neither an atomic
per-query balance nor a measure
of RSS. The scaladoc on `CometArrowImportAllocator` and
`Tracing.logArrowMemory` says the same.
Worth noting the direction: these bytes make the reported JVM-own figure an
underestimate, so the
unaccounted memory this PR exists to surface is marginally larger than the
counter suggests, not
smaller.
**Fixed the UDF case,** since that one is both the largest and avoidable.
`CometUdfBridge` now
transfers the result to the root allocator before export when the UDF
allocated it elsewhere.
`TransferPair.transfer()` re-parents buffer ownership through
`referenceManager.transferOwnership(...)` without copying the payload, so
this costs an ownership
move rather than a copy, and the transferred vector is closed alongside the
emptied original.
**Tests,** both of the cases you asked for:
- A characterization test in `NativeUtilSuite` asserting the import
allocator holds strictly more
than the sum of the imported buffers, which pins the struct and validity
overhead rather than
pretending it is absent.
- A regression test in `CometUdfBridgeSuite` with a UDF that allocates its
output from
`inputs.head.getAllocator()`. It asserts before the exported structs are
released, since that is
the window in which the export keeps the buffers alive and the miscounting
is observable. It
failed at 32768 bytes charged to the import allocator before the fix and
passes after it.
I also added the import child to the allocator inventory in the memory
management guide, as
suggested.
On making the child exclusively foreign: I looked at it and decided against
it. Both the struct and
the validity synthesis happen inside Arrow's own import path using the
single allocator we pass, so
excluding them would mean vendoring further into Arrow's importer and
re-deriving that logic, which
would be fragile across Arrow upgrades for a few hundred bytes per batch.
Narrowing the claim and
removing the one unbounded contaminant seemed the better trade, but I am
happy to revisit if you
disagree.
On the performance question: tracing is off by default, so the added JNI
calls only occur when it is
enabled. A traced TPC-H SF100 run on 2 executors x 8 cores (Spark 4.1.1, 16g
off-heap, build with
`jemalloc,alloc-accounting`) completed its iteration in 208.6 s against a
roughly 220 s untraced
baseline on the same machine, so the two extra calls per sample are inside
run-to-run variation. For
reference, that run measured peak `native_allocated` 2193.7 MB,
`jvm_arrow_allocated` 144.0 MB and
`jvm_arrow_imported` 53.7 MB, with 0 of 16970 paired samples showing
imported exceeding allocated.
--
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]
