date:20211122

On Mon, 22 Nov 2021 08:14:20 GMT, Alan Bateman  wrote:

>  It might be that jlink is throwing IAE for cases where another exception is 
> more appropriate, but it does suggests something intermittent in the jpackage 
> tests to trigger it.

Issue https://bugs.openjdk.java.net/browse/JDK-8277058 now contains a comment 
which includes the underlying exception stacktrace when that particular test 
was run using -Djlink.debug=true manually.

-

PR: https://git.openjdk.java.net/jdk/pull/6491

Re: RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures

On Sun, 21 Nov 2021 12:39:17 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which proposes to add more 
> diagnostics when a failure occurs in the jlink tool during the jpackage tests?
> 
> As noted in https://bugs.openjdk.java.net/browse/JDK-8277507, so far 3 
> failures have been reported in jpackage tests (across different test cases) 
> with the same underlying cause coming from the jlink tool. Since this failure 
> isn't specific to one or two tests and seems to be keep happening across 
> these tests in `test/jdk/tools/jpackage/`, I decided to set this system 
> property from one central location in the `HelloApp` which gets used by these 
> tests. These failures have only been reported on macos (and specifically 
> aarch64), but the commit here doesn't do any OS name checks, to keep this 
> change simple. Furthermore, looking at the 
> `jdk.tools.jlink.internal.JlinkTask` code, enabling this system property will 
> _not_ generate any additional logs unless there's an exception thrown by the 
> `jlink` tool, in which case it prints the exception stacktrace. So enabling 
> this by default won't end up increasing the log output or flooding the logs.
> 
> With this change, I have run the 3 tests noted in those issues locally to 
> make sure this doesn't break anything else. I have also verified manually 
> that enabling this system property does indeed get propagated to the 
> `JlinkTask` and checked the logs to see that the command line does pass it:
> 
>> 
>> [17:51:07.123] TRACE: exec: Execute 
>> [macosx-x86_64-server-release/images/jdk/bin/jpackage --input ./test/input 
>> --dest ./test/output --name "Name With Space" --type pkg 
>> -J-Djlink.debug=true --main-jar hello.jar --main-class Hello --verbose](15); 
>> inherit I/O...
>> [17:51:07.364] Building PKG package for Name With Space.
>> [17:51:19.191] Command [PID: -1]:
>> jlink --output 
>> /var/folders/7v/cnkwrnx154926cr3289w4rd8gp/T/jdk.jpackage13608930262477285540/images/image-3713034571561734902/Name
>>  With Space.app/Contents/runtime/Contents/Home --module-path 
>> macosx-x86_64-server-release/images/jdk/jmods --add-modules 
>> java.rmi,jdk.management.jfr,jdk.jdi,jdk.charsets,jdk.xml.dom,java.xml,java.datatransfer,jdk.jstatd,jdk.httpserver,java.desktop,java.security.sasl,jdk.zipfs,java.base,jdk.crypto.ec,jdk.javadoc,jdk.management.agent,jdk.jshell,jdk.editpad,jdk.jsobject,java.sql.rowset,jdk.sctp,jdk.unsupported,jdk.jlink,java.smartcardio,java.security.jgss,jdk.nio.mapmode,java.compiler,jdk.dynalink,jdk.unsupported.desktop,jdk.accessibility,jdk.security.jgss,jdk.incubator.vector,java.sql,java.xml.crypto,java.logging,java.transaction.xa,jdk.jfr,jdk.crypto.cryptoki,jdk.net,jdk.random,java.naming,jdk.internal.ed,java.prefs,java.net.http,jdk.compiler,jdk.internal.opt,jdk.naming.rmi,jdk.jconsole,jdk.attach,jdk.internal.le,java.management,jdk.jdwp.agent,jd
 
k.incubator.foreign,jdk.internal.jvmstat,java.instrument,jdk.management,jdk.security.auth,java.scripting,jdk.jdeps,jdk.jartool,jdk.jpackage,java.management.rmi,jdk.naming.dns,jdk.localedata
 --strip-native-commands --strip-debug --no-man-pages --no-header-files
>> [17:51:19.192] Output:
>> WARNING: Using incubator modules: jdk.incubator.foreign, 
>> jdk.incubator.vector
>> 
>> [17:51:19.192] Returned: 0
>> 
> 
> These tests get run in `tier2` and I have no way of running the entire 
> `tier2` locally or relying of GitHub actions which only runs `tier1`. If this 
> change looks OK and is approved, then I would like to request for help 
> running the entire tests against this PR before merging it.

Thank you Alan. I'll wait for someone from jpackage area to take a look at this 
change.

-

PR: https://git.openjdk.java.net/jdk/pull/6491

Re: RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures

2021-11-22 Thread Alexander Matveev

On Sun, 21 Nov 2021 12:39:17 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which proposes to add more 
> diagnostics when a failure occurs in the jlink tool during the jpackage tests?
> 
> As noted in https://bugs.openjdk.java.net/browse/JDK-8277507, so far 3 
> failures have been reported in jpackage tests (across different test cases) 
> with the same underlying cause coming from the jlink tool. Since this failure 
> isn't specific to one or two tests and seems to be keep happening across 
> these tests in `test/jdk/tools/jpackage/`, I decided to set this system 
> property from one central location in the `HelloApp` which gets used by these 
> tests. These failures have only been reported on macos (and specifically 
> aarch64), but the commit here doesn't do any OS name checks, to keep this 
> change simple. Furthermore, looking at the 
> `jdk.tools.jlink.internal.JlinkTask` code, enabling this system property will 
> _not_ generate any additional logs unless there's an exception thrown by the 
> `jlink` tool, in which case it prints the exception stacktrace. So enabling 
> this by default won't end up increasing the log output or flooding the logs.
> 
> With this change, I have run the 3 tests noted in those issues locally to 
> make sure this doesn't break anything else. I have also verified manually 
> that enabling this system property does indeed get propagated to the 
> `JlinkTask` and checked the logs to see that the command line does pass it:
> 
>> 
>> [17:51:07.123] TRACE: exec: Execute 
>> [macosx-x86_64-server-release/images/jdk/bin/jpackage --input ./test/input 
>> --dest ./test/output --name "Name With Space" --type pkg 
>> -J-Djlink.debug=true --main-jar hello.jar --main-class Hello --verbose](15); 
>> inherit I/O...
>> [17:51:07.364] Building PKG package for Name With Space.
>> [17:51:19.191] Command [PID: -1]:
>> jlink --output 
>> /var/folders/7v/cnkwrnx154926cr3289w4rd8gp/T/jdk.jpackage13608930262477285540/images/image-3713034571561734902/Name
>>  With Space.app/Contents/runtime/Contents/Home --module-path 
>> macosx-x86_64-server-release/images/jdk/jmods --add-modules 
>> java.rmi,jdk.management.jfr,jdk.jdi,jdk.charsets,jdk.xml.dom,java.xml,java.datatransfer,jdk.jstatd,jdk.httpserver,java.desktop,java.security.sasl,jdk.zipfs,java.base,jdk.crypto.ec,jdk.javadoc,jdk.management.agent,jdk.jshell,jdk.editpad,jdk.jsobject,java.sql.rowset,jdk.sctp,jdk.unsupported,jdk.jlink,java.smartcardio,java.security.jgss,jdk.nio.mapmode,java.compiler,jdk.dynalink,jdk.unsupported.desktop,jdk.accessibility,jdk.security.jgss,jdk.incubator.vector,java.sql,java.xml.crypto,java.logging,java.transaction.xa,jdk.jfr,jdk.crypto.cryptoki,jdk.net,jdk.random,java.naming,jdk.internal.ed,java.prefs,java.net.http,jdk.compiler,jdk.internal.opt,jdk.naming.rmi,jdk.jconsole,jdk.attach,jdk.internal.le,java.management,jdk.jdwp.agent,jd
 
k.incubator.foreign,jdk.internal.jvmstat,java.instrument,jdk.management,jdk.security.auth,java.scripting,jdk.jdeps,jdk.jartool,jdk.jpackage,java.management.rmi,jdk.naming.dns,jdk.localedata
 --strip-native-commands --strip-debug --no-man-pages --no-header-files
>> [17:51:19.192] Output:
>> WARNING: Using incubator modules: jdk.incubator.foreign, 
>> jdk.incubator.vector
>> 
>> [17:51:19.192] Returned: 0
>> 
> 
> These tests get run in `tier2` and I have no way of running the entire 
> `tier2` locally or relying of GitHub actions which only runs `tier1`. If this 
> change looks OK and is approved, then I would like to request for help 
> running the entire tests against this PR before merging it.

Marked as reviewed by almatvee (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6491

Re: RFR: 8277322: Document that setting an invalid property `jdk.serialFilter` disables deserialization

2021-11-22 Thread Stuart Marks

On Mon, 22 Nov 2021 19:57:25 GMT, Roger Riggs  wrote:

> The effects of an invalid `jdk.serialFilter` property are not completely 
> documented. If the value of the system property jdk.serialFilter is invalid, 
> deserialization should not be possible and it should be clear in the 
> specification. 
> 
> Specify an implementation specific exception is thrown in the case where 
> deserialization is invoked after reporting the invalid jdk.serialFilter.

src/java.base/share/classes/java/io/ObjectInputFilter.java line 529:

> 527:  * if the filter string is invalid, an {@link 
> ExceptionInInitializerError} is thrown
> 528:  * and the initialization fails; subsequent attempts to use the 
> configuration or
> 529:  * serialization will fail with an implementation specific exception.

I'm confused about exactly what happens after `ExceptionInInitializerError`.

> Subsequent attempts to use the configuration or serialization will fail

Which configuration? I thought OIF.Config is a utility class and thus has no 
instances. If its class initialization fails, then other code cannot use 
`Config.setSerialFilter` to set a global filter (which might be desirable, but 
throws NCDFE instead of `IllegalStateException`) and other code can't use 
`Config.createFilter` to create individual filters. Is that right? It seems 
like there ought to be a better arrangement than to have the system come up in 
some dysfunctional way, where any subsequent reference to `OIF.Config` results 
in NCDFE.

And surely this affects deserialization, not serialization?

-

PR: https://git.openjdk.java.net/jdk/pull/6508

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v5]

> Please review this PR which aims to optimize the implementation of the 
> `toString` method we provide for records. A benchmark comparing the 
> implementation we are providing for records with lombok found out that lombok 
> is much faster mainly because our implementation uses `String::format`. This 
> fix is basically delegating on StringConcatFactory::makeConcatWithConstants 
> which is faster.
> 
> TIA
> 
> This is the result of the benchmark comparing records to lombok with vanilla 
> JDK:
> 
> Benchmark  Mode  CntScoreError  Units
> MyBenchmark.base   avgt40.849 ±  0.111  ns/op
> MyBenchmark.equals_record  avgt47.343 ±  2.740  ns/op
> MyBenchmark.equals_value   avgt46.644 ±  1.920  ns/op
> MyBenchmark.record_hash_code   avgt45.763 ±  3.882  ns/op
> MyBenchmark.record_to_string   avgt4  262.626 ± 12.574  ns/op 
>   <-- Before
> MyBenchmark.value_class_to_string  avgt4   30.325 ± 21.389  ns/op
> MyBenchmark.value_hash_codeavgt45.048 ±  3.936  ns/op
> 
> 
> after this patch:
> 
> Benchmark  Mode  Cnt   Score   Error  Units
> MyBenchmark.base   avgt4   0.680 ± 0.185  ns/op
> MyBenchmark.equals_record  avgt4   5.599 ± 1.348  ns/op
> MyBenchmark.equals_value   avgt4   5.718 ± 4.633  ns/op
> MyBenchmark.record_hash_code   avgt4   4.628 ± 4.368  ns/op
> MyBenchmark.record_to_string   avgt4  26.791 ± 1.817  ns/op   
>   <--- After
> MyBenchmark.value_class_to_string  avgt4  35.473 ± 2.626  ns/op
> MyBenchmark.value_hash_codeavgt4   6.152 ± 5.101  ns/op

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  fixing typos and adding a comment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6403/files
  - new: https://git.openjdk.java.net/jdk/pull/6403/files/2d3240e1..52d04ecc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6403=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6403=03-04

  Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased [v2]

2021-11-22 Thread Stuart Marks

On Fri, 19 Nov 2021 20:47:40 GMT, Roger Riggs  wrote:

>> The `jdk.internal.ValueBased` annotation was incorrectly applied to 
>> subclasses of java.util.AbstractMap.
>> [ValueBased](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html)
>>  requires that supertypes have no instance fields; AbstractMap has instance 
>> fields keySet and values.
>> 
>> Remove the internal @ValueBased annotation for subclasses of AbstractMap 
>> including:
>> AbstractImmutableMap, Map1, and MapN.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comment explaining why immutable Maps are not 'ValueBased'

Marked as reviewed by smarks (Reviewer).

Yes, we want to leave the "value-based" stipulation in the specification, even 
if the ValueBased annotation is removed. In the future we might be able to 
remove the dependency on `AbstractMap` and thus avoid the mutable fields in the 
unmodifiable collections. Or maybe `AbstractMap` should lose them entirely. The 
`keySet` and `values` collections are fairly small objects themselves, so 
caching them might not be much benefit in the first place.

-

PR: https://git.openjdk.java.net/jdk/pull/6480

Re: [External] : Re: jpackage - notarizing

> On Nov 22, 2021, at 9:01 PM, Kevin Rushforth  
> wrote:
> 
> By way of clarification, JNF was never part of the JDK. What we removed was 
> the JDK's *usage* of JNF. And we did so because Apple deprecated it and never 
> even provide a JNF framework for aarch64.
> 
> Applications either need to migrate off JNF and find an alternative, or take 
> Apple's open-source project, build it yourself, and include it with your 
> application. Either way, this isn't a JDK problem.
> 
> — Kevin

None of what Apple contributed to the Mac port project was part of the JDK. But 
legacy Apple Java code still used it. Probably less over time, allowing it to 
be phased out. All I was thinking of was somewhere it would still be available 
to compile old code. What Alan pointed out would probably provide that if Apple 
stops including it with Xcode - if I figure it out. If you look at the GitHub 
project the contributors include people you still see active on the openjdk 
lists. All I basically need is it there to compile old code I don’t actively 
develop. 

Alan’s usage sounded a little more critical.

Re: [External] : Re: jpackage - notarizing

2021-11-22 Thread Kevin Rushforth

By way of clarification, JNF was never part of the JDK. What we removed 
was the JDK's *usage* of JNF. And we did so because Apple deprecated it 
and never even provide a JNF framework for aarch64.


Applications either need to migrate off JNF and find an alternative, or 
take Apple's open-source project, build it yourself, and include it with 
your application. Either way, this isn't a JDK problem.


-- Kevin


On 11/22/2021 11:56 AM, Michael Hall wrote:



On Nov 22, 2021, at 12:39 PM, Alan Snyder  wrote:



On Nov 22, 2021, at 10:12 AM, Kevin Rushforth  
wrote:

JNF was removed from the JDK if that's what you are asking.



Indeed, that is why there is an issue.

The JDK may not be using JNF, but library developers still use it.

The JDK replacement for JNF is not supported for use outside the JDK.

JNF is not just convenient, there is at least one essential use for which there 
is no adequate replacement.

See https://bugs.openjdk.java.net/browse/JDK-8274596




I have a fair amount of code that would break if some JNF option wasn’t 
available. I have no estimate for how much rewriting I would need to do. Two of 
the projects I needed to recompile to get notarized needed it. I don’t actively 
do anything with it anymore but have some older code that did.

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Mon, 22 Nov 2021 15:53:53 GMT, Claes Redestad  wrote:

> FWIW I did a few experiments trying to move the chunking to `SCF`. Most made 
> things worse, but this is getting close: 
> https://github.com/openjdk/jdk/compare/master...cl4es:scf_split?expand=1
> 
> The threshold for when the JIT fails to optimize seem to be pushed up a bit 
> and I get a 4x gains when concatenating 100 Strings (though it takes a good 
> while for the microbenchmark to stabilize). It also fails to make much of a 
> difference when looking at 200 arguments (maybe 1.4x win). The experiment I 
> have done so far are crude and aggregates the results into a String builder 
> with no size estimation, so it adds a but of unnecessary allocation that 
> could be improved. I think this needs way more work to be a good enhancement 
> and that splitting up outside is going to remain better for now.
> 
> A "hidden" drawback with chunking is that you're likely going to be 
> allocating more.
> 
> I also re-realized that it'll be pretty hard (or impossible) to get around 
> having some MH slot limit: even though we chunk it up internally, the MH we 
> return must have a type that matches the type given to the bootstrap method, 
> and any operations on the tree that require some temporary arguments we need 
> to pass around will require some argument slots. The current strategy doesn't 
> use too many temporaries, so the real limit might be around 250, but it makes 
> sense to give some room for alternative implementations that use more 
> temporaries if that makes things more efficient.

Thanks for sharing this and for doing the experiment!

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8277322: Document that setting an invalid property `jdk.serialFilter` disables deserialization

On Mon, 22 Nov 2021 19:57:25 GMT, Roger Riggs  wrote:

> The effects of an invalid `jdk.serialFilter` property are not completely 
> documented. If the value of the system property jdk.serialFilter is invalid, 
> deserialization should not be possible and it should be clear in the 
> specification. 
> 
> Specify an implementation specific exception is thrown in the case where 
> deserialization is invoked after reporting the invalid jdk.serialFilter.

src/java.base/share/classes/java/io/ObjectInputFilter.java line 530:

> 528:  * and the initialization fails; subsequent attempts to use the 
> configuration or
> 529:  * serialization will fail with an implementation specific exception.
> 530:  * If the system property {@code jdk.serialFilter} is not set on the 
> command line

Hello Roger,
Thank you for rearranging these lines. It reads much more clearly. One tiny 
final question - this new line now states `If the system property {@code 
jdk.serialFilter} is not set on the command line it can be set with `. 
However, this property if not set on the command line could have instead been 
set as a `java.security.Security` property (in a file). The javadoc does 
mention this a few lines back. So do you think this new line should be reworded 
to something like `If the filter is neither set as a system property on the 
command line nor as a security property then it can be set with...`

-

PR: https://git.openjdk.java.net/jdk/pull/6508

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]

On Sat, 20 Nov 2021 12:10:40 GMT, Jim Laskey  wrote:

>> @stuart-marks yes, a general purpose splitting logic moved into the 
>> `StringConcatFactory` would be able to get rid of the arbitrary 200 slot 
>> limit (we would hit a harder but less arbitrary limit at around 253 instead).
>> 
>> @JimLaskey I don't see why it wouldn't work generally from the point of view 
>> of the `StringConcatFactory`: Vicente's code operates on a `MethodHandle[]` 
>> with getters as inputs to the `SCF` bootstrap method, whereas `SCF` would 
>> deal with arguments directly (retrieved from an `Object[]`). I think the 
>> code changes from the patch here after moving the logic into `SCF` should be 
>> pretty minimal and straightforward: if I'm not missing anything we'd only 
>> conceptually be replacing the `filterArguments` on line 313 with an 
>> `insertArguments`.
>
> @cl4es I was thinking of the more general case 200 < N. You were in the 200 < 
> N < 253 case. I think it would be better to pass in a chunk size (<= 200) and 
> an array of ptype and get an array of method handles each taking a chunk size 
> of arguments plus result of prior call. This is more like what javac does.

@JimLaskey I'm generating several recipes one per each invocation of 
StringConcatFactory::makeConcatWithConstants I think that generating only one 
recipe is possible only if the number of `getters` we are dealing with is less 
than the maximum number of slots we want to split the `getters` into

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8277606: String(String) constructor could copy hashIsZero

On Mon, 22 Nov 2021 22:59:21 GMT, PROgrm_JARvis  wrote:

> This is a trivial fix to make `String(String)` constructor copy the value of 
> `hashIsZero` field.

LGTM

-

Marked as reviewed by redestad (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6511

Re: Constructor `String(String)` does not copy `hashIsZero`

2021-11-22 Thread Japris Pogrammer

Thanks, I've created the PR [1] with the fix.

[1]: https://github.com/openjdk/jdk/pull/6511

вт, 23 нояб. 2021 г., 01:42 Claes Redestad :

> Hi,
>
> this appears to be unintentionally left out. I've filed a bug[1].
>
> While marked as an @IntrinsicCandidate, I can't see that HotSpot/C2 is
> actually intrinsifying this constructor. The signature is used for
> some pattern matching in the legacy stringopts.cpp code, though. So
> the trivial fix should help a bit.
>
> Thanks
> Claes
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8277606
>
> On 2021-11-22 22:20, Japris Pogrammer wrote:
> > According to openjdk/jdk [1] current copy-constructor of String class
> does
> > not copy the value of hashIsZero field which may lead to 0-hash being
> > recalculated on copy even if it is known to be 0.
> > Could you please tell me if I am right with this point or if this
> behaviour
> > is intentional?
> > If this is actually a bug then I am ready to propose a trivial fix for
> > it[2].
> >
> > [1]:
> >
> https://github.com/openjdk/jdk/blob/05a9a51dbfc46eb52bc28f1f9a618c75ee2597e9/src/java.base/share/classes/java/lang/String.java#L259-L264
> > [2]: https://github.com/JarvisCraft/jdk/tree/String-copy-zero-hashCode
> >
>

RFR: 8277606: String(String) constructor could copy hashIsZero

2021-11-22 Thread PROgrm_JARvis

This is a trivial fix to make `String(String)` constructor copy the value of 
`hashIsZero` field.

-

Commit messages:
 - fix: copy `hashIsZero` in `String#String(String)`

Changes: https://git.openjdk.java.net/jdk/pull/6511/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6511=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277606
  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6511.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6511/head:pull/6511

PR: https://git.openjdk.java.net/jdk/pull/6511

Re: Constructor `String(String)` does not copy `hashIsZero`


Hi,

this appears to be unintentionally left out. I've filed a bug[1].

While marked as an @IntrinsicCandidate, I can't see that HotSpot/C2 is
actually intrinsifying this constructor. The signature is used for
some pattern matching in the legacy stringopts.cpp code, though. So
the trivial fix should help a bit.

Thanks
Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8277606

On 2021-11-22 22:20, Japris Pogrammer wrote:

According to openjdk/jdk [1] current copy-constructor of String class does
not copy the value of hashIsZero field which may lead to 0-hash being
recalculated on copy even if it is known to be 0.
Could you please tell me if I am right with this point or if this behaviour
is intentional?
If this is actually a bug then I am ready to propose a trivial fix for
it[2].

[1]:
https://github.com/openjdk/jdk/blob/05a9a51dbfc46eb52bc28f1f9a618c75ee2597e9/src/java.base/share/classes/java/lang/String.java#L259-L264
[2]: https://github.com/JarvisCraft/jdk/tree/String-copy-zero-hashCode

Re: RFR: 8276904: Optional.toString() is unnecessarily expensive

2021-11-22 Thread Stuart Marks

On Wed, 10 Nov 2021 17:44:04 GMT, Eamonn McManus  wrote:

> Use string concatenation instead of `String.format`.

I talked to Brian about this a bit. Changing `format` to concatenation is 
probably ok this case for two reasons: 1) it's the simplest possible such 
transformation, and as such it's not a very good example for the templating 
stuff; and 2) presumably you bothered filing the bug and the PR because you 
believe the performance improvement would be noticeable in actual systems.

I note that some Amazon folks had filed 
[JDK-8275190](https://bugs.openjdk.java.net/browse/JDK-8275190) for what is 
essentially the same issue. (I closed it as a dupe.) They did include some 
performance numbers, but not the actual code they measured, so without guessing 
it's hard to say what they actually measured. Could you run some numbers and 
post them, and can you share some estimates of the impact it might have on your 
systems?

The reason I'm interested in some data here is that this sort of change is 
worth doing only if it has a real, measurable impact. In turn that implies that 
you're probably using `Optional::toString` a lot. That's an important 
criterion; we don't want to do a wholesale replacement of `format` with 
concatenation in the JDK, based on some micro benchmarks, that end up garbaging 
up the JDK code base while not actually improving any real systems.

-

PR: https://git.openjdk.java.net/jdk/pull/6337

Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v26]

2021-11-22 Thread Joe Darcy

On Mon, 22 Nov 2021 12:09:30 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-419 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request with a new target base due 
> to a merge or a rebase. The pull request now contains 35 commits:
> 
>  - Merge branch 'master' into JEP-419
>  - Fix javadoc issues found in CSR review
>  - Adopt blessed modofier order
>  - Merge branch 'master' into JEP-419
>  - Revert removal of upcall MH customization
>(This change caused spurious VM crashes, so reverting to baseline)
>  - Further tweak upcall safety considerations
>  - Clarify safety considerations for upcalls
>  - Rename MemorySegment::ofAddressNative to MemorySegment::ofAddress
>(which is consistent with other restricted factories in VaList and 
> NativeSymbol)
>  - Streamline javadoc for package-info
>  - * Add two new CLinker static methods to compute upcall/downcall method 
> types
>* Clarify section on CLinker downcall type
>* Add section on CLinker safety guarantees
>  - ... and 25 more: 
> https://git.openjdk.java.net/jdk/compare/d427c79d...29cc6c60

Marked as reviewed by darcy (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5907

Re: RFR: 8277322: Document that setting an invalid property `jdk.serialFilter` disables deserialization

2021-11-22 Thread Iris Clark

On Mon, 22 Nov 2021 19:57:25 GMT, Roger Riggs  wrote:

> The effects of an invalid `jdk.serialFilter` property are not completely 
> documented. If the value of the system property jdk.serialFilter is invalid, 
> deserialization should not be possible and it should be clear in the 
> specification. 
> 
> Specify an implementation specific exception is thrown in the case where 
> deserialization is invoked after reporting the invalid jdk.serialFilter.

Associated CSR also Reviewed.

-

Marked as reviewed by iris (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6508

Constructor `String(String)` does not copy `hashIsZero`

2021-11-22 Thread Japris Pogrammer

According to openjdk/jdk [1] current copy-constructor of String class does
not copy the value of hashIsZero field which may lead to 0-hash being
recalculated on copy even if it is known to be 0.
Could you please tell me if I am right with this point or if this behaviour
is intentional?
If this is actually a bug then I am ready to propose a trivial fix for
it[2].

[1]:
https://github.com/openjdk/jdk/blob/05a9a51dbfc46eb52bc28f1f9a618c75ee2597e9/src/java.base/share/classes/java/lang/String.java#L259-L264
[2]: https://github.com/JarvisCraft/jdk/tree/String-copy-zero-hashCode

Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v2]

2021-11-22 Thread Mikael Vidstedt

On Mon, 22 Nov 2021 04:30:38 GMT, Joe Darcy  wrote:

>> The time to get JDK 19 underway draws nigh, please review this usual set of 
>> start-of-release updates, including CSRs for the javac and javax.lang.model 
>> updates:
>> 
>> JDK-8277512: Add SourceVersion.RELEASE_19
>> https://bugs.openjdk.java.net/browse/JDK-8277512
>> 
>> JDK-8277514: Add source 19 and target 19 to javac
>> https://bugs.openjdk.java.net/browse/JDK-8277514
>> 
>> Clean local tier 1 test runs for langtools, jdk, and hotspot.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by mikael (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6493

String.indexOf(single-char-String)

2021-11-22 Thread Michael Bien


Hello,

I kept forgetting which variants of the String methods perform better 
with single-char-Strings and which with char (IDEs had the tendency to 
suggest the wrong variant since it changed between JDK releases). So i 
wrote JMH benchmarks and noticed that the last method with a performance 
difference seems to be String.indexOf() - all other variants performed 
equally (unless I overlooked some).


this might be fairly easy to fix:

https://github.com/openjdk/jdk/pull/6509

(side effect: contains("c") is also faster)

I haven't looked into the intrinsified code of StringLatin1 and 
StringUTF16 to check if it could be fixed there (mostly because i 
actually don't know how the JVM assembles those intrinsics). It might be 
possible to improve this for short Strings in general, not just for 
chars, dependent on why the intrinsified version is actually slower for 
single-char-Strings. I opted for the trivial fix in java code.


best regards,

michael

Re: RFR: 8272042: java.util.ImmutableCollections$Map1 and MapN should not be @ValueBased [v2]

2021-11-22 Thread Roger Riggs

On Sun, 21 Nov 2021 11:15:53 GMT, Jens Li  wrote:

> Should this change also update the Javadoc on `java.util.Map` and remove the 
> statement that the return values of `Map#of` are value-based?
> 

This PR is only correcting the application of the internal use annotation to 
some implementation classes.
The descriptions of unmodifiable maps identify the conditions the clients of 
Map.of may and may not assume about the returned instances including those 
related to identity.

-

PR: https://git.openjdk.java.net/jdk/pull/6480

Re: RFR: JDK-8277451: java.lang.reflect.Field::set on static field with invalid argument type should throw IAE [v2]

On Mon, 22 Nov 2021 20:01:50 GMT, Mandy Chung  wrote:

>> java.lang.reflect.Field::set on static field with invalid argument type 
>> should throw IAE.  But this regression is introduced by JEP 416 throwing NPE 
>> instead.
>> 
>> `ensureObj` is called as the first check of the `Field::set` method to 
>> ensure the receiver object is checked first before the argument.   For a 
>> Field instance with write-access, the method handle invocation will check 
>> the receiver.  Therefore for `Field::setXXX` methods to set a primitive 
>> value, `ensureObj` is only called if it's a read-only Field instance to 
>> ensure IllegalArgumentException is thrown first before 
>> IllegalAccessException to keep the existing behavior to avoid duplicated 
>> receiver check.
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Improve the exception message when the type is not available

Marked as reviewed by alanb (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6490

RFR: 8277322: Document that setting an invalid property `jdk.serialFilter` disables deserialization

2021-11-22 Thread Roger Riggs

The effects of an invalid `jdk.serialFilter` property are not completely 
documented. If the value of the system property jdk.serialFilter is invalid, 
deserialization should not be possible and it should be clear in the 
specification. 

Specify an implementation specific exception is thrown in the case where 
deserialization is invoked after reporting the invalid jdk.serialFilter.

-

Commit messages:
 - 8277322: Document that setting an invalid property `jdk.serialFilter` 
disables deserialization

Changes: https://git.openjdk.java.net/jdk/pull/6508/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6508=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277322
  Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6508.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6508/head:pull/6508

PR: https://git.openjdk.java.net/jdk/pull/6508

Re: RFR: JDK-8277451: java.lang.reflect.Field::set on static field with invalid argument type should throw IAE [v2]

2021-11-22 Thread Mandy Chung

> java.lang.reflect.Field::set on static field with invalid argument type 
> should throw IAE.  But this regression is introduced by JEP 416 throwing NPE 
> instead.
> 
> `ensureObj` is called as the first check of the `Field::set` method to ensure 
> the receiver object is checked first before the argument.   For a Field 
> instance with write-access, the method handle invocation will check the 
> receiver.  Therefore for `Field::setXXX` methods to set a primitive value, 
> `ensureObj` is only called if it's a read-only Field instance to ensure 
> IllegalArgumentException is thrown first before IllegalAccessException to 
> keep the existing behavior to avoid duplicated receiver check.

Mandy Chung has updated the pull request incrementally with one additional 
commit since the last revision:

  Improve the exception message when the type is not available

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6490/files
  - new: https://git.openjdk.java.net/jdk/pull/6490/files/1c679d7a..63134634

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6490=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6490=00-01

  Stats: 27 lines in 2 files changed: 14 ins; 10 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6490.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6490/head:pull/6490

PR: https://git.openjdk.java.net/jdk/pull/6490

Re: RFR: JDK-8277451: java.lang.reflect.Field::set on static field with invalid argument type should throw IAE [v2]

2021-11-22 Thread Mandy Chung

On Sun, 21 Nov 2021 21:05:19 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/jdk/internal/reflect/MethodHandleFieldAccessorImpl.java
>>  line 72:
>> 
>>> 70:  */
>>> 71: protected IllegalArgumentException 
>>> newGetIllegalArgumentException(Object o) {
>>> 72: return new IllegalArgumentException(getMessage(true, o != null 
>>> ? o.getClass().getName() : ""));
>> 
>> Hello Mandy,
>> With this change the `getMessage` may get passed an empty string, so it 
>> would end up printing something like `Can not get ... field  on`. 
>> Do you think the `getMessage` implementation should be tweaked not to print 
>> the "on" if the `attemptedType` is empty?
>
>> Hello Mandy,
>> With this change the `getMessage` may get passed an empty string, so it 
>> would end up printing something like `Can not get ... field  on`. 
>> Do you think the `getMessage` implementation should be tweaked not to print 
>> the "on" if the `attemptedType` is empty?
> 
> I suspect this was meant to be "null" rather than "".

With this patch, `newGetIllegalArgumentException` and 
`newSetIllegalArgumentException` are now only called when obj is non-null and 
the receiver type is of an invalid type.But it's better to improve this 
exception message to handle null obj.I pushed the change to clean up this 
code.

-

PR: https://git.openjdk.java.net/jdk/pull/6490

Re: jpackage - notarizing




> On Nov 22, 2021, at 12:39 PM, Alan Snyder  wrote:
> 
> 
>> On Nov 22, 2021, at 10:12 AM, Kevin Rushforth  
>> wrote:
>> 
>> JNF was removed from the JDK if that's what you are asking.
> 
> 
> 
> Indeed, that is why there is an issue.
> 
> The JDK may not be using JNF, but library developers still use it.
> 
> The JDK replacement for JNF is not supported for use outside the JDK.
> 
> JNF is not just convenient, there is at least one essential use for which 
> there is no adequate replacement.
> 
> See https://bugs.openjdk.java.net/browse/JDK-8274596
> 
> 
> 
I have a fair amount of code that would break if some JNF option wasn’t 
available. I have no estimate for how much rewriting I would need to do. Two of 
the projects I needed to recompile to get notarized needed it. I don’t actively 
do anything with it anymore but have some older code that did.

Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v3]

2021-11-22 Thread Paul Sandoz

On Fri, 19 Nov 2021 10:42:23 GMT, kabutz  wrote:

> > I think the functionality in this PR is worth pursuing, but with the JDK 18 
> > rampdown 1 date fast approaching, as a non-urgent issue, I think we 
> > shouldn't try to rush it into JDK 18.
> 
> > I looked more closely and now understand a bit more about the threshold. It 
> > would be useful to have some implementation notes detailing approximately 
> > when the parallel execution kicks in. For `a*a` it's `>= 
> > TOOM_COOK_SQUARE_THRESHOLD(216)` and for `a*b` it's `>= 
> > TOOM_COOK_THRESHOLD(240)`, so we could refer to certain bit lengths.
> > The branching factor is 4 (well 3 i guess, but its easier to think in 
> > powers of 2). It might be reasonable to assume the problem gets split 
> > equally in 4 parts. I don't know in practice what the depth of recursion 
> > might, its hard to see this getting completely out of control, but we could 
> > always keep track of the depth and cut off in proportion to the # runtime 
> > processors if need be.
> > Given the existing degree of specialization, the minimal changes to code, 
> > and the fact that the creation of recursive tasks is in the noise this PR 
> > looks quite reasonable.
> 
> I have run some more tests. For my fibonacci algorithm, here are the worst 
> cases for the various calculations.
> 
> ```
> n   most_bits tasks  time_ms
>  1000 694 0  1
>10_000   6_942 0  1
>   100_000  69_42418 13
> 1_000_000 694_241   468143
>10_000_000   6_942_41811_718   1049
>   100_000_000  69_424_191   292_968  13695
> 1_000_000_000 694_241_913 7_324_218 237282
> ```
> 
> Each data point has 10x the number of bits in the final result and the number 
> of tasks in the final calculation is 25x more. Perhaps I can make the 
> threshold the recursive depth up to which we would run in parallel. And that 
> recursive depth could the availableProcessors() or some multiple thereof.
> 

Defense in depth :-) Perhaps a depth that is between some multiple of log2 and 
log4 of `availableProcessors()`, given the branching factor.

-

PR: https://git.openjdk.java.net/jdk/pull/6409

Re: RFR: 8276141: XPathFactory set/getProperty method [v10]

On Thu, 18 Nov 2021 23:43:20 GMT, Joe Wang  wrote:

>> Add setProperty/getProperty methods to the XPath API so that properties can 
>> be supported in the future.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update as commented

Marked as reviewed by alanb (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6215

Re: jpackage - notarizing

> On Nov 22, 2021, at 10:12 AM, Kevin Rushforth  
> wrote:
> 
> JNF was removed from the JDK if that's what you are asking.

Indeed, that is why there is an issue.

The JDK may not be using JNF, but library developers still use it.

The JDK replacement for JNF is not supported for use outside the JDK.

JNF is not just convenient, there is at least one essential use for which there 
is no adequate replacement.

See https://bugs.openjdk.java.net/browse/JDK-8274596

Re: jpackage - notarizing

2021-11-22 Thread Kevin Rushforth


JNF was removed from the JDK if that's what you are asking.

-- Kevin


On 11/22/2021 10:11 AM, Alan Snyder wrote:

Well, if you look at the JNF part of the tree, the only include I see that 
comes from the JDK is jni.h.

Isn’t that what you were questioning?





On Nov 22, 2021, at 10:06 AM, Michael Hall  wrote:




On Nov 22, 2021, at 11:59 AM, Alan Snyder mailto:javali...@cbfiddle.com>> wrote:

I’m talking about the Xcode project for JNF.


I thought I was too. Unless it’s changed since I cloned or the one I’m looking at is 
somehow different. The URL I saved was  https://github.com/apple/openjdk/ 
 which looks to build JNF. Not that much a 
concern as of yet if the SDK still has it.

Thanks again.

Re: jpackage - notarizing

Well, if you look at the JNF part of the tree, the only include I see that 
comes from the JDK is jni.h.

Isn’t that what you were questioning?




> On Nov 22, 2021, at 10:06 AM, Michael Hall  wrote:
> 
> 
> 
>> On Nov 22, 2021, at 11:59 AM, Alan Snyder > > wrote:
>> 
>> I’m talking about the Xcode project for JNF.
>> 
> 
> I thought I was too. Unless it’s changed since I cloned or the one I’m 
> looking at is somehow different. The URL I saved was  
> https://github.com/apple/openjdk/  which 
> looks to build JNF. Not that much a concern as of yet if the SDK still has it.
> 
> Thanks again.
>

Integrated: 8277429: Conflicting jpackage static library name

2021-11-22 Thread Alexey Semenyuk

On Sat, 20 Nov 2021 03:26:51 GMT, Alexey Semenyuk  wrote:

> 8277429: Conflicting jpackage static library name

This pull request has now been integrated.

Changeset: e3911a85
Author:Alexey Semenyuk 
URL:   
https://git.openjdk.java.net/jdk/commit/e3911a8532e9b93ba5e65c613bd79864485db5ce
Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod

8277429: Conflicting jpackage static library name

Reviewed-by: almatvee, herrick, erikj

-

PR: https://git.openjdk.java.net/jdk/pull/6485

Re: jpackage - notarizing

> On Nov 22, 2021, at 11:59 AM, Alan Snyder  wrote:
> 
> I’m talking about the Xcode project for JNF.
> 

I thought I was too. Unless it’s changed since I cloned or the one I’m looking 
at is somehow different. The URL I saved was  https://github.com/apple/openjdk/ 
 which looks to build JNF. Not that much a 
concern as of yet if the SDK still has it.

Thanks again.

Re: jpackage - notarizing

I’m talking about the Xcode project for JNF.

> On Nov 22, 2021, at 9:54 AM, Michael Hall  wrote:
> 
> 
> 
>> On Nov 22, 2021, at 11:44 AM, Alan Snyder  wrote:
>> 
>> JNF is just an Objective-C library that uses jni.h. Otherwise, it is not 
>> dependent on the JDK.
>> 
>>> On Nov 22, 2021, at 9:23 AM, Michael Hall  wrote:
>>> 
>>> 
>>> 
 On Nov 22, 2021, at 10:40 AM, Alan Snyder  wrote:
 
 I’m still hoping someone will step up to provide a home for the compiled 
 JavaNativeFoundation framework.
 
 In any case, the sources are available and you can build it yourself.
 
 https://github.com/apple/openjdk.git 
>>> I had found this one as well.  It included an Xcode project and seemed to 
>>> be around jdk14? 
>>> I didn’t figure out what to do with it but it seemed like it might have a 
>>> limited shelf life too where it will work without a fair amount of effort.
>>> 
>>> Thanks.
>> 
> 
> I had cloned it and just saw this
> openjdk-xcodejdk14-release
> 
> The Xcode project doesn’t appear to include any java version dependencies.
> There is this…
> HEADER_SEARCH_PATHS = $(SRCROOT)/../../src/java.base/share/native/include 
> $(SRCROOT)/../../src/java.base/unix/native/include
> 
> But I’m not sure where that’s really pointing. I usually do command line.

Re: jpackage - notarizing




> On Nov 22, 2021, at 11:44 AM, Alan Snyder  wrote:
> 
> JNF is just an Objective-C library that uses jni.h. Otherwise, it is not 
> dependent on the JDK.
> 
>> On Nov 22, 2021, at 9:23 AM, Michael Hall  wrote:
>> 
>> 
>> 
>>> On Nov 22, 2021, at 10:40 AM, Alan Snyder  wrote:
>>> 
>>> I’m still hoping someone will step up to provide a home for the compiled 
>>> JavaNativeFoundation framework.
>>> 
>>> In any case, the sources are available and you can build it yourself.
>>> 
>>> https://github.com/apple/openjdk.git 
>> I had found this one as well.  It included an Xcode project and seemed to be 
>> around jdk14? 
>> I didn’t figure out what to do with it but it seemed like it might have a 
>> limited shelf life too where it will work without a fair amount of effort.
>> 
>> Thanks.
> 

I had cloned it and just saw this
openjdk-xcodejdk14-release

The Xcode project doesn’t appear to include any java version dependencies.
There is this…
HEADER_SEARCH_PATHS = $(SRCROOT)/../../src/java.base/share/native/include 
$(SRCROOT)/../../src/java.base/unix/native/include

But I’m not sure where that’s really pointing. I usually do command line.

Re: jpackage - notarizing

JNF is just an Objective-C library that uses jni.h. Otherwise, it is not 
dependent on the JDK.

> On Nov 22, 2021, at 9:23 AM, Michael Hall  wrote:
> 
> 
> 
>> On Nov 22, 2021, at 10:40 AM, Alan Snyder  wrote:
>> 
>> I’m still hoping someone will step up to provide a home for the compiled 
>> JavaNativeFoundation framework.
>> 
>> In any case, the sources are available and you can build it yourself.
>> 
>> https://github.com/apple/openjdk.git 
> I had found this one as well.  It included an Xcode project and seemed to be 
> around jdk14? 
> I didn’t figure out what to do with it but it seemed like it might have a 
> limited shelf life too where it will work without a fair amount of effort.
> 
> Thanks.

Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v2]

2021-11-22 Thread Iris Clark

On Mon, 22 Nov 2021 04:30:38 GMT, Joe Darcy  wrote:

>> The time to get JDK 19 underway draws nigh, please review this usual set of 
>> start-of-release updates, including CSRs for the javac and javax.lang.model 
>> updates:
>> 
>> JDK-8277512: Add SourceVersion.RELEASE_19
>> https://bugs.openjdk.java.net/browse/JDK-8277512
>> 
>> JDK-8277514: Add source 19 and target 19 to javac
>> https://bugs.openjdk.java.net/browse/JDK-8277514
>> 
>> Clean local tier 1 test runs for langtools, jdk, and hotspot.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by iris (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6493

Re: jpackage - notarizing




> On Nov 22, 2021, at 10:40 AM, Alan Snyder  wrote:
> 
> I’m still hoping someone will step up to provide a home for the compiled 
> JavaNativeFoundation framework.
> 
> In any case, the sources are available and you can build it yourself.
> 
> https://github.com/apple/openjdk.git 
I had found this one as well.  It included an Xcode project and seemed to be 
around jdk14? 
I didn’t figure out what to do with it but it seemed like it might have a 
limited shelf life too where it will work without a fair amount of effort.

Thanks.

Re: RFR: 8277429: Conflicting jpackage static library name

2021-11-22 Thread Erik Joelsson

On Sat, 20 Nov 2021 03:26:51 GMT, Alexey Semenyuk  wrote:

> 8277429: Conflicting jpackage static library name

The problem arises when running the "static-libs" build, where every library 
and executable is just linked into a static-lib for downstream consumers 
(Graal).

-

PR: https://git.openjdk.java.net/jdk/pull/6485

Re: jpackage - notarizing

I’m still hoping someone will step up to provide a home for the compiled 
JavaNativeFoundation framework.

In any case, the sources are available and you can build it yourself.

https://github.com/apple/openjdk.git

> On Nov 22, 2021, at 7:15 AM, Michael Hall  wrote:
> 
> In order to use JavaNativeFoundation. Is there nothing that can be done about 
> JavaNativeFoundation code breaking when Apple decides not to include it in 
> the Xcode SDK’s?

It will probably be there for a while, as Xcode contains a Java program.

Integrated: JDK-8274685 - Documentation suggests there are ArbitrarilyJumpableGenerator when none

On Thu, 11 Nov 2021 13:34:21 GMT, Jim Laskey  wrote:

> "There is also an interface RandomGenerator.ArbitrarilyJumpableGenerator for 
> algorithms that allow jumping along the state cycle by any user-specified 
> distance. In this package, implementations of these interfaces include 
> "Xoroshiro128PlusPlus", and "Xoshiro256PlusPlus" is incorrect. 
> "Xoroshiro128PlusPlus", and "Xoshiro256PlusPlus" are actually 
> RandomGenerator.JumpableGenerator.

This pull request has now been integrated.

Changeset: 8683de5e
Author:Jim Laskey 
URL:   
https://git.openjdk.java.net/jdk/commit/8683de5eda2d1a04f187073f969140245908f324
Stats: 18 lines in 1 file changed: 7 ins; 2 del; 9 mod

8274685: Documentation suggests there are ArbitrarilyJumpableGenerator when none

Co-authored-by: Guy Steele 
Reviewed-by: rriggs

-

PR: https://git.openjdk.java.net/jdk/pull/6349

Integrated: JDK-8273792 - JumpableGenerator.rngs() documentation refers to wrong method

On Thu, 11 Nov 2021 13:54:37 GMT, Jim Laskey  wrote:

> Documentation in `RandomGenerator.rngs()` refers to 
> `JumpableGenerator.jump()` when it should be `JumpableGenerator.jumps()`

This pull request has now been integrated.

Changeset: 6b4fbaed
Author:Jim Laskey 
URL:   
https://git.openjdk.java.net/jdk/commit/6b4fbaedbb782c5f028735ac1d92838895589192
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8273792: JumpableGenerator.rngs() documentation refers to wrong method

Co-authored-by: Guy Steele 
Reviewed-by: rriggs

-

PR: https://git.openjdk.java.net/jdk/pull/6352

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Mon, 22 Nov 2021 16:03:16 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 314:
>> 
>>> 312: ).getTarget();
>>> 313: mhs[splitIndex] = 
>>> MethodHandles.filterArguments(mhs[splitIndex], 0, currentSplitGetters);
>>> 314: mhs[splitIndex] = MethodHandles.permuteArguments(
>> 
>> This is some gnarly logic. Could we add some comments on what 
>> permuteArguments with a reorder array of just zeros is doing here?
>
> This is not unusual. It spreads a single argument across several "getters". 
> But a comment wouldn't hurt.

I haven't seen it used like this before. The duplication behavior is mentioned 
explicitly with an example in the javadoc, sure, but it still took me a deep 
reading of the docs to understand what was going on here and what the intent 
was.

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Withdrawn: 8274544: Langtools command's usage were garbled on Japanese Windows

2021-11-22 Thread Ichiroh Takiguchi

On Thu, 30 Sep 2021 09:36:34 GMT, Ichiroh Takiguchi  
wrote:

> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13.
> After JDK18-b13, javac and some other langtool command's usage were garbled 
> on Japanese Windows.
> These commands use PrintWriter instead of standard out/err with PrintStream.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]

2021-11-22 Thread Ichiroh Takiguchi

On Fri, 19 Nov 2021 16:48:03 GMT, Naoto Sato  wrote:

>> Hello @naotoj .
>> For PrintStream.getCharset(), following changes may be required.
>> 
>> +++ src/java.base/share/classes/java/io/OutputStreamWriter.java
>> +Charset getCharset() {
>> +return se.getCharset();
>> +}
>> 
>> +++ src/java.base/share/classes/java/io/PrintStream.java
>> +public Charset getCharset() {
>> +return charOut.getCharset();
>> +}
>> 
>> +++ src/java.base/share/classes/sun/nio/cs/StreamEncoder.java
>> +public Charset getCharset() {
>> +return cs;
>> +}
>> 
>> For javac code, we may not use PrintStream.getCharset() directly because 
>> javac code is compiled by boot compiler.
>> We need to use reflection, like:
>> 
>> +++ src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
>> +private static Charset getCharset(PrintStream ps) {
>> +try {
>> +Method getCharset = 
>> PrintStream.class.getDeclaredMethod("getCharset");
>> +return (Charset)getCharset.invoke(ps);
>> +} catch (Exception e) {
>> +return Charset.defaultCharset();
>> +}
>> +}
>> 
>> If we add following constructors against PrintWriter, we just change javap 
>> and jshell code.
>> But I cannot evaluate this code changes.
>> 
>> +++ src/java.base/share/classes/java/io/PrintWriter.java
>> +public PrintWriter(PrintStream ps) {
>> +   this((OutputStream)ps, false, ps.getCharset());
>> +}
>> +public PrintWriter(PrintStream ps, boolean autoFlush) {
>> +   this((OutputStream)ps, autoFlush, ps.getCharset());
>> +}
>> 
>> I really appreciate if you handle this kind of code change via JEP-400.
>
> I think this PR can now safely be withdrawn, as 
> https://github.com/openjdk/jdk/pull/6401 is now integrated. @takiguc, if you 
> do not mind, I will create a PR for the remaining jshell issue. Please let me 
> know.

Thanks @naotoj .
I opened new pr via 8274784.
I'd like to close this pr since main issue was fixed by #6401.

-

PR: https://git.openjdk.java.net/jdk/pull/5771

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Sun, 21 Nov 2021 00:29:46 GMT, Vicente Romero  wrote:

>> Please review this PR which aims to optimize the implementation of the 
>> `toString` method we provide for records. A benchmark comparing the 
>> implementation we are providing for records with lombok found out that 
>> lombok is much faster mainly because our implementation uses 
>> `String::format`. This fix is basically delegating on 
>> StringConcatFactory::makeConcatWithConstants which is faster.
>> 
>> TIA
>> 
>> This is the result of the benchmark comparing records to lombok with vanilla 
>> JDK:
>> 
>> Benchmark  Mode  CntScoreError  Units
>> MyBenchmark.base   avgt40.849 ±  0.111  ns/op
>> MyBenchmark.equals_record  avgt47.343 ±  2.740  ns/op
>> MyBenchmark.equals_value   avgt46.644 ±  1.920  ns/op
>> MyBenchmark.record_hash_code   avgt45.763 ±  3.882  ns/op
>> MyBenchmark.record_to_string   avgt4  262.626 ± 12.574  ns/op
>><-- Before
>> MyBenchmark.value_class_to_string  avgt4   30.325 ± 21.389  ns/op
>> MyBenchmark.value_hash_codeavgt45.048 ±  3.936  ns/op
>> 
>> 
>> after this patch:
>> 
>> Benchmark  Mode  Cnt   Score   Error  Units
>> MyBenchmark.base   avgt4   0.680 ± 0.185  ns/op
>> MyBenchmark.equals_record  avgt4   5.599 ± 1.348  ns/op
>> MyBenchmark.equals_value   avgt4   5.718 ± 4.633  ns/op
>> MyBenchmark.record_hash_code   avgt4   4.628 ± 4.368  ns/op
>> MyBenchmark.record_to_string   avgt4  26.791 ± 1.817  ns/op  
>><--- After
>> MyBenchmark.value_class_to_string  avgt4  35.473 ± 2.626  ns/op
>> MyBenchmark.value_hash_codeavgt4   6.152 ± 5.101  ns/op
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   setting max split size to 20

hi guys, can I have a thumbs up for this one? I will continue doing additional 
research for the `hashCode` and `equals` methods as a follow up but would like 
to close this one as the performance for the `Record::toString` was the worst 
compared to lombok.

TIA

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Mon, 22 Nov 2021 15:56:46 GMT, Claes Redestad  wrote:

>> Vicente Romero has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   setting max split size to 20
>
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 314:
> 
>> 312: ).getTarget();
>> 313: mhs[splitIndex] = 
>> MethodHandles.filterArguments(mhs[splitIndex], 0, currentSplitGetters);
>> 314: mhs[splitIndex] = MethodHandles.permuteArguments(
> 
> This is some gnarly logic. Could we add some comments on what 
> permuteArguments with a reorder array of just zeros is doing here?

This is not unusual. It spreads a single argument across several "getters". But 
a comment wouldn't hurt.

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Sun, 21 Nov 2021 00:29:46 GMT, Vicente Romero  wrote:

>> Please review this PR which aims to optimize the implementation of the 
>> `toString` method we provide for records. A benchmark comparing the 
>> implementation we are providing for records with lombok found out that 
>> lombok is much faster mainly because our implementation uses 
>> `String::format`. This fix is basically delegating on 
>> StringConcatFactory::makeConcatWithConstants which is faster.
>> 
>> TIA
>> 
>> This is the result of the benchmark comparing records to lombok with vanilla 
>> JDK:
>> 
>> Benchmark  Mode  CntScoreError  Units
>> MyBenchmark.base   avgt40.849 ±  0.111  ns/op
>> MyBenchmark.equals_record  avgt47.343 ±  2.740  ns/op
>> MyBenchmark.equals_value   avgt46.644 ±  1.920  ns/op
>> MyBenchmark.record_hash_code   avgt45.763 ±  3.882  ns/op
>> MyBenchmark.record_to_string   avgt4  262.626 ± 12.574  ns/op
>><-- Before
>> MyBenchmark.value_class_to_string  avgt4   30.325 ± 21.389  ns/op
>> MyBenchmark.value_hash_codeavgt45.048 ±  3.936  ns/op
>> 
>> 
>> after this patch:
>> 
>> Benchmark  Mode  Cnt   Score   Error  Units
>> MyBenchmark.base   avgt4   0.680 ± 0.185  ns/op
>> MyBenchmark.equals_record  avgt4   5.599 ± 1.348  ns/op
>> MyBenchmark.equals_value   avgt4   5.718 ± 4.633  ns/op
>> MyBenchmark.record_hash_code   avgt4   4.628 ± 4.368  ns/op
>> MyBenchmark.record_to_string   avgt4  26.791 ± 1.817  ns/op  
>><--- After
>> MyBenchmark.value_class_to_string  avgt4  35.473 ± 2.626  ns/op
>> MyBenchmark.value_hash_codeavgt4   6.152 ± 5.101  ns/op
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   setting max split size to 20

Marked as reviewed by redestad (Reviewer).

src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 276:

> 274: int namesIndex = 0;
> 275: do {
> 276: /* StringConcatFactory::makeConcatWithConstants can only 
> deal with 200 spots, longs and double occupy two

typos: spot -> slot, chunck -> chunk, diference -> difference

src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 314:

> 312: ).getTarget();
> 313: mhs[splitIndex] = 
> MethodHandles.filterArguments(mhs[splitIndex], 0, currentSplitGetters);
> 314: mhs[splitIndex] = MethodHandles.permuteArguments(

This is some gnarly logic. Could we add some comments on what permuteArguments 
with a reorder array of just zeros is doing here?

src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 330:

> 328: 
> 329: /**
> 330:  * Chops the getters into smaller chunks according to the maximum 
> number of spots

typo: spots -> slots

src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 333:

> 331:  * StringConcatFactory::makeConcatWithConstants can chew
> 332:  * @param getters the current getters
> 333:  * @return chunks that wont surpass the maximum number of spots 
> StringConcatFactory::makeConcatWithConstants can chew

ditto

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Sun, 21 Nov 2021 00:29:46 GMT, Vicente Romero  wrote:

>> Please review this PR which aims to optimize the implementation of the 
>> `toString` method we provide for records. A benchmark comparing the 
>> implementation we are providing for records with lombok found out that 
>> lombok is much faster mainly because our implementation uses 
>> `String::format`. This fix is basically delegating on 
>> StringConcatFactory::makeConcatWithConstants which is faster.
>> 
>> TIA
>> 
>> This is the result of the benchmark comparing records to lombok with vanilla 
>> JDK:
>> 
>> Benchmark  Mode  CntScoreError  Units
>> MyBenchmark.base   avgt40.849 ±  0.111  ns/op
>> MyBenchmark.equals_record  avgt47.343 ±  2.740  ns/op
>> MyBenchmark.equals_value   avgt46.644 ±  1.920  ns/op
>> MyBenchmark.record_hash_code   avgt45.763 ±  3.882  ns/op
>> MyBenchmark.record_to_string   avgt4  262.626 ± 12.574  ns/op
>><-- Before
>> MyBenchmark.value_class_to_string  avgt4   30.325 ± 21.389  ns/op
>> MyBenchmark.value_hash_codeavgt45.048 ±  3.936  ns/op
>> 
>> 
>> after this patch:
>> 
>> Benchmark  Mode  Cnt   Score   Error  Units
>> MyBenchmark.base   avgt4   0.680 ± 0.185  ns/op
>> MyBenchmark.equals_record  avgt4   5.599 ± 1.348  ns/op
>> MyBenchmark.equals_value   avgt4   5.718 ± 4.633  ns/op
>> MyBenchmark.record_hash_code   avgt4   4.628 ± 4.368  ns/op
>> MyBenchmark.record_to_string   avgt4  26.791 ± 1.817  ns/op  
>><--- After
>> MyBenchmark.value_class_to_string  avgt4  35.473 ± 2.626  ns/op
>> MyBenchmark.value_hash_codeavgt4   6.152 ± 5.101  ns/op
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   setting max split size to 20

FWIW I did a few experiments trying to move the chunking to `SCF`. Most made 
things worse, but this is getting close: 
https://github.com/openjdk/jdk/compare/master...cl4es:scf_split?expand=1

The threshold for when the JIT fails to optimize seem to be pushed up a bit and 
I get a 4x gains when concatenating 100 Strings (though it takes a good while 
for the microbenchmark to stabilize). It also fails to make much of a 
difference when looking at 200 arguments (maybe 1.4x win). The experiment I 
have done so far are crude and aggregates the results into a String builder 
with no size estimation, so it adds a but of unnecessary allocation that could 
be improved. I think this needs way more work to be a good enhancement and that 
splitting up outside is going to remain better for now. 

A "hidden" drawback with chunking is that you're likely going to be allocating 
more.

I also re-realized that it'll be pretty hard (or impossible) to get around 
having some MH slot limit: even though we chunk it up internally, the MH we 
return must have a type that matches the type given to the bootstrap method, 
and any operations on the tree that require some temporary arguments we need to 
pass around will require some argument slots. The current strategy doesn't use 
too many temporaries, so the real limit might be around 250, but it makes sense 
to give some room for alternative implementations that use more temporaries if 
that makes things more efficient.

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]

On Sun, 21 Nov 2021 00:29:46 GMT, Vicente Romero  wrote:

>> Please review this PR which aims to optimize the implementation of the 
>> `toString` method we provide for records. A benchmark comparing the 
>> implementation we are providing for records with lombok found out that 
>> lombok is much faster mainly because our implementation uses 
>> `String::format`. This fix is basically delegating on 
>> StringConcatFactory::makeConcatWithConstants which is faster.
>> 
>> TIA
>> 
>> This is the result of the benchmark comparing records to lombok with vanilla 
>> JDK:
>> 
>> Benchmark  Mode  CntScoreError  Units
>> MyBenchmark.base   avgt40.849 ±  0.111  ns/op
>> MyBenchmark.equals_record  avgt47.343 ±  2.740  ns/op
>> MyBenchmark.equals_value   avgt46.644 ±  1.920  ns/op
>> MyBenchmark.record_hash_code   avgt45.763 ±  3.882  ns/op
>> MyBenchmark.record_to_string   avgt4  262.626 ± 12.574  ns/op
>><-- Before
>> MyBenchmark.value_class_to_string  avgt4   30.325 ± 21.389  ns/op
>> MyBenchmark.value_hash_codeavgt45.048 ±  3.936  ns/op
>> 
>> 
>> after this patch:
>> 
>> Benchmark  Mode  Cnt   Score   Error  Units
>> MyBenchmark.base   avgt4   0.680 ± 0.185  ns/op
>> MyBenchmark.equals_record  avgt4   5.599 ± 1.348  ns/op
>> MyBenchmark.equals_value   avgt4   5.718 ± 4.633  ns/op
>> MyBenchmark.record_hash_code   avgt4   4.628 ± 4.368  ns/op
>> MyBenchmark.record_to_string   avgt4  26.791 ± 1.817  ns/op  
>><--- After
>> MyBenchmark.value_class_to_string  avgt4  35.473 ± 2.626  ns/op
>> MyBenchmark.value_hash_codeavgt4   6.152 ± 5.101  ns/op
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   setting max split size to 20

Marked as reviewed by jlaskey (Reviewer).

src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 276:

> 274: if (i != names.size() - 1) {
> 275: constants[1 + 2 * i + 1] = ", ";
> 276: }

Wondering if it would be cleaner to use a full recipe.


StringBuilder recipeSB = new StringBuilder();
recipeSB.append(receiverClass.getSimpleName());
recipeSB.append('[');

for (int i = 0; i < names.size(); i++) {
if (i != 0) {
recipeSB.append(", ");
}

recipeSB.append(names.get(i));
recipeSB.append('=');
recipeSB.append('\1');
}

recipeSB.append(']');
String recipe = recipeSB.toString();

-

PR: https://git.openjdk.java.net/jdk/pull/6403

Re: RFR: 8277429: Conflicting jpackage static library name

2021-11-22 Thread Magnus Ihse Bursie

On Sat, 20 Nov 2021 03:26:51 GMT, Alexey Semenyuk  wrote:

> 8277429: Conflicting jpackage static library name

How come BUILD_JPACKAGE_APPLAUNCHEREXE results in a library? It sounds like an 
executable...

That is, I realize this patch fixes the problem as described, I just don't 
understand why it happens, so maybe we're solving the symptoms and not the 
problem.

-

PR: https://git.openjdk.java.net/jdk/pull/6485

While I was looking at —mac-dmg-content I thought I would also look at
notarization. I had downloaded my app on an older machine while my new was
being serviced and got uninviting messages about Apple being unable to
determine it was full of malware. Notarization I found out was supposed to be
the way to eliminate these messages.

I did manage notarization eventually using Mac/Xcode command line tools.

Am I duplicating anything jpackage would do for me somehow? Some option I’m not
aware of?

I did have to recompile some code because binaries were before the 10.9 SDK.

This had the issue where I have to include
SDK_ROOT="$(xcrun --show-sdk-path)”
gcc …
-I${SDK_ROOT}/System/Library/Frameworks/JavaVM.framework/Versions/A/Frameworks/JavaNativeFoundation.framework/Versions/A/Headers...-F${SDK_ROOT}/System/Library/Frameworks/JavaVM.framework/Frameworks

In order to use JavaNativeFoundation. Is there nothing that can be done about
JavaNativeFoundation code breaking when Apple decides not to include it in the
Xcode SDK’s?

Also, again I think an Apple issue, but if someone else runs into it. I
currently seemed to be in some kind of command line tool (CLT) limbo in
compiling. I was getting AppKit errors. Google showed others having this
problem where using Xcode itself instead of the CLT was the solution. I have
not used Xcode for JNI, it didn’t seem well suited. This appeared to be some
problem where my OS/X 10.15 Catalina version is getting a little dated,
including with my Xcode 12.4 version.

I was unable to figure out how to CLT compile on my current machine but was
able to on my older Mac.

Again I assume this is an Apple thing.

Notarization was finally successful though.

Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v6]

2021-11-22 Thread Daniel Fuchs

On Fri, 19 Nov 2021 22:04:47 GMT, Andrew Leonard  wrote:

>> Both jar and jmod utilise java.io file operations whose methods define no 
>> ordering of the return file lists, and in fact rely on OS query file 
>> ordering, which can differ by underlying OS architecture.
>> This PR adds sort processing to the creation of such jar's and jmod's to 
>> enable a deterministic content ordering.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Andrew Leonard has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8276764: Enable deterministic file content ordering for Jar and Jmod
>   
>   Signed-off-by: Andrew Leonard 

src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java line 772:

> 770: // Keep a sorted set of files to be processed, so that the 
> jmod
> 771: // content is reproducible as Files.walkFileTree order is 
> not defined
> 772: SortedMap filesToProcess  = new 
> TreeMap();

Nit: there seems to be two whitespaces (instead of one) before the `=` sign on 
this line.

-

PR: https://git.openjdk.java.net/jdk/pull/6395

Re: jpackage --mac-dmg-content

Nit, if you open the dmg the additional content doesn’t quite fit in the bottom 
of the window.

My thought had been smaller icons for the additional. This doesn’t seem 
possible?

Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v7]

> The commit here is a potential fix for the issue noted in 
> https://bugs.openjdk.java.net/browse/JDK-8258117.
> 
> The change here repurposes an existing internal interface `ModuleInfoEntry` 
> to keep track of the last modified timestamp of a `module-info.class` 
> descriptor.
> 
> This commit uses the timestamp of the `module-info.class` on the filesystem 
> to set the time on the `JarEntry`. There are a couple of cases to consider 
> here:
> 
> 1. When creating a jar  (using `--create`), we use the source 
> `module-info.class` from the filesystem and then add extended info to it 
> (attributes like packages, module version etc...). In such cases, this patch 
> will use the lastmodified timestamp from the filesystem of 
> `module-info.class` even though we might end up updating/extending/modifying 
> (for example by adding a module version) its content while storing it as a 
> `JarEntry`. 
> 
> 2. When updating a jar (using `--update`), this patch will use the 
> lastmodified timestamp of `module-info.class` either from the filesystem or 
> from the source jar's entry (depending on whether a new `module-info.class` 
> is being passed to the command). Here too, it's possible that we might end up 
> changing/modifying/extending the `module-info.class` (for example, changing 
> the module version to a new version) that gets written into the updated jar 
> file, but this patch _won't_ use `System.currentTimeMillis()` even in such 
> cases.
> 
> If we do have to track actual changes that might happen to 
> `module-info.class` while extending its info (in `extendedInfoBytes()`) and 
> then decide whether to use current system time as last modified time, then 
> this will require a bigger change and also a discussion on what kind of 
> extending of module-info.class content will require a change to the 
> lastmodifiedtime of that entry.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  use proper FileTime centric APIs

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5486/files
  - new: https://git.openjdk.java.net/jdk/pull/5486/files/8ff75835..9ab5ad8a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=05-06

  Stats: 12 lines in 1 file changed: 1 ins; 2 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5486.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486

PR: https://git.openjdk.java.net/jdk/pull/5486

jpackage --mac-dmg-content

[08:26:41.669] jdk.jpackage.internal.PackagerException: Error: Option 
[--mac-dmg-content] is not valid with type [default]

dmg is the default type? Why should this be an error?

Re: RFR: 8277429: Conflicting jpackage static library name

2021-11-22 Thread Erik Joelsson

On Sat, 20 Nov 2021 03:26:51 GMT, Alexey Semenyuk  wrote:

> 8277429: Conflicting jpackage static library name

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6485

Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v2]

2021-11-22 Thread Erik Joelsson

On Mon, 22 Nov 2021 04:30:38 GMT, Joe Darcy  wrote:

>> The time to get JDK 19 underway draws nigh, please review this usual set of 
>> start-of-release updates, including CSRs for the javac and javax.lang.model 
>> updates:
>> 
>> JDK-8277512: Add SourceVersion.RELEASE_19
>> https://bugs.openjdk.java.net/browse/JDK-8277512
>> 
>> JDK-8277514: Add source 19 and target 19 to javac
>> https://bugs.openjdk.java.net/browse/JDK-8277514
>> 
>> Clean local tier 1 test runs for langtools, jdk, and hotspot.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6493

Re: RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod [v6]

2021-11-22 Thread Lance Andersen

On Fri, 19 Nov 2021 22:04:47 GMT, Andrew Leonard  wrote:

>> Both jar and jmod utilise java.io file operations whose methods define no 
>> ordering of the return file lists, and in fact rely on OS query file 
>> ordering, which can differ by underlying OS architecture.
>> This PR adds sort processing to the creation of such jar's and jmod's to 
>> enable a deterministic content ordering.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Andrew Leonard has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8276764: Enable deterministic file content ordering for Jar and Jmod
>   
>   Signed-off-by: Andrew Leonard 

Changes look fine.

I can run the changes through mach5 tiers 1  - 3 for you.  Let me know

-

PR: https://git.openjdk.java.net/jdk/pull/6395

Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v26]

2021-11-22 Thread Maurizio Cimadamore

> This PR contains the API and implementation changes for JEP-419 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/419

Maurizio Cimadamore has updated the pull request with a new target base due to 
a merge or a rebase. The pull request now contains 35 commits:

 - Merge branch 'master' into JEP-419
 - Fix javadoc issues found in CSR review
 - Adopt blessed modofier order
 - Merge branch 'master' into JEP-419
 - Revert removal of upcall MH customization
   (This change caused spurious VM crashes, so reverting to baseline)
 - Further tweak upcall safety considerations
 - Clarify safety considerations for upcalls
 - Rename MemorySegment::ofAddressNative to MemorySegment::ofAddress
   (which is consistent with other restricted factories in VaList and 
NativeSymbol)
 - Streamline javadoc for package-info
 - * Add two new CLinker static methods to compute upcall/downcall method types
   * Clarify section on CLinker downcall type
   * Add section on CLinker safety guarantees
 - ... and 25 more: https://git.openjdk.java.net/jdk/compare/d427c79d...29cc6c60

-

Changes: https://git.openjdk.java.net/jdk/pull/5907/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5907=25
  Stats: 14700 lines in 193 files changed: 6958 ins; 5126 del; 2616 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5907.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907

PR: https://git.openjdk.java.net/jdk/pull/5907

Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v25]

2021-11-22 Thread Maurizio Cimadamore

> This PR contains the API and implementation changes for JEP-419 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/419

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix javadoc issues found in CSR review

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5907/files
  - new: https://git.openjdk.java.net/jdk/pull/5907/files/79d3d685..1817975f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=24
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=23-24

  Stats: 10 lines in 4 files changed: 2 ins; 6 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5907.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907

PR: https://git.openjdk.java.net/jdk/pull/5907

Re: RFR: 8277535: Remove redundant Stream.distinct()/sorted() steps [v2]

2021-11-22 Thread Andrey Turbanov

> 1. Stream.distinct() is redundant before toSet() collector. Duplicates will 
> be collapsed by Collector.
> 2. Stream.sorted() is redundant before toMap() collector. Keys will be 
> shuffled by Collector (it's a HashMap in current implementation)

Andrey Turbanov has updated the pull request incrementally with one additional 
commit since the last revision:

  8277535: Remove redundant Stream.distinct()/sorted() steps
  update copyright year

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5714/files
  - new: https://git.openjdk.java.net/jdk/pull/5714/files/b326898d..e0312d2d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5714=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5714=00-01

  Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5714.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5714/head:pull/5714

PR: https://git.openjdk.java.net/jdk/pull/5714

Re: RFR: 8277535: Remove redundant Stream.distinct()/sorted() steps

2021-11-22 Thread Pavel Rappo

On Mon, 27 Sep 2021 11:20:53 GMT, Andrey Turbanov  wrote:

> 1. Stream.distinct() is redundant before toSet() collector. Duplicates will 
> be collapsed by Collector.
> 2. Stream.sorted() is redundant before toMap() collector. Keys will be 
> shuffled by Collector (it's a HashMap in current implementation)

Looks good. Please update the copyright years before integrating.

Note that in general, removing distinct() immediately before Collectors.toSet() 
might change the behavior. If operated on an ordered stream, distinct() is 
guaranteed to yield a stable order.

-

Marked as reviewed by prappo (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5714

RFR: 8277535: Remove redundant Stream.distinct()/sorted() steps

2021-11-22 Thread Andrey Turbanov

1. Stream.distinct() is redundant before toSet() collector. Duplicates will be 
collapsed by Collector.
2. Stream.sorted() is redundant before toMap() collector. Keys will be shuffled 
by Collector (it's a HashMap in current implementation)

-

Commit messages:
 - [PATCH] Remove redundant Stream.distinct()/sorted() steps

Changes: https://git.openjdk.java.net/jdk/pull/5714/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5714=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277535
  Stats: 4 lines in 3 files changed: 0 ins; 4 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5714.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5714/head:pull/5714

PR: https://git.openjdk.java.net/jdk/pull/5714

Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]

On Mon, 22 Nov 2021 09:09:37 GMT, Jaikiran Pai  wrote:

>> test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java
>>  line 60:
>> 
>>> 58: private static final String UPDATED_MODULE_VERSION = "1.2.4";
>>> 59: private static final String MAIN_CLASS = "jdk.test.foo.Foo";
>>> 60: private static final Path MODULE_CLASSES_DIR = 
>>> Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();
>> 
>> You can use Path.of here.
>
> Done. Replaced this and one other place in this test to use `Path.of`.

Test continues to pass with all these new changes.

-

PR: https://git.openjdk.java.net/jdk/pull/5486

Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]

On Mon, 22 Nov 2021 08:25:38 GMT, Alan Bateman  wrote:

>> Jaikiran Pai 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:
>> 
>>  - Merge latest from master branch
>>  - use testng asserts
>>  - Remove "final" usage from test
>>  - convert test to testng
>>  - Merge latest from master branch
>>  - Merge latest from master branch
>>  - 8258117: jar tool sets the time stamp of module-info.class entries to the 
>> current time
>
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 806:
> 
>> 804: Long lastModified = f.lastModified() == 0 ? null : 
>> f.lastModified();
>> 805: moduleInfos.putIfAbsent(name,
>> 806: new StreamedModuleInfoEntry(name, 
>> Files.readAllBytes(f.toPath()), lastModified));
> 
> I'd prefer to see this split into two two statements as there is just too 
> much going on one the one line.

Done. I have updated the PR to split this line into more than 1 statement.

> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1785:
> 
>> 1783: private final String name;
>> 1784: private final byte[] bytes;
>> 1785: private final Long lastModifiedTime;
> 
> It might be better to use a FileTime here, otherwise we risk a NPE when 
> unboxing.

Sounds good. I've updated the PR to replace this to use `FileTime`.

> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2108:
> 
>> 2106: byte[] extended = extendedInfoBytes(md, bytes, packages);
>> 2107: // replace the entry value with the extended bytes
>> 2108: e.setValue(new 
>> StreamedModuleInfoEntry(e.getValue().name(), extended, 
>> e.getValue().getLastModifiedTime()));
> 
> There are 3 calls to getValue and each one I need to remember that e.getValue 
> is a ModuleInfoEntry. If you add ModuleInfo entry = e.getValue() then it 
> might help the readability.

That makes sense. The updated PR now has this change.

> test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java 
> line 60:
> 
>> 58: private static final String UPDATED_MODULE_VERSION = "1.2.4";
>> 59: private static final String MAIN_CLASS = "jdk.test.foo.Foo";
>> 60: private static final Path MODULE_CLASSES_DIR = 
>> Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();
> 
> You can use Path.of here.

Done. Replaced this and one other place in this test to use `Path.of`.

-

PR: https://git.openjdk.java.net/jdk/pull/5486

Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v6]

> The commit here is a potential fix for the issue noted in 
> https://bugs.openjdk.java.net/browse/JDK-8258117.
> 
> The change here repurposes an existing internal interface `ModuleInfoEntry` 
> to keep track of the last modified timestamp of a `module-info.class` 
> descriptor.
> 
> This commit uses the timestamp of the `module-info.class` on the filesystem 
> to set the time on the `JarEntry`. There are a couple of cases to consider 
> here:
> 
> 1. When creating a jar  (using `--create`), we use the source 
> `module-info.class` from the filesystem and then add extended info to it 
> (attributes like packages, module version etc...). In such cases, this patch 
> will use the lastmodified timestamp from the filesystem of 
> `module-info.class` even though we might end up updating/extending/modifying 
> (for example by adding a module version) its content while storing it as a 
> `JarEntry`. 
> 
> 2. When updating a jar (using `--update`), this patch will use the 
> lastmodified timestamp of `module-info.class` either from the filesystem or 
> from the source jar's entry (depending on whether a new `module-info.class` 
> is being passed to the command). Here too, it's possible that we might end up 
> changing/modifying/extending the `module-info.class` (for example, changing 
> the module version to a new version) that gets written into the updated jar 
> file, but this patch _won't_ use `System.currentTimeMillis()` even in such 
> cases.
> 
> If we do have to track actual changes that might happen to 
> `module-info.class` while extending its info (in `extendedInfoBytes()`) and 
> then decide whether to use current system time as last modified time, then 
> this will require a bigger change and also a discussion on what kind of 
> extending of module-info.class content will require a change to the 
> lastmodifiedtime of that entry.

Jaikiran Pai has updated the pull request incrementally with four additional 
commits since the last revision:

 - review suggestion - use FileTime instead of Long to prevent any potential 
NPEs during unboxing
 - review suggestion - split into multiple statements to make it easily readable
 - review suggestion - Use Path.of instead of Paths.get in testcase
 - review suggestion - store e.getValue() and reuse the stored value

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5486/files
  - new: https://git.openjdk.java.net/jdk/pull/5486/files/945fde6f..8ff75835

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=04-05

  Stats: 21 lines in 2 files changed: 3 ins; 1 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5486.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486

PR: https://git.openjdk.java.net/jdk/pull/5486

Re: RFR: JDK-8273146: Start of release updates for JDK 19 [v2]

On Mon, 22 Nov 2021 04:30:38 GMT, Joe Darcy  wrote:

>> The time to get JDK 19 underway draws nigh, please review this usual set of 
>> start-of-release updates, including CSRs for the javac and javax.lang.model 
>> updates:
>> 
>> JDK-8277512: Add SourceVersion.RELEASE_19
>> https://bugs.openjdk.java.net/browse/JDK-8277512
>> 
>> JDK-8277514: Add source 19 and target 19 to javac
>> https://bugs.openjdk.java.net/browse/JDK-8277514
>> 
>> Clean local tier 1 test runs for langtools, jdk, and hotspot.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to review feedback.

It's not possible to manual review the sym files but everything else looks 
okay. I searched the test tree for any additional tests that might need 
updating but didn't spot any.

-

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6493

Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]

On Sat, 20 Nov 2021 01:58:54 GMT, Jaikiran Pai  wrote:

>> The commit here is a potential fix for the issue noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8258117.
>> 
>> The change here repurposes an existing internal interface `ModuleInfoEntry` 
>> to keep track of the last modified timestamp of a `module-info.class` 
>> descriptor.
>> 
>> This commit uses the timestamp of the `module-info.class` on the filesystem 
>> to set the time on the `JarEntry`. There are a couple of cases to consider 
>> here:
>> 
>> 1. When creating a jar  (using `--create`), we use the source 
>> `module-info.class` from the filesystem and then add extended info to it 
>> (attributes like packages, module version etc...). In such cases, this patch 
>> will use the lastmodified timestamp from the filesystem of 
>> `module-info.class` even though we might end up updating/extending/modifying 
>> (for example by adding a module version) its content while storing it as a 
>> `JarEntry`. 
>> 
>> 2. When updating a jar (using `--update`), this patch will use the 
>> lastmodified timestamp of `module-info.class` either from the filesystem or 
>> from the source jar's entry (depending on whether a new `module-info.class` 
>> is being passed to the command). Here too, it's possible that we might end 
>> up changing/modifying/extending the `module-info.class` (for example, 
>> changing the module version to a new version) that gets written into the 
>> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` 
>> even in such cases.
>> 
>> If we do have to track actual changes that might happen to 
>> `module-info.class` while extending its info (in `extendedInfoBytes()`) and 
>> then decide whether to use current system time as last modified time, then 
>> this will require a bigger change and also a discussion on what kind of 
>> extending of module-info.class content will require a change to the 
>> lastmodifiedtime of that entry.
>
> Jaikiran Pai 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:
> 
>  - Merge latest from master branch
>  - use testng asserts
>  - Remove "final" usage from test
>  - convert test to testng
>  - Merge latest from master branch
>  - Merge latest from master branch
>  - 8258117: jar tool sets the time stamp of module-info.class entries to the 
> current time

src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 806:

> 804: Long lastModified = f.lastModified() == 0 ? null : 
> f.lastModified();
> 805: moduleInfos.putIfAbsent(name,
> 806: new StreamedModuleInfoEntry(name, 
> Files.readAllBytes(f.toPath()), lastModified));

I'd prefer to see this split into two two statements as there is just too much 
going on one the one line.

src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1785:

> 1783: private final String name;
> 1784: private final byte[] bytes;
> 1785: private final Long lastModifiedTime;

It might be better to use a FileTime here, otherwise we risk a NPE when 
unboxing.

src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2108:

> 2106: byte[] extended = extendedInfoBytes(md, bytes, packages);
> 2107: // replace the entry value with the extended bytes
> 2108: e.setValue(new StreamedModuleInfoEntry(e.getValue().name(), 
> extended, e.getValue().getLastModifiedTime()));

There are 3 calls to getValue and each one I need to remember that e.getValue 
is a ModuleInfoEntry. If you add ModuleInfo entry = e.getValue() then it might 
help the readability.

test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java 
line 60:

> 58: private static final String UPDATED_MODULE_VERSION = "1.2.4";
> 59: private static final String MAIN_CLASS = "jdk.test.foo.Foo";
> 60: private static final Path MODULE_CLASSES_DIR = 
> Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();

You can use Path.of here.

-

PR: https://git.openjdk.java.net/jdk/pull/5486

Re: RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures