Review/comment needed for the new public java.util.Base64 class
Ulf Zibis
Ulf.Zibis at CoSoCo.de
Fri Nov 30 22:01:09 UTC 2012
Hi Sherman,
is this ssue still open ?
-Ulf
Am 03.11.2012 21:33, schrieb 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
>
>
More information about the core-libs-dev
mailing list