Skip to content

perf(arrow/array): copy view buffers directly - #1324

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:perf/arrow-view-concat-direct
Open

fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:perf/arrow-view-concat-direct

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What

  • Copy BinaryView headers and ListView sizes directly from ArrayData during concatenation.

Why

  • Avoid one temporary memory.Buffer wrapper per input chunk.
  • Reduce allocations for fragmented view arrays.

Implementation

  • Use the existing concatFixedWidthBuffers helper for BinaryView headers and ListView/LargeListView sizes.
  • Added a benchmark for 64, 1,024, and 8,192 chunks.
  • Existing concatenate tests cover sliced and nullable view arrays.

Benchmark on an Apple M1 Pro:

Case Runtime before -> after Allocations before -> after
BinaryView, 1,024 chunks 1.02ms -> 0.93ms 1,050 -> 25
BinaryView, 8,192 chunks roughly flat 8,227 -> 34
ListView, 8,192 chunks 2.27ms -> 1.92ms 26,477 -> 18,388
LargeListView, 8,192 chunks 2.28ms -> 1.95ms 26,124 -> 17,304

Tests:

  • go test ./arrow/array -count=1
  • go vet ./arrow/array
  • git diff --check

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concatFixedWidthBuffers introduces a recovered-panic allocation leak for malformed BinaryView/StringView data. It allocates and resizes the destination before slicing each source header buffer. If a non-empty source has a short non-nil header buffer, that slice panics; concat recovers and returns an error, but the local destination has not yet been assigned to out.buffers[1], so out.Release() cannot free it.

I reproduced this on the current head with CheckedAllocator: one malformed one-element BinaryView leaked 64 bytes from concatFixedWidthBuffers. The old path sliced all sources in gatherFixedBuffers before allocating the destination. Please add failure cleanup around the new allocation (or publish it to the output before a bounds-sensitive operation) and cover the recovered short-header path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants