RFR: 8248188: Add HotSpotIntrinsicCandidate and API for Base64 decoding

[CoreyAshford]

On Fri, 25 Sep 2020 13:45:15 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> This patch set encompasses the following commits:
>> 
>> - Adds a new HotSpot intrinsic candidate to the java.lang.Base64 class - decodeBlock(), and provides a flexible API for
>>   the intrinsic.  The API is similar to the existing encodeBlock intrinsic.
>> - Adds the code in HotSpot to check and martial the new intrinsic's arguments to the arch-specific intrinsic
>>   implementation
>> - Adds a Power64LE-specific implementation of the decodeBlock intrinsic.
>> - Adds a JMH microbenchmark for both Base64 encoding and encoding.
>> - Enhances the JTReg hotspot intrinsic "TestBase64.java" regression test to more fully test both decoding and encoding.
>
> src/java.base/share/classes/java/util/Base64.java line 747:
> 
>> 745:          * Decodes base64 characters, and returns the number of data bytes
>> 746:          * produced by the base64 decode intrinsic.
>> 747:          *
> 
> "intrinsic" can be omitted. Both the java and the intrinsic produce the same output.

Good point.  Will fix.

> src/java.base/share/classes/java/util/Base64.java line 759:
> 
>> 757:          * src, it must process a multiple of four of them, making the
>> 758:          * returned destination length a multiple of three.
>> 759:          *
> 
> If the dst array is too short does the intrinsic stop short or throw?
> The java code would throw an ArrayIndexOutOfBoundsException.
> It should not occur since the java code allocates the proper buffer but...
> It might be worth mentioning.

The intrinsic doesn't have enough information to know how long the dst buffer is.  It only knows the starting address
and the offset from that starting address; it has to trust the caller.  encodeBlock() has the same limitation.  I will
add a comment to that effect.

> src/java.base/share/classes/java/util/Base64.java line 820:
> 
>> 818:                     dp += dl;
>> 819:                 }
>> 820:                 if (sp == sl) {
> 
> I'd rather see  (sp >= s1) instead of the equality, its would be consistent with the condition of the while loop.
> (And is already lines 767-768).

Agreed.  Will change that comparison.

> test/hotspot/jtreg/compiler/intrinsics/base64/TestBase64.java line 260:
> 
>> 258:     }
>> 259:
>> 260:     private static final byte[] nonBase64 = {
> 
> Please add a comment describing this test data.
> (It looks like it could be generated more compactly than an explicit array).
> Ditto nonBase64URL below.

Will do.

> test/hotspot/jtreg/compiler/intrinsics/base64/TestBase64.java line 240:
> 
>> 238:     }
>> 239:
>> 240:     private static byte[] hexStringToByteArray(String s) {
> 
> A method to convert a Hex String to a byte array already exists in test/lib/jdk/test/lib/Utils.java  in  toByteArray(s).
> Add "jdk.test.lib.Utils" to the "@build" line at the top of the test.

Cool! Thanks.  I was unaware of that function.  Will fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/293