Re: RFR: JDK-8256844: Make NMT late-initializable [v2]
On Sun, 1 Aug 2021 08:17:08 GMT, Thomas Stuefe wrote: >> Short: this patch makes NMT available in custom-launcher scenarios and >> during gtests. It simplifies NMT initialization. It adds a lot of >> NMT-specific testing, cleans them up and makes them sideeffect-free. >> >> - >> >> NMT continues to be an extremely useful tool for SAP to tackle memory >> problems in the JVM. >> >> However, NMT is of limited use due to the following restrictions: >> >> - NMT cannot be used if the hotspot is embedded into a custom launcher >> unless the launcher actively cooperates. Just creating and invoking the JVM >> is not enough, it needs to do some steps prior to loading the hotspot. This >> limitation is not well known (nor, do I believe, documented). Many products >> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this >> problem limits NMT usefulness greatly since our VMs are often embedded into >> custom launchers and modifying every launcher is impossible. >> - Worse, if that custom launcher links the libjvm *statically* there is just >> no way to activate NMT at all. This is the reason NMT cannot be used in the >> `gtestlauncher`. >> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` >> and `-XX:Flags=`. >> - The fact that NMT cannot be used in gtests is really a pity since it would >> allow us to both test NMT itself more rigorously and check for memory leaks >> while testing other stuff. >> >> The reason for all this is that NMT initialization happens very early, on >> the first call to `os::malloc()`. And those calls happen already during >> dynamic C++ initialization - a long time before the VM gets around parsing >> arguments. So, regular VM argument parsing is too late to parse NMT >> arguments. >> >> The current solution is to pass NMT arguments via a specially prepared >> environment variable: `NMT_LEVEL_=`. That environment >> variable has to be set by the embedding launcher, before it loads the >> libjvm. Since its name contains the PID, we cannot even set that variable in >> the shell before starting the launcher. >> >> All that means that every launcher needs to especially parse and process the >> NMT arguments given at the command line (or via whatever method) and prepare >> the environment variable. `java` itself does this. This only works before >> the libjvm.so is loaded, before its dynamic C++ initialization. For that >> reason, it does not work if the launcher links statically against the >> hotspot, since in that case C++ initialization of the launcher and hotspot >> are folded into one phase with no possibility of executing code beforehand. >> >> And since it bypasses argument handling in the VM, it bypasses a number of >> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`. >> >> -- >> >> This patch fixes these shortcomings by making NMT late-initializable: it can >> now be initialized after normal VM argument parsing, like all other parts of >> the VM. This greatly simplifies NMT initialization and makes it work >> automagically for every third party launcher, as well as within our gtests. >> >> The glaring problem with late-initializing NMT is the NMT malloc headers. If >> we rule out just always having them (unacceptable in terms of memory >> overhead), there is no safe way to determine, in os::free(), if an >> allocation came from before or after NMT initialization ran, and therefore >> what to do with its malloc headers. For a more extensive explanation, please >> see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett >> and @zhengyu123 in the JBS comment section. >> >> The heart of this patch is a new way to track early, pre-NMT-init >> allocations. These are tracked via a lookup table. This was a suggestion by >> Kim and it worked out well. >> >> Changes in detail: >> >> - pre-NMT-init handling: >> - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init >> handling. They contain a small global lookup table managing C-heap blocks >> allocated in the pre-NMT-init phase. >> - `os::malloc()/os::realloc()/os::free()` defer to this code before >> doing anything else. >> - Please see the extensive comment block at the start of >> `nmtPreinit.hpp` explaining the details. >> >> - Changes to NMT: >> - Before, NMT initialization was spread over two phases, `initialize()` >> and `late_initialize()`. Those were merged into one and simplified - there >> is only one initialization now which happens after argument parsing. >> - Minor changes were needed for the `NMT_TrackingLevel` enum - to >> simplify code, I changed NMT_unknown to be numerically 0. A new comment >> block in `nmtCommon.hpp` now clearly specifies what's what, including >> allowed level state transitions. >> - New utility functions to translate tracking level from/to strings >> added to `NMTUtil` >> - NMT has never been able to handle virtual memory
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v5]
On Tue, 3 Aug 2021 13:33:07 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 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 six additional commits since > the last revision: > > - fix bugs > - Merge branch 'master' of https://gitee.com/ustc-wh/jdk into JDK-8268231 > - fix style and add unalign test case > - refact codes > - draft of refactor > - 8268231: Aarch64: Use ldp in intrinsics for String.compareTo Please provide the updated benchmark results for this version. Are you able to test it on several different machines? src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4892: > 4890: __ cmp(tmp1, tmp2); > 4891: __ ccmp(tmp1h, tmp2h, 0, Assembler::EQ); > 4892: __ br(__ NE, DIFF); The line above uses `Assembler::EQ` for the condition code but this line uses `__ NE`. Better to be consistent and use `Assembler::` everywhere. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places [v4]
On Tue, 3 Aug 2021 22:44:57 GMT, Raffaello Giulietti wrote: >> Hello, >> >> please review the changes in documentation of floorDiv() and floorMod() in >> all their variants. Some are clarifications, some are corrections. >> >> Here's an example of a confusing formulation in the current doc >>> "the / operator returns the integer closest to zero" >> >> Literally, the integer closest to zero is zero! >> >> >> The doc of floorMod(int, int) also states: >>> "If the signs of arguments are unknown and a positive modulus >>> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)." >> >> That's a questionable advice, as the sum in parentheses can lead to >> irrecoverable overflow (beside requiring two divisions instead of one). >> E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the >> quite incorrect result -3, which is not even positive! >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in > some places Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4963
Integrated: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
On Tue, 3 Aug 2021 19:05:55 GMT, Weijun Wang wrote: > `oddPart` contains a lot of info on the `modInverse` output, sometimes it's > even the same. Clearing it in case the result is sensitive. > > No new regression test since it's difficult to access a temporary local > variable in an internal class. Existing tier1-2 tests passed. This pull request has now been integrated. Changeset: a8408708 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/a8408708b065a877278acc6b007ad6a9baaf2561 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result Reviewed-by: bpb, darcy, valeriep - PR: https://git.openjdk.java.net/jdk/pull/4973
Re: RFR: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
On Tue, 3 Aug 2021 19:05:55 GMT, Weijun Wang wrote: > `oddPart` contains a lot of info on the `modInverse` output, sometimes it's > even the same. Clearing it in case the result is sensitive. > > No new regression test since it's difficult to access a temporary local > variable in an internal class. Existing tier1-2 tests passed. Marked as reviewed by valeriep (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4973
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places [v4]
> Hello, > > please review the changes in documentation of floorDiv() and floorMod() in > all their variants. Some are clarifications, some are corrections. > > Here's an example of a confusing formulation in the current doc >> "the / operator returns the integer closest to zero" > > Literally, the integer closest to zero is zero! > > > The doc of floorMod(int, int) also states: >> "If the signs of arguments are unknown and a positive modulus >> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)." > > That's a questionable advice, as the sum in parentheses can lead to > irrecoverable overflow (beside requiring two divisions instead of one). > E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the > quite incorrect result -3, which is not even positive! > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places - Changes: - all: https://git.openjdk.java.net/jdk/pull/4963/files - new: https://git.openjdk.java.net/jdk/pull/4963/files/dc81446f..21e4f370 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4963=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4963=02-03 Stats: 7 lines in 2 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4963.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4963/head:pull/4963 PR: https://git.openjdk.java.net/jdk/pull/4963
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places [v4]
On Tue, 3 Aug 2021 22:41:52 GMT, Raffaello Giulietti wrote: >> Hello, >> >> please review the changes in documentation of floorDiv() and floorMod() in >> all their variants. Some are clarifications, some are corrections. >> >> Here's an example of a confusing formulation in the current doc >>> "the / operator returns the integer closest to zero" >> >> Literally, the integer closest to zero is zero! >> >> >> The doc of floorMod(int, int) also states: >>> "If the signs of arguments are unknown and a positive modulus >>> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)." >> >> That's a questionable advice, as the sum in parentheses can lead to >> irrecoverable overflow (beside requiring two divisions instead of one). >> E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the >> quite incorrect result -3, which is not even positive! >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in > some places Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4963
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places [v3]
On Tue, 3 Aug 2021 22:37:12 GMT, Raffaello Giulietti
wrote:
>> Hello,
>>
>> please review the changes in documentation of floorDiv() and floorMod() in
>> all their variants. Some are clarifications, some are corrections.
>>
>> Here's an example of a confusing formulation in the current doc
>>> "the / operator returns the integer closest to zero"
>>
>> Literally, the integer closest to zero is zero!
>>
>>
>> The doc of floorMod(int, int) also states:
>>> "If the signs of arguments are unknown and a positive modulus
>>> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)."
>>
>> That's a questionable advice, as the sum in parentheses can lead to
>> irrecoverable overflow (beside requiring two divisions instead of one).
>> E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the
>> quite incorrect result -3, which is not even positive!
>>
>>
>> Greetings
>> Raffaello
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in
> some places
src/java.base/share/classes/java/lang/Math.java line 1250:
> 1248: * Returns the largest (closest to positive infinity)
> 1249: * {@code int} value that is less than or equal to the algebraic
> quotient.
> 1250: * There is one special case: if the dividend is the
I think the "the" at the end of line 1250 can be deleted.
src/java.base/share/classes/java/lang/Math.java line 1268:
> 1266: * while the {@code /} operator returns the smallest integer
> greater
> 1267: * than or equal to the quotient.
> 1268: * There's a difference if and only if the quotient is not an
> integer.
Maybe "There's a difference" -> "They differ" ?
src/java.base/share/classes/java/lang/Math.java line 1295:
> 1293: * Returns the largest (closest to positive infinity)
> 1294: * {@code long} value that is less than or equal to the algebraic
> quotient.
> 1295: * There is one special case: if the dividend is the
Again, "the" can go away.
src/java.base/share/classes/java/lang/Math.java line 1324:
> 1322: * Returns the largest (closest to positive infinity)
> 1323: * {@code long} value that is less than or equal to the algebraic
> quotient.
> 1324: * There is one special case: if the dividend is the
Delete "the" again.
src/java.base/share/classes/java/lang/StrictMath.java line 1054:
> 1052: * Returns the largest (closest to positive infinity)
> 1053: * {@code int} value that is less than or equal to the algebraic
> quotient.
> 1054: * There is one special case: if the dividend is the
"the"
src/java.base/share/classes/java/lang/StrictMath.java line 1078:
> 1076: * Returns the largest (closest to positive infinity)
> 1077: * {@code long} value that is less than or equal to the algebraic
> quotient.
> 1078: * There is one special case: if the dividend is the
"the"
src/java.base/share/classes/java/lang/StrictMath.java line 1102:
> 1100: * Returns the largest (closest to positive infinity)
> 1101: * {@code long} value that is less than or equal to the algebraic
> quotient.
> 1102: * There is one special case: if the dividend is the
"the"
-
PR: https://git.openjdk.java.net/jdk/pull/4963
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places [v3]
> Hello, > > please review the changes in documentation of floorDiv() and floorMod() in > all their variants. Some are clarifications, some are corrections. > > Here's an example of a confusing formulation in the current doc >> "the / operator returns the integer closest to zero" > > Literally, the integer closest to zero is zero! > > > The doc of floorMod(int, int) also states: >> "If the signs of arguments are unknown and a positive modulus >> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)." > > That's a questionable advice, as the sum in parentheses can lead to > irrecoverable overflow (beside requiring two divisions instead of one). > E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the > quite incorrect result -3, which is not even positive! > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places - Changes: - all: https://git.openjdk.java.net/jdk/pull/4963/files - new: https://git.openjdk.java.net/jdk/pull/4963/files/cab4c54c..dc81446f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4963=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4963=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4963.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4963/head:pull/4963 PR: https://git.openjdk.java.net/jdk/pull/4963
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places [v2]
> Hello, > > please review the changes in documentation of floorDiv() and floorMod() in > all their variants. Some are clarifications, some are corrections. > > Here's an example of a confusing formulation in the current doc >> "the / operator returns the integer closest to zero" > > Literally, the integer closest to zero is zero! > > > The doc of floorMod(int, int) also states: >> "If the signs of arguments are unknown and a positive modulus >> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)." > > That's a questionable advice, as the sum in parentheses can lead to > irrecoverable overflow (beside requiring two divisions instead of one). > E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the > quite incorrect result -3, which is not even positive! > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places - Changes: - all: https://git.openjdk.java.net/jdk/pull/4963/files - new: https://git.openjdk.java.net/jdk/pull/4963/files/423af129..cab4c54c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4963=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4963=00-01 Stats: 21 lines in 2 files changed: 2 ins; 0 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/4963.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4963/head:pull/4963 PR: https://git.openjdk.java.net/jdk/pull/4963
Re: RFR: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
On Tue, 3 Aug 2021 19:05:55 GMT, Weijun Wang wrote: > `oddPart` contains a lot of info on the `modInverse` output, sometimes it's > even the same. Clearing it in case the result is sensitive. > > No new regression test since it's difficult to access a temporary local > variable in an internal class. Existing tier1-2 tests passed. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4973
Re: RFR: 8271599: Javadoc of floorDiv() and floorMod() families is inaccurate in some places
On Mon, 2 Aug 2021 22:34:58 GMT, Raffaello Giulietti wrote: > Hello, > > please review the changes in documentation of floorDiv() and floorMod() in > all their variants. Some are clarifications, some are corrections. > > Here's an example of a confusing formulation in the current doc >> "the / operator returns the integer closest to zero" > > Literally, the integer closest to zero is zero! > > > The doc of floorMod(int, int) also states: >> "If the signs of arguments are unknown and a positive modulus >> is needed it can be computed as (floorMod(x, y) + abs(y)) % abs(y)." > > That's a questionable advice, as the sum in parentheses can lead to > irrecoverable overflow (beside requiring two divisions instead of one). > E.g., setting x = Integer.MAX_VALUE - 1, y = Integer.MAX_VALUE leads to the > quite incorrect result -3, which is not even positive! > > > Greetings > Raffaello Looks fine; please make the same edits to StrictMath for a final code review and CSR. - PR: https://git.openjdk.java.net/jdk/pull/4963
Re: RFR: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
On Tue, 3 Aug 2021 19:05:55 GMT, Weijun Wang wrote: > `oddPart` contains a lot of info on the `modInverse` output, sometimes it's > even the same. Clearing it in case the result is sensitive. > > No new regression test since it's difficult to access a temporary local > variable in an internal class. Existing tier1-2 tests passed. Looks fine. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4973
RFR: 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result
oddPart contains a lot of info on the modInverse output, sometimes it's even the same. Clearing it in case the result is sensitive. No new regression test since it's difficult to access a temporary local variable in an internal class. Existing tier1-2 tests passed. - Commit messages: - 8271616: oddPart in MutableBigInteger::mutableModInverse contains info on final result Changes: https://git.openjdk.java.net/jdk/pull/4973/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4973=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271616 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4973.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4973/head:pull/4973 PR: https://git.openjdk.java.net/jdk/pull/4973
Integrated: JDK-8271344: Windows product version issue
On Mon, 2 Aug 2021 13:45:17 GMT, Andy Herrick wrote: > JDK-8271344: Windows product version issue This pull request has now been integrated. Changeset: 0199b03e Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/0199b03eb3bd9ffe79b5193ff1792144b5c488f1 Stats: 21 lines in 5 files changed: 5 ins; 2 del; 14 mod 8271344: Windows product version issue Reviewed-by: asemenyuk, almatvee - PR: https://git.openjdk.java.net/jdk/pull/4958
Re: [External] : Re: jpackage MacOS Notarization
OK - took me a while to see what you are doing.
The problem is you are signing the pkg but building it from an unsigned
app-image.
If you are building in two phases (app-image, then installer package)
you need the --mac-sign option on both phases.
/Andy
On 8/3/2021 10:37 AM, Andy Herrick wrote:
I was having problems notarizing anything yesterday, but fixed them today.
I can notarize my own test - but get failure when trying to notarize
your app as built from non-modular branch of
https://github.com/danielpeintner/Java11Test/
am looking into it ...
/Andy
On 8/2/2021 9:57 AM, Daniel Peintner wrote:
Hi Andy,
sorry - code looks for certificate key starting with: "Developer
ID Application: " + in order
to not have to put full user name in. I missed that that with
null user name that causes it to look for anything starting with
"Developer ID Application: " (same thing with "Developer ID
Installer: " for .pkg signing). And macos looks at the
non-default keychains as well as the default ones when no
keychain is specified.
Thanks for your feedback and yes in the simple case (with one
certificate installed) it works without any further information.
May I ask you whether you were able to reproduce the issue?
Is there anything I can do to help?
Thanks,
-- Daniel
/Andy
On 7/29/2021 10:00 AM, Daniel Peintner wrote:
Hi Andy,
Since I don't know your setup I did not put anything there.
'--mac-sign' is enough to use the defaults in my setup.
It looks for the signing keys installed on my machine that start
with "Developer ID Application " similar to
'--mac-signing-key-user-name', 'Developer ID Application: '
etc.
If you want to test it you need to add your credentials which I
do not know.
Hope this clarifies things,
-- Daniel
On Thu, Jul 29, 2021 at 3:29 PM Andy Herrick
mailto:andy.herr...@oracle.com>> wrote:
The 'build.gradle' in this branch has
--mac-signing-key-user-name commented out.
installerOptions += [
'--mac-sign',
//
'--mac-s'SIGNING_KEY_USER_NAME'igning-key-user-name',
System.getenv('SIGNING_KEY_USER_NAME'),
// '--mac-signing-keychain',
System.getenv('SIGNING_KEYCHAIN_PATH')
]
clearly this cannot work, I assume you were just trying
things ...
What is the full name of the certificate you intended to
use, what keychain is it shown in "Keychain Access", and
what are you normal values for your environment variables:
'SIGNING_KEY_USER_NAME' and 'SIGNING_KEYCHAIN_PATH' ?
/Andy
On 7/29/2021 4:36 AM, Daniel Peintner wrote:
Kevin, Andy,
Thanks for your quick response.
Full support for notarization in jpackage was added in
JDK 17. Can you
try an early access build of JDK 17 [1] and see if that
works for you?
I did try JDK17-ea-32 also with the same result.
Since I do understand it is difficult reproduce the problem
I put together a *very* simple test application which you
can find in the "non-modular" branch here:
https://github.com/danielpeintner/Java11Test/tree/non-modular
It is a gradle project. It uses Java 11 to run but in
build.gradle on line#83[1] one can set the
jpackage location (JDK17-ea-32 in this case).
The process is as follows
* ./gradlew build
* ./gradlew jpackage // creates the dmg/pkg in folder
build/jpackage
* afterwards Apple notarization process can be started
Note: notarization of dmg or pkg lead to the same failure.
See [2] for the full log w.r.t. pkg.
I hope this helps you to be able to reproduce the issue.
Thanks for your investigations!
-- Daniel
[1]
https://github.com/danielpeintner/Java11Test/blob/6e5f34b1a0ba9c1e8ba1f6b15d6915237d8f5b7e/build.gradle#L83
[2]
https://osxapps-ssl.itunes.apple.com/itunes-assets/Enigma115/v4/90/4a/11/904a111c-01c7-ecc1-466c-40e7e8a09c56/developer_log.json?accessKey=1627741411_2564804966498057981_aHPs%2Fq9bzxGsY5Kd46U1QyWR8hmHJjLJLbUPpbvBqinIjiylTLsQy1APCJPkNN2w%2BZknT9OCl6zkzAyUm99EIBrm6tnOkZoWiwNG7TyukwCtAnIh%2FGpNAkLYfBpyDYjMaf7jQq8JekzxjYewhFuPDcJufWNrfuEX%2FN6zZoyz73I%3D
Re: RFR: 8271225: Add floorDivExact() method to java.lang.[Strict]Math
On Tue, 3 Aug 2021 08:42:18 GMT, Raffaello Giulietti wrote: >> The `floorDivExact()` methods are identical to their `floorDiv()` >> counterparts aside from that they throw an `ArithmeticException` when the >> dividend is `MIN_VALUE` and the divisor is `-1`. These methods behave with >> respect to the methods `floorDiv()` as the methods `divideExact()` behave >> with respect to the division operator `/`. > > Hello @bplb, > > the doc of floorDivExact() is not, well..., exact. > > For example, >> The floor rounding mode gives different results from truncation > when the exact result is negative. > > The truth is that the results are different when the exact quotient _is not > an integer and is negative_. > The description following this sentence is imprecise as well. > > The doc seems to have been copied and adapted from the doc of floorDiv(), > which is imprecise in the first place. > You might want to take a look at [PR > 4963](https://github.com/openjdk/jdk/pull/4963) which addresses this and wait > until that one is thoroughly discussed for mathematical accuracy and correct > wording. > > > Greetings > Raffaello @rgiulietti Yes, that doc was copied. I tend to agree that it would be better to get 4963 right and hold off on this PR until then. - PR: https://git.openjdk.java.net/jdk/pull/4941
Re: Quick question on JDK-8198997 and normalization of user.dir
Thanks for the quick confirmation!
On Tue, Aug 3, 2021 at 4:41 PM Alan Bateman wrote:
>
> On 03/08/2021 15:36, Volker Simonis wrote:
> > Hi,
> >
> > I have a quick question on JDK-8198997 [1] (a follow-up of JDK-8194154
> > [2]) which introduced normalization for the cached "user.dir" property
> > in order to avoid "inefficient, repeated normalization".
> >
> > However, from what I can see, WinNTFileSystem.getUserPath(), the only
> > place where the cached and normalized "userDir" is used, still calls
> > normalize() on each invocation:
> >
> > class WinNTFileSystem extends FileSystem {
> > ...
> > private final String userDir;
> > ...
> > public WinNTFileSystem() {
> > ...
> > userDir = normalize(props.getProperty("user.dir"));
> >}
> > ...
> > private String getUserPath() {
> > ...
> > return normalize(userDir);
> > }
> >
> > Is this just an oversight (in which case I'll fix it) or am I missing
> > something?
> >
> It looks like a left over, it's harmless, but it should be okay to
> change to just return userDir.
>
> -Alan
Re: Quick question on JDK-8198997 and normalization of user.dir
On 03/08/2021 15:36, Volker Simonis wrote:
Hi,
I have a quick question on JDK-8198997 [1] (a follow-up of JDK-8194154
[2]) which introduced normalization for the cached "user.dir" property
in order to avoid "inefficient, repeated normalization".
However, from what I can see, WinNTFileSystem.getUserPath(), the only
place where the cached and normalized "userDir" is used, still calls
normalize() on each invocation:
class WinNTFileSystem extends FileSystem {
...
private final String userDir;
...
public WinNTFileSystem() {
...
userDir = normalize(props.getProperty("user.dir"));
}
...
private String getUserPath() {
...
return normalize(userDir);
}
Is this just an oversight (in which case I'll fix it) or am I missing something?
It looks like a left over, it's harmless, but it should be okay to
change to just return userDir.
-Alan
Quick question on JDK-8198997 and normalization of user.dir
Hi,
I have a quick question on JDK-8198997 [1] (a follow-up of JDK-8194154
[2]) which introduced normalization for the cached "user.dir" property
in order to avoid "inefficient, repeated normalization".
However, from what I can see, WinNTFileSystem.getUserPath(), the only
place where the cached and normalized "userDir" is used, still calls
normalize() on each invocation:
class WinNTFileSystem extends FileSystem {
...
private final String userDir;
...
public WinNTFileSystem() {
...
userDir = normalize(props.getProperty("user.dir"));
}
...
private String getUserPath() {
...
return normalize(userDir);
}
Is this just an oversight (in which case I'll fix it) or am I missing something?
Thank you and best regards,
Volker
[1] https://bugs.openjdk.java.net/browse/JDK-8198997
[2] https://bugs.openjdk.java.net/browse/JDK-8194154
Re: [External] : Re: jpackage MacOS Notarization
I was having problems notarizing anything yesterday, but fixed them today.
I can notarize my own test - but get failure when trying to notarize
your app as built from non-modular branch of
https://github.com/danielpeintner/Java11Test/
am looking into it ...
/Andy
On 8/2/2021 9:57 AM, Daniel Peintner wrote:
Hi Andy,
sorry - code looks for certificate key starting with: "Developer
ID Application: " + in order
to not have to put full user name in. I missed that that with
null user name that causes it to look for anything starting with
"Developer ID Application: " (same thing with "Developer ID
Installer: " for .pkg signing). And macos looks at the
non-default keychains as well as the default ones when no keychain
is specified.
Thanks for your feedback and yes in the simple case (with one
certificate installed) it works without any further information.
May I ask you whether you were able to reproduce the issue?
Is there anything I can do to help?
Thanks,
-- Daniel
/Andy
On 7/29/2021 10:00 AM, Daniel Peintner wrote:
Hi Andy,
Since I don't know your setup I did not put anything there.
'--mac-sign' is enough to use the defaults in my setup.
It looks for the signing keys installed on my machine that start
with "Developer ID Application " similar to
'--mac-signing-key-user-name', 'Developer ID Application: '
etc.
If you want to test it you need to add your credentials which I
do not know.
Hope this clarifies things,
-- Daniel
On Thu, Jul 29, 2021 at 3:29 PM Andy Herrick
mailto:andy.herr...@oracle.com>> wrote:
The 'build.gradle' in this branch has
--mac-signing-key-user-name commented out.
installerOptions += [
'--mac-sign',
//
'--mac-s'SIGNING_KEY_USER_NAME'igning-key-user-name',
System.getenv('SIGNING_KEY_USER_NAME'),
// '--mac-signing-keychain',
System.getenv('SIGNING_KEYCHAIN_PATH')
]
clearly this cannot work, I assume you were just trying
things ...
What is the full name of the certificate you intended to use,
what keychain is it shown in "Keychain Access", and what are
you normal values for your environment variables:
'SIGNING_KEY_USER_NAME' and 'SIGNING_KEYCHAIN_PATH' ?
/Andy
On 7/29/2021 4:36 AM, Daniel Peintner wrote:
Kevin, Andy,
Thanks for your quick response.
Full support for notarization in jpackage was added in
JDK 17. Can you
try an early access build of JDK 17 [1] and see if that
works for you?
I did try JDK17-ea-32 also with the same result.
Since I do understand it is difficult reproduce the problem
I put together a *very* simple test application which you
can find in the "non-modular" branch here:
https://github.com/danielpeintner/Java11Test/tree/non-modular
It is a gradle project. It uses Java 11 to run but in
build.gradle on line#83[1] one can set the jpackage location
(JDK17-ea-32 in this case).
The process is as follows
* ./gradlew build
* ./gradlew jpackage // creates the dmg/pkg in folder
build/jpackage
* afterwards Apple notarization process can be started
Note: notarization of dmg or pkg lead to the same failure.
See [2] for the full log w.r.t. pkg.
I hope this helps you to be able to reproduce the issue.
Thanks for your investigations!
-- Daniel
[1]
https://github.com/danielpeintner/Java11Test/blob/6e5f34b1a0ba9c1e8ba1f6b15d6915237d8f5b7e/build.gradle#L83
[2]
https://osxapps-ssl.itunes.apple.com/itunes-assets/Enigma115/v4/90/4a/11/904a111c-01c7-ecc1-466c-40e7e8a09c56/developer_log.json?accessKey=1627741411_2564804966498057981_aHPs%2Fq9bzxGsY5Kd46U1QyWR8hmHJjLJLbUPpbvBqinIjiylTLsQy1APCJPkNN2w%2BZknT9OCl6zkzAyUm99EIBrm6tnOkZoWiwNG7TyukwCtAnIh%2FGpNAkLYfBpyDYjMaf7jQq8JekzxjYewhFuPDcJufWNrfuEX%2FN6zZoyz73I%3D
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v4]
On Thu, 15 Jul 2021 03:30:46 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: > > fix style and add unalign test case Thank you for your suggestion. I have pushed new commit. - PR: https://git.openjdk.java.net/jdk/pull/4722
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v5]
> 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 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 six additional commits since the last revision: - fix bugs - Merge branch 'master' of https://gitee.com/ustc-wh/jdk into JDK-8268231 - fix style and add unalign test case - refact codes - draft of refactor - 8268231: Aarch64: Use ldp in intrinsics for String.compareTo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4722/files - new: https://git.openjdk.java.net/jdk/pull/4722/files/c85cd126..60dd0516 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4722=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4722=03-04 Stats: 9839 lines in 397 files changed: 5783 ins; 2526 del; 1530 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: 8271225: Add floorDivExact() method to java.lang.[Strict]Math
On Thu, 29 Jul 2021 23:28:07 GMT, Brian Burkhalter wrote: >> Add methods `floorDivExact(int,int)` and `floorDivExact(long,long)` to >> `java.lang.Math` and `java.lang.StrictMath`. > > The `floorDivExact()` methods are identical to their `floorDiv()` > counterparts aside from that they throw an `ArithmeticException` when the > dividend is `MIN_VALUE` and the divisor is `-1`. These methods behave with > respect to the methods `floorDiv()` as the methods `divideExact()` behave > with respect to the division operator `/`. Hello @bplb, the doc of floorDivExact() is not, well..., exact. For example, > The floor rounding mode gives different results from truncation when the exact result is negative. The truth is that the results are different when the exact quotient _is not an integer and is negative_. The description following this sentence is imprecise as well. The doc seems to have been copied and adapted from the doc of floorDiv(), which is imprecise in the first place. You might want to take a look at [PR 4963](https://github.com/openjdk/jdk/pull/4963) which addresses this and wait until that one is thoroughly discussed for mathematical accuracy and correct wording. Greetings Raffaello - PR: https://git.openjdk.java.net/jdk/pull/4941
