Skip to content
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.

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

THRIFT-4555 Optionally disable copies of binary fields in Java constructors, getters, and setters #1540

Merged
merged 1 commit into from Apr 17, 2018

Conversation

bpodgursky
Copy link
Contributor

THRIFT-2233 added safeguards when working with binary fields by performing TBaseHelper.copyBinary on the input bytes in relevant constructors, getters and setters in the Java client. While this is safer, it incurs a heavy overhead cost in GC churn by unnecessarily creating and destroying byte arrays when the input does happen to be safe.

This PR introduces a flag to disable these copies of binary fields, to improve performance in situations where developers are sure that the input (or output) will not later be mutated.

This builds without issue for me locally. This does not currently have any tests; I can add tests if this new flag is plausibly acceptable to the maintainers.

@dcelasun
Copy link
Member

Sounds reasonable to me, but I'd like to get a few more +1s before I merge this.

And yes, please add tests.

@dcelasun dcelasun requested a review from jeking3 April 13, 2018 08:31
@bpodgursky bpodgursky force-pushed the allow_unsafe_binaries branch 2 times, most recently from e16ee80 to 7d38cf7 Compare April 15, 2018 05:14
@bpodgursky
Copy link
Contributor Author

@dcelasun I have added tests. Let me know if anything else would be helpful.

Is there anyone else I should tag to pull in a couple other +1s?

@dcelasun
Copy link
Member

dcelasun commented Apr 16, 2018

There seems to be a related CI failure:

FAILED: lib/java/CMakeFiles/ThriftJava 
cd /thrift/src/thrift-1.0.0-dev/lib/java && /thrift/src/thrift-1.0.0-dev/lib/java/gradlew assemble --console=plain --no-daemon -Prelease=true -Pthrift.version=1.0.0 -Pbuild.dir=/thrift/src/thrift-1.0.0-dev/cmake_build/lib/java/build

[...]

FAILURE: Build failed with an exception.
* Where:
Script '/thrift/src/thrift-1.0.0-dev/lib/java/gradle/generateTestThrift.gradle' line: 40
* What went wrong:
A problem occurred evaluating script.
> assert thriftFile.exists()
         |          |
         |          false
         /thrift/src/thrift-1.0.0-dev/test/UnsafeTypes.thrift

@bpodgursky
Copy link
Contributor Author

Is there a way to force a rebuild? I'm unable to reproduce this error locally, and that file is definitely in the PR. Maybe travis got confused by a rebase?

@dcelasun
Copy link
Member

Just restarted the build, let's see how it goes.

@bpodgursky
Copy link
Contributor Author

Ok, I actually had missed one place where the thrift file needed to be referenced. The CI builds are all passing now.

@dcelasun dcelasun merged commit 50bfc56 into apache:master Apr 17, 2018
@dcelasun
Copy link
Member

LGTM, merged.

@bpodgursky
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants