Review/comment needed for the new public java.util.Base64 class (original) (raw)

Ulf Zibis Ulf.Zibis at CoSoCo.de
Sat Nov 3 20:33:42 UTC 2012

Am 30.10.2012 23:40, schrieb Xueming Shen:

I'm "confused" about the order of xxcode() and Xxcoder. In other places e.g. charsets, we have de... before en..., which is also true alphabetical

should not be an issue. javadoc output should be in alphabetic order. If it is really concerns you, I can do a copy/paste:-)

Yes, it doesn't matter much, but would be a nice conform style, so for me "personally" I would like the copy/paste:-)

- What (should) happen(s), if lineSeparator.length == 0 ? do nothing. expected?

I would say, empty line separator should not be allowed, so you might check: Objects.requireNonEmpty(lineSeparator);

603 static { 604 Arrays.fill(fromBase64, -1); 605 for (int i = 0; i < Encoder.toBase64.length; i++) 606 fromBase64[Encoder.toBase64[i]] = i; 607 fromBase64['='] = -2; 608 } This causes the inner Encoder class to be loaded, even if not needed. So maybe move the toBase64 table to the outer class. Have you compared performance with fromBase64 as byte[] (including local 'b' in decode() as byte)? understood.

It seems you have mixed 2 tweaks to one. ;-) See additional paragraph at the end ...

but it appears the hotspot likes it the constant/fixed length lookup table is inside encoder. Yes, but see my suggestion 12 lines below.

Same as you suggestion in your previous email to use String in source and expand it during runtime. It saves about 500 bytes but slows the server vm.

Are you really sure? As it only runs once at class init, JIT should not compile this code. Executing the bytecode to init the final int[], value for value, by interpreter should not be faster as expanding a String source in a loop.

Those repeating lines of "isURL? ...." is also supposed to be a performance tweak to eliminate the array boundary check in loop.

Yep, so I was thinking about: 653 private final int[] base64; 654 private final boolean isMIME; 655 656 private Decoder(boolean isURL, boolean isMIME) { 657 base64 = isURL ? fromBase64URL : fromBase64; 658 this.isMIME = isMIME; 659 } ..... 911 private int decodeBuffer(ByteBuffer src, ByteBuffer dst) { 912 int[] base64 = this.base64; // local copy for performance reasons (but maybe superfluous if HotSpot is intelligent enough) or: 911 private int decodeBuffer(ByteBuffer src, ByteBuffer dst, int[] base64) { .....

Why you mix the algorithms to check the padding? : 824 if (b == -2) { // padding byte 890 if (b == '=') {

It is supposed reduce one "if" check for normal base64 character inside the loop. I'm not that sure it really helps though.

925 if (b == '=') { --> causes one additional "if" in the main path of the loop, so should be slower for regular input

859 if (b == -2) { // padding byte --> causes one additional "if" in the side path of the loop, so should only affect irregular input Maybe the following code is little faster as no loading of the constant '-2' is required: 858 if ((b = base64[b]) < 0) { 859 if (++b < 0) { // padding byte '='

Once again (the following was meant for the decode algorithm, not initialisation): Have you compared performance with fromBase64 as byte[] (including local 'b' in decode() as byte)? Retrieving the bytes by b = base64[x] then could be done without address-shift and smaller/faster LoadB operations could be used by JIT. In an int[] table, the index offset must be shifted by 2 before. Maybe this doesn't directly affect the performance on x86/IA64 CPU, but wastes space in CPU cache for other tasks as a side effect. On ARM architectures I imagine, directly addressing a byte-stepped table could be faster than addressing a 4-byte-stepped table. At least the footprint of the table would be smaller.

Little nit: You could delete line 641 for conformity with 629.

-Ulf

More information about the core-libs-dev mailing list