Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v5]
On Thu, 21 Apr 2022 11:35:57 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh
src/java.base/share/classes/java/lang/ThreadLocal.java line 179:
> 177: private T get(Thread t) {
> 178: ThreadLocalMap map = getMap(t);
> 179: if (map != null && map != ThreadLocalMap.NOT_SUPPORTED) {
Due to the way `setInitialValue` is implemented, `getMap(t)` will currently be
called twice when `ThreadLocal`s are disabled.
This method should probably be changed so that when `map ==
ThreadLocalMap.NOT_SUPPORTED`, it simply does:
return initialValue();
Suggestion:
if (map != null) {
if (map == ThreadLocalMap.NOT_SUPPORTED) {
return initialValue();
}
src/java.base/share/classes/java/lang/ThreadLocal.java line 423:
> 421: * Construct a new map without a table.
> 422: */
> 423: ThreadLocalMap() {
It might be possible for this to be `private`:
Suggestion:
private ThreadLocalMap() {
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: Consider enhancing Comparable with lessThan(), greaterThan() and friends
Yes, this has been proposed before. See
https://bugs.openjdk.java.net/browse/JDK-8241056
It's not obviously a bad idea, but there are a bunch of costs that seem to
counterbalance the benefits.
I think the biggest problem is that adding a bunch of default methods to a
widely-implemented interface (Comparable) runs the risk of name clashes. An
alternative would be to add static methods with nice names, but I'm still not sure
it's worthwhile.
Perhaps the next biggest problem is that it adds a lot of clutter to the API, and it
doesn't add any new abstractions, it doesn't strengthen any existing abstraction, it
doesn't increase performance of anything, etc. It arguably improves readability, but
it also arguably could lead to confusion.
Personally I don't find `if (object.compareTo(other) > 0)` objectionable. Just move
the comparison operator between the two operands. Then again, I'm an old C hacker
who grew up with `if (strcmp(a, b) > 0)` which is basically the same thing, so I'm
used to it.
(Interestingly, I looked at a bunch of Java tutorial sites, and -- setting aside
their mistakes -- they all talked about how to *implement* Comparable, and how use
Comparable objects for sorting, but not about how to compare the return value
against zero to compare two objects. The javadocs don't explain this either. So
maybe we have a documentation problem.)
Named methods and their alternatives seem to be something along the lines of:
if (object.greaterThan(other))
if (object.isGreaterThan(other))
if (object.gt(other))
(Choice of names deferred to a bike shed to be had at a later time.)
This is kind of ok, until we get to Comparator, which would need the same thing.
Instead of
if (c.compare(a, b) > 0)
we might want
if (c.greaterThan(a, b))
if (c.isGreaterThan(a, b))
if (c.gt(a, b))
Here we have to do the same mental gymnastics of moving the operator between the
operands. This doesn't seem all that helpful to me.
There's also the difference between equals() and comparesEqual() or whatever. Worse,
something like isNotEquals() is *not* the inverse of equals()! I think adding such
methods could increase confusion instead of reducing it.
While the idiom of comparing the return value of compareTo() against zero is pretty
cryptic, I think trying to solve it by adding a bunch of named methods somewhere
potentially runs into a bunch of other problems. Maybe these can be avoided, but it
seems like a lot of effort for not much gain. Maybe it would be more fruitful to
find better ways to teach people about the compare-against-zero idiom.
s'marks
On 4/21/22 1:15 AM, Petr Janeček wrote:
Hello,
I'm pretty sure this must have come up a few times now, but
I was unable to find a discussion anywhere, so here goes:
The `if (object.compareTo(other) > 0)` construct for Comparables,
while it works, is an annoyance to readers, and I often have to
do a double-take when seeing it, to make sure it says what I think
it says.
Did we ever consider adding human-friendly default methods
to Comparable like e.g. these (names obviously subject to change):
```java
public default boolean lessThan(T other) {
return compareTo(other) < 0;
}
public default boolean lessThanOrEqual(T other) {
return compareTo(other) <= 0;
}
public default boolean comparesEqual(T other) {
return compareTo(other) == 0;
}
public default boolean greaterThanOrEqual(T other) {
return compareTo(other) >= 0;
}
public default boolean greaterThan(T other) {
return compareTo(other) > 0;
}
```
These are pure human convenience methods to make the code easier
to read, we do not *need* them. Still, I absolutely personally
think the simplification they'd provide is worth the cost.
Are there any problems with the proposed methods that prevent them
to ever appear? Wise from the CharSequence.isEmpty() incompatibility
(https://stuartmarks.wordpress.com/2020/09/22/incompatibilities-with-jdk-15-charsequence-isempty/)
I looked at common libraries to look up potential matches, but
did not find any. The closest thing I found was AssertJ with
isGreaterThan(), and some greaterThan() methods with two or more
parameters in some obscure libraries. Now, I'm sure there *will*
be matches somewhere, and this is a risk that needs to be assessed.
Or was it simply a "meh" feature not worth the hassle?
Thank you,
PJ
P.S. I'm not a native English speaker, so the method names are
up for a potential discussion if we agree they'd make our lives
easier. I can imagine something like `comparesLessThan` or similar
variations, too.
P.P.S. The `Comparator` interface does also come into mind as it
could take similar methods, but I did not see a clear naming
pattern there. I'm open to suggestions.
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold I would delete the last sentence: > By default, the size is set to 0, meaning that the JVM chooses the size for > NIO direct-buffer allocations automatically. and replace with: > If not set, the flag is ignored and the JVM chooses the size for NIO > direct-buffer allocations automatically. - PR: https://git.openjdk.java.net/jdk/pull/8222
RFR: 8283620: System.out does not use the encoding/charset specified in the Javadoc
Promoting the internal system properties for `System.out` and `System.err` so that users can override the encoding used for those streams to `UTF-8`, aligning to the `Charset.defaultCharset()`. A CSR has also been drafted. - Commit messages: - 8283620: System.out does not use the encoding/charset specified in the Javadoc Changes: https://git.openjdk.java.net/jdk/pull/8270/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8270=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283620 Stats: 57 lines in 8 files changed: 24 ins; 4 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/8270.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8270/head:pull/8270 PR: https://git.openjdk.java.net/jdk/pull/8270
need volunteer for JDK-8285405 add test and check for negative argument to HashMap::newHashMap et al
There's a small bug tail from JDK-8186958, which added a few methods to create pre-sized HashMap and related instances. * JDK-8285386 WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 Some of our test configurations failed with OOME. Since they're internal systems, I've handled this myself. * JDK-8285405 add test and check for negative argument to HashMap::newHashMap et al Some internal discussions revealed this issue; the robustness of the system under maintenance could be improved by adding an explicit argument check to these methods and tests for these cases. (Probably -1 and Integer.MIN_VALUE would be sufficient.) I'd like a volunteer to handle this. Since Xeno Amess authored the original feature, I nominate Xeno to do this work. :-) Please let us know if you can pick up this work. Thanks. s'marks
Integrated: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958
On Thu, 21 Apr 2022 22:08:00 GMT, Stuart Marks wrote: > Adds `othervm` and `-Xmx2g` options to this test, because the default heap of > 768m isn't enough. This pull request has now been integrated. Changeset: 58155a72 Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/58155a723e3ce57ee736b9e0468591e386feceee Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 Reviewed-by: lancea - PR: https://git.openjdk.java.net/jdk/pull/8352
Integrated: 8283324: CLDRConverter run time increased by 3x
On Mon, 18 Apr 2022 23:16:18 GMT, Naoto Sato wrote: > Fixing performance regression caused by the fix to > https://bugs.openjdk.java.net/browse/JDK-8176706. The fix introduced extra > looping through the resource map multiple times which was not necessary. The > execution time of the tool now got back on par with close to JDK18. This pull request has now been integrated. Changeset: f6e9ca0c Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/f6e9ca0cbe671502b6b3b1d0f8fd86f0928f64ea Stats: 16 lines in 2 files changed: 10 ins; 0 del; 6 mod 8283324: CLDRConverter run time increased by 3x Reviewed-by: ihse - PR: https://git.openjdk.java.net/jdk/pull/8288
Re: RFR: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 [v2]
On Thu, 21 Apr 2022 22:19:32 GMT, Stuart Marks wrote: >> Adds `othervm` and `-Xmx2g` options to this test, because the default heap >> of 768m isn't enough. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add bug id. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8352
Re: RFR: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 [v2]
> Adds `othervm` and `-Xmx2g` options to this test, because the default heap of > 768m isn't enough. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add bug id. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8352/files - new: https://git.openjdk.java.net/jdk/pull/8352/files/2e892be4..4191e2a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8352=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8352=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8352.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8352/head:pull/8352 PR: https://git.openjdk.java.net/jdk/pull/8352
RFR: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958
Adds `othervm` and `-Xmx2g` options to this test, because the default heap of 768m isn't enough. - Commit messages: - Add -Xmx2g to ensure JVM has enough heap. Changes: https://git.openjdk.java.net/jdk/pull/8352/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8352=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285386 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8352.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8352/head:pull/8352 PR: https://git.openjdk.java.net/jdk/pull/8352
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 16:30:42 GMT, Daniel Fuchs wrote: > > @dfuch I have only updated files in `src`, so if the incorrect spelling is > > tested, that test will now fail. I'm unfortunately not well versed in what > > tests cover serviceability code. Can you suggest a suitable set of tests to > > run? > > Folks from serviceability-dev will be more able to answer that - but I would > suggest running tier2-tier3 as a precaution. I sent Magnus a PM with info on all the svc tests that can be run. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold Should no changes be made to the code, instead just remove "the size is set to 0, meaning that" from the description of MaxDirectMemorySize? -XX:MaxDirectMemorySize=size Sets the maximum total size (in bytes) of the java.nio package, direct-buffer allocations. Append the letter k or K to indicate kilobytes, m or M to indicate megabytes, or g or G to indicate gigabytes. By default, **the size is set to 0, meaning that** the JVM chooses the size for NIO direct-buffer allocations automatically. - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 17:22:04 GMT, Magnus Ihse Bursie wrote: >> src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: >> >>> 36: jboolean pending; /* Is an invoke requested? */ >>> 37: jboolean started; /* Is an invoke happening? */ >>> 38: jboolean available;/* Is the thread in an invocable state? */ >> >> invocable could perhaps stay as invokable ? >> Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which >> we clearly don't want to change, and I see Internet dictionary evidence of >> invokable being acceptable. > > But on the other hand we have `javax.script.Invocable`. :-) > > Codespell suggested this change, and I based my decision to keep it based on > [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not > even listing "invokable" as an alternate spelling, and that "invocable" has > about 3x the number of Google hits than "invokable". > > But sure, there is perhaps reason to consider "invokable" an acceptable > alternative and keep it. I guess it depends on if you consider the word to be > based on "invoke" with a suffix, or a latinized form, like "invocation". > > I'll wait a while for others to chime in, otherwise I'll revert the > "invokable" changes. Sure, I just thought there was enough evidence that invokable is not definitely wrong, even if invocable is more correct and popular, so it's not obvious it should be changed. Don't lose sleep over it. 8-) More importantly, on the tests -- I see the changes in exception messages searched for the wrong text in the test directories, and didn't find any matches that looked like checks. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 16:17:20 GMT, Kevin Walls wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: > >> 36: jboolean pending; /* Is an invoke requested? */ >> 37: jboolean started; /* Is an invoke happening? */ >> 38: jboolean available;/* Is the thread in an invocable state? */ > > invocable could perhaps stay as invokable ? > Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which > we clearly don't want to change, and I see Internet dictionary evidence of > invokable being acceptable. But on the other hand we have `javax.script.Invocable`. :-) Codespell suggested this change, and I based my decision to keep it based on [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not even listing "invokable" as an alternate spelling, and that "invocable" has about 3x the number of Google hits than "invokable". But sure, there is perhaps reason to consider "invokable" an acceptable alternative and keep it. I guess it depends on if you consider the word to be based on "invoke" with a suffix, or a latinized form, like "invocation". I'll wait a while for others to chime in, otherwise I'll revert the "invokable" changes. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > LGTM. I spotted one fix in a exception message. I don't expect that there > will be tests depending on that, but it might still be a good idea to run the > serviceability tests to double check. Although I guess the test would have > had the same typo and would have been fixed too were it the case :-) > @dfuch I have only updated files in `src`, so if the incorrect spelling is > tested, that test will now fail. I'm unfortunately not well versed in what > tests cover serviceability code. Can you suggest a suitable set of tests to > run? Folks from serviceability-dev will be more able to answer that - but I would suggest running tier2-tier3 as a precaution. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. All looks good to me, just the invokable which you might want to leave as is, unless there are other strong feelings. 8-) src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: > 36: jboolean pending; /* Is an invoke requested? */ > 37: jboolean started; /* Is an invoke happening? */ > 38: jboolean available;/* Is the thread in an invocable state? */ invocable could perhaps stay as invokable ? Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which we clearly don't want to change, and I see Internet dictionary evidence of invokable being acceptable. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
On Thu, 21 Apr 2022 00:48:00 GMT, Stuart Marks wrote: >> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare >> values by identity. Updated API documentation of these two methods >> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object))) >> to mention such behavior. > > Thanks for working on this. Yes I can review and sponsor this change. > > Sorry I got a bit distracted. I started looking at the test here, and this > lead me to inquire about what other tests we have for `IdentityHashMap`, and > the answer is: not enough. See > [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295). (But that > should be handled separately.) @stuart-marks Updated. In addition, for API docs change, should we add apiNote on object equality code, like call computeIfPresent? - PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > LGTM. I spotted one fix in a exception message. I don't expect that there > will be tests depending on that, but it might still be a good idea to run the > serviceability tests to double check. Although I guess the test would have > had the same typo and would have been fixed too were it the case :-) @dfuch I have only updated files in `src`, so if the incorrect spelling is tested, that test will now fail. I'm unfortunately not well versed in what tests cover serviceability code. Can you suggest a suitable set of tests to run? And yes, ideally the tests should be spell checked as well. It's just that: 1) the product source is (imho) more important to begin with, 2) test comments are generally of a lower quality and more likely to contain more typos (imho), meaning even more work for me to publish a PR i believe is correct, and 3) the tests in the JDK are so intertwined and messy that I'm having a hard time understanding what groups to post reviews to. I could make one mega-PR touching the entire test code base, but I doubt it would be very popular. :-) - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Thu, 21 Apr 2022 10:13:11 GMT, Lance Andersen wrote: > Looks fine Brian. Any thoughts as to whether a release note is warranted? Thanks, @LanceAndersen. The issue is labelled as needing a release note so you are spot on. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. LGTM. I spotted one fix in a exception message. I don't expect that there will be tests depending on that, but it might still be a good idea to run the serviceability tests to double check. Although I guess the test would have had the same typo and would have been fixed too were it the case :-) - PR: https://git.openjdk.java.net/jdk/pull/8334
Integrated: JDK-8283084 RandomGenerator nextDouble(double, double) is documented incorrectly
On Tue, 5 Apr 2022 14:05:57 GMT, Jim Laskey wrote:
> `default float nextFloat(float origin, float bound); ` and `default double
> nextDouble(double origin, double bound); ` are documented incorrectly. The
> default method checks (origin < bound) and (bound - origin) < +infinity.
>
> The exception conditions are incorrect:
> "if {@code origin} is not finite,
> or {@code bound} is not finite, or {@code origin}
> is greater than or equal to {@code bound}"
>
> This is not true. Calling with -Double.MAX_VALUE and Double.MAX_VALUE
> satisfies the documented requirements but actually throws an exception.
This pull request has now been integrated.
Changeset: 85641c65
Author:Jim Laskey
URL:
https://git.openjdk.java.net/jdk/commit/85641c651d1099adcdce6ae355d8d89cfbd7e040
Stats: 18 lines in 1 file changed: 4 ins; 0 del; 14 mod
8283084: RandomGenerator nextDouble(double, double) is documented incorrectly
Reviewed-by: bpb, darcy
-
PR: https://git.openjdk.java.net/jdk/pull/8109
Integrated: JDK-8274683 Code example provided by RandomGeneratorFactory does not compile
On Tue, 5 Apr 2022 13:47:57 GMT, Jim Laskey wrote:
> A DESCRIPTION OF THE PROBLEM :
> In
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGeneratorFactory.html
> the code provided as:
> RandomGeneratorFactory best = RandomGeneratorFactory.all()
>
> .sorted(Comparator.comparingInt(RandomGenerator::stateBits).reversed())
> .findFirst()
> .orElse(RandomGeneratorFactory.of("Random"));
> does not compile.
This pull request has now been integrated.
Changeset: 4732b1d0
Author:Jim Laskey
URL:
https://git.openjdk.java.net/jdk/commit/4732b1d038d086aba31b7644c18e5db083277969
Stats: 7 lines in 1 file changed: 1 ins; 0 del; 6 mod
8274683: Code example provided by RandomGeneratorFactory does not compile
Reviewed-by: darcy
-
PR: https://git.openjdk.java.net/jdk/pull/8108
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v6]
On Thu, 21 Apr 2022 09:15:15 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup, fixing tests. LGTM - Marked as reviewed by bibou...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/8182
Zlib update
Since the new upstream version for zlib 1.2.12 is available since 4 weeks and I don’t see them in GitHub (not even after April cpu merges) I wonder when is an update planned? (I also noticed at least one vendor claims to have a zlib fix, I am not yet sure if this is a vendor specific thing) If the system zlib on Linux is used, can OS updates to zlib be applied, is this compatible? Gruss Bernd -- http://bernd.eckenfels.net
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v2]
On Sat, 16 Apr 2022 14:59:55 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/vm/ThreadContainers.java line 184:
>>
>>> 182: * with the Thread API.
>>> 183: */
>>> 184: private static class RootContainer extends ThreadContainer {
>>
>> This implementation could be clearer if split out into two, and the value
>> assigned to `INSTANCE` is selected based on the system property. Something
>> to consider later perhaps.
>
> We've been undecided on whether to just track all threads. If we do that then
> there would be only one implementation. But yes, if we continue with two then
> they should be split out.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v2]
On Fri, 15 Apr 2022 21:31:09 GMT, Paul Sandoz wrote:
>> Alan Bateman has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refresh
>
> src/java.base/share/classes/jdk/internal/vm/Continuation.java line 264:
>
>> 262: } finally {
>> 263: fence();
>> 264: StackChunk c = tail;
>
> `c` is not used
Ron has done some cleanup so this is removed.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Tue, 19 Apr 2022 01:11:56 GMT, Mandy Chung wrote: >> Alan Bateman has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Refresh > > src/java.base/share/classes/java/lang/System.java line 2173: > >> 2171: >> 2172: // start Finalizer and Reference Handler threads >> 2173: SharedSecrets.getJavaLangRefAccess().startThreads(); > > I think it would avoid any confusion if `VM.initLevel(1)` is the last step in > `initPhase1`, i.e. call after this line. Previously, the finalizer starts > very early and it has to wait until initLevel is set to level 1 which > guarantees that `JavaLangAccess` is available. With this change, > `JavaLangAccess` is already set. Yes, that would be best, just wasn't possible before now. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v5]
> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which > JDK version to target. > > We will refresh this PR periodically to pick up changes and fixes from the > loom repo. > > Most of the new mechanisms in the HotSpot VM are disabled by default and > require running with `--enable-preview` to enable. > > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero > and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. > > There are changes in many areas. To reduce notifications/mails, the labels > have been trimmed down for now to hotspot, serviceability and core-libs. > We'll add the complete set of labels when the PR is further along. > > The changes include a refresh of java.util.concurrent and ForkJoinPool from > Doug Lea's CVS. These changes will probably be proposed and integrated in > advance of this PR. > > The changes include some non-exposed and low-level infrastructure to support > the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to > make life a bit easier and avoid having to separate VM changes and juggle > branches at this time. Alan Bateman has updated the pull request incrementally with one additional commit since the last revision: Refresh - Changes: - all: https://git.openjdk.java.net/jdk/pull/8166/files - new: https://git.openjdk.java.net/jdk/pull/8166/files/ff1ef712..5e202eca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8166=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8166=03-04 Stats: 1827 lines in 289 files changed: 682 ins; 377 del; 768 mod Patch: https://git.openjdk.java.net/jdk/pull/8166.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166 PR: https://git.openjdk.java.net/jdk/pull/8166
RFR: 8285366: Fix typos in serviceability
I ran `codespell` on modules owned by the serviceability team (`java.instrument java.management.rmi java.management jdk.attach jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted those changes where it indeed discovered real typos. I will update copyright years using a script before pushing (otherwise like every second change would be a copyright update, making reviewing much harder). The long term goal here is to make tooling support for running `codespell`. The trouble with automating this is of course all false positives. But before even trying to solve that issue, all true positives must be fixed. Hence this PR. - Commit messages: - Pass #2 - Pass #1 Changes: https://git.openjdk.java.net/jdk/pull/8334/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8334=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285366 Stats: 203 lines in 137 files changed: 0 ins; 0 del; 203 mod Patch: https://git.openjdk.java.net/jdk/pull/8334.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8334/head:pull/8334 PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
On Wed, 20 Apr 2022 00:32:25 GMT, Brent Christian wrote:
> Please review this change to replace the finalizer in
> `AbstractLdapNamingEnumeration` with a Cleaner.
>
> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
> there, the change is fairly mechanical.
>
> Details of note:
> 1. Some operations need to change the state values (the update() method is
> probably the most interesting).
> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
> `homeCtx` from the superclass's `state`.
>
> The test case is based on a copy of
> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case
> might be possible, but this was done for expediency.
>
> The test only confirms that the new Cleaner use does not keep the object
> reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration`
> or `LdapBindingEnumeration`, though all are subclasses of
> `AbstractLdapNamingEnumeration`).
>
> Thanks.
I also agree with Roger's suggestions.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 73:
> 71: public void run() {
> 72: if (enumClnt != null) {
> 73: enumClnt.clearSearchReply(res, homeCtx.reqCtls);
It's a bit strange to see that there is no guard here to verify that `homeCtx
!= null`, when line 76 implies that it might. My reading is that `homeCtxt` is
not supposed to be `null` when `enumClnt` is not `null`. That could be
explained in a comment, or alternatively asserted just before line 73 (`assert
homeCtx != null;`)
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 83:
> 81: }
> 82:
> 83: private CleaningAction state;
I wonder if state should be final?
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8283324: CLDRConverter run time increased by 3x
On Mon, 18 Apr 2022 23:16:18 GMT, Naoto Sato wrote: > Fixing performance regression caused by the fix to > https://bugs.openjdk.java.net/browse/JDK-8176706. The fix introduced extra > looping through the resource map multiple times which was not necessary. The > execution time of the tool now got back on par with close to JDK18. Thanks for fixing this! - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8288
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Looks fine Brian. Any thoughts as to whether a release note is warranted? - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v6]
> This is a (preliminary) patch for javac implementation for the third preview > of pattern matching for switch (type patterns in switches). > > Draft JLS: > http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html > > The changes are: > -there are no guarded patterns anymore, guards are not bound to the > CaseElement (JLS 15.28) > -a new contextual keyword `when` is used to add a guard, instead of `&&` > -`null` selector value is handled on switch level (if a switch has `case > null`, it is used, otherwise a NPE is thrown), rather than on pattern > matching level. > -total patterns are allowed in `instanceof` > -`java.lang.MatchException` is added for the case where a switch is > exhaustive (due to sealed types) at compile-time, but not at runtime. > > Feedback is welcome! > > Thanks! Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Cleanup, fixing tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8182/files - new: https://git.openjdk.java.net/jdk/pull/8182/files/dc001541..76f2d05c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8182=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8182=04-05 Stats: 6 lines in 4 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8182.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8182/head:pull/8182 PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8309
Consider enhancing Comparable with lessThan(), greaterThan() and friends
Hello,
I'm pretty sure this must have come up a few times now, but
I was unable to find a discussion anywhere, so here goes:
The `if (object.compareTo(other) > 0)` construct for Comparables,
while it works, is an annoyance to readers, and I often have to
do a double-take when seeing it, to make sure it says what I think
it says.
Did we ever consider adding human-friendly default methods
to Comparable like e.g. these (names obviously subject to change):
```java
public default boolean lessThan(T other) {
return compareTo(other) < 0;
}
public default boolean lessThanOrEqual(T other) {
return compareTo(other) <= 0;
}
public default boolean comparesEqual(T other) {
return compareTo(other) == 0;
}
public default boolean greaterThanOrEqual(T other) {
return compareTo(other) >= 0;
}
public default boolean greaterThan(T other) {
return compareTo(other) > 0;
}
```
These are pure human convenience methods to make the code easier
to read, we do not *need* them. Still, I absolutely personally
think the simplification they'd provide is worth the cost.
Are there any problems with the proposed methods that prevent them
to ever appear? Wise from the CharSequence.isEmpty() incompatibility
(https://stuartmarks.wordpress.com/2020/09/22/incompatibilities-with-jdk-15-charsequence-isempty/)
I looked at common libraries to look up potential matches, but
did not find any. The closest thing I found was AssertJ with
isGreaterThan(), and some greaterThan() methods with two or more
parameters in some obscure libraries. Now, I'm sure there *will*
be matches somewhere, and this is a risk that needs to be assessed.
Or was it simply a "meh" feature not worth the hassle?
Thank you,
PJ
P.S. I'm not a native English speaker, so the method names are
up for a potential discussion if we agree they'd make our lives
easier. I can imagine something like `comparesLessThan` or similar
variations, too.
P.P.S. The `Comparator` interface does also come into mind as it
could take similar methods, but I did not see a clear naming
pattern there. I'm open to suggestions.
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8309
