[
https://issues.apache.org/jira/browse/THRIFT-4555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16440465#comment-16440465
]
ASF GitHub Bot commented on THRIFT-4555:
----------------------------------------
dcelasun closed pull request #1540: THRIFT-4555 Optionally disable copies of
binary fields in Java constructors, getters, and setters
URL: https://github.com/apache/thrift/pull/1540
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/compiler/cpp/src/thrift/generate/t_java_generator.cc
b/compiler/cpp/src/thrift/generate/t_java_generator.cc
index 754601f508..34ba65f065 100644
--- a/compiler/cpp/src/thrift/generate/t_java_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_java_generator.cc
@@ -72,6 +72,7 @@ class t_java_generator : public t_oop_generator {
undated_generated_annotations_ = false;
suppress_generated_annotations_ = false;
handle_runtime_exceptions_ = false;
+ unsafe_binaries_ = false;
for( iter = parsed_options.begin(); iter != parsed_options.end(); ++iter) {
if( iter->first.compare("beans") == 0) {
bean_style_ = true;
@@ -103,6 +104,8 @@ class t_java_generator : public t_oop_generator {
} else {
throw "unknown option java:" + iter->first + "=" + iter->second;
}
+ } else if( iter->first.compare("unsafe_binaries") == 0) {
+ unsafe_binaries_ = true;
} else {
throw "unknown option java:" + iter->first;
}
@@ -411,6 +414,7 @@ class t_java_generator : public t_oop_generator {
bool undated_generated_annotations_;
bool suppress_generated_annotations_;
bool handle_runtime_exceptions_;
+ bool unsafe_binaries_;
};
@@ -934,8 +938,12 @@ void
t_java_generator::generate_union_constructor(ofstream& out, t_struct* tstru
<< "(byte[] value) {" << endl;
indent(out) << " " << type_name(tstruct) << " x = new " <<
type_name(tstruct) << "();"
<< endl;
- indent(out) << " x.set" << get_cap_name((*m_iter)->get_name())
- << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+ indent(out) << " x.set" << get_cap_name((*m_iter)->get_name());
+ if(unsafe_binaries_) {
+ indent(out) << "(java.nio.ByteBuffer.wrap(value));" << endl;
+ }else{
+ indent(out) << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+ }
indent(out) << " return x;" << endl;
indent(out) << "}" << endl << endl;
}
@@ -977,9 +985,17 @@ void
t_java_generator::generate_union_getters_and_setters(ofstream& out, t_struc
<< get_cap_name(field->get_name()) << "() {" << endl;
indent(out) << " if (getSetField() == _Fields." <<
constant_name(field->get_name()) << ") {"
<< endl;
- indent(out)
+
+ if(unsafe_binaries_){
+ indent(out)
+ << " return (java.nio.ByteBuffer)getFieldValue();"
+ << endl;
+ }else{
+ indent(out)
<< " return
org.apache.thrift.TBaseHelper.copyBinary((java.nio.ByteBuffer)getFieldValue());"
<< endl;
+ }
+
indent(out) << " } else {" << endl;
indent(out) << " throw new java.lang.RuntimeException(\"Cannot get
field '" << field->get_name()
<< "' because union is currently set to \" +
getFieldDesc(getSetField()).name);"
@@ -1013,8 +1029,14 @@ void
t_java_generator::generate_union_getters_and_setters(ofstream& out, t_struc
}
indent(out) << "public void set" << get_cap_name(field->get_name()) <<
"(byte[] value) {"
<< endl;
- indent(out) << " set" << get_cap_name(field->get_name())
- << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+ indent(out) << " set" << get_cap_name(field->get_name());
+
+ if(unsafe_binaries_){
+ indent(out) << "(java.nio.ByteBuffer.wrap(value));" << endl;
+ }else{
+ indent(out) << "(java.nio.ByteBuffer.wrap(value.clone()));" << endl;
+ }
+
indent(out) << "}" << endl;
out << endl;
@@ -1544,9 +1566,15 @@ void
t_java_generator::generate_java_struct_definition(ofstream& out,
if ((*m_iter)->get_req() != t_field::T_OPTIONAL) {
t_type* type = get_true_type((*m_iter)->get_type());
if (type->is_binary()) {
- indent(out) << "this." << (*m_iter)->get_name()
- << " = org.apache.thrift.TBaseHelper.copyBinary(" <<
(*m_iter)->get_name()
- << ");" << endl;
+ if(unsafe_binaries_){
+ indent(out) << "this." << (*m_iter)->get_name()
+ << " = " << (*m_iter)->get_name()
+ << ";" << endl;
+ }else{
+ indent(out) << "this." << (*m_iter)->get_name()
+ << " = org.apache.thrift.TBaseHelper.copyBinary(" <<
(*m_iter)->get_name()
+ << ");" << endl;
+ }
} else {
indent(out) << "this." << (*m_iter)->get_name() << " = " <<
(*m_iter)->get_name() << ";"
<< endl;
@@ -2406,8 +2434,13 @@ void
t_java_generator::generate_java_bean_boilerplate(ofstream& out, t_struct* t
indent(out) << "public java.nio.ByteBuffer buffer" <<
get_cap_name("for") << cap_name << "() {"
<< endl;
- indent(out) << " return org.apache.thrift.TBaseHelper.copyBinary(" <<
field_name << ");"
- << endl;
+ if(unsafe_binaries_){
+ indent(out) << " return " << field_name << ";"
+ << endl;
+ }else {
+ indent(out) << " return org.apache.thrift.TBaseHelper.copyBinary(" <<
field_name << ");"
+ << endl;
+ }
indent(out) << "}" << endl << endl;
} else {
if (optional) {
@@ -2468,8 +2501,14 @@ void
t_java_generator::generate_java_bean_boilerplate(ofstream& out, t_struct* t
out << type_name(tstruct);
}
out << " set" << cap_name << "(byte[] " << field_name << ") {" << endl;
- indent(out) << " this." << field_name << " = " << field_name << " ==
null ? (java.nio.ByteBuffer)null"
- << " : java.nio.ByteBuffer.wrap(" << field_name <<
".clone());" << endl;
+ indent(out) << " this." << field_name << " = " << field_name << " ==
null ? (java.nio.ByteBuffer)null";
+
+ if(unsafe_binaries_){
+ indent(out) << " : java.nio.ByteBuffer.wrap(" << field_name << ");" <<
endl;
+ }else{
+ indent(out) << " : java.nio.ByteBuffer.wrap(" << field_name <<
".clone());" << endl;
+ }
+
if (!bean_style_) {
indent(out) << " return this;" << endl;
}
@@ -2488,7 +2527,7 @@ void
t_java_generator::generate_java_bean_boilerplate(ofstream& out, t_struct* t
<< type_name(type) << " " << field_name << ") {" << endl;
indent_up();
indent(out) << "this." << field_name << " = ";
- if (type->is_binary()) {
+ if (type->is_binary() && !unsafe_binaries_) {
out << "org.apache.thrift.TBaseHelper.copyBinary(" << field_name << ")";
} else {
out << field_name;
@@ -5400,4 +5439,5 @@ THRIFT_REGISTER_GENERATOR(
"set/map.\n"
" generated_annotations=[undated|suppress]:\n"
" undated: suppress the date at @Generated
annotations\n"
- " suppress: suppress @Generated annotations
entirely\n")
+ " suppress: suppress @Generated annotations entirely\n"
+ " unsafe_binaries: Do not copy ByteBuffers in constructors, getters,
and setters.\n")
diff --git a/lib/java/gradle/generateTestThrift.gradle
b/lib/java/gradle/generateTestThrift.gradle
index c3479179b1..2b53739979 100644
--- a/lib/java/gradle/generateTestThrift.gradle
+++ b/lib/java/gradle/generateTestThrift.gradle
@@ -24,10 +24,11 @@ ext.genSrc = file("$buildDir/gen-java")
ext.genBeanSrc = file("$buildDir/gen-javabean")
ext.genReuseSrc = file("$buildDir/gen-javareuse")
ext.genFullCamelSrc = file("$buildDir/gen-fullcamel")
+ext.genUnsafeSrc = file("$buildDir/gen-unsafe")
// Add the generated code directories to the test source set
sourceSets {
- test.java.srcDirs genSrc, genBeanSrc, genReuseSrc, genFullCamelSrc
+ test.java.srcDirs genSrc, genBeanSrc, genReuseSrc, genFullCamelSrc,
genUnsafeSrc
}
// ----------------------------------------------------------------------------
@@ -107,3 +108,12 @@ task generateFullCamelJava(group: 'Build') {
thriftCompile(it, 'ReuseObjects.thrift', 'java:reuse-objects', genReuseSrc)
}
+
+task generateUnsafeBinariesJava(group: 'Build') {
+ description = 'Generate the thrift gen-unsafebinaries source'
+ generate.dependsOn it
+
+ ext.outputBuffer = new ByteArrayOutputStream()
+
+ thriftCompile(it, 'UnsafeTypes.thrift', 'java:unsafe_binaries',
genUnsafeSrc)
+}
diff --git a/lib/java/test/org/apache/thrift/TestUnsafeBinaries.java
b/lib/java/test/org/apache/thrift/TestUnsafeBinaries.java
new file mode 100644
index 0000000000..d1fc213683
--- /dev/null
+++ b/lib/java/test/org/apache/thrift/TestUnsafeBinaries.java
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.thrift;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+
+import thrift.test.SafeBytes;
+import thrift.test.UnsafeBytes;
+
+// test generating types with un-copied byte[]/ByteBuffer input/output
+//
+public class TestUnsafeBinaries extends TestStruct {
+
+ private static byte[] input() {
+ return new byte[]{1, 1};
+ }
+
+ //
+ // verify that the unsafe_binaries option modifies behavior
+ //
+
+ // constructor doesn't copy
+ public void testUnsafeConstructor() throws Exception {
+
+ byte[] input = input();
+ UnsafeBytes struct = new UnsafeBytes(ByteBuffer.wrap(input));
+
+ input[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{2, 1},
+ struct.getBytes())
+ );
+
+ }
+
+ // getter doesn't copy
+ // note: this behavior is the same with/without the flag, but if this
default ever changes, the current behavior
+ // should be retained when using this flag
+ public void testUnsafeGetter(){
+ UnsafeBytes struct = new UnsafeBytes(ByteBuffer.wrap(input()));
+
+ byte[] val = struct.getBytes();
+ val[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{2, 1},
+ struct.getBytes())
+ );
+
+ }
+
+ // setter doesn't copy
+ public void testUnsafeSetter(){
+ UnsafeBytes struct = new UnsafeBytes();
+
+ byte[] val = input();
+ struct.setBytes(val);
+
+ val[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{2, 1},
+ struct.getBytes())
+ );
+
+ }
+
+ // buffer doens't copy
+ public void testUnsafeBufferFor(){
+ UnsafeBytes struct = new UnsafeBytes(ByteBuffer.wrap(input()));
+
+ ByteBuffer val = struct.bufferForBytes();
+ val.array()[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{2, 1},
+ struct.getBytes())
+ );
+
+ }
+
+ //
+ // verify that the default generator does not change behavior
+ //
+
+ public void testSafeConstructor() {
+
+ byte[] input = input();
+ SafeBytes struct = new SafeBytes(ByteBuffer.wrap(input));
+
+ input[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{1, 1},
+ struct.getBytes())
+ );
+
+ }
+
+ public void testSafeSetter() {
+
+ byte[] input = input();
+ SafeBytes struct = new SafeBytes(ByteBuffer.wrap(input));
+
+ input[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{1, 1},
+ struct.getBytes())
+ );
+
+ }
+
+ public void testSafeBufferFor(){
+ SafeBytes struct = new SafeBytes(ByteBuffer.wrap(input()));
+
+ ByteBuffer val = struct.bufferForBytes();
+ val.array()[0] = 2;
+
+ assertTrue(Arrays.equals(
+ new byte[]{1, 1},
+ struct.getBytes())
+ );
+
+ }
+
+}
diff --git a/test/JavaTypes.thrift b/test/JavaTypes.thrift
index fcb0ab2a63..8c733ada8b 100644
--- a/test/JavaTypes.thrift
+++ b/test/JavaTypes.thrift
@@ -96,3 +96,8 @@ service AsyncNonblockingService {
7: Map somemap,
) throws (1:Exception ex);
}
+
+struct SafeBytes {
+ 1: binary bytes;
+}
+
diff --git a/test/Makefile.am b/test/Makefile.am
index 7267066b7b..68f198615f 100755
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -155,6 +155,7 @@ EXTRA_DIST = \
StressTest.thrift \
ThriftTest.thrift \
TypedefTest.thrift \
+ UnsafeTypes.thrift \
known_failures_Linux.json \
test.py \
tests.json \
diff --git a/test/UnsafeTypes.thrift b/test/UnsafeTypes.thrift
new file mode 100644
index 0000000000..b38c905eb6
--- /dev/null
+++ b/test/UnsafeTypes.thrift
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+namespace java thrift.test
+
+struct UnsafeBytes {
+ 1: binary bytes;
+}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Getter of binary field in Java creates unnecessary copy
> -------------------------------------------------------
>
> Key: THRIFT-4555
> URL: https://issues.apache.org/jira/browse/THRIFT-4555
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler
> Affects Versions: 0.11.0
> Reporter: Joel Croteau
> Priority: Major
>
> The get[field] method in generated Java code generates a new copy of a binary
> field every time it is called. This seems incredibly inefficient. Take a
> simple example struct:
> {code:java}
> struct StructWithBinary {
> 1: required binary field;
> }
> {code}
> a portion of the generated code for this is:
> {code:java}
> public byte[] getField() {
> setField(org.apache.thrift.TBaseHelper.rightSize(field));
> return field == null ? null : field.array();
> }
> ...
> public StructWithBinary setField(java.nio.ByteBuffer field) {
> this.field = org.apache.thrift.TBaseHelper.copyBinary(field);
> return this;
> }
> {code}
> So whenever getField is called, setField calls copyBinary and generates
> another copy. This adds quite a lot of overhead to the getter here and should
> be fixed.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
