On Thu, 29 Jun 2023 15:50:33 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Add a benchmark
>> - Merge branch 'master' into 8310813
>> - Use fewer range checks to improve performance
>> - Improve
>> - Initial commit
>
> LGTM; I assume the comment about aarch64 perf was a 10% improvement.
Here's some explanation for the recent commits I've added since @RogerRiggs
reviewed this PR.
1. Since `BigInteger.hashCode()` is unspecified, we can change it. Again: I
think that the proposed implementation is no worse in hashing quality than the
current one; if you disagree, please voice your concerns.
FWIW, we can keep the existing `BigInteger.hashCode()` values whilst still
improving the implementation, using JDK-internal support:
@Override
public int hashCode() {
return ArraysSupport.vectorizedHashCode(mag, 0, mag.length, 1,
ArraysSupport.T_INT) * signum;
}
AFAIU, such an implementation would always yield exactly the same values the
current (i.e. mainline) implementation does. But that could be a little slower
than the original proposal, especially for a smaller BigInteger.
The key thing that allows to keep the current hash-code values in the above
implementation is that `1` argument, which is the initial hash-code value,
which cannot be specified in `Arrays.hashCode`. Unfortunately, we don't have
mid-layer methods in between Arrays.hashCode and
ArraysSupport.vectorizedHashCode like that of Arrays.mismatch and
ArraysSupport.vectorizedMismatch. It's either all the null check but
short-circuits or unconditional vectorization but the initial value. I wonder
if we could consider `ArraysSupport.hashCode($type[] array, int fromIndex, int
length, int initialValue)` overloads, which could be useful beyond BigInteger,
as I've already seen in JDK. Contributors to ArraysSupport, @PaulSandoz,
@ChrisHegarty, @cl4es, @stuart-marks; thoughts?
2. Maybe surprisingly, but we don't have microbenchmarks for BigInteger's
equals, hashCode, and compareTo. While I don't know how often people use the
former two methods, I reckon, the latter method is essential. Anyway, I added
benchmarks to cover all three. Note: benchmark for hashCode shows only its
performance, not its hashing quality. Again: if you think the current version
is in any way worse than the mainline version, please voice your concerns.
AFAIK, the biggest consumer of BigInteger in JDK is security area. So, I assume
a good way to judge this change is to run security benchmarks to make sure they
haven't slipped.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/14630#issuecomment-1621705028
