Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v4]
> Dear all, > Can you do me a favor to review this patch. This patch use `ldp` to > implement String.compareTo. > > * We add a JMH test case > * Here is the result of this test case > > Benchmark|(size)| Mode| Cnt|Score | Error |Units > -|--|-||--||- > StringCompare.compareLL | 64 | avgt| 5 |7.992 | ±0.005|us/op > StringCompare.compareLL | 72 | avgt| 5 |15.029| ±0.006|us/op > StringCompare.compareLL | 80 | avgt| 5 |14.655| ±0.011|us/op > StringCompare.compareLL | 91 | avgt| 5 |16.363| ±0.12 |us/op > StringCompare.compareLL | 101 | avgt| 5 |16.966| ±0.007|us/op > StringCompare.compareLL | 121 | avgt| 5 |19.276| ±0.006|us/op > StringCompare.compareLL | 181 | avgt| 5 |19.002| ±0.417|us/op > StringCompare.compareLL | 256 | avgt| 5 |24.707| ±0.041|us/op > StringCompare.compareLLWithLdp| 64 | avgt| 5 |8.001| ± > 0.121|us/op > StringCompare.compareLLWithLdp| 72 | avgt| 5 |11.573| ±0.003|us/op > StringCompare.compareLLWithLdp| 80 | avgt| 5 |6.861 | ±0.004|us/op > StringCompare.compareLLWithLdp| 91 | avgt| 5 |12.774| ±0.201|us/op > StringCompare.compareLLWithLdp| 101 | avgt| 5 |8.691 | ±0.004|us/op > StringCompare.compareLLWithLdp| 121 | avgt| 5 |11.091| ±1.342|us/op > StringCompare.compareLLWithLdp| 181 | avgt| 5 |14.64 | ±0.581|us/op > StringCompare.compareLLWithLdp| 256 | avgt| 5 |25.879| ±1.775|us/op > StringCompare.compareUU | 64 | avgt| 5 |13.476| ±0.01 |us/op > StringCompare.compareUU | 72 | avgt| 5 |15.078| ±0.006|us/op > StringCompare.compareUU | 80 | avgt| 5 |23.512| ±0.011|us/op > StringCompare.compareUU | 91 | avgt| 5 |24.284| ±0.008|us/op > StringCompare.compareUU | 101 | avgt| 5 |20.707| ±0.017|us/op > StringCompare.compareUU | 121 | avgt| 5 |29.302| ±0.011|us/op > StringCompare.compareUU | 181 | avgt| 5 |39.31| ± > 0.016|us/op > StringCompare.compareUU | 256 | avgt| 5 |54.592| ±0.392|us/op > StringCompare.compareUUWithLdp| 64 | avgt| 5 |16.389| ±0.008|us/op > StringCompare.compareUUWithLdp| 72 | avgt| 5 |10.71 | ±0.158|us/op > StringCompare.compareUUWithLdp| 80 | avgt| 5 |11.488| ±0.024|us/op > StringCompare.compareUUWithLdp| 91 | avgt| 5 |13.412| ±0.006|us/op > StringCompare.compareUUWithLdp| 101 | avgt| 5 |16.245| ±0.434|us/op > StringCompare.compareUUWithLdp| 121 | avgt| 5 |16.597| ±0.016|us/op > StringCompare.compareUUWithLdp| 181 | avgt| 5 |27.373| ±0.017|us/op > StringCompare.compareUUWithLdp| 256 | avgt| 5 |41.74 | ±3.5 |us/op > > From this table, we can see that in most cases, our patch is better than old > one. > > Thank you for your review. Any suggestions are welcome. Wang Huang has updated the pull request incrementally with one additional commit since the last revision: fix style and add unalign test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/4722/files - new: https://git.openjdk.java.net/jdk/pull/4722/files/3fa9afcb..c85cd126 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4722=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4722=02-03 Stats: 32 lines in 2 files changed: 22 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/4722.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4722/head:pull/4722 PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v3]
On Wed, 14 Jul 2021 08:27:36 GMT, Nick Gasson wrote:
>> Wang Huang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> refact codes
>
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4990:
>
>> 4988: __ lsrv(tmp2, tmp2, rscratch2);
>> 4989: if (isLL) {
>> 4990: __ uxtbw(tmp1, tmp1);
>
> Convention is to indent with two spaces but have four here.
Thank you for your suggestion. I have fixed the style and add unalign test case
in my new commit.
-
PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Tue, 8 Jun 2021 13:37:10 GMT, Thomas Stuefe wrote: >> Henry Jen 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 seven additional commits >> since the last revision: >> >> - Cast type >> - Merge >> - Change java -X output for -Xss >> - Merge >> - Only try to round-up when current value failed >> - Avoid overflow on page size >> - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on >> macOS > > Please make sure the failing tests have nothing to do with your patch. > `gc/shenandoah/compiler/TestLinkToNativeRBP.java` > sounds at least suggestive. Still pending CSR, also considering adapt hotspot align up as suggested by @tstuefe. - PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: JDK-8269387: jpackage --add-launcher should have option to not create shortcuts for additional launchers [v4]
On Wed, 14 Jul 2021 16:50:39 GMT, Andy Herrick wrote: >> JDK-8269387: jpackage --add-launcher should have option to not create >> shortcuts for additional launchers > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8269387: jpackage --add-launcher should have option to not create > shortcuts for additional launchers Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4730
Integrated: Merge jdk17
On Wed, 14 Jul 2021 21:35:34 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: 7d0edb57 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/7d0edb5743aacfc22f76ee8aa7b03d7dc0f90dca Stats: 328 lines in 19 files changed: 240 ins; 14 del; 74 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/4786
Re: RFR: JDK-8269387: jpackage --add-launcher should have option to not create shortcuts for additional launchers [v4]
On Wed, 14 Jul 2021 16:50:39 GMT, Andy Herrick wrote: >> JDK-8269387: jpackage --add-launcher should have option to not create >> shortcuts for additional launchers > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8269387: jpackage --add-launcher should have option to not create > shortcuts for additional launchers Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4730
Re: RFR: 8260265: UTF-8 by Default [v3]
On Wed, 14 Jul 2021 20:52:54 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/io/Console.java line 587:
>>
>>> 585: try {
>>> 586: cs = Charset.forName(csname);
>>> 587: } catch (Exception ignored) { }
>>
>> A separate enhancement...
>> I've long thought that should be a way to avoid the exception here.
>> For example, a Charset.forName(csname, default);
>> The caller might have a default in mind or supply null and then be able to
>> test for null.
>
> Agreed. Will file an RFE for this.
https://bugs.openjdk.java.net/browse/JDK-8270490
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v3]
On Wed, 14 Jul 2021 21:03:53 GMT, Roger Riggs wrote: >> That was intentional. Only those two are supported, others continue to work >> as before (but not supported). > > Still it leaves an uncomfortable feeling, perhaps remedied by an "other > values have unspecified behavior" > or the "other values are implementation specific". Added the clarifying sentence to the spec. - PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v3]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/981200f7..182dcb6d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=01-02 Stats: 28 lines in 2 files changed: 0 ins; 0 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18 - Commit messages: - Merge - 8270422: Test build/AbsPathsInImage.java fails after JDK-8259848 - 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes - 8270075: SplittableRandom extends AbstractSplittableGenerator - 8266889: [macosx-aarch64] Crash with SIGBUS in MarkActivationClosure::do_code_blob during vmTestbase/nsk/jvmti/.../bi04t002 test run - 8259499: Handling type arguments from outer classes for inner class in javadoc - 8268620: InfiniteLoopException test may fail on x86 platforms - 8269865: Async UL needs to handle ERANGE on exceeding SEM_VALUE_MAX - 8270056: Generated lambda class can not access protected static method of target class The webrevs contain the adjustments done while merging with regards to each parent branch: - master: https://webrevs.openjdk.java.net/?repo=jdk=4786=00.0 - jdk17: https://webrevs.openjdk.java.net/?repo=jdk=4786=00.1 Changes: https://git.openjdk.java.net/jdk/pull/4786/files Stats: 328 lines in 19 files changed: 240 ins; 14 del; 74 mod Patch: https://git.openjdk.java.net/jdk/pull/4786.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4786/head:pull/4786 PR: https://git.openjdk.java.net/jdk/pull/4786
Withdrawn: 8266936: Add a finalization JFR event
On Tue, 18 May 2021 20:55:10 GMT, Brent Christian wrote: > Please review this enhancement to add a new JFR event, generated whenever a > finalizer is run. > (The makeup is similar to the Deserialization event, > [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).) > > The event's only datum (beyond those common to all jfr events) is the class > of the object that was finalized. > > The Category for the event: > `"Java Virtual Machine" / "GC" / "Finalization"` > is what made sense to me, even though the event is generated from library > code. > > Along with the new regtest, I added a run mode to the basic finalizer test to > enable jfr. > Automated testing looks good so far. > > Thanks, > -Brent This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4101
Re: RFR: 8260265: UTF-8 by Default [v2]
On Wed, 14 Jul 2021 20:53:34 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/io/FileReader.java line 41:
>>
>>> 39: * @see InputStreamReader
>>> 40: * @see FileInputStream
>>> 41: * @see java.nio.charset.Charset#defaultCharset()
>>
>> The @ see duplicates the link above, the javadoc can do without the @ see.
>
> If I remove that `@see`, I don't see the link in `See Also` section. Am I
> missing something?
In my view the @ linkplain is sufficient to allow the reader to navigate; but
YMMV.
>> src/java.base/share/classes/java/lang/System.java line 802:
>>
>>> 800: * {@systemProperty file.encoding}
>>> 801: * The name of the default charset. Users may specify
>>> 802: * {@code UTF-8} or {@code COMPAT} on the command line to the
>>> value.
>>
>> The wording could imply that only those two values can be supplied.
>> It could be rephrased to say that *if* the property is supplied on the
>> command line
>> it overrides the default UTF-8.
>
> That was intentional. Only those two are supported, others continue to work
> as before (but not supported).
Still it leaves an uncomfortable feeling, perhaps remedied by an "other values
have unspecified behavior"
or the "other values are implementation specific".
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v2]
On Wed, 14 Jul 2021 16:20:28 GMT, Jesse Glick
wrote:
>> src/java.base/share/classes/java/io/PrintStream.java line 49:
>>
>>> 47: * All characters printed by a {@code PrintStream} are converted
>>> into
>>> 48: * bytes using the given encoding or charset, or the default
>>> 49: * console charset if not specified.
>>
>> JEP 400 doesn't give a rationale for using the console charset for
>> PrintStream.
>> PrintStreams are used for output to files and other media other than just a
>> tty/console.
>> The charset of system.out/err should use the console charset.
>
> This was my thinking in
> https://github.com/openjdk/jdk/pull/4733#issuecomment-876793372.
OK, I am now conviced. Modified not to default to Console.charset() for generic
PrintStream w/o charset constructor.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v2]
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 Naoto Sato 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 three additional commits since the last revision: - Reflects review comments - Merge branch 'master' into JDK-8260265 - 8260265: UTF-8 by Default - Changes: - all: https://git.openjdk.java.net/jdk/pull/4733/files - new: https://git.openjdk.java.net/jdk/pull/4733/files/107210cf..981200f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=00-01 Stats: 6434 lines in 314 files changed: 3877 ins; 1452 del; 1105 mod Patch: https://git.openjdk.java.net/jdk/pull/4733.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733 PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default [v2]
On Wed, 14 Jul 2021 14:55:39 GMT, Roger Riggs wrote:
>> Naoto Sato 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 three additional
>> commits since the last revision:
>>
>> - Reflects review comments
>> - Merge branch 'master' into JDK-8260265
>> - 8260265: UTF-8 by Default
>
> src/java.base/share/classes/java/io/Console.java line 587:
>
>> 585: try {
>> 586: cs = Charset.forName(csname);
>> 587: } catch (Exception ignored) { }
>
> A separate enhancement...
> I've long thought that should be a way to avoid the exception here.
> For example, a Charset.forName(csname, default);
> The caller might have a default in mind or supply null and then be able to
> test for null.
Agreed. Will file an RFE for this.
> src/java.base/share/classes/java/io/FileReader.java line 41:
>
>> 39: * @see InputStreamReader
>> 40: * @see FileInputStream
>> 41: * @see java.nio.charset.Charset#defaultCharset()
>
> The @ see duplicates the link above, the javadoc can do without the @ see.
If I remove that `@see`, I don't see the link in `See Also` section. Am I
missing something?
> src/java.base/share/classes/java/lang/System.java line 802:
>
>> 800: * {@systemProperty file.encoding}
>> 801: * The name of the default charset. Users may specify
>> 802: * {@code UTF-8} or {@code COMPAT} on the command line to the
>> value.
>
> The wording could imply that only those two values can be supplied.
> It could be rephrased to say that *if* the property is supplied on the
> command line
> it overrides the default UTF-8.
That was intentional. Only those two are supported, others continue to work as
before (but not supported).
> src/java.base/share/classes/java/nio/charset/Charset.java line 601:
>
>> 599: * value designates {@code COMPAT}, the default charset is derived
>> from
>> 600: * the {@code native.encoding} system property, which typically
>> depends
>> 601: * upon the locale and charset of the underlying operating system.
>
> The description in java.lang.System of the file.encoding property does not
> indicate it is 'implementation specific'.
> In that context, it appears to be part of the JavaSE spec.
> Having the spec in a single place with references to it from others could
> avoid duplication.
`file.encoding` is listed under `@implNote` tag in `System.getProperties()`, so
it is implementation-specific.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8260265: UTF-8 by Default
On Wed, 14 Jul 2021 12:39:46 GMT, Giacomo Baso
wrote:
> > Consider an application that creates a java.io.FileWriter with its
> > one-argument constructor and then uses it to write some text to a file. The
> > resulting file will contain a sequence of bytes encoded using the default
> > charset of the JDK running the application. A second application, run on a
> > different machine or by a different user on the same machine, creates a
> > java.io.FileReader with its one-argument constructor and uses it to read
> > the bytes in that file. The resulting text contains a sequence of
> > characters decoded using the default charset of the JDK running the second
> > application. If the default charset differs between the JDK of the first
> > application and the JDK of the second application, then the resulting text
> > may be silently corrupted or incomplete, since these APIs replace erroneous
> > input rather than fail.
>
> It's even worse than that, because many OpenSSH installs are configured by
> default to [forward](https://man.openbsd.org/ssh_config.5#SendEnv) and
> [accept](https://man.openbsd.org/sshd_config.5#AcceptEnv) the user locale
> (see e.g. for [RHEL 7](https://access.redhat.com/solutions/974273)).
>
> So a single application, on a single remote machine, can be unknowingly
> started by a single user with different locales, and therefore different
> encodings, depending on how the user connected to the remote machine. For
> example, on Windows connecting via powershell results in `LANG=en_US.UTF-8`,
> while using WSL2 results in `LANG=C.UTF-8`. On Java 11 in a RHEL7 machine,
> `file.encoding` results in `UTF-8` in the first case, but `ANSI_X3.4-1968` in
> the second, leading to a default charset `ASCII`.
>
> Worth mentioning is also that `Charset.forName("default")` is just an alias
> to `ASCII`, per `sun.nio.cs.StandardCharsets$Aliases`.
Thanks. Updated the JEP.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8075806: divideExact is missing in java.lang.Math
On Wed, 14 Jul 2021 16:45:20 GMT, Joe Darcy wrote: > Are there examples in the JDK or its tests where a method with this > definition would be used? I have not identified any as yet. I did verify however that there are no uses in `open/src/**/*.java` of either `absExact()` or `negateExact()`, only their implementations in `[Strict]Math`. - PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math [v2]
On Wed, 14 Jul 2021 16:43:32 GMT, Joe Darcy wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8075806: Separate div-by-0 verbiage; change impl
>
> src/java.base/share/classes/java/lang/Math.java line 1008:
>
>> 1006: * Returns the quotient of the arguments, throwing an exception if
>> the
>> 1007: * result overflows an {@code int}. Such overflow can occur if
>> and only
>> 1008: * if either {@code y} is zero, or both {@code x} is
>
> I think the divide by zero case should be discussed separately from overflow.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math [v2]
> Please consider this proposal to add `divideExact()` methods for integral > data types to `java.lang.Math` thereby rounding out "exact" support to all > four basic arithmetic operations. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8075806: Separate div-by-0 verbiage; change impl - Changes: - all: https://git.openjdk.java.net/jdk/pull/4770/files - new: https://git.openjdk.java.net/jdk/pull/4770/files/db216a6d..6843e666 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4770=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4770=00-01 Stats: 29 lines in 1 file changed: 8 ins; 3 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/4770.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4770/head:pull/4770 PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math
Mysteries of JIT compilation... On 2021-07-14 21:53, Brian Burkhalter wrote: On Tue, 13 Jul 2021 17:21:52 GMT, Brian Burkhalter wrote: Please consider this proposal to add `divideExact()` methods for integral data types to `java.lang.Math` thereby rounding out "exact" support to all four basic arithmetic operations. @rgiulietti Actually your trick is the fastest: `yours > via negateExact() > original`. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math
On Tue, 13 Jul 2021 17:21:52 GMT, Brian Burkhalter wrote: > Please consider this proposal to add `divideExact()` methods for integral > data types to `java.lang.Math` thereby rounding out "exact" support to all > four basic arithmetic operations. @rgiulietti Actually your trick is the fastest: `yours > via negateExact() > original`. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math
On Tue, 13 Jul 2021 17:21:52 GMT, Brian Burkhalter wrote: > Please consider this proposal to add `divideExact()` methods for integral > data types to `java.lang.Math` thereby rounding out "exact" support to all > four basic arithmetic operations. Thanks. I was thinking of leveraging the `negateExact()` intrinsic instead: // Leverage a potential intrinsic of negateExact() return y == -1 ? Math.negateExact(x) : x/y; - PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math
Sorry, typo
int q = x / y;
if ((x & y & q) >= 0) {
return q; // q, not r
}
throw new ArithmeticException("integer overflow");
On 2021-07-14 21:26, Raffaello Giulietti wrote:
One could also divide first and then check the result:
int q = x / y;
if ((x & y & q) >= 0) {
return r;
}
throw new ArithmeticException("integer overflow");
No idea about relative perf.
On 2021-07-13 19:32, Brian Burkhalter wrote:
Please consider this proposal to add `divideExact()` methods for
integral data types to `java.lang.Math` thereby rounding out "exact"
support to all four basic arithmetic operations.
-
Commit messages:
- 8075806: divideExact is missing in java.lang.Math
Changes: https://git.openjdk.java.net/jdk/pull/4770/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4770=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8075806
Stats: 109 lines in 2 files changed: 100 ins; 5 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/4770.diff
Fetch: git fetch https://git.openjdk.java.net/jdk
pull/4770/head:pull/4770
PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8075806: divideExact is missing in java.lang.Math
One could also divide first and then check the result:
int q = x / y;
if ((x & y & q) >= 0) {
return r;
}
throw new ArithmeticException("integer overflow");
No idea about relative perf.
On 2021-07-13 19:32, Brian Burkhalter wrote:
Please consider this proposal to add `divideExact()` methods for integral data types to
`java.lang.Math` thereby rounding out "exact" support to all four basic
arithmetic operations.
-
Commit messages:
- 8075806: divideExact is missing in java.lang.Math
Changes: https://git.openjdk.java.net/jdk/pull/4770/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4770=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8075806
Stats: 109 lines in 2 files changed: 100 ins; 5 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/4770.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4770/head:pull/4770
PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG wrote: >> This PR-*draft* is **work in progress** and an invitation to discuss a >> possible solution for issue >> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not >> yet* intended for a final review. >> >> As proposed in JDK-8265891, this PR provides an implementation for >> `Channels.newInputStream().transferTo()` which provide superior performance >> compared to the current implementation. The changes are: >> * Prevents transfers through the JVM heap as much as possibly by offloading >> to deeper levels via NIO, hence allowing the operating system to optimize >> the transfer. >> * Using more JRE heap in the fallback case when no NIO is possible (still >> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern >> hardware / fast I/O devides. >> >> Using JMH I have benchmarked both, the original implementation and this >> implementation, and (depending on the used hardware and use case) >> performance change was approx. doubled performance. So this PoC proofs that >> it makes sense to finalize this work and turn it into an actual OpenJDK >> contribution. >> >> I encourage everybody to discuss this draft: >> * Are there valid arguments for *not* doing this change? >> * Is there a *better* way to improve performance of >> `Channels.newInputStream().transferTo()`? >> * How to go on from here: What is missing to get this ready for an actual >> review? > > Markus KARG has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Draft: Renaming i and separating code into several methods > > Signed-off-by: Markus Karg Mockito is not approved for distribution and/or hosting by Java (Oracle JDK / OpenJDK). I understand not wanting to have a lot of useless code. Have you looked into other tests which have implemented Providers? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v3]
On Fri, 2 Jul 2021 06:20:29 GMT, Markus KARG wrote: >> This PR-*draft* is **work in progress** and an invitation to discuss a >> possible solution for issue >> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not >> yet* intended for a final review. >> >> As proposed in JDK-8265891, this PR provides an implementation for >> `Channels.newInputStream().transferTo()` which provide superior performance >> compared to the current implementation. The changes are: >> * Prevents transfers through the JVM heap as much as possibly by offloading >> to deeper levels via NIO, hence allowing the operating system to optimize >> the transfer. >> * Using more JRE heap in the fallback case when no NIO is possible (still >> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern >> hardware / fast I/O devides. >> >> Using JMH I have benchmarked both, the original implementation and this >> implementation, and (depending on the used hardware and use case) >> performance change was approx. doubled performance. So this PoC proofs that >> it makes sense to finalize this work and turn it into an actual OpenJDK >> contribution. >> >> I encourage everybody to discuss this draft: >> * Are there valid arguments for *not* doing this change? >> * Is there a *better* way to improve performance of >> `Channels.newInputStream().transferTo()`? >> * How to go on from here: What is missing to get this ready for an actual >> review? > > Markus KARG has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. I cloned the `InputStream/TransferTo` test in a way which uses providers, but when started to implement the providers I noticed that a huge amount of *empty* source code is needed just to make the compiler happy: `FileChannel` already has dozens of abstract methods I have to override but they never will be used by any of the tests... Having that said, it would be good if I would be allowed to spare us thousands of useless codelines: * May I just implement the `content()` test or do you insist on me really implementing *all* the tests found in `InputStream/TransferTo` for *each* of the overloaded `transferTo` implementations? * May I use a mocking framework to actually just provide *partial* implementations of the channels, or do you insist on me actually overloading *all* methods of *all* channels in actual Java source code just for the sake of making the compiler happy? While I understand the necessity of tests, and while I am convinced that the performance benefit outweighs the weeks of work this would imply just for adding all those test code, I actually like to propose that I only cover the `content()` tests plus using Mockito, so we would have 80% of the benefit for 20% of the coding costs. WDYT? - PR: https://git.openjdk.java.net/jdk/pull/4263
Re: RFR: JDK-8269387: jpackage --add-launcher should have option to not create shortcuts for additional launchers [v4]
> JDK-8269387: jpackage --add-launcher should have option to not create > shortcuts for additional launchers Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8269387: jpackage --add-launcher should have option to not create shortcuts for additional launchers - Changes: - all: https://git.openjdk.java.net/jdk/pull/4730/files - new: https://git.openjdk.java.net/jdk/pull/4730/files/d88b1dc7..96758fe8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4730=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4730=02-03 Stats: 18 lines in 1 file changed: 12 ins; 6 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4730.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4730/head:pull/4730 PR: https://git.openjdk.java.net/jdk/pull/4730
Re: RFR: 8075806: divideExact is missing in java.lang.Math
On Tue, 13 Jul 2021 17:21:52 GMT, Brian Burkhalter wrote:
> Please consider this proposal to add `divideExact()` methods for integral
> data types to `java.lang.Math` thereby rounding out "exact" support to all
> four basic arithmetic operations.
Are there examples in the JDK or its tests where a method with this definition
would be used?
src/java.base/share/classes/java/lang/Math.java line 1008:
> 1006: * Returns the quotient of the arguments, throwing an exception if
> the
> 1007: * result overflows an {@code int}. Such overflow can occur if and
> only
> 1008: * if either {@code y} is zero, or both {@code x} is
I think the divide by zero case should be discussed separately from overflow.
-
PR: https://git.openjdk.java.net/jdk/pull/4770
Re: RFR: 8211002: test/jdk/java/lang/Math/PowTests.java skips testing for non-corner-case values [v2]
On Tue, 13 Jul 2021 23:29:38 GMT, Joe Darcy wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8211002: Change so that Tests.testUlpDiff() handles the NaNs and accepts a
>> ulp tolerance of 2
>
> test/jdk/java/lang/Math/PowTests.java line 65:
>
>> 63: int failures = 0;
>> 64: if (Double.isNaN(smResult)) {
>> 65: if (!Double.isNaN(mResult)) failures = 1;
>
> Pretty sure the Tests.testUlpDiff call will handle two NaNs as well as
> NaN/non-NaN argument.
Looks like that is true.
> test/jdk/java/lang/Math/PowTests.java line 69:
>
>> 67: failures += Tests.testUlpDiff(
>> 68: "StrictMath.pow(double, double) vs Math.pow(double,
>> double)",
>> 69: input1, input2, mResult, smResult, 1.0
>
> In theory at least, one pow method could round an ulp up and the other could
> round an ulp down so the difference should be 2 ulps (ignoring the special
> case of values straddling a power of two).
Good point.
-
PR: https://git.openjdk.java.net/jdk/pull/4758
Re: RFR: 8211002: test/jdk/java/lang/Math/PowTests.java skips testing for non-corner-case values [v2]
> Please consider this proposal to add some test coverage comparing `Math` and > `StrictMath` results of `pow()`. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8211002: Change so that Tests.testUlpDiff() handles the NaNs and accepts a ulp tolerance of 2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4758/files - new: https://git.openjdk.java.net/jdk/pull/4758/files/0cebcfc8..2fe3ff35 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4758=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4758=00-01 Stats: 10 lines in 1 file changed: 0 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4758.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4758/head:pull/4758 PR: https://git.openjdk.java.net/jdk/pull/4758
Re: RFR: 8260265: UTF-8 by Default
On Wed, 14 Jul 2021 15:09:41 GMT, Roger Riggs wrote:
>> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of
>> the changes is `Charset.defaultCharset()` returning `UTF-8` and
>> `file.encoding` system property being added in the spec, but another notable
>> modification is in `java.io.PrintStream` where it continues to use the
>> `Console` encoding as the default charset instead of `UTF-8`. Other changes
>> are mostly clarification of the term "default charset" and their links.
>> Corresponding CSR has also been drafted.
>>
>> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266
>
> src/java.base/share/classes/java/io/PrintStream.java line 49:
>
>> 47: * All characters printed by a {@code PrintStream} are converted into
>> 48: * bytes using the given encoding or charset, or the default
>> 49: * console charset if not specified.
>
> JEP 400 doesn't give a rationale for using the console charset for
> PrintStream.
> PrintStreams are used for output to files and other media other than just a
> tty/console.
> The charset of system.out/err should use the console charset.
This was my thinking in
https://github.com/openjdk/jdk/pull/4733#issuecomment-876793372.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Integrated: 6506405: Math.abs(float) is slow
On Wed, 7 Jul 2021 20:28:37 GMT, Brian Burkhalter wrote: > Please consider this change to make the `float` and `double` versions of > `java.lang.Math.abs()` branch-free. This pull request has now been integrated. Changeset: c0d4efff Author:Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/c0d4efff3c7b853cd663726b668d49d01e0f8ee0 Stats: 129 lines in 4 files changed: 121 ins; 0 del; 8 mod 6506405: Math.abs(float) is slow Reviewed-by: darcy - PR: https://git.openjdk.java.net/jdk/pull/4711
Re: RFR: 8260265: UTF-8 by Default
On Thu, 8 Jul 2021 21:23:00 GMT, Naoto Sato wrote:
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of
> the changes is `Charset.defaultCharset()` returning `UTF-8` and
> `file.encoding` system property being added in the spec, but another notable
> modification is in `java.io.PrintStream` where it continues to use the
> `Console` encoding as the default charset instead of `UTF-8`. Other changes
> are mostly clarification of the term "default charset" and their links.
> Corresponding CSR has also been drafted.
>
> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266
src/java.base/share/classes/java/io/ByteArrayOutputStream.java line 291:
> 289: * method, which takes an encoding-name or charset argument,
> 290: * or the {@code toString()} method, which uses the default
> 291: * charset.
Fold to previous line.
src/java.base/share/classes/java/io/Console.java line 587:
> 585: try {
> 586: cs = Charset.forName(csname);
> 587: } catch (Exception ignored) { }
A separate enhancement...
I've long thought that should be a way to avoid the exception here.
For example, a Charset.forName(csname, default);
The caller might have a default in mind or supply null and then be able to test
for null.
src/java.base/share/classes/java/io/FileReader.java line 41:
> 39: * @see InputStreamReader
> 40: * @see FileInputStream
> 41: * @see java.nio.charset.Charset#defaultCharset()
The @ see duplicates the link above, the javadoc can do without the @ see.
src/java.base/share/classes/java/io/InputStreamReader.java line 39:
> 37: * java.nio.charset.Charset charset}. The charset that it uses
> 38: * may be specified by name or may be given explicitly, or the
> 39: * {@link Charset#defaultCharset() default charset} may be accepted.
"may be accepted" seems like the API has some choice in the matter.
Perhaps "accepted" -> "used".
And in other classes below if there's a suitable replacement.
src/java.base/share/classes/java/io/PrintStream.java line 49:
> 47: * All characters printed by a {@code PrintStream} are converted into
> 48: * bytes using the given encoding or charset, or the default
> 49: * console charset if not specified.
JEP 400 doesn't give a rationale for using the console charset for PrintStream.
PrintStreams are used for output to files and other media other than just a
tty/console.
The charset of system.out/err should use the console charset.
src/java.base/share/classes/java/lang/System.java line 802:
> 800: * {@systemProperty file.encoding}
> 801: * The name of the default charset. Users may specify
> 802: * {@code UTF-8} or {@code COMPAT} on the command line to the
> value.
The wording could imply that only those two values can be supplied.
It could be rephrased to say that *if* the property is supplied on the command
line
it overrides the default UTF-8.
src/java.base/share/classes/java/net/URLDecoder.java line 92:
> 90:
> 91: // The default charset
> 92: static String dfltEncName = URLEncoder.dfltEncName;
Perhaps add the value of file.encoding to the StaticProperties either as a
string or as the Charset.
That would allow a few different lookups of the property to be simplified.
src/java.base/share/classes/java/net/URLEncoder.java line 165:
> 163: try {
> 164: str = encode(s, dfltEncName);
> 165: } catch (UnsupportedEncodingException e) {
Perhaps a separate cleanup, the Charset should be cached, not just the name and
use the `encode(s, charset)` method.
src/java.base/share/classes/java/nio/charset/Charset.java line 601:
> 599: * value designates {@code COMPAT}, the default charset is derived
> from
> 600: * the {@code native.encoding} system property, which typically
> depends
> 601: * upon the locale and charset of the underlying operating system.
The description in java.lang.System of the file.encoding property does not
indicate it is 'implementation specific'.
In that context, it appears to be part of the JavaSE spec.
Having the spec in a single place with references to it from others could avoid
duplication.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 6506405: Math.abs(float) is slow [v9]
On Mon, 12 Jul 2021 17:59:32 GMT, Brian Burkhalter wrote: >> Please consider this change to make the `float` and `double` versions of >> `java.lang.Math.abs()` branch-free. > > Brian Burkhalter has refreshed the contents of this pull request, and > previous commits have been removed. The incremental views will show > differences compared to the previous content of the PR. Marked as reviewed by darcy (Reviewer). Updated tests look fine. Next time I'm in the neighborhood of the jdk/test/java/lang/Math/Tests.java code, I'll look to add some overloads to make it more lambda/method reference friendly. - PR: https://git.openjdk.java.net/jdk/pull/4711
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v3]
On Wed, 14 Jul 2021 08:47:56 GMT, Nick Gasson wrote: > I tried that on N1 and it's very slightly slower than with the 16B alignment Sorry, ignore that, the result is actually the other way round. Not sure what's going on there, but there's no significant difference. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v10]
> There is a few JDK classes duplicating the contents of Long.hashCode() for > hash code calculation. They should explicitly delegate to Long.hashCode(). Сергей Цыпанов 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 12 additional commits since the last revision: - 8270160 Revert changes in BitSet.hashCode - Merge branch 'master' into 8268113 - 8270160 Revert changes in BitSet.hashCode - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - Merge branch 'master' into 8268113 - 8268113: Inline local vars where reasonable - ... and 2 more: https://git.openjdk.java.net/jdk/compare/2f04364f...1d619c73 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4309/files - new: https://git.openjdk.java.net/jdk/pull/4309/files/a72a09b6..1d619c73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4309=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4309=08-09 Stats: 6977 lines in 323 files changed: 3755 ins; 2117 del; 1105 mod Patch: https://git.openjdk.java.net/jdk/pull/4309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4309/head:pull/4309 PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8260265: UTF-8 by Default
On Thu, 8 Jul 2021 21:23:00 GMT, Naoto Sato wrote:
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of
> the changes is `Charset.defaultCharset()` returning `UTF-8` and
> `file.encoding` system property being added in the spec, but another notable
> modification is in `java.io.PrintStream` where it continues to use the
> `Console` encoding as the default charset instead of `UTF-8`. Other changes
> are mostly clarification of the term "default charset" and their links.
> Corresponding CSR has also been drafted.
>
> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266
> Consider an application that creates a java.io.FileWriter with its
> one-argument constructor and then uses it to write some text to a file. The
> resulting file will contain a sequence of bytes encoded using the default
> charset of the JDK running the application. A second application, run on a
> different machine or by a different user on the same machine, creates a
> java.io.FileReader with its one-argument constructor and uses it to read the
> bytes in that file. The resulting text contains a sequence of characters
> decoded using the default charset of the JDK running the second application.
> If the default charset differs between the JDK of the first application and
> the JDK of the second application, then the resulting text may be silently
> corrupted or incomplete, since these APIs replace erroneous input rather than
> fail.
It's even worse than that, because many OpenSSH installs are configured by
default to [forward](https://man.openbsd.org/ssh_config.5#SendEnv) and
[accept](https://man.openbsd.org/sshd_config.5#AcceptEnv) the user locale (see
e.g. for [RHEL 7](https://access.redhat.com/solutions/974273)).
So a single application, on a single remote machine, can be unknowingly started
by a single user with different locales, and therefore different encodings,
depending on how the user connected to the remote machine. For example, on
Windows connecting via powershell results in `LANG=en_US.UTF-8`, while using
WSL2 results in `LANG=C.UTF-8`. On Java 11 in a RHEL7 machine, `file.encoding`
results in `UTF-8` in the first case, but `ANSI_X3.4-1968` in the second,
leading to a default charset `ASCII`.
Worth mentioning is also that `Charset.forName("default")` is just an alias to
`ASCII`, per `sun.nio.cs.StandardCharsets$Aliases`.
-
PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v9]
On Tue, 13 Jul 2021 13:04:36 GMT, Attila Szegedi wrote: >> src/java.base/share/classes/java/util/BitSet.java line 1040: >> >>> 1038: h ^= words[i] * (i + 1); >>> 1039: >>> 1040: return Long.hashCode(h); >> >> Here `>>` instead of `>>>` in original code seems to be a typo > > It is specified as `>>` in JavaDoc just above the implementation. As the > algorithm is part of the public API and thus part of the specification, I > don't think you can change it just here in the implementation; you'd need to > at least submit a CSR for it. Good point! I'll then revert this change - PR: https://git.openjdk.java.net/jdk/pull/4309
[jdk17] Integrated: 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes
On Fri, 25 Jun 2021 18:53:59 GMT, Jim Laskey wrote: > The wording of the @implSpec referred to internal methods in the description. > The patch rewords the @implSpec to be more descriptive of the algorithm than > the methods used. This pull request has now been integrated. Changeset: 72db09b1 Author:Jim Laskey URL: https://git.openjdk.java.net/jdk17/commit/72db09b1f393722074cae2fbff0fc369f0f2718c Stats: 54 lines in 2 files changed: 23 ins; 4 del; 27 mod 8266313: (JEP-356) - RandomGenerator spec implementation requirements tightly coupled to JDK internal classes Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk17/pull/151
[jdk17] Integrated: JDK-8270075 SplittableRandom extends AbstractSplittableGenerator
On Mon, 12 Jul 2021 14:26:23 GMT, Jim Laskey wrote: > Random.AbstractSplittableGenerator is an internal class that should not be > exposed in a public API. This pull request has now been integrated. Changeset: 3bbd2332 Author:Jim Laskey URL: https://git.openjdk.java.net/jdk17/commit/3bbd2332bd4876b5529ccdf90e5e5d6c515e9d58 Stats: 48 lines in 1 file changed: 29 ins; 1 del; 18 mod 8270075: SplittableRandom extends AbstractSplittableGenerator Reviewed-by: rriggs, bpb - PR: https://git.openjdk.java.net/jdk17/pull/243
Re: RFR: 8266054: VectorAPI rotate operation optimization [v9]
On Wed, 30 Jun 2021 11:02:41 GMT, Jatin Bhateja wrote: >> Current VectorAPI Java side implementation expresses rotateLeft and >> rotateRight operation using following operations:- >> >> vec1 = lanewise(VectorOperators.LSHL, n) >> vec2 = lanewise(VectorOperators.LSHR, n) >> res = lanewise(VectorOperations.OR, vec1 , vec2) >> >> This patch moves above handling from Java side to C2 compiler which >> facilitates dismantling the rotate operation if target ISA does not support >> a direct rotate instruction. >> >> AVX512 added vector rotate instructions vpro[rl][v][dq] which operate over >> long and integer type vectors. For other cases (i.e. sub-word type vectors >> or for targets which do not support direct rotate operations ) instruction >> sequence comprising of vector SHIFT (LEFT/RIGHT) and vector OR is emitted. >> >> Please find below the performance data for included JMH benchmark. >> Machine: Cascade Lake Server (Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz) >> >> >> Benchmark | (TESTSIZE) | Shift | Baseline AVX3 (ops/ms) | Withopt AVX3 >> (ops/ms) | Gain % | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain % >> -- | -- | -- | -- | -- | -- | -- | -- | -- >> | | | | | | | | >> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 17223.35 | 17094.69 | >> -0.75 | 17008.32 | 17488.06 | 2.82 >> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 8944.98 | 8811.34 | -1.49 >> | 8878.17 | 9218.68 | 3.84 >> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 17195.75 | 17137.32 | >> -0.34 | 16789.01 | 17780.34 | 5.90 >> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 9052.67 | 8838.60 | -2.36 >> | 8814.62 | 9206.01 | 4.44 >> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 17100.19 | 16950.64 | >> -0.87 | 16827.73 | 17720.37 | 5.30 >> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 9079.95 | 8471.26 | -6.70 >> | .44 | 9167.68 | 3.14 >> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 21231.33 | 21513.08 | 1.33 >> | 21824.51 | 21479.48 | -1.58 >> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 11103.62 | 11180.16 | 0.69 >> | 11173.67 | 11529.22 | 3.18 >> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 21119.14 | 21552.04 | >> 2.05 | 21693.05 | 21915.37 | 1.02 >> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 11048.68 | 11094.20 | >> 0.41 | 11049.90 | 11439.07 | 3.52 >> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 21506.31 | 21391.41 | >> -0.53 | 21263.18 | 21986.29 | 3.40 >> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 11056.12 | 11232.78 | >> 1.60 | 10941.59 | 11397.09 | 4.16 >> RotateBenchmark.testRotateLeftB | 512.00 | 7.00 | 17976.56 | 18180.85 | 1.14 >> | 1212.26 | 2533.34 | 108.98 >> RotateBenchmark.testRotateLeftB | 512.00 | 15.00 | 17553.70 | 18219.07 | >> 3.79 | 1256.73 | 2537.41 | 101.91 >> RotateBenchmark.testRotateLeftB | 512.00 | 31.00 | 17618.03 | 17738.15 | >> 0.68 | 1214.69 | 2533.83 | 108.60 >> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 7258.87 | 7468.88 | 2.89 | >> 7115.12 | 7117.26 | 0.03 >> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 3586.65 | 3950.85 | 10.15 >> | 3532.17 | 3595.80 | 1.80 >> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 1835.07 | 1999.68 | 8.97 | >> 1789.90 | 1819.93 | 1.68 >> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 7273.36 | 7410.91 | 1.89 >> | 7198.60 | 6994.79 | -2.83 >> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 3674.98 | 3926.27 | 6.84 >> | 3549.90 | 3755.09 | 5.78 >> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 1840.94 | 1882.25 | 2.24 >> | 1801.56 | 1872.89 | 3.96 >> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 7457.11 | 7361.48 | -1.28 >> | 6975.33 | 7385.94 | 5.89 >> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 3570.74 | 3929.30 | 10.04 >> | 3635.37 | 3736.67 | 2.79 >> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 1902.32 | 1960.46 | 3.06 >> | 1812.32 | 1813.88 | 0.09 >> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 11174.24 | 12044.52 | 7.79 >> | 11509.87 | 11273.44 | -2.05 >> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 5981.47 | 6073.70 | 1.54 | >> 5593.66 | 5661.93 | 1.22 >> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 2932.49 | 3069.54 | 4.67 | >> 2950.86 | 2892.42 | -1.98 >> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 11764.11 | 12098.63 | >> 2.84 | 11069.52 | 11476.93 | 3.68 >> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 5855.20 | 6080.40 | 3.85 >> | 5919.11 | 5607.04 | -5.27 >> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 2989.05 | 3048.56 | 1.99 >> | 2902.63 | 2821.83 | -2.78 >> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 11652.84 | 11965.40 | >> 2.68 | 11525.62 | 11459.83 | -0.57 >> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 5851.82 | 6164.94 | 5.35 >> | 5882.60 | 5842.30 | -0.69 >> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 3015.99 | 3043.79 | 0.92 >> | 2963.71 | 2947.97 | -0.53 >> RotateBenchmark.testRotateLeftI | 512.00 | 7.00 |
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v3]
On Tue, 13 Jul 2021 07:37:31 GMT, Wang Huang wrote: >> Dear all, >> Can you do me a favor to review this patch. This patch use `ldp` to >> implement String.compareTo. >> >> * We add a JMH test case >> * Here is the result of this test case >> >> Benchmark |(size)| Mode| Cnt|Score | Error |Units >> -|--|-||--||- >> StringCompare.compareLL | 64 | avgt| 5 |7.992 | ± 0.005|us/op >> StringCompare.compareLL | 72 | avgt| 5 |15.029| ± 0.006|us/op >> StringCompare.compareLL | 80 | avgt| 5 |14.655| ± 0.011|us/op >> StringCompare.compareLL | 91 | avgt| 5 |16.363| ± 0.12 |us/op >> StringCompare.compareLL | 101 | avgt| 5 |16.966| ± 0.007|us/op >> StringCompare.compareLL | 121 | avgt| 5 |19.276| ± 0.006|us/op >> StringCompare.compareLL | 181 | avgt| 5 |19.002| ± 0.417|us/op >> StringCompare.compareLL | 256 | avgt| 5 |24.707| ± 0.041|us/op >> StringCompare.compareLLWithLdp| 64 | avgt| 5 |8.001 | ± >> 0.121|us/op >> StringCompare.compareLLWithLdp| 72 | avgt| 5 |11.573| ± 0.003|us/op >> StringCompare.compareLLWithLdp| 80 | avgt| 5 |6.861 | ± 0.004|us/op >> StringCompare.compareLLWithLdp| 91 | avgt| 5 |12.774| ± 0.201|us/op >> StringCompare.compareLLWithLdp| 101 | avgt| 5 |8.691 | ± 0.004|us/op >> StringCompare.compareLLWithLdp| 121 | avgt| 5 |11.091| ± 1.342|us/op >> StringCompare.compareLLWithLdp| 181 | avgt| 5 |14.64 | ± 0.581|us/op >> StringCompare.compareLLWithLdp| 256 | avgt| 5 |25.879| ± 1.775|us/op >> StringCompare.compareUU | 64 | avgt| 5 |13.476| ± 0.01 |us/op >> StringCompare.compareUU | 72 | avgt| 5 |15.078| ± 0.006|us/op >> StringCompare.compareUU | 80 | avgt| 5 |23.512| ± 0.011|us/op >> StringCompare.compareUU | 91 | avgt| 5 |24.284| ± 0.008|us/op >> StringCompare.compareUU | 101 | avgt| 5 |20.707| ± 0.017|us/op >> StringCompare.compareUU | 121 | avgt| 5 |29.302| ± 0.011|us/op >> StringCompare.compareUU | 181 | avgt| 5 |39.31 | ± >> 0.016|us/op >> StringCompare.compareUU | 256 | avgt| 5 |54.592| ± 0.392|us/op >> StringCompare.compareUUWithLdp| 64 | avgt| 5 |16.389| ± 0.008|us/op >> StringCompare.compareUUWithLdp| 72 | avgt| 5 |10.71 | ± 0.158|us/op >> StringCompare.compareUUWithLdp| 80 | avgt| 5 |11.488| ± 0.024|us/op >> StringCompare.compareUUWithLdp| 91 | avgt| 5 |13.412| ± 0.006|us/op >> StringCompare.compareUUWithLdp| 101 | avgt| 5 |16.245| ± 0.434|us/op >> StringCompare.compareUUWithLdp| 121 | avgt| 5 |16.597| ± 0.016|us/op >> StringCompare.compareUUWithLdp| 181 | avgt| 5 |27.373| ± 0.017|us/op >> StringCompare.compareUUWithLdp| 256 | avgt| 5 |41.74 | ± 3.5 |us/op >> >> From this table, we can see that in most cases, our patch is better than old >> one. >> >> Thank you for your review. Any suggestions are welcome. > > Wang Huang has updated the pull request incrementally with one additional > commit since the last revision: > > refact codes Have you tested when the data in the `byte[]` array is not 16B aligned? With the default JVM options the array data naturally starts 16B into the object, but you can force a different alignment with e.g. `-XX:-UseCompressedClassPointers`. I tried that on N1 and it's very slightly slower than with the 16B alignment, but still faster than the non-LDP version for length 1024 strings. On A72 the difference is a bit bigger but again faster than non-LDP. N1, -UseCompressedClassPointers Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings 10231024 avgt5 67.789 ? 0.095 us/op StringCompare.compareLLDiffStringsWithLdp10231024 avgt5 45.912 ? 0.059 us/op StringCompare.compareUUDiffStrings 10231024 avgt5 133.365 ? 0.086 us/op StringCompare.compareUUDiffStringsWithLdp10231024 avgt5 89.009 ? 0.312 us/op N1, +UseCompressedClassPointers Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings 10231024 avgt5 67.878 ? 0.156 us/op StringCompare.compareLLDiffStringsWithLdp10231024 avgt5 46.487 ? 0.115 us/op StringCompare.compareUUDiffStrings 10231024 avgt5 133.576 ? 0.111 us/op StringCompare.compareUUDiffStringsWithLdp10231024 avgt5 90.462 ? 0.176 us/op A72, -UseCompressedClassPointers Benchmark (diff_pos) (size) Mode Cnt Score Error Units StringCompare.compareLLDiffStrings 10231024 avgt5 122.697 ? 0.235 us/op StringCompare.compareLLDiffStringsWithLdp10231024 avgt5 73.883 ?
