rustyconover opened a new pull request, #965: URL: https://github.com/apache/arrow-nanoarrow/pull/965
When `ArrowIpcArrayStreamReader` reads from an input stream created with `ArrowIpcInputStreamInitBuffer()`, 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 and `use_shared_buffers = 0` behave exactly as before. Things worth a look in review: - Decoded arrays now keep the whole input buffer alive, not just their own message body. That's the tradeoff `use_shared_buffers` already makes within a message, and the input was already resident. I've noted it in the `ArrowIpcInputStreamInitBuffer()` docs. - A body is only referenced in place if it starts at an 8-byte-aligned address. Bodies are 8-byte aligned within a valid stream, so this only fails when the caller's buffer itself starts at an odd address. In that case the reader falls back to copying, so decoded arrays stay as aligned as they are today. - R's `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()` in `dev/benchmarks` does: it reads the whole stream from a borrowed in-memory buffer. Fixtures are from `generate-fixtures.py`, plus a 23 MB stream of 1M rows (int32, double, string). Release build, Apple M3, median of 50 runs: | fixture | main | this PR | |---|---|---| | float64_basic (10.5 MB, 2 batches) | 0.52 ms | 0.004 ms | | float64_long (10.5 MB, 20 batches) | 0.16 ms | 0.006 ms | | float64_wide (10.6 MB, 1280 columns) | 0.58 ms | 0.35 ms | | 1M rows int32/double/string | 0.66 ms | 0.28 ms | 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_DEFAULT` takes 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 to `dev/benchmarks` if that's useful. Tests: `StreamReaderBufferInputSharedBuffers` covers these cases: - With shared buffers, decoded values point into the input buffer. - Without shared buffers, or with a misaligned input, they are copied (and still aligned). - An input that is already a shared buffer works. - Arrays stay valid after the stream has been released. 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 on `main` under ASan. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
