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

For performance reasons, use `java.util.Base64` instead of Base64 in Apache Commons Codec and Guava #6913

Merged
merged 3 commits into from Jan 26, 2019

Conversation

Projects
None yet
4 participants
@asdf2014
Copy link
Member

asdf2014 commented Jan 25, 2019

  • Add few methods about base64 into StringUtils
  • Use java.util.Base64 instead of others
  • Add org.apache.commons.codec.binary.Base64 & com.google.common.io.BaseEncoding.base64 into druid-forbidden-apis
* Add few methods about base64 into StringUtils
* Use `java.util.Base64` instead of others

* Add org.apache.commons.codec.binary.Base64 & com.google.common.io.BaseEncoding into druid-forbidden-apis
@clintropolis
Copy link
Member

clintropolis left a comment

LGTM, minor suggestion about method naming to make the functions behavior a bit more clear and symmetrical.

Thanks for doing this! 👍

* @param input The byte array to convert to base64
* @return the base64 of the input in string form
*/
public static String encodeBase64ToString(byte[] input)

This comment has been minimized.

@clintropolis

clintropolis Jan 25, 2019

Member

Nit: I think encodeToBase64String would read a bit more smoothly and more like the java.util version encodeToString than the current encodeBase64ToString. It would also be nice if it matched the other encode method, so maybe change it to encodeToBase64, or change this one to encodeBase64String and leave the other as is.

Likewise, calling one of the decode methods out explicitly as the string one would be nice, maybe decodeBase64 and decodeBase64String?

This comment has been minimized.

@asdf2014

asdf2014 Jan 25, 2019

Author Member

Hi, @clintropolis . Thanks for your comment. Indeed, encodeBase64String would be better.

Likewise, calling one of the decode methods out explicitly as the string one would be nice, maybe decodeBase64 and decodeBase64String?

Hmmm... About this, do you mean code implementation like the following?

public static String decodeBase64String(byte[] input)
{
  return fromUtf8(BASE64_DECODER.decode(input));
}

This comment has been minimized.

@clintropolis

clintropolis Jan 25, 2019

Member

Oh for the decode methods, i mean to distinguish the one that takes a String argument from the one that takes a byte[] argument since both are just named decodeBase64 right now

This comment has been minimized.

@asdf2014

asdf2014 Jan 25, 2019

Author Member

I see. I feel the same. Thx.

@clintropolis
Copy link
Member

clintropolis left a comment

LGTM 👍

@jihoonson

This comment has been minimized.

Copy link
Contributor

jihoonson commented Jan 25, 2019

@asdf2014 would you please include this performance comparison in this PR as well? Also, the benchmark was conducted almost 5 years ago. Is the benchmark result still valid?

@clintropolis

This comment has been minimized.

Copy link
Member

clintropolis commented Jan 26, 2019

I re-ran code that made those benchmarks pasted in the other PR with the versions of libs we are using, results look the same-ish to me.

bytes:

NameEncode, 100 bytesDecode, 100 bytesEncode, 1000 bytesDecode, 1000 bytesEncode, 200000000 bytesDecode, 200000000 bytes
ApacheImpl3.813 sec5.74 sec1.634 sec1.88 sec2.004 sec1.632 sec
GuavaImpl2.595 sec2.624 sec2.186 sec1.799 sec2.779 sec2.397 sec
Java8Impl0.468 sec0.695 sec0.261 sec0.584 sec0.52 sec0.693 sec
strings:
NameEncode, 100 bytesDecode, 100 bytesEncode, 1000 bytesDecode, 1000 bytesEncode, 200000000 bytesDecode, 200000000 bytes
ApacheImpl3.183 sec3.452 sec1.796 sec2.209 sec1.521 sec1.876 sec
GuavaImpl2.0 sec1.647 sec2.25 sec1.635 sec2.225 sec1.497 sec
Java8Impl0.471 sec1.252 sec0.455 sec0.798 sec0.398 sec0.729 sec
@clintropolis

This comment has been minimized.

Copy link
Member

clintropolis commented Jan 26, 2019

Amusingly/coincidentally, our versions of guava and commons-codec are both that old or older than the original benchmarks (which appear to be released 2014 and 2012 respectively)

@jihoonson
Copy link
Contributor

jihoonson left a comment

@clintropolis thanks for the benchmark! @asdf2014 LGTM, thanks!

@clintropolis clintropolis merged commit 72a571f into apache:master Jan 26, 2019

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:base64 branch Jan 26, 2019

@jon-wei jon-wei added this to the 0.14.0 milestone Feb 20, 2019

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