Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Thu, 10 Aug 2023 11:38:08 GMT, Pavel Rappo 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 13 additional >> commits since the last revision: >> >> - Fix bugs in Shared.createSingle >> - Merge branch 'master' into 8310813 >> - Group params coarser (suggested by @cl4es) >> >>- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. >> Every testXYZ method invokes M operations, where M is the maximum >> number of elements in a group. Shorter groups are cyclically padded. >>- Uses the org.openjdk.jmh.infra.Blackhole API and increases >> benchmark time. >>- Fixes a bug in Shared that precluded 0 from being in a pair. >> - Use better overloads (suggested by @cl4es) >> >>- Uses simpler, more suitable overloads for the subrange >> starting from 0 >> - Improve benchmarks >> - Merge branch 'master' into 8310813 >> - Restore hash code values >> >>BigInteger is old and ubiquitous enough so that there might be >>inadvertent dependencies on its hash code. >> >>This commit also includes a test, to make sure hash code is >>unchanged. >> - Merge branch 'master' into 8310813 >> - Add a benchmark >> - Merge branch 'master' into 8310813 >> - ... and 3 more: https://git.openjdk.org/jdk/compare/3619ef21...6fa3f694 > > Here's a status update for this PR. I'm currently benchmarking baseline > against this PR and this PR plus changes in > https://github.com/openjdk/jdk/pull/15197. It's a 3-way benchmark, so to > speak. Its purpose is to see whether performance degradation brought by this > PR to `equals` and `compareTo` can be sufficiently offset by the improved > `ArraysSupport.mismatch` mechanics. Some context: On the `equals` and `compareTo` microbenchmarks @pavelrappo is adding in this PR there's a tiny regression from using `ArraysSupport.mismatch` when the `value` array has just a single element. Since such small `BigInteger`s appears to be exceedingly common (huh) it's been deemed hard to brush aside. Still this regression comes from the code having some extra branch, and is less than 2ns/op. All the while the speedup on huge `BigInteger`s is substantial. I think it's reasonable to move forward with this patch and accept the tiny regression on `equals/hashCode` with a magnitude of 1 (like we've done in other places such as `String.startsWith`) and separately continue investigating if `ArraysSupport.mismatch` can be improved to remove this deficiency. - PR Comment: https://git.openjdk.org/jdk/pull/14630#issuecomment-1674582483
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Wed, 9 Aug 2023 22:54:28 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> equals, hashCode, and compareTo for BigInteger. If you have any performance >> concerns, please raise them. >> >> This PR is cherry-picked from a bigger, not-yet-published PR, to test the >> waters. That latter PR will be published soon. > > 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 13 additional > commits since the last revision: > > - Fix bugs in Shared.createSingle > - Merge branch 'master' into 8310813 > - Group params coarser (suggested by @cl4es) > >- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. > Every testXYZ method invokes M operations, where M is the maximum > number of elements in a group. Shorter groups are cyclically padded. >- Uses the org.openjdk.jmh.infra.Blackhole API and increases > benchmark time. >- Fixes a bug in Shared that precluded 0 from being in a pair. > - Use better overloads (suggested by @cl4es) > >- Uses simpler, more suitable overloads for the subrange > starting from 0 > - Improve benchmarks > - Merge branch 'master' into 8310813 > - Restore hash code values > >BigInteger is old and ubiquitous enough so that there might be >inadvertent dependencies on its hash code. > >This commit also includes a test, to make sure hash code is >unchanged. > - Merge branch 'master' into 8310813 > - Add a benchmark > - Merge branch 'master' into 8310813 > - ... and 3 more: https://git.openjdk.org/jdk/compare/983eb997...6fa3f694 > Here's a status update for this PR. I'm currently benchmarking baseline > against this PR and this PR plus changes in #15197. It's a 3-way benchmark, > so to speak. Its purpose is to see whether performance degradation brought by > this PR to `equals` and `compareTo` can be sufficiently offset by the > improved `ArraysSupport.mismatch` mechanics. Sadly #15197 doesn't pan out as well as I'd hoped: the win on some array sizes is cancelled out by losses on others. I'll be shifting the focus of that PR over to simplifying in ways that will be performance neutral and enhancing the targeted `ArraysMismatch` microbenchmarks so that they work on tiny array sizes. - PR Comment: https://git.openjdk.org/jdk/pull/14630#issuecomment-1674563442
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Thu, 10 Aug 2023 11:40:52 GMT, Pavel Rappo wrote: >> My bad. Forget my note > >> Some have a preference for providing a seed for `Random` instances in >> micros. Either hard-coded or through a `@Param` (I find this a bit >> excessive). Doing so might reduce run-to-run noise. > > I hear you, but unless your opinion is strong, I'll leave it as is. Hard-coding a seed would remove the run-to-run randomness, too, so would remove the need the `randomness` tag if this utility were to be used from a functional test. But yes, the reason to not do true randomness when setting up data for micros is to ensure we don't get multimodal results from arbitrary differences in the data setup. There are other ways around that, e.g. randomize data continually between iterations, run more forks to get more samples about flicker etc. A lot of work with an end result which is unlikely to add little more quality than a "random" set of data that we keep static from run-to-run. - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1289995785
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Thu, 10 Aug 2023 11:28:39 GMT, Raffaello Giulietti wrote: >>> AFAIK, if you need reproducible randoms in tests, you should add the tags: >>> >>> ``` >>> * @key randomness >>> * @library /test/lib >>> ``` >>> >>> and initialize your random generator with >>> >>> ``` >>> import jdk.test.lib.RandomFactory; >>> ... >>> Random rnd = RandomFactory.getRandom(); >>> ``` >>> >>> This prints the seed to STDOUT. >> >> It'd be applicable for tests, but not for benchmarks. Also, the point that >> @cl4es seems to make is that randomness is captured and set up to reduce >> noise, not to be able to reproduce "benchmark failures". > > My bad. Forget my note > Some have a preference for providing a seed for `Random` instances in micros. > Either hard-coded or through a `@Param` (I find this a bit excessive). Doing > so might reduce run-to-run noise. I hear you, but unless your opinion is strong, I'll leave it as is. - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1289994437
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Wed, 9 Aug 2023 22:54:28 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> equals, hashCode, and compareTo for BigInteger. If you have any performance >> concerns, please raise them. >> >> This PR is cherry-picked from a bigger, not-yet-published PR, to test the >> waters. That latter PR will be published soon. > > 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 13 additional > commits since the last revision: > > - Fix bugs in Shared.createSingle > - Merge branch 'master' into 8310813 > - Group params coarser (suggested by @cl4es) > >- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. > Every testXYZ method invokes M operations, where M is the maximum > number of elements in a group. Shorter groups are cyclically padded. >- Uses the org.openjdk.jmh.infra.Blackhole API and increases > benchmark time. >- Fixes a bug in Shared that precluded 0 from being in a pair. > - Use better overloads (suggested by @cl4es) > >- Uses simpler, more suitable overloads for the subrange > starting from 0 > - Improve benchmarks > - Merge branch 'master' into 8310813 > - Restore hash code values > >BigInteger is old and ubiquitous enough so that there might be >inadvertent dependencies on its hash code. > >This commit also includes a test, to make sure hash code is >unchanged. > - Merge branch 'master' into 8310813 > - Add a benchmark > - Merge branch 'master' into 8310813 > - ... and 3 more: https://git.openjdk.org/jdk/compare/16b32b66...6fa3f694 Here's a status update for this PR. I'm currently benchmarking baseline against this PR and this PR plus changes in https://github.com/openjdk/jdk/pull/15197. It's a 3-way benchmark, so to speak. Its purpose is to see whether performance degradation brought by this PR to `equals` and `compareTo` can be sufficiently offset by the improved `ArraysSupport.mismatch` mechanics. - PR Comment: https://git.openjdk.org/jdk/pull/14630#issuecomment-1673056244
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Wed, 9 Aug 2023 22:36:47 GMT, Claes Redestad 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 13 additional >> commits since the last revision: >> >> - Fix bugs in Shared.createSingle >> - Merge branch 'master' into 8310813 >> - Group params coarser (suggested by @cl4es) >> >>- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. >> Every testXYZ method invokes M operations, where M is the maximum >> number of elements in a group. Shorter groups are cyclically padded. >>- Uses the org.openjdk.jmh.infra.Blackhole API and increases >> benchmark time. >>- Fixes a bug in Shared that precluded 0 from being in a pair. >> - Use better overloads (suggested by @cl4es) >> >>- Uses simpler, more suitable overloads for the subrange >> starting from 0 >> - Improve benchmarks >> - Merge branch 'master' into 8310813 >> - Restore hash code values >> >>BigInteger is old and ubiquitous enough so that there might be >>inadvertent dependencies on its hash code. >> >>This commit also includes a test, to make sure hash code is >>unchanged. >> - Merge branch 'master' into 8310813 >> - Add a benchmark >> - Merge branch 'master' into 8310813 >> - ... and 3 more: https://git.openjdk.org/jdk/compare/3425a24d...6fa3f694 > > test/micro/org/openjdk/bench/java/math/Shared.java line 82: > >> 80: } >> 81: int nBytes = (nBits + 7) / 8; >> 82: var r = new Random(); > > Some have a preference for providing a seed for `Random` instances in micros. > Either hard-coded or through a `@Param` (I find this a bit excessive). Doing > so might reduce run-to-run noise. AFAIK, if you need reproducible randoms in tests, you should add the tags: * @key randomness * @library /test/lib and initialize your random generator with import jdk.test.lib.RandomFactory; ... Random rnd = RandomFactory.getRandom(); This prints the seed to STDOUT. - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1289966641
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Thu, 10 Aug 2023 11:23:02 GMT, Pavel Rappo wrote: >> AFAIK, if you need reproducible randoms in tests, you should add the tags: >> >> * @key randomness >> * @library /test/lib >> >> and initialize your random generator with >> >> import jdk.test.lib.RandomFactory; >> ... >> Random rnd = RandomFactory.getRandom(); >> >> This prints the seed to STDOUT. > >> AFAIK, if you need reproducible randoms in tests, you should add the tags: >> >> ``` >> * @key randomness >> * @library /test/lib >> ``` >> >> and initialize your random generator with >> >> ``` >> import jdk.test.lib.RandomFactory; >> ... >> Random rnd = RandomFactory.getRandom(); >> ``` >> >> This prints the seed to STDOUT. > > It'd be applicable for tests, but not for benchmarks. Also, the point that > @cl4es seems to make is that randomness is captured and set up to reduce > noise, not to be able to reproduce "benchmark failures". My bad. Forget my note - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1289981733
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Thu, 10 Aug 2023 11:13:12 GMT, Raffaello Giulietti wrote: > AFAIK, if you need reproducible randoms in tests, you should add the tags: > > ``` > * @key randomness > * @library /test/lib > ``` > > and initialize your random generator with > > ``` > import jdk.test.lib.RandomFactory; > ... > Random rnd = RandomFactory.getRandom(); > ``` > > This prints the seed to STDOUT. It'd be applicable for tests, but not for benchmarks. Also, the point that @cl4es seems to make is that randomness is captured and set up to reduce noise, not to be able to reproduce "benchmark failures". - PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1289976177
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
On Wed, 9 Aug 2023 22:54:28 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> equals, hashCode, and compareTo for BigInteger. If you have any performance >> concerns, please raise them. >> >> This PR is cherry-picked from a bigger, not-yet-published PR, to test the >> waters. That latter PR will be published soon. > > 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 13 additional > commits since the last revision: > > - Fix bugs in Shared.createSingle > - Merge branch 'master' into 8310813 > - Group params coarser (suggested by @cl4es) > >- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. > Every testXYZ method invokes M operations, where M is the maximum > number of elements in a group. Shorter groups are cyclically padded. >- Uses the org.openjdk.jmh.infra.Blackhole API and increases > benchmark time. >- Fixes a bug in Shared that precluded 0 from being in a pair. > - Use better overloads (suggested by @cl4es) > >- Uses simpler, more suitable overloads for the subrange > starting from 0 > - Improve benchmarks > - Merge branch 'master' into 8310813 > - Restore hash code values > >BigInteger is old and ubiquitous enough so that there might be >inadvertent dependencies on its hash code. > >This commit also includes a test, to make sure hash code is >unchanged. > - Merge branch 'master' into 8310813 > - Add a benchmark > - Merge branch 'master' into 8310813 > - ... and 3 more: https://git.openjdk.org/jdk/compare/d5d285cb...6fa3f694 This looks good, thanks for the microbenchmark updates. Your work on this PR was what prompted me to look at `ArraysSupport.mismatch` performance. The patch in #15197 should mitigate at least some of the overhead we've seen on small `BigInteger` objects on `equals` and `compareTo` micros test/micro/org/openjdk/bench/java/math/Shared.java line 82: > 80: } > 81: int nBytes = (nBits + 7) / 8; > 82: var r = new Random(); Some have a preference for providing a seed for `Random` instances in micros. Either hard-coded or through a `@Param` (I find this a bit excessive). Doing so might reduce run-to-run noise. - Marked as reviewed by redestad (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14630#pullrequestreview-1570712845 PR Review Comment: https://git.openjdk.org/jdk/pull/14630#discussion_r1289313113
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v6]
> Please review this PR to use modern APIs and language features to simplify > equals, hashCode, and compareTo for BigInteger. If you have any performance > concerns, please raise them. > > This PR is cherry-picked from a bigger, not-yet-published PR, to test the > waters. That latter PR will be published soon. 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 13 additional commits since the last revision: - Fix bugs in Shared.createSingle - Merge branch 'master' into 8310813 - Group params coarser (suggested by @cl4es) - Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. Every testXYZ method invokes M operations, where M is the maximum number of elements in a group. Shorter groups are cyclically padded. - Uses the org.openjdk.jmh.infra.Blackhole API and increases benchmark time. - Fixes a bug in Shared that precluded 0 from being in a pair. - Use better overloads (suggested by @cl4es) - Uses simpler, more suitable overloads for the subrange starting from 0 - Improve benchmarks - Merge branch 'master' into 8310813 - Restore hash code values BigInteger is old and ubiquitous enough so that there might be inadvertent dependencies on its hash code. This commit also includes a test, to make sure hash code is unchanged. - Merge branch 'master' into 8310813 - Add a benchmark - Merge branch 'master' into 8310813 - ... and 3 more: https://git.openjdk.org/jdk/compare/5907d416...6fa3f694 - Changes: - all: https://git.openjdk.org/jdk/pull/14630/files - new: https://git.openjdk.org/jdk/pull/14630/files/5af2d5dc..6fa3f694 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14630=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14630=04-05 Stats: 23666 lines in 977 files changed: 10910 ins; 6105 del; 6651 mod Patch: https://git.openjdk.org/jdk/pull/14630.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14630/head:pull/14630 PR: https://git.openjdk.org/jdk/pull/14630
