Skip to content

Commit 4827725

Browse files
rustyconoverclaude
andcommitted
perf(rpc): rewrite SerializeToIpcBytes around the payload API
SerializeToIpcBytes was the canonical "Arrow batch -> std::vector<uint8_t>" helper used by every aggregate, table_in_out, table_buffering, bind and init code path. The body wrote into a BufferOutputStream via MakeStreamWriter (which geometrically grows the underlying heap buffer through realloc + memcpy as bytes accumulate), then Finish()'d to a Buffer, then copied that Buffer into a freshly allocated std::vector. Two effectively redundant allocations and one full copy per call. Drive arrow::ipc's payload API by hand instead: GetSchemaPayload -> schema FlatBuffer once CollectDictionaries / -> dictionary batch payloads (empty for non-dict GetDictionaryPayload schemas, no extra cost) GetRecordBatchPayload -> batch FlatBuffer + body buffers once GetPayloadSize -> exact size for each std::vector<uint8_t> allocated once at the precomputed total WriteIpcPayload writes each piece into a FixedSizeBufferWriter slice of the destination Single Assemble pass, no realloc chain, no final copy. Dictionary handling is the subtle part. The Arrow IPC streaming format requires a dictionary-batch message between the schema and record-batch messages for every column that uses a dictionary encoding (DuckDB enums, some struct fields). MakeStreamWriter's IpcFormatWriter handles this via WriteDictionaries(batch); replicating it here is mandatory or every enum-bearing table fails to round-trip. The first attempt at this commit skipped the dictionary messages and broke ~20 integration tests (filter_pushdown/enums, settings/*, catalog/multi_branch_*, aggregate/ nest_tensor) with "Tried reading schema message, was null or length 0" on the worker side. The CollectDictionaries + GetDictionaryPayload loop fixes that; the full integration suite (178 cases, 7670 assertions) passes again. Verified byte-identical output against the old MakeStreamWriter path on BIGINT, string, multi-column, empty-struct, list, struct-of-int+string, and dictionary-of-int8+utf8 schemas. Measured win (mimalloc pool, 15-iter steady state, python+subprocess): aggregate_sum t=1, 100k rows +5.3% aggregate_sum t=1, 1M rows +3.3% aggregate_sum t=4, 100k rows +5.0% aggregate_sum t=4, 1M rows +1.9% table_in_out_sum_all t=4, 100k rows +13.2% table_in_out_sum_all t=4, 1M rows +11.8% table_in_out_echo t=4, 1M rows, 1KB payload +4.0% (others within +-2% noise floor) The table_in_out_sum_all numbers are the headline: that function calls SerializeToIpcBytes per input batch on every duckdb worker thread, so eliminating one Assemble pass plus the growing-buffer realloc chain compounds across threads. Cold paths (bind, init) also benefit but their share of the workload is negligible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 195fb13 commit 4827725

1 file changed

Lines changed: 70 additions & 20 deletions

File tree

src/vgi_rpc_types.cpp

Lines changed: 70 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -257,27 +257,77 @@ BuildOptionalStringMapScalar(const std::optional<std::vector<std::pair<std::stri
257257
// ============================================================================
258258

259259
std::vector<uint8_t> SerializeToIpcBytes(const std::shared_ptr<arrow::RecordBatch> &batch) {
260-
auto sink_result = arrow::io::BufferOutputStream::Create();
261-
if (!sink_result.ok()) {
262-
throw IOException("Failed to create buffer: " + sink_result.status().ToString());
263-
}
264-
auto sink = sink_result.ValueUnsafe();
265-
266-
auto writer_result = arrow::ipc::MakeStreamWriter(sink, batch->schema());
267-
if (!writer_result.ok()) {
268-
throw IOException("Failed to create IPC writer: " + writer_result.status().ToString());
269-
}
270-
auto writer = writer_result.ValueUnsafe();
271-
272-
CheckStatus(writer->WriteRecordBatch(*batch), "write batch to IPC");
273-
CheckStatus(writer->Close(), "close IPC writer");
260+
// Single-allocation path: drive the payload-level Arrow APIs by hand so
261+
// RecordBatchSerializer::Assemble runs ONCE, GetPayloadSize tells us the
262+
// exact total bytes, and we allocate the destination std::vector at the
263+
// correct size up front. Skips the realloc chain that
264+
// BufferOutputStream + MakeStreamWriter incur and the extra
265+
// vector-from-buffer copy the prior implementation did at the end.
266+
// Wire bytes are identical to what MakeStreamWriter+WriteRecordBatch+Close
267+
// would have produced (same primitive Arrow calls underneath).
268+
//
269+
// Dictionary columns require an extra dictionary-batch message between
270+
// the schema and record batch messages (IpcFormatWriter::WriteDictionaries
271+
// in MakeStreamWriter's implementation). We replicate that ordering
272+
// explicitly via CollectDictionaries + GetDictionaryPayload so enum/dict
273+
// schemas (e.g. DuckDB enums) round-trip correctly.
274+
const auto &options = arrow::ipc::IpcWriteOptions::Defaults();
275+
arrow::ipc::DictionaryFieldMapper mapper(*batch->schema());
276+
277+
arrow::ipc::IpcPayload schema_payload;
278+
CheckStatus(arrow::ipc::GetSchemaPayload(*batch->schema(), options, mapper, &schema_payload),
279+
"build schema payload");
280+
281+
// Collect dictionary payloads (empty for non-dict schemas → no extra cost).
282+
auto dictionaries_result = arrow::ipc::CollectDictionaries(*batch, mapper);
283+
if (!dictionaries_result.ok()) {
284+
throw IOException("Arrow collect dictionaries failed: %s",
285+
dictionaries_result.status().ToString());
286+
}
287+
const auto dictionaries = std::move(dictionaries_result).ValueUnsafe();
288+
std::vector<arrow::ipc::IpcPayload> dict_payloads(dictionaries.size());
289+
for (size_t i = 0; i < dictionaries.size(); ++i) {
290+
CheckStatus(arrow::ipc::GetDictionaryPayload(dictionaries[i].first, dictionaries[i].second,
291+
options, &dict_payloads[i]),
292+
"build dictionary payload");
293+
}
294+
295+
arrow::ipc::IpcPayload batch_payload;
296+
CheckStatus(arrow::ipc::GetRecordBatchPayload(*batch, /*custom_metadata=*/nullptr, options, &batch_payload),
297+
"build record-batch payload");
298+
299+
// EOS marker: 4-byte continuation token 0xFFFFFFFF + 4-byte zero length.
300+
// Matches arrow::ipc PayloadStreamWriter::WriteEOS (non-legacy format).
301+
static constexpr uint8_t kEosMarker[8] = {0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00};
302+
const int64_t schema_size = arrow::ipc::GetPayloadSize(schema_payload, options);
303+
std::vector<int64_t> dict_sizes(dict_payloads.size());
304+
int64_t dicts_total = 0;
305+
for (size_t i = 0; i < dict_payloads.size(); ++i) {
306+
dict_sizes[i] = arrow::ipc::GetPayloadSize(dict_payloads[i], options);
307+
dicts_total += dict_sizes[i];
308+
}
309+
const int64_t batch_size = arrow::ipc::GetPayloadSize(batch_payload, options);
310+
const int64_t total = schema_size + dicts_total + batch_size + static_cast<int64_t>(sizeof(kEosMarker));
311+
312+
std::vector<uint8_t> out(static_cast<size_t>(total));
313+
int32_t mlen = 0; // discarded; only here to satisfy the API
314+
auto write_payload_at = [&](const arrow::ipc::IpcPayload &p, int64_t offset, int64_t size, const char *what) {
315+
auto slice = std::make_shared<arrow::MutableBuffer>(out.data() + offset, size);
316+
auto sink = std::make_shared<arrow::io::FixedSizeBufferWriter>(slice);
317+
CheckStatus(arrow::ipc::WriteIpcPayload(p, options, sink.get(), &mlen), what);
318+
};
274319

275-
auto finish_result = sink->Finish();
276-
if (!finish_result.ok()) {
277-
throw IOException("Failed to finish IPC buffer: " + finish_result.status().ToString());
278-
}
279-
auto buffer = finish_result.ValueUnsafe();
280-
return std::vector<uint8_t>(buffer->data(), buffer->data() + buffer->size());
320+
int64_t cursor = 0;
321+
write_payload_at(schema_payload, cursor, schema_size, "write schema payload");
322+
cursor += schema_size;
323+
for (size_t i = 0; i < dict_payloads.size(); ++i) {
324+
write_payload_at(dict_payloads[i], cursor, dict_sizes[i], "write dictionary payload");
325+
cursor += dict_sizes[i];
326+
}
327+
write_payload_at(batch_payload, cursor, batch_size, "write record-batch payload");
328+
cursor += batch_size;
329+
std::memcpy(out.data() + cursor, kEosMarker, sizeof(kEosMarker));
330+
return out;
281331
}
282332

283333
std::shared_ptr<arrow::RecordBatch> DeserializeFromIpcBytes(const uint8_t *data, size_t len) {

0 commit comments

Comments
 (0)