Skip to content

THRIFT-6080: Stop copying a binary field on every call to its byte[] getter #3966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
slachiewicz wants to merge 1 commit into
base: master
Choose a base branch
from
Open

THRIFT-6080: Stop copying a binary field on every call to its byte[] getter #3966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions compiler/cpp/src/thrift/generate/t_java_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2616,8 +2616,10 @@ void t_java_generator::generate_java_bean_boilerplate(ostream& out, t_struct* ts
indent(out) << "@Deprecated" << '\n';
}
indent(out) << "public byte[] get" << cap_name << "() {" << '\n';
indent(out) << " set" << cap_name << "(org.apache.thrift.TBaseHelper.rightSize("
<< field_name << "));" << '\n';
// Assign the right-sized buffer directly: going through the setter would copy it again
// (copyBinary) on every call, although the returned array is the field's own either way.
indent(out) << " this." << field_name << " = org.apache.thrift.TBaseHelper.rightSize("
<< field_name << ");" << '\n';
indent(out) << " return " << field_name << " == null ? null : " << field_name << ".array();"
<< '\n';
indent(out) << "}" << '\n' << '\n';
Expand Down
15 changes: 15 additions & 0 deletions lib/java/src/test/java/org/apache/thrift/TestStruct.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

Expand Down Expand Up @@ -427,4 +429,17 @@ public void testSubStructValidation() throws Exception {
b = new StructB().setAb(valid).setAa(invalid);
assertThrows(TException.class, b::validate);
}

@Test
public void testBinaryGetterDoesNotCopyOnEveryRead() {
OneOfEach ooe = new OneOfEach();
ooe.setBase64(ByteBuffer.wrap(new byte[] {0, 1, 2, 3, 4}, 1, 3));

byte[] first = ooe.getBase64();
assertArrayEquals(new byte[] {1, 2, 3}, first);
// Once right-sized, the field is read as it is, not copied again on each read.
assertSame(first, ooe.getBase64());
// bufferForBase64() still hands out a copy.
assertNotSame(first, ooe.bufferForBase64().array());
}
}
Loading