RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v3]

[Volodymyr Paprotski]

On Tue, 23 Apr 2024 19:55:57 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> Volodymyr Paprotski has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Comments from Jatin and Tony
>
> src/java.base/share/classes/sun/security/ec/ECOperations.java line 204:
> 
>> 202:      * @return the product
>> 203:      */
>> 204:     public MutablePoint multiply(AffinePoint affineP, byte[] s) {
> 
> It seems like there could be some combining of both `multiply()`.  If `multiply(AffinePoint, ...)` is called, it can call `DefaultMultiplier` with the `affineP`, but internally call the other `multiply(ECPoint, ...)` for the other situations.  I'd rather not have two methods doing most of the same code, but different methods.

Thanks, they indeed look identical, didnt notice. Fixed. (repeated the same hashmap refactoring and didnt notice I produced identical code twice)

> src/java.base/share/classes/sun/security/ec/ECOperations.java line 467:
> 
>> 465:     sealed static abstract class SmallWindowMultiplier implements PointMultiplier
>> 466:         permits DefaultMultiplier, DefaultMontgomeryMultiplier {
>> 467:         private final AffinePoint affineP;
> 
> I don't think `affineP` needs to be a class variable anymore.  It's only used in the constructor

Didn't notice, thanks, fixed.

> src/java.base/share/classes/sun/security/ec/ECOperations.java line 592:
> 
>> 590:         }
>> 591: 
>> 592:         private final ProjectivePoint.Immutable[][] points;
> 
> Can you define this at the top please.

Done

> src/java.base/share/classes/sun/security/ec/ECOperations.java line 668:
> 
>> 666:         }
>> 667: 
>> 668:         private final BigInteger[] base;
> 
> Can you define this at the top.  You use it in the constructor but it's defined later on.

Done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578117929
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578147190
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578148562
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1578150303