Repository navigation
feat(ipc): Reference message bodies in place when reading from a buffer - #965
rustyconover wants to merge 1 commit into
Conversation
When an ArrowIpcArrayStreamReader using shared buffers reads from an input stream created with ArrowIpcInputStreamInitBuffer(), slice each message body out of the input buffer instead of copying it. The input buffer is held as a shared buffer, so arrays decoded from it keep it alive after the stream has been released. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
==========================================
- Coverage 78.24% 78.24% -0.01%
==========================================
Files 106 106
Lines 16867 16902 +35
Branches 1993 2002 +9
==========================================
+ Hits 13198 13225 +27
- Misses 2437 2440 +3
- Partials 1232 1237 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
paleolimbot
left a comment
There was a problem hiding this comment.
This is a great idea; however, I think we should add a callback to the ArrowIpcInputStream (or update the existing one) to handle this case, which comes up when reading files. Maybe something like
ArrowErrorCode (*peek_at)(struct ArrowIpcInputStream* stream,
int64_t offset_from_current,
int64_t buf_size_bytes,
const uint8_t** buf,
struct ArrowError* error);
ArrowErrorCode (*read_at)(struct ArrowIpcInputStream* stream,
int64_t offset_from_current,
int64_t buf_size_bytes, struct ArrowBuffer* out,
struct ArrowError* error);Those callbacks can return ENOTSUP if IO would otherwise be required but the reader could try them (the first variant for metadata to avoid a copy, and the second variant for buffers). There's one other case we eventually have to handle which is firing off a bunch of concurrentread_at and waiting for the result; however, I don't think we need to handle that here.
Also, while bodies usually contain aligned buffers when the bodies themselves are aligned, there's no guarantee of this. We can provide a separate ArrowArrayEnsureAligned() that recursively walks the array and ensures buffers are aligned to at least the fixed width of their type.
|
Thanks, that makes sense: callbacks on the input stream are a better fit than special-casing the buffer stream, and they cover files too. Two questions before I rework it. How should the struct gain the callbacks?
My preference is (2), unless nanoarrow is fine with breaking out-of-tree streams in a release. Semantics. For Alignment: agreed. I'd write |
|
It's a good point, and (2) does seem safer since it's not an obvious compilation break. I am not sure how that interacts with the existing reader in terms of what needs to be rewritten specifically for the random access reader. As long as this doesn't add a fully duplicate path to the array stream reader (which I think can be done), go for it!
advancing the stream position I think is separate (probably need a seek) |
When
ArrowIpcArrayStreamReaderreads from an input stream created withArrowIpcInputStreamInitBuffer(), it copies every message body out of the input buffer into a newly allocated buffer before decoding it. With shared buffers, which are the default wherever C11 atomics are available, the decoded arrays then reference that copy. The bytes were already in memory and owned by the stream, so the copy isn't needed.With this PR, the buffer input stream holds its input as a shared buffer, or takes it over directly if it already is one. When the reader uses shared buffers, it clones and slices each message body out of that buffer. This is the same way
ArrowIpcDecoderDecodeArrayFromShared()already slices a body into array buffers. There's no API change. File input, other input streams anduse_shared_buffers = 0behave exactly as before.Things worth a look in review:
use_shared_buffersalready makes within a message, and the input was already resident. I've noted it in theArrowIpcInputStreamInitBuffer()docs.read_nanoarrow()on a raw vector goes through this path with default options, so it benefits too. Its buffers are released through the preserve registry, which already handles release from other threads. I don't have R set up locally, so I'm relying on CI for the R tests.Timings come from a harness that does what
BaseBenchmarIpcFixtureBuffer()indev/benchmarksdoes: it reads the whole stream from a borrowed in-memory buffer. Fixtures are fromgenerate-fixtures.py, plus a 23 MB stream of 1M rows (int32, double, string). Release build, Apple M3, median of 50 runs:For the float64 fixtures, the copy was nearly the entire cost of reading. What's left on the wide fixture is per-column decoding, and what's left on the mixed fixture is full validation of the string offsets. Together with #964, reading the mixed fixture at
NANOARROW_VALIDATION_LEVEL_DEFAULTtakes 0.010 ms. That's the case I care about: a WebAssembly build of DuckDB that I'm moving from Arrow C++ to nanoarrow, where Arrow JS hands it IPC streams that are already in memory. I'm happy to add the mixed fixture todev/benchmarksif that's useful.Tests:
StreamReaderBufferInputSharedBufferscovers these cases:The test fails on
main. ctest passes locally (macOS arm64, Debug, extra warnings). Under ASan/UBSan, the IPC and integration tests passed 10 repeated runs. The only failures in the full ASan suite are the six allocation tests that also fail onmainunder ASan.🤖 Generated with Claude Code