Skip to content

Avoid unnecessary copies of Thrift binary types#6459

Open
ctubbsii wants to merge 3 commits into
apache:mainfrom
ctubbsii:fix-thrift-binaries
Open

Avoid unnecessary copies of Thrift binary types#6459
ctubbsii wants to merge 3 commits into
apache:mainfrom
ctubbsii:fix-thrift-binaries

Conversation

@ctubbsii

@ctubbsii ctubbsii commented Jul 1, 2026

Copy link
Copy Markdown
Member

No description provided.

@ctubbsii ctubbsii added this to the 4.0.0 milestone Jul 1, 2026
@ctubbsii ctubbsii self-assigned this Jul 1, 2026
@DomGarguilo

Copy link
Copy Markdown
Member

In #6430, I suggested ByteBufferUtil because it preserved the original behavior: the old code read from ByteBuffer fields and made defensive copies. Without that, the new object may reuse Thrift’s internal byte array, so changes to the Thrift object could also change the constructed Key/Column/Mutation.

That being said, if it doesn't seem like that is an issue here, I think this is a good fix for the mentioned issue we are seeing.

@ctubbsii ctubbsii removed this from the 4.0.0 milestone Jul 2, 2026
@ctubbsii

ctubbsii commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Closing due to discussion on #6455 ; I didn't realize the getter was also calling the setter.

@ctubbsii ctubbsii added this to the 4.0.0 milestone Jul 3, 2026
Avoid making copies using the public API setter when truncating the
underlying stream to get the array by assigning the result of the
optional resize operation directly to the variable, and avoiding the
setter that performs the unneeded copy.
@ctubbsii

ctubbsii commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Based on the discussion in #6455, I've updated this PR with an additional patch to the generated code that fixes what appears to be a bug in the upstream thrift that makes an unneeded extra copy whenever the byte array getter is called.

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