Review/comment needed for the new public java.util.Base64 class

[Ulf Zibis]

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