RFR 8177784 Use CounterMode intrinsic for AES/GCM

[Anthony Scarpino]

On 04/10/2017 10:27 AM, Paul Sandoz wrote:
>
>> On 7 Apr 2017, at 12:47, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
>>
>> On 04/07/2017 06:58 AM, Chris Hegarty wrote:
>>> On 06/04/17 21:39, Anthony Scarpino wrote:
>>>>
>>>> I'd like to get a review for this performance change to use the existing
>>>> CounterMode parallelized intrinsic instead of GCTR's own version. The
>>>> two classes were nearly identical except for the doFinal() method which
>>>> doesn't belong in CounterMode.java.
>>>>
>>>> I could have been more aggressive with this change, but I'm looking to
>>>> get this into 9, so I stayed away from completely merging GCTR into
>>>> CounterMode in case of incompatibilities.  All tests security and
>>>> hotspot tests pass.
>>>>
>>>> http://cr.openjdk.java.net/~ascarpino/8177784/webrev/
>>>
>>> This change looks good to me. Trivially, the class-level comment in
>>> GCTR should be updated ( it refers to removed fields ). Also,
>>> CounterMode.counter could be protected ( rather than package-private ).
>>>
>>> -Chris.
>>
>> Thanks Chris,
>>
>> I left CounterMode.counter as package-private because the package is what becomes the SunJCE provider.  I don't believe there should be any outside package classes accessing this code.
>>
>> I updated the webrev at with the comment update:
>>  http://cr.openjdk.java.net/~ascarpino/8177784/webrev.01/
>>
>
> It’s the same pattern for “iv” with CounterMode and FeedbackCipher, although i prefer protected as well if never accessed by other classes in the same package, it makes it easier to reason about the code, since i know more about what can access it.
>
> Suggestion, take it or leave it: personally i would prefer to see an additional constructor in CounterMode:
>
>   GCTR(SymmetricCipher cipher, byte[] initialCounterBlk) {
>     super(cipher, checkBlockSize(initialCounterBlk));
>    }
>
> Where new CounterMode constructor performs the clone, assignmentm and reset, the static method checkBlockSize checks the array length is equal to AES_BLOCK_SIZE Thereby you have a cleaner separation of concerns.
>
> Paul.
>

I contemplated putting changing the constructors, but went against it 
for the moment so a jdk9 change would be as minimal as possible.  A 
future change would most likely include the new constructor in CounterMode.

Tony