Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Fri, 17 May 2024 09:26:03 GMT, Alan Bateman wrote: >> My main concern here is the addition of clutter (checking two flags, and >> creating two levels of nested "impl" methods) at every call site. We may >> need to rearrange our internal API for JFR (jdk.internal.events) in order to >> clean up the call sites without loading additional classes unnecessarily. >> >> I think the main performance comparison to make is the impact on startup >> time of loading the additional FileReadEvent class. That is, to compare the >> startup time of the baseline with code that loads the FileReadEvent class, >> with JFR disabled in both cases. I don't know how to do this. Anyway, if >> loading of additional classes imposes unacceptable overhead, then that >> justifies doing more work to avoid it. That's separate from whether adding >> the `jfrTracing` flag as done in this PR is an effective way to do it. > > I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable > as it would avoid calling going through the traceXXX methods when the flag is > enabled but the specific event is not enabled. Collapsing the extra layer of methods and combining the test into if (jfrTracing && FileReadEvent.enabled()) would indeed keep things a little neater. I'm still questioning the need for `jfrTracing` at all. There's the matter of how this boolean gets set and unset, and whether this is done in a way that comports with the memory model. Setting this aside, is the only benefit that it avoids loading an extra class at JVM startup time (without JFR)? In this case that would be the `FileReadEvent` class, which is the stub class in `jdk.internal.event`. Wouldn't this class be in the CDS archive, making it not worth the effort of bringing up this new `jfrTracing` mechanism simply to avoid loading it unnecessarily? I observe that in JDK 22 some (but not all) of the event classes in `jdk.internal.event` seem to be present in the CDS archive. These include various virtual thread events. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1609024695
Re: RFR: 8305457: Implement java.io.IO [v13]
On Thu, 23 May 2024 17:14:19 GMT, Pavel Rappo wrote:
>> Please review this PR which introduces the `java.io.IO` top-level class and
>> three methods to `java.io.Console` for [Implicitly Declared Classes and
>> Instance Main Methods (Third Preview)].
>>
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft
>> PR].
>>
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 23 commits:
>
> - Merge branch 'master' into 8305457-Implement-java.io.IO
> - Force reasonable terminal size
>
>JLine outputs unexpected stuff if the terminal isn't dumb and small,
>such as that of our CI machines:
>
>if (newLines.size() > displaySize && !isTerminalDumb()) {
>StringBuilder sb = new StringBuilder(">");
>
>This causes the IO test to fail with timeout, because the expected
>prompt is never matched. To avoid that, we reasonably size the
>terminal.
> - Restructure the test
> - Add diagnostic output
> - Use "expect" that was found
> - Merge branch 'master' into 8305457-Implement-java.io.IO
>
># Conflicts:
># src/java.base/share/classes/java/io/ProxyingConsole.java
># src/java.base/share/classes/jdk/internal/io/JdkConsole.java
># src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java
>#
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
># src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java
> - Escape prompt
> - Merge branch 'master' into 8305457-Implement-java.io.IO
> - Clarify input charset
> - Make IO final
> - ... and 13 more: https://git.openjdk.org/jdk/compare/e19a421c...258ce133
Updates look good.
-
Marked as reviewed by smarks (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2074746185
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v9]
On Fri, 24 May 2024 15:38:16 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 11 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8332084-restoreEcho-readLock > - Used a dedicate lock for restoreEcho > - Merge branch 'master' into JDK-8332084-restoreEcho-readLock > - Merge branch 'master' into JDK-8332084-restoreEcho-readLock > - copyright year > - Merge branch 'master' into JDK-8332084-restoreEcho-readLock > - get/setEcho() > - Addresses a review comment > - Replaced another unused exception with _ > - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java > >Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > - ... and 1 more: https://git.openjdk.org/jdk/compare/b2ea52b6...69ec27d6 Latest updates look good. - Marked as reviewed by smarks (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19184#pullrequestreview-2077341454
Re: RFR: 8328821: Map.of().entrySet() mutators should throw UnsupportedOperationException
On Wed, 27 Mar 2024 17:36:28 GMT, Liam Miller-Cushon wrote: > This change overrides mutator methods in the implementation returned by > `Map.of().entrySet()` to throw `UnsupportedOperationException`. Sorry for the delay, I'm still totally backlogged on other stuff. I can't commit to get this into JDK 23. - PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-2130014264
Re: RFR: 8333599: Improve description of \b matcher in j.u.r.Pattern
On Thu, 6 Jun 2024 18:16:14 GMT, Raffaello Giulietti wrote: > A documentation-only change to match the original intent and the implemented > behavior. Looks good. The old version with the lookahead and lookbehind was hard to understand and it was inaccurate as well. The descriptive text is easier to understand and more accurate. - Marked as reviewed by smarks (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19583#pullrequestreview-2103079803
Re: RFR: 8333599: Improve description of \b matcher in j.u.r.Pattern
On Thu, 6 Jun 2024 18:16:14 GMT, Raffaello Giulietti wrote: > A documentation-only change to match the original intent and the implemented > behavior. Oh, this needs a CSR too, since it's a change to a normative assertion. Should be pretty simple though. - PR Comment: https://git.openjdk.org/jdk/pull/19583#issuecomment-2153628785
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v3]
On Sun, 16 Jun 2024 06:36:00 GMT, Jaikiran Pai wrote: >> Can I please get a review of this enhancement which proposes to enhance >> `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? >> >> The actual work for this was done a few years back when we discussed the >> proposed approaches and then I raised a RFR. At that time I couldn't take >> this to completion. The current changes in this PR involve the >> implementation that was discussed at that time and also have implemented the >> review suggestions from that time. Here are those previous discussions and >> reviews: >> >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html >> >> To summarize those discussions, we had concluded that: >> - `Deflater` and `Inflater` will implement the `AutoCloseable` interface >> - In the `close()` implementation we will invoke the `end()` method >> (`end()` can be potentially overridden by subclasses). >> - `close()` will be specified and implemented to be idempotent. Calling >> `close()` a second time or more will be a no-op. >> - Calling `end()` and then `close()`, although uncommon, will also support >> idempotency and that `close()` call will be a no-op. >> - However, calling `close()` and then `end()` will not guarantee idempotency >> and depending on the implementing subclass, the `end()` may throw an >> exception. >> >> New tests have been included as part of these changes and they continue to >> pass along with existing tests in tier1, tier2 and tier3. When I had >> originally added these new tests, I hadn't used junit. I can convert them to >> junit if that's preferable. >> >> I'll file a CSR shortly. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Chen's suggestion - improve code comment Hi Jai, thanks for picking this up. Several points of discussion here. Nothing fatal, but as I said in 2019 :-) just a few wrinkles to iron out. I looked mainly at Deflater, but I think the issues also all apply to Inflater too. 1) class specification I think the class specification needs to be clearer about the positioning of end() and close(). The end() method has done the real work of "closing" since the classes were introduced. We're retrofitting them to to have a close() method that's essentially just a synonym for end(), and it's still perfectly legal for clients and subclasses to call end() instead of close(). I think we need to say that close() is present mainly to support AutoCloseable and try-with-resources. 2) end() specification I don't think that "may throw an exception" is strong enough. Also, on these classes (not subclasses) end() is idemopotent, isn't it? If so, this should be specified. The end() specs need to say something like, closes and releases resources, etc., henceforth operations will throw an exception (which? -- see below) but that subsequent calls to end() do nothing. 3) Which exceptions on closed Deflater/Inflater? Oddly, it doesn't seem to be specified anywhere what exceptions are thrown if operations are attempted on a closed object. And the implementation actually throws NPE??!? Ideally we should specify what exception is thrown in this circumstance, but NPE seems wrong. Maybe we want to change the behavior to throw a different exception type. IllegalStateException seems like a reasonable candidate. 4) close() specification The 2019 discussion kind of assumed that we want close() to be idempotent. I'm not sure this is necessary. Alan said the implementation might need to be "heroic" but Peter Levart's arrangement (carried through here, apparently) isn't terribly bad. And @liach suggests more comments on this code, quite reasonably... but the comment begs the question, why do we need to avoid calling end() more than once? I'm rethinking the need for close() to be idempotent. Maybe close() should simply call end(). It would simplify the implementation and the specs -- the implSpec would simply say that it calls the end() method. This would only be a problem if a) some arrangement of TWR ends up calling close() twice, which apparently it doesn't; b) there is a subclass; and c) the subclass's end() method is not idempotent. As far as I can tell we haven't found any example of any of these. It thus seems to me that having extra logic in close() to avoid calling end() more than once is solving a non-problem. It also fits more closely to the model of "close() is simply a synonym for end()". - PR Comment: https://git.openjdk.org/jdk/pull/19675#issuecomment-2172433871
Re: RFR: 8335637: Add explicit non-null return value expectations to Object.toString() [v4]
On Wed, 10 Jul 2024 05:02:36 GMT, Joe Darcy wrote:
>> Make well-behaved implementation expectations of Object.{toString, hashCode}
>> explicit.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Narrow scope of the change.
Marked as reviewed by smarks (Reviewer).
Narrowed scope to non-null toString return looks good.
-
PR Review: https://git.openjdk.org/jdk/pull/20063#pullrequestreview-2169697515
PR Comment: https://git.openjdk.org/jdk/pull/20063#issuecomment-2220970513
Re: RFR: 8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode}
On Wed, 10 Jul 2024 22:33:54 GMT, Joe Darcy wrote: > First pass at adding some quality of implementation discussions around the > overridable methods of Object. src/java.base/share/classes/java/lang/Object.java line 48: > 46: * to provide reasonable implementations of these methods. In > 47: * particular, the implementations should take care to avoid using > 48: * excessive memory, computational time, or any other resources. While it's not wrong, advising against "excessive" usage is so vague as not to be actionable. Given some piece of code it's hard to say whether or not it does anything excessive. Consider a List with a million elements; as a practical matter, any one of these methods requires visiting every element. Is that excessive? Why or why not? - PR Review Comment: https://git.openjdk.org/jdk/pull/20128#discussion_r1673223579
Re: RFR: 8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode}
On Wed, 10 Jul 2024 22:33:54 GMT, Joe Darcy wrote:
> First pass at adding some quality of implementation discussions around the
> overridable methods of Object.
src/java.base/share/classes/java/lang/Object.java line 53:
> 51: * {@link VirtualMachineError} is possible during the execution of a
> 52: * method, often due to factors outside of the method's direct
> 53: * control.
"Should not throw any exception or other throwable" is overly broad. However,
there is a narrower sense where code that implements these methods "shouldn't"
throw anything. I'd suggest focusing on precondition checking. Specifically, no
object should ever be in a state such that calling one of these methods results
in IllegalStateException or other exception based on the state of the object.
In addition, no argument passed to equals() should ever cause
IllegalArgumentException, ClassCastException, NullPointerException, or other
exception based on the argument.
(This comment applies to other locations where the "excessive" wording is used.)
src/java.base/share/classes/java/lang/Object.java line 191:
> 189: * should not be thrown if the argument has an incomparable type
> 190: * to this object and {@link NullPointerException} should not be
> 191: * thrown if the argument is {@code null}. The implementation
For these cases the recommendation should be to return false instead of
throwing such exceptions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20128#discussion_r1673226617
PR Review Comment: https://git.openjdk.org/jdk/pull/20128#discussion_r1673227589
Re: RFR: 8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode}
On Wed, 10 Jul 2024 22:33:54 GMT, Joe Darcy wrote: > First pass at adding some quality of implementation discussions around the > overridable methods of Object. @kevinb9n You should take a look at this. - PR Comment: https://git.openjdk.org/jdk/pull/20128#issuecomment-2221761107
Re: RFR: 8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode}
On Wed, 10 Jul 2024 23:16:07 GMT, Joe Darcy wrote: >> src/java.base/share/classes/java/lang/Object.java line 126: >> >>> 124: * >>> 125: * As as a quality of implementation concern, a particular >>> 126: * implementation of this method may or may not support generating >> >> "may not support generating hash codes" sounds weird; maybe like "may or may >> not guard against cyclic data structures in recursive hash code generation." >> >> I think the key is "recursive" here, as only recursive implementations are >> at risk of these faults. > > Well, recursion is the easiest way to accidentally write an infinite loop, > but I think a few other ones would be possible too ;-) I'll considering > updating the wording to highlight the most likely hazards; thanks. Unfortunately the typical implementation pattern is to recur through the component fields of the object. Not recursion through `this` but rather the hashCode implementation of some object will typically invoke hashCode of its component fields. Of course this can result in infinite recursion (really, StackOverflowError) if the object graph contains cycles, but in general, implementations of these methods don't guard against this possibility. (Nor should they. Adding such guard code seems impractical.) Note that the default implementations of these methods for records and for Collection classes doesn't guard against cycles. (The collections contain some special cases for collection that contains itself, but not cycles in general.) I have a general concern about advice that isn't actionable, such as "may not support" or "care should be taken" etc. If we really want to say something about cyclic data structures, maybe we could say something like this. For data structures that may contain cycles, these implementations should avoid traversing component fields that may lead to cycles. A class could simply inherit the method implementations from Object. Alternatively, the class could contain a unique ID that's used for determining the result of equals/hashCode/toString. (Of course, determining uniqueness is up to the application.) - PR Review Comment: https://git.openjdk.org/jdk/pull/20128#discussion_r1673238207
Re: RFR: 8328821: Map.of() derived view collection mutators should throw UnsupportedOperationException [v7]
On Tue, 9 Jul 2024 23:17:47 GMT, Liam Miller-Cushon wrote: >> This change overrides mutator methods in the implementation returned by >> `Map.of().entrySet()` to throw `UnsupportedOperationException`. > > Liam Miller-Cushon has updated the pull request incrementally with one > additional commit since the last revision: > > Update test/jdk/java/util/Collection/MOAT.java > > Co-authored-by: Chen Liang Please see the analysis I just added to the bug report: https://bugs.openjdk.org/browse/JDK-8328821?focusedId=14688968&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14688968 - PR Comment: https://git.openjdk.org/jdk/pull/18522#issuecomment-129207
Re: RFR: 8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode}
On Wed, 10 Jul 2024 22:33:54 GMT, Joe Darcy wrote: > First pass at adding some quality of implementation discussions around the > overridable methods of Object. Bloch's _Effective Java, 3/e,_ Items 10, 11, and 12 have some useful background information on implementing equals, hashCode, and toString. - PR Comment: https://git.openjdk.org/jdk/pull/20128#issuecomment-2223871986
Re: RFR: 8325679: Optimize ArrayList subList sort [v4]
On Tue, 9 Jul 2024 18:53:44 GMT, Attila Szegedi wrote: >> Attila Szegedi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove test > > Keep it open, please. @szegedi Hi Attila, sorry for the long delay on this. If you update the PR per @liach's comment on 2024-07-09, I can sponsor it. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/17818#issuecomment-2313653413
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v8]
> The concept of the shutdown sequence needs to be specified more clearly. This > PR adds text for this into the class specification of `java.lang.Runtime`. > Also includes adjustments to related areas in `addShutdownHook`, `halt`, and > in the `System` and `Thread` classes. The changes here should coordinate with > similar changes to JLS 12.8, JVMS 5.7, and the Invocation API chapter of the > _JNI Specification._ Stuart Marks has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision: - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec - Minor adjustments - Revise Implementation Note discussing JNI Invocation API. - Updates from Alan, David, and Alex. - More edits from Alex's suggestions. - More updates after discussion with Alex. - Updates after conversation with Alan: - specify that starting a shutdown hook explicitly has an unspecified effect on the shutdown sequence - link Thread class doc to shutdown sequence - link to Thread.UncaughtExceptionHandler - clarify that only live non-daemon threads are significant - use "thread termination" to conform to the text in the Thread class - adjust line breaks and whitespace - Additional wording changes to Runtime specs. - HTML fixups; updates in response to review comments. - Updates to Runtime class spec, exit, halt, and System.exit - ... and 3 more: https://git.openjdk.org/jdk/compare/4b81849d...6b559c4c - Changes: - all: https://git.openjdk.org/jdk/pull/9437/files - new: https://git.openjdk.org/jdk/pull/9437/files/9796557d..6b559c4c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=06-07 Stats: 190478 lines in 3326 files changed: 95187 ins; 75075 del; 20216 mod Patch: https://git.openjdk.org/jdk/pull/9437.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9437/head:pull/9437 PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8065554: MatchResult should provide values of named-capturing groups [v7]
On Thu, 22 Sep 2022 10:22:40 GMT, Raffaello Giulietti wrote: >> Add support for named groups to java.util.regex.MatchResult > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8065554: MatchResult should provide values of named-capturing groups OK I took another look at everything and I think this is good to integrate. The tests seem adequate but it seems like they would benefit from some refactoring. It might be an interesting exercise to revisit them and try out the new JUnit 5 APIs. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/1
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v4]
On Fri, 6 May 2022 22:05:35 GMT, liach 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. > > liach has updated the pull request with a new target base due to a merge or a > rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains six additional commits since the > last revision: > > - Move tests > - Merge branch 'master' into fix/identityhashmap-default > - Fix assertions > - Revamp test and changes. Let ci run the tests > - Fix indent > - 8178355: IdentityHashMap uses identity-based comparison for values > everywhere except remove(K,V) and replace(K,V,V) I swear, I'll get to this soon! I even pulled over the patch and started looking at it. @liach Actually one thing that would be helpful would if you could merge a recent master into the PR branch. Thanks. - PR: https://git.openjdk.org/jdk/pull/8259
Re: RFR: 8279361: Error in documentation of third Stream.reduce method
On Wed, 28 Sep 2022 15:36:50 GMT, Viktor Klang wrote: > This JavaDoc change attempts to shine some light on the `combiner`-function > as it relates to the third variant of `Stream.reduce` since it according to > the bug submission in JBS can be confusing that the `combiner` is not > mentioned in the code example in the documentation. One of the problems with pseudo-code is that people can interpret it too literally. Even though we say things like "the result is as if..." it still invites confusion. For example, the proposed chunk code invokes the combiner even in the case where there is only one chunk. However, the clause that follows says: > In the case where there are less than two chunks, invoking the > combiner is not needed as there is no merging to be done. seems to be quasi-normative in that it implies (or does it?) that the combiner _won't_ be called if there is only a single chunk. While that's true of the current implementation, the current spec doesn't guarantee it, and I don't think we want to add such a requirement even though it might be desirable. (At least, we shouldn't add a requirement without good reason.) So there's still a question of whether or not the combiner is called in sequential cases, and also about its fundamental role in the reduce operation. I've answered a couple Stack Overflow questions on it: https://stackoverflow.com/questions/24308146/why-is-a-combiner-needed-for-reduce-method-that-converts-type-in-java-8/24316429#24316429 https://stackoverflow.com/questions/29210176/can-a-collectors-combiner-function-ever-be-used-on-sequential-streams/29295055#29295055 (And maybe @amaembo has too. And yes Tagir we're still thinking about ordered reduce or fold operations.) So maybe a prose description of the splitting and combining process would be preferable. Or possibly a more-pseudo pseudo-code example that doesn't have as much detail, so people don't read into it so much. - PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v6]
On Thu, 11 Aug 2022 10:35:55 GMT, Daniel Fuchs wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revise Implementation Note discussing JNI Invocation API. > > src/java.base/share/classes/java/lang/Runtime.java line 197: > >> 195: * as possible. They should also not rely blindly upon services >> that may >> 196: * have registered their own shutdown hooks and therefore may >> themselves in >> 197: * the process of shutting down. Attempts to use other thread-based > > Is there a "be" missing here? >> may themselves *be* in the process ... Yes, "be" is missing. Hm, I hadn't edited that text... Ha that typo was introduced by MR in 1999! OK, fixed. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8279361: Error in documentation of third Stream.reduce method
On Wed, 28 Sep 2022 15:36:50 GMT, Viktor Klang wrote: > This JavaDoc change attempts to shine some light on the `combiner`-function > as it relates to the third variant of `Stream.reduce` since it according to > the bug submission in JBS can be confusing that the `combiner` is not > mentioned in the code example in the documentation. Deleting the pseudocode might be the right way to go. To restate the meta-issue, a problem with pseudocode is that it's difficult to separate the accidental from the essential. With something that has as much essential complexity as parallel reduction, it seems like pseudocode introduces a lot of accidental complexity that can be confusing or misleading. So yeah, if pseudocode isn't helping, then get rid of it. - PR: https://git.openjdk.org/jdk/pull/10469
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v7]
On Thu, 1 Sep 2022 06:31:11 GMT, David Holmes wrote:
>> There are subtle differences (invoking a virtual Thread's run method
>> directly does no nothing) but that is too much detail and would confusing to
>> say anything about in the introduction paragraphs.
>>
>> The following is closer to what I think we should have. The existing
>> paragraph 2 describes starting a thread so this is where the Thread.run
>> method is introduced. Paragraph 3 follows to describe termination.
>>
>>
>> * {@code Thread} defines constructors and a {@link Builder} to create
>> threads.
>> * {@linkplain #start() Starting} a thread schedules it to execute its
>> {@link #run() run}
>> * method. The newly started thread executes concurrently with the thread
>> that caused
>> * it to start.
>> *
>> * A thread terminates if either its {@code run} method completes
>> normally,
>> * or if its {@code run} method completes abruptly and the appropriate
>> {@linkplain
>> * Thread.UncaughtExceptionHandler uncaught exception handler} completes
>> normally or
>> * abruptly. With no code left to run, the thread has completed execution.
>> Thread
>> * defines the {@link #join() join} method to wait for a thread to terminate.
>
> That sounds reasonable to me.
OK, I've applied Alan's suggestion with slight edits.
-
PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v4]
On Thu, 11 Aug 2022 07:23:28 GMT, David Holmes wrote: >> What you have is okay although I would have preferred if the sentence on the >> join method was in the previous paragraph rather as it fits with termination. > > Plus one on the join() bit. You could also argue for mentioning isAlive() > here too. Merged `join` sentence into previous paragraph per David's earlier suggestion. I didn't mention `isAlive` though, no need to complicate this further. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v9]
> The concept of the shutdown sequence needs to be specified more clearly. This > PR adds text for this into the class specification of `java.lang.Runtime`. > Also includes adjustments to related areas in `addShutdownHook`, `halt`, and > in the `System` and `Thread` classes. The changes here should coordinate with > similar changes to JLS 12.8, JVMS 5.7, and the Invocation API chapter of the > _JNI Specification._ Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Additional edits to Runtime and Thread specs. - Changes: - all: https://git.openjdk.org/jdk/pull/9437/files - new: https://git.openjdk.org/jdk/pull/9437/files/6b559c4c..ca369058 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=07-08 Stats: 24 lines in 2 files changed: 2 ins; 5 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/9437.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9437/head:pull/9437 PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v9]
On Fri, 30 Sep 2022 21:46:34 GMT, Stuart Marks wrote: >> The concept of the shutdown sequence needs to be specified more clearly. >> This PR adds text for this into the class specification of >> `java.lang.Runtime`. Also includes adjustments to related areas in >> `addShutdownHook`, `halt`, and in the `System` and `Thread` classes. The >> changes here should coordinate with similar changes to JLS 12.8, JVMS 5.7, >> and the Invocation API chapter of the _JNI Specification._ > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Additional edits to Runtime and Thread specs. OK, I finally got back to this. I incorporated most suggested edits. Please re-review. - PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v2]
On Fri, 30 Sep 2022 17:24:40 GMT, Joe Darcy wrote: >> While the floating-point == operation is *not* an equivalence relation, >> there are useful equivalence relations that can be defined over >> floating-point values. Text is added to java.lang.Double to discuss and name >> those relations. > > Joe Darcy has updated the pull request incrementally with two additional > commits since the last revision: > > - Add discussion of numerical equality. > - Fix typo. src/java.base/share/classes/java/lang/Double.java line 149: > 147: * equivalence relations can be defined over floating-point > 148: * values. Comparing numerical equality to various kinds of > 149: * equivalence: The overall discussion is good, and the inclusion of numerical equality is good, but now this introductory paragraph is invalid. It starts off: > While `==` is not an equivalence relation, OK, this continues the discussion, picking up from the assertion several paragraphs above that `==` is not an equivalence relation. Then, > several useful equivalence relations can be defined over floating-point > values. Comparing numerical equality to various kinds of equivalence: It sounds like what follows will be a list of equivalence relations that will be contrasted with numerical equality, yet the very first example is numerical equality and is _not_ an equivalence relation. I think this just needs to be reworded to be more inclusive. Maybe something like, "the following is a comparison of various relations among floating-point values." - PR: https://git.openjdk.org/jdk/pull/10498
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v2]
On Fri, 30 Sep 2022 17:24:40 GMT, Joe Darcy wrote:
>> While the floating-point == operation is *not* an equivalence relation,
>> there are useful equivalence relations that can be defined over
>> floating-point values. Text is added to java.lang.Double to discuss and name
>> those relations.
>
> Joe Darcy has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Add discussion of numerical equality.
> - Fix typo.
src/java.base/share/classes/java/lang/Double.java line 181:
> 179: * {@code +0.0} and {@code -0.0} are distinguished from each other.
> 180: * every bit pattern encoding a NaN is considered equivalent to
> each other
> 181: * an infinite value is equivalent to an infinite value of the same
> sign
Seems like this line on infinities could be reworded. I wouldn't quibble over
this except that I had to read it several times to figure out what it meant.
The statement on NaN is universally quantified, whereas the statement on
infinite values starts off sounding like an existential quantifier. Possibly:
"all infinite values of the same sign are considered equivalent to each other."
-
PR: https://git.openjdk.org/jdk/pull/10498
Re: Collections.shuffle to accept RandomGenerator
I'm ok with not adding a default shuffle() method to List. Shuffling does seem fairly rare, and it potentially has a some semantic issues regarding updates to the RNG state. Those would make it difficult to specify or to override without possibly breaking the contract. Having a List::shuffle default method might provide a path toward solving the issues with shuffing a CopyOnWriteArrayList, as described recently [1]. However, that seems like an extremely rare case, and there's a workaround (albeit fairly painful). And there are some things that COWAL simply doesn't support (like modifications from a ListIterator) or are unspecified (modifications to a subList). The Arrays class does need some attention and probably should be considered separately. It's lacking some other things too, like reverse(). One issue with modifying the Arrays class is providing overloads for some or all of the primitives. We've kind of held off because adding primitive overloads adds to the clutter of an already-cluttered class. There are some functions that support only "common" primitives int/long/double (see Arrays::setAll) which reduces the clutter; but I've missed overloads for certain arrays like byte[]. (I'm not saying not to do this; I'm saying this is more than dropping in a single Arrays::shuffle method.) I'm not sure we want to change the behavior of the existing Collections::shuffle with respect to ThreadLocalRandom. It seems like there are some weird tradeoffs here; see JDK-8218282 [2]. s'marks [1]: https://mail.openjdk.org/pipermail/core-libs-dev/2022-August/093777.html [2]: https://bugs.openjdk.org/browse/JDK-8218282 On 10/1/22 6:48 PM, Jason Mehrens wrote: Tagir, Yes I have mixed feeling about promoting shuffle to List. I did notice that java.util.Arrays doesn't have a shuffle method. Perhaps adding RandomGenerator shuffle and slice variant of shuffle to that class would be something to consider. Array backed lists could then use that method to perform a shuffle without having to use asList. Jason From: Tagir Valeev Sent: Saturday, October 1, 2022 2:24 AM To: Jason Mehrens Cc: Paul Sandoz; core-libs-dev@openjdk.org Subject: Re: Collections.shuffle to accept RandomGenerator Thanks, Paul, Jason. I've filed an issue: https://bugs.openjdk.org/browse/JDK-8294693 I'm not sure about the lifting shuffle() and making it an instance method of List interface. The shuffle() is used much less often, compared to sort(). Also, it produces an unspecified side-effect. Namely, it updates the state of the supplied RandomGenerator. Currently, it does this in a very stable way, calling nextInt() size-1 times for any list implementation. So if I initialize a RNG with a specific seed, then shuffle several lists, they will be shuffled in predictable manner, regardless of a particular list implementation. This might be desired, e.g., to write tests or reproduce problems. If we move it to an instance method, implementations might vary, and passing identical RNG may produce different shuffle order for lists with identical content but having different implementations. Moreover, it may even leave RNG in a different state afterwards which may affect subsequent operations performed with the same RNG. With sorting, any implementation must produce the same result, so such questions don't rise. With best regards, Tagir Valeev On Thu, Sep 29, 2022 at 4:27 AM Jason Mehrens wrote: JDK20 has Random.from(RandomGenerator) which could be used to do something like Collections.shuffle(List, Random.from(rg)). List.shuffle(RandomGenerator ) would allow for more efficient CopyOnWriteArrayList shuffle. Jason From: core-libs-dev on behalf of Paul Sandoz Sent: Wednesday, September 28, 2022 10:36 AM To: Tagir Valeev Cc: core-libs-dev@openjdk.org Subject: Re: Collections.shuffle to accept RandomGenerator That looks reasonable. Thinking a little more broadly. We could also change Collections.shuffle(List) to use ThreadLocalRandom so it does not have to cache the Random instance, plus it has better concurrent and PRNG properties. The spec does not describe the precise details of randomness. It’s tempting to lift Collections.shuffle(List, RandomGenerator) to List.shuffle(RandomGenerator ), similar to what we did for Collections.sort, to align with that pattern and which also aligns with reverse of sequenced collections. There is likely not much performance advantage to do doing so as there was with sort, but that location is much easier to find and use IMHO. Since the method accepts RandomGenerator the compatibility risk would likely be low. Paul. On Sep 27, 2022, at 3:11 AM, Tagir Valeev wrote: Hello! Currently, Collections.shuffle(List, Random) accepts an outdated Random class instead of RandomGenerator interface. It looks like, RandomGenerator would suffice. The code change looks trivial (aside from documentatio
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface
On Sat, 1 Oct 2022 08:06:44 GMT, Tagir F. Valeev wrote: > Java 17 added RandomGenerator interface. However, existing method > Collections.shuffle accepts old java.util.Random class. While since Java 19, > it's possible to use Random.from(RandomGenerator) wrapper, it would be more > convenient to provide direct overload shuffle(List list, RandomGenerator > rnd). > > As we are here, it would also be nice to get rid of Collections.r static > random generator which is used by default, and use ThreadLocalRandom instead. > This will reduce the contention if shuffle() without explicit random > generator is called from different threads concurrently. See my comments in [JDK-8218282](https://bugs.openjdk.org/browse/JDK-8218282). While updating the one-arg Collections::shuffle to use ThreadLocalRandom seems obvious, it's not clear to me that we actually want to do that. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v3]
On Tue, 4 Oct 2022 05:55:00 GMT, Joe Darcy wrote: >> While the floating-point == operation is *not* an equivalence relation, >> there are useful equivalence relations that can be defined over >> floating-point values. Text is added to java.lang.Double to discuss and name >> those relations. > > Joe Darcy 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 five additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8294539 > - Add discussion of numerical equality. > - Fix typo. > - JDK-8294539: Augment discussion of equivlance relations on floating-point > values Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10498
Re: RFR: JDK-8294539: Augment discussion of equivalence relations on floating-point values [v2]
On Tue, 4 Oct 2022 19:28:24 GMT, Joe Darcy wrote: >> Update as suggested and added some cross-links in from BigDecimal; thanks. > > PS To further spell things out, I added an additional trailing paragraph: > > "For two binary floating-point values a and b, if neither of a and b is zero > or NaN, then the three relations numerical equality, bit-wise equivalence, > and representation equivalence of a and b have the same true/false value. In > other words, for binary floating-point values, the three relations only > differ if at least one argument is zero or NaN." The additional paragraph sounds fine. - PR: https://git.openjdk.org/jdk/pull/10498
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v4]
On Wed, 31 Aug 2022 05:47:21 GMT, liach wrote: >> liach has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains six additional commits since >> the last revision: >> >> - Move tests >> - Merge branch 'master' into fix/identityhashmap-default >> - Fix assertions >> - Revamp test and changes. Let ci run the tests >> - Fix indent >> - 8178355: IdentityHashMap uses identity-based comparison for values >> everywhere except remove(K,V) and replace(K,V,V) > > Let's wait @liach Thanks for merging in a recent master. I started looking at this now. The tests and implementation look good. I've run this through our internal build&test system and the results look good too. Per @dfuch comment of June 1 I also looked at various method specs to see whether they would need to be updated to have reference-equality semantics. The specs are all in mostly pretty good shape, but I did add a few clauses here and there and I also added some cross-references, so I think the spec is overall much tighter now. I've pushed a branch with my proposed spec changes. Please merge from https://github.com/stuart-marks/jdk/tree/JDK-8178355-IdentityHashMap which should be based on the current head of this PR branch. The next step would be the CSR. I'd suggest updating in the (slightly) revised specs of remove() and replace() in the Specification section, but otherwise leaving them pretty much as they are, and not bothering with diffs for all the other tweaks I did. I'll generate a specdiff (internal tool, sorry, basically a structured HTML diff) and then post that to the CSR to provide a complete record of the changes. - PR: https://git.openjdk.org/jdk/pull/8259
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v10]
> The concept of the shutdown sequence needs to be specified more clearly. This > PR adds text for this into the class specification of `java.lang.Runtime`. > Also includes adjustments to related areas in `addShutdownHook`, `halt`, and > in the `System` and `Thread` classes. The changes here should coordinate with > similar changes to JLS 12.8, JVMS 5.7, and the Invocation API chapter of the > _JNI Specification._ Stuart Marks 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 15 additional commits since the last revision: - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec - Additional edits to Runtime and Thread specs. - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec - Minor adjustments - Revise Implementation Note discussing JNI Invocation API. - Updates from Alan, David, and Alex. - More edits from Alex's suggestions. - More updates after discussion with Alex. - Updates after conversation with Alan: - specify that starting a shutdown hook explicitly has an unspecified effect on the shutdown sequence - link Thread class doc to shutdown sequence - link to Thread.UncaughtExceptionHandler - clarify that only live non-daemon threads are significant - use "thread termination" to conform to the text in the Thread class - adjust line breaks and whitespace - Additional wording changes to Runtime specs. - ... and 5 more: https://git.openjdk.org/jdk/compare/c273feff...9fa2950d - Changes: - all: https://git.openjdk.org/jdk/pull/9437/files - new: https://git.openjdk.org/jdk/pull/9437/files/ca369058..9fa2950d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9437&range=08-09 Stats: 16840 lines in 388 files changed: 8252 ins; 7002 del; 1586 mod Patch: https://git.openjdk.org/jdk/pull/9437.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9437/head:pull/9437 PR: https://git.openjdk.org/jdk/pull/9437
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v10]
On Wed, 5 Oct 2022 03:34:47 GMT, Stuart Marks wrote: >> The concept of the shutdown sequence needs to be specified more clearly. >> This PR adds text for this into the class specification of >> `java.lang.Runtime`. Also includes adjustments to related areas in >> `addShutdownHook`, `halt`, and in the `System` and `Thread` classes. The >> changes here should coordinate with similar changes to JLS 12.8, JVMS 5.7, >> and the Invocation API chapter of the _JNI Specification._ > > Stuart Marks 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 15 additional > commits since the last revision: > > - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec > - Additional edits to Runtime and Thread specs. > - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec > - Minor adjustments > - Revise Implementation Note discussing JNI Invocation API. > - Updates from Alan, David, and Alex. > - More edits from Alex's suggestions. > - More updates after discussion with Alex. > - Updates after conversation with Alan: >- specify that starting a shutdown hook explicitly has an unspecified > effect on the shutdown sequence >- link Thread class doc to shutdown sequence >- link to Thread.UncaughtExceptionHandler >- clarify that only live non-daemon threads are significant >- use "thread termination" to conform to the text in the Thread class >- adjust line breaks and whitespace > - Additional wording changes to Runtime specs. > - ... and 5 more: https://git.openjdk.org/jdk/compare/fd88b65e...9fa2950d CSR drafted, please review: https://bugs.openjdk.org/browse/JDK-8294817 - PR: https://git.openjdk.org/jdk/pull/9437
Re: Sequenced Collections
On 10/4/22 9:38 PM, Ernie Rael wrote:
Summary of key points (maybe the mail was TL;DR)
OK thanks, I was still mulling over the previous email wondering which parts were
significant enough to reply to.
* SequencedCollection methods addFirst,addLast are the only methods in
Collection hierarchy (AFAIK) that might modify the collection and do
not return/signal if the collection was modified. Seems like
offerFirst,offerLast are more consistent with Collections and still
avoid method proliferation.
The problem is that the boolean return values from add() and from offerX() mean
different things, and having them be adjacent on List would be confusing. (Yes,
they're both present on Deque, which is one of the reasons Deque is too
complicated.) And we couldn't adjust the semantics of SequencedCollection.offerX()
to match add(), as that would clash with the existing semantics on Deque.
From your other messages it seems like you want the boolean return value in order
to keep track of whether the collection changed such that it affects equals() and
hashCode(). There are other methods that might modify collections where you can't
tell whether they actually modified the collection; consider Collection::clear or
List::replaceAll. So I don't think the boolean return value from add/offer is really
buying you all that much.
* With LinkedHashSet, seems like listIterator is missing. Rather than
making that part of SequencedCollection, maybe an "interface
ListIterable { ListIterator listIterator(int index); }". In addition
to modification, bi-directional might also be optional, to support
it's use with list equals.
ListIterator has indexes and so it's pretty much tied to List. Maybe what you're
missing from LinkedHashSet is the ability to insert or reposition an element
somewhere in the middle? This is certainly a valid use case, but it's quite rare,
and I'm not sure I'd want to support it using an Iterator style mechanism anyway.
Surveying the usages of ListIterator, I found that it's mainly used for iteration in
reverse, and secondarily for replacing all the elements of a List (somewhat
supplanted by replaceAll). We did run into one case where ListIterator was used to
edit items within a list but it turned out to be INCREDIBLY clumsy to use. So if we
want to support that (fairly rare) use case, I wouldn't start with a ListIterator or
some variant with indexes removed. I'd also want to see use cases before considering
adding new mechanisms.
s'marks
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v6]
On Wed, 5 Oct 2022 03:40:27 GMT, liach 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. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Spec updates and clarifications. OK, thanks for the updates. I've attached a specdiff and have tweaked the CSR in a few places and I've marked it reviewed. Go ahead and mark it Finalized. (I could actually do this myself, but I'd have to reassign the CSR to myself, make it Finalized, then reassign it back to you. That would save a messaging round-trip, but it would generate more notification spam. Plus I figured I'd give you the pleasure of hitting the **Finalize** button. :-D ) - PR: https://git.openjdk.org/jdk/pull/8259
Re: RFR: 8290036: Define and specify Runtime shutdown sequence [v10]
On Wed, 5 Oct 2022 03:34:47 GMT, Stuart Marks wrote: >> The concept of the shutdown sequence needs to be specified more clearly. >> This PR adds text for this into the class specification of >> `java.lang.Runtime`. Also includes adjustments to related areas in >> `addShutdownHook`, `halt`, and in the `System` and `Thread` classes. The >> changes here should coordinate with similar changes to JLS 12.8, JVMS 5.7, >> and the Invocation API chapter of the _JNI Specification._ > > Stuart Marks 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 15 additional > commits since the last revision: > > - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec > - Additional edits to Runtime and Thread specs. > - Merge branch 'master' into JDK-8290036-Runtime-addShutdownHook-spec > - Minor adjustments > - Revise Implementation Note discussing JNI Invocation API. > - Updates from Alan, David, and Alex. > - More edits from Alex's suggestions. > - More updates after discussion with Alex. > - Updates after conversation with Alan: >- specify that starting a shutdown hook explicitly has an unspecified > effect on the shutdown sequence >- link Thread class doc to shutdown sequence >- link to Thread.UncaughtExceptionHandler >- clarify that only live non-daemon threads are significant >- use "thread termination" to conform to the text in the Thread class >- adjust line breaks and whitespace > - Additional wording changes to Runtime specs. > - ... and 5 more: https://git.openjdk.org/jdk/compare/feeda6bc...9fa2950d I guess I got into a race with the bot and lost. Retrying. - PR: https://git.openjdk.org/jdk/pull/9437
Integrated: 8290036: Define and specify Runtime shutdown sequence
On Fri, 8 Jul 2022 23:00:15 GMT, Stuart Marks wrote: > The concept of the shutdown sequence needs to be specified more clearly. This > PR adds text for this into the class specification of `java.lang.Runtime`. > Also includes adjustments to related areas in `addShutdownHook`, `halt`, and > in the `System` and `Thread` classes. The changes here should coordinate with > similar changes to JLS 12.8, JVMS 5.7, and the Invocation API chapter of the > _JNI Specification._ This pull request has now been integrated. Changeset: d4142d84 Author:Stuart Marks URL: https://git.openjdk.org/jdk/commit/d4142d8441172fc54c9abf0a735c30b0ac8638c5 Stats: 194 lines in 3 files changed: 77 ins; 49 del; 68 mod 8290036: Define and specify Runtime shutdown sequence Reviewed-by: dholmes, alanb - PR: https://git.openjdk.org/jdk/pull/9437
Re: [External] : Re: Sequenced Collections
It sounds like you're after some generalized notion of "consistency", and the fact
that offer*() return a boolean whereas add*() do not seems inconsistent.
Unfortunately, the methods have different semantics. After add(obj), obj is *always*
a member of the collection, whereas after offer*(obj), obj *might or might not* be a
member of the collection. The semantics of addFirst/addLast are more closely aligned
with those of the add*() methods on Collection, so I decided to promote
addFirst/addLast to SequencedCollection instead of offerFirst/offerLast.
On your other point, if you want to compare elements of a SequencedCollection to
those of another, in encounter order, you can just use regular Iterators for that.
ListIterator isn't necessary.
s'marks
On 10/5/22 3:36 PM, Ernie Rael wrote:
On 10/5/22 9:34 AM, Stuart Marks wrote:
On 10/4/22 9:38 PM, Ernie Rael wrote:
Summary of key points (maybe the mail was TL;DR)
OK thanks, I was still mulling over the previous email wondering which parts were
significant enough to reply to.
* SequencedCollection methods addFirst,addLast are the only methods in
Collection hierarchy (AFAIK) that might modify the collection and do
not return/signal if the collection was modified. Seems like
offerFirst,offerLast are more consistent with Collections and still
avoid method proliferation.
The problem is that the boolean return values from add() and from offerX() mean
different things, and having them be adjacent on List would be confusing. (Yes,
they're both present on Deque, which is one of the reasons Deque is too
complicated.) And we couldn't adjust the semantics of SequencedCollection.offerX()
to match add(), as that would clash with the existing semantics on Deque.
It is not uncommon for a sub-interface/sub-class to clarify the precise meaning of a
method. If there's a general definition of SequencedCollection.offerFirst(e), then
the Deque definition clarifies the meaning in that context. Consider:
Collections.add(e) - Returns true if this collection changed as a result of the
call
List.add(e) - Return true
Set.add(e) - Returns true if this set did not already contain the specified
element
From your other messages it seems like you want the boolean return value in order
to keep track of whether the collection changed such that it affects equals() and
hashCode().
No, I was just discussing the relationship of change and equals() when working with
a SequencedCollection; it's more observations around using SeqCol. It's interesting
that an addAll() can permute the structure, and end up at the same place.
There are other methods that might modify collections where you can't tell whether
they actually modified the collection; consider Collection::clear or
List::replaceAll.
I'll be more precise: methods that work with a single item return/signal change;
most bulk operators such as removeif(), retainAll(), removeAll(), addAll() also
return/signal change.
My main point is that "void SequencedCollection.addFirst(e)" is inconsistent with
Collections' design. clear() is, well, clear(). replaceAll() seems to be an
exception (a lone exception?).
So I don't think the boolean return value from add/offer is really buying you all
that much.
When I put together a class based on a Collection, I like to follow the general
design pattern. Not sure if/when I may have used the "return change" when using a
collection. But when sub-classing a collection, since everything does it, so do I;
I'll return change in any additional methods I might add. Consistent, least surprise...
* With LinkedHashSet, seems like listIterator is missing. Rather than
making that part of SequencedCollection, maybe an "interface
ListIterable { ListIterator listIterator(int index); }". In addition
to modification, bi-directional might also be optional, to support
it's use with list equals.
ListIterator has indexes and so it's pretty much tied to List. Maybe what you're
missing from LinkedHashSet
I want to be able to do List.equals(SequencedCollection) and vice versa (in
particular with LinkedHashSet). In the list equals() implementations I've looked at,
they all use two ListIterator to do the compare; only forward iteration. For
equals(), I think can wrap the SequencedCollection iterator in a forward,
uni-directional listIterator, a little messy, and use that for equals(); support
from the infrastructure would be nice. Which is where the idea of "ListIterator
Collections.listIterator(iterator, index)" in the other email comes from.
Some daydreaming: For equals(), indexes don't matter except for initialization. And
as far as "index ... tied to list", if SequencedCollection had a listIterator, I
think I could form sub-sequence from that, with only for
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Random -> ThreadLocalRandom change test/jdk/java/util/Collections/Shuffle.java line 92: > 90: throw new RuntimeException(list.getClass() + ": " + list + " > != " + copy); > 91: } > 92: } Is there a way to factor out the `shuffle` calls and thereby collapse these two methods into one? Is it worth it? I'm thinking you could pass in a `Consumer>`. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Random -> ThreadLocalRandom change test/jdk/java/util/Collections/Shuffle.java line 66: > 64: RandomGeneratorFactory factory = > RandomGeneratorFactory.getDefault(); > 65: list.sort(null); > 66: Collections.shuffle(list, factory.create(1)); This assumes that the default factory will accept a seed value that initializes its state so that the pseudorandom sequence is repeatable. Not an unreasonable assumption, but `create(long)` essentially says that it can ignore the seed, and `getDefault` says the algorithm may change over time. On the other hand if something does change such that the pseudorandom sequence isn't repeatable, this test will most likely fail immediately. I was poking around for an explicit way to request some kind of PRNG that's guaranteed to be started in repeatable state, but I couldn't find one. Hmmm. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote:
>> Java 17 added RandomGenerator interface. However, existing method
>> Collections.shuffle accepts old java.util.Random class. While since Java 19,
>> it's possible to use Random.from(RandomGenerator) wrapper, it would be more
>> convenient to provide direct overload shuffle(List list, RandomGenerator
>> rnd).
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove Random -> ThreadLocalRandom change
src/java.base/share/classes/java/util/Collections.java line 485:
> 483: * list-iterator does not support the {@code set} operation.
> 484: * @since 20
> 485: */
It looks like this comment was copied from `shuffle(List, Random)` and
`shuffle(List)` which is fine. But since this method is now preferred over the
others, maybe we can reduce the duplication and have those other methods simply
be defined in terms of this one. We'd have to come up with the right "weasel
words" to describe the source of randomness used by `shuffle(List)`. In
addition, if you don't want to deprecate the other methods, perhaps some
wording can be found that clearly indicates this new method is now the
preferred one.
-
PR: https://git.openjdk.org/jdk/pull/10520
Re: [External] : Re: New candidate JEP: 431: Sequenced Collections
Hi Rémi,
On 10/14/22 1:20 AM, Remi Forax wrote:
People will again think that i'm the grumpy guy but i prefer to voice my
concerns.
- nobody cares, i'm back from Devoxx and nobody cares about Sequenced Collections, i've
tried to ask several friends what they think about it and the answer was "meh".
The bar to introduce new interfaces in the collection API is really really
high given how the Java ecosystem works.
Once a library starts to use those interfaces as method parameter, it
pressures the other library authors to write methods that provides object typed
as those interfaces.
Not enough people care and the cost for the community (not only Oracle) is
high, it looks like a recipe for failure.
Fortunately, we don't make decisions in OpenJDK via opinion polls. :-)
I'm obviously not in a position to address the concerns of your interlocutors.
However, the history in the bug database and the recurring discussions on
core-libs-dev (some of which are linked in the JEP) show the need for this.
Introduction of new types always poses a dilemma for library maintainers. They can
move forward aggressively and embrace new features, or they can remain on older
releases in order to reach a broader audience. That's not a reason to avoid
introducing new types.
- LinkedHashMap can be tweaked in two ways, by passing an access order as 3rd
parameter of the constructor or by overriding removeEldesEntry(), in both cases
the resulting LinkedHashMap is at odds with the contract of SequencedMap but
the compiler will happily allow to see those LinkedHashMap as SequencedMap.
SequencedMap specifies that entries have an encounter order but it doesn't make any
requirements on how that order is established. LinkedHashMap's access-order mode is
unusual in that it specifies that specific methods additionally have the side effect
of reordering the relevant entry. The removeEldestEntry() method modifies the
behavior of mapping-insertion methods to optionally remove the first ("eldest")
entry. Neither of these behaviors conflicts with anything in SequencedMap.
- LinkedHashMap/LinkedHashSet are dinosaurs, there are more efficient
implementations of the concepts of ordered set / ordered map both in term of
memory footprint and runtime execution, so adding new interfaces without
exploring new implementations using Valhalla value class and in relation with
the Collection Literal JEP seems premature to me.
LinkedHashMap and LinkedHashSet are in fact used fairly frequently. They're not used
as much as HashMap, but they're used more than ArrayDeque or TreeMap. Presumably
this is because people find LinkedHashMap/Set useful and that the overhead compared
to their non-linked counterparts is acceptable for the additional services they provide.
The performance and footprint of LinkedHashMap/Set are not impacted either way by
the JEP, nor are any potential future performance improvements (including those that
might arise because of Valhalla) precluded by the JEP.
s'marks
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v3]
On Wed, 12 Oct 2022 13:26:29 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Fixes according to review > > 1. Reduce duplication in tests > 2. Use JumpableGenerator#copy() instead of create(1) in tests, as according > to the spec, seed can be ignored > 3. Simplify documentation for shuffle(List, Random) to avoid duplication. I'm having difficulty imagining how `SequencedCollection::replaceAll` could work or be useful. Obviously it's on List already. It could be added to Deque, because like List, its elements are explicitly positioned (positioning via the API, not by value) and more importantly, the elements are unconstrained. That is, an element could be replaced by any other value and not affect or be affected by any other elements in the collection. However, `LinkedHashSet` is a `Set` and so has a unique-elements constraint. What should `replaceAll` do if the replacement element already exists in the set? (For a shuffle operation, the first swap operation by definition replaces an element with another element that's somewhere else in the set, so this issue arises immediately.) Maybe the unique-elements constraint is suspended for the duration of the call, and it's only enforced at the end of the call. Conceptually that might work, but I have trouble envisioning an implementation that can do that. A `replaceAll` operation on a `NavigableSet` seems right out. The elements are positioned according to their sort order, so there's no notion of updating an element "in-place" at all. The comparison to `Map::replaceAll` doesn't seem to help; that method replaces only the values of the map entries, which are unconstrained. All of the constraints of maps (uniqueness, sort order) are on the keys. For these reasons I don't think it makes sense to couple this PR to JEP 431. - PR: https://git.openjdk.org/jdk/pull/10520
Re: [External] : Re: New candidate JEP: 431: Sequenced Collections
On 10/18/22 8:04 AM, fo...@univ-mlv.fr wrote:
Introduction of new types always poses a dilemma for library maintainers. They
can
move forward aggressively and embrace new features, or they can remain on older
releases in order to reach a broader audience. That's not a reason to avoid
introducing new types.
I agree in absolute, but in this specific case you want to introduce interfaces
deep into the existing hierarchy of interfaces, not on the top.
The introduction of CharSequence has been painful and very slow, that interface
was added in 1.4 but we had to wait 9 more than 10 years later to have
parseInt() that takes a CharSequence.
As an application developer, a decade is the kind of time i will have to wait
before i can use a sequenced collection.
Adding default methods is far faster in term of adoption, computeIfAbsent or
removeIf were available at the time of the release of 1.8.
Default methods have issues too, it only works if a good default implementation
exists.
You seem to be talking about introduction of new types in opposition to introduction
of default methods. This proposal does both. Clearly if you're using an old library,
you can't use new types with it. But if the old library consumes or produces say, a
List, new applications can definitely use all the default methods added to List by
this proposal.
We've previously discussed adding new default methods (getFirst, reversed) to some
existing type (like Collection) instead of on a new type. The problem is that they
appear on all collections, even those that have no encounter order. Their
specifications would thus be so weak as to be meaningless. That's the justification
for adding a new type: to give these methods meaning.
- LinkedHashMap can be tweaked in two ways, by passing an access order as 3rd
parameter of the constructor or by overriding removeEldesEntry(), in both cases
the resulting LinkedHashMap is at odds with the contract of SequencedMap but
the compiler will happily allow to see those LinkedHashMap as SequencedMap.
SequencedMap specifies that entries have an encounter order but it doesn't make
any
requirements on how that order is established. LinkedHashMap's access-order mode
is
unusual in that it specifies that specific methods additionally have the side
effect
of reordering the relevant entry. The removeEldestEntry() method modifies the
behavior of mapping-insertion methods to optionally remove the first ("eldest")
entry. Neither of these behaviors conflicts with anything in SequencedMap.
It conflicts with the idea of the first element (or last element)
SequencedSet set = ...
var first = set.getFirst();
assertNotNull(first);
set.add("foo");
var first2 = set.getFirst();
assertEquals(first, first2); // should always be true, right ?
(LinkedHashSet doesn't have a removeEldestEntry() method; that's only on
LinkedHashMap. But let's consider the example to be rewritten as if it were about
LinkedHashMap, or a SequencedSet built from the keySet of a LinkedHashMap.)
See draft specification here:
https://github.com/openjdk/jdk/pull/7387/files#diff-be823ac1fb09a9858a40bdd29359911e534f5d9d3079e69ed31a34e37f72c7a0
There is a definition of encounter order, but there is no specification of how the
encounter order is established or how it's modified. That's left to implementations
and subtypes.
This example 1) gets the first element, 2) adds another element, and 3) gets the
first element again. Step 2 is a modification to the collection, so there's no basis
for asserting that the element obtained in step 1 always equals the element obtained
in step 3.
In particular, LinkedHashMap's "removeEldestEntry" feature says that add()
potentially has additional side effects beyond adding the argument element,
including of course removing the first element.
Also, in the example, if SequencedSet is a TreeSet containing "x", then
adding "foo" quite sensibly changes the first element between steps 1 and 3.
- LinkedHashMap/LinkedHashSet are dinosaurs, there are more efficient
implementations of the concepts of ordered set / ordered map both in term of
memory footprint and runtime execution, so adding new interfaces without
exploring new implementations using Valhalla value class and in relation with
the Collection Literal JEP seems premature to me.
LinkedHashMap and LinkedHashSet are in fact used fairly frequently. They're not
used
as much as HashMap, but they're used more than ArrayDeque or TreeMap. Presumably
this is because people find LinkedHashMap/Set useful and that the overhead
compared
to their non-linked counterparts is acceptable for the additional services they
provide.
The performance and footprint of LinkedHashMap/Set are not impacted either way
by
the JEP, nor are any potential future performance improvements (including those
that
might arise because of Valhalla) precluded by the JEP.
I don't disagree with all you say above, i'm saying that with Valhalla, we need
Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v7]
On Mon, 31 Oct 2022 20:11:34 GMT, Jim Laskey wrote:
>> Enhance the Java programming language with string templates, which are
>> similar to string literals but contain embedded expressions. A string
>> template is interpreted at run time by replacing each expression with the
>> result of evaluating that expression, possibly after further validation and
>> transformation. This is a [preview language feature and
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add @SafeVarargs declarations
src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 99:
> 97: private static List toList(E... elements) {
> 98: return JUCA.listFromTrustedArrayNullsAllowed(elements);
> 99: }
I'm ok with using JUCA to create an unmodifiable list that can contain nulls.
However, it "trusts" the argument array, meaning that the array is assumed to
be referenced exclusively and so the array reference is used directly in the
resulting List object. That implies that one needs to be very careful about the
array that gets passed in, otherwise, the resulting List might not actually be
unmodifiable.
In particular, the call site in StringTemplate.of()
https://github.com/openjdk/jdk/pull/10889/files#diff-d4e02e5ead5ad4f2cfe509c58d1145f599285cd6736bbf37e4116045b2fd50bcR309
passes the array obtained from a List parameter that comes directly from a
public call, meaning that malicious code could keep a reference to the array
returned by `toArray` and modify it later. You could clone the array, or just
revert back to the slow path.
-
PR: https://git.openjdk.org/jdk/pull/10889
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v6]
On Wed, 5 Oct 2022 03:40:27 GMT, liach 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. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Spec updates and clarifications. Now that the CSR is approved, this should be close to ready to go in. I just ran this through our CI system and there were a bunch of failures. Most likely they are unrelated, but I need to track this down and rerun the tests. I'll post an update when I get this figured out. - PR: https://git.openjdk.org/jdk/pull/8259
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v6]
On Wed, 5 Oct 2022 03:40:27 GMT, liach 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. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Spec updates and clarifications. Sorry, the issues with the test failures were a distraction. I had used the wrong version of an internal test suite. Once I figured that out and used the right version, everything passes. OK to integrate! I will sponsor this. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/8259
Re: RFR: 8292317: Missing null check for Iterator.forEachRemaining implementations
On Tue, 15 Nov 2022 02:10:01 GMT, Jaikiran Pai wrote:
> Can I please get a review of this change which proposes to fix
> https://bugs.openjdk.org/browse/JDK-8292317?
>
> The `java.util.Iterator` has a `forEachRemaining(Consumer action)`
> method. As per its contract, the implementations are expected to throw a
> `NullPointerException` if the passed `action` is `null`.
>
> `java.util.Collections` has a couple of places where it wraps the passed
> `action` into another and passes that wrapped `action` to the underlying
> `java.util.Iterator`'s default `forEachRemaining` implementation which is:
>
>
> Objects.requireNonNull(action);
> while (hasNext())
> action.accept(next());
>
> Since the passed `action` is now a non-null wrapper, the implementation goes
> ahead and advances the iterator to the next entry and invokes on the non-null
> `action` to carry out its action. That non-null wrapper action then calls the
> `null` user passed action and runs into an expected `NullPointerException`.
> However, at this point the iterator has already advanced and that is what the
> bug is.
>
> The commit in this PR introduces a trivial null check on the `action` very
> early in the call even before wrapping such an `action`. This prevents any
> further logic to execute if `action` is null.
>
> New test methods have been added to the existing test class
> `test/jdk/java/util/Collections/DelegatingIteratorForEachRemaining.java`.
> This test class was introduced in https://bugs.openjdk.org/browse/JDK-8205184
> where this delegation logic was added for the `forEachRemaining` methods.
> These new test methods reproduce the failure and verify the fix.
test/jdk/java/util/Collections/DelegatingIteratorForEachRemaining.java line 235:
> 233: // verify the iterator didn't advance
> 234: Assert.assertTrue("Iterator unexpectedly doesn't have any
> entry", it.hasNext());
> 235: }
The checks in Collections.java look good.
The tests can be simplified, I think. The reproducers in the original bug
report wrap an empty map in either an unmodifiable map or a checked map, so you
could just test them with entrySet().iterator().forEachRemaining(null). Those
cases do nothing in the current JDK when they indeed should throw NPE. Probably
just test for NPE thrown/not-thrown instead of trying to ascertain the position
of the iterator.
-
PR: https://git.openjdk.org/jdk/pull/11154
Re: RFR: 8292317: Missing null check for Iterator.forEachRemaining implementations [v2]
On Thu, 17 Nov 2022 06:05:40 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix >> https://bugs.openjdk.org/browse/JDK-8292317? >> >> The `java.util.Iterator` has a `forEachRemaining(Consumer >> action)` method. As per its contract, the implementations are expected to >> throw a `NullPointerException` if the passed `action` is `null`. >> >> `java.util.Collections` has a couple of places where it wraps the passed >> `action` into another and passes that wrapped `action` to the underlying >> `java.util.Iterator`'s default `forEachRemaining` implementation which is: >> >> >> Objects.requireNonNull(action); >> while (hasNext()) >> action.accept(next()); >> >> Since the passed `action` is now a non-null wrapper, the implementation goes >> ahead and advances the iterator to the next entry and invokes on the >> non-null `action` to carry out its action. That non-null wrapper action then >> calls the `null` user passed action and runs into an expected >> `NullPointerException`. However, at this point the iterator has already >> advanced and that is what the bug is. >> >> The commit in this PR introduces a trivial null check on the `action` very >> early in the call even before wrapping such an `action`. This prevents any >> further logic to execute if `action` is null. >> >> New test methods have been added to the existing test class >> `test/jdk/java/util/Collections/DelegatingIteratorForEachRemaining.java`. >> This test class was introduced in >> https://bugs.openjdk.org/browse/JDK-8205184 where this delegation logic was >> added for the `forEachRemaining` methods. These new test methods reproduce >> the failure and verify the fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > review suggestion - simplify test Updated tests look good, thanks. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/11154
Re: RFR: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt()) [v4]
On Tue, 22 Nov 2022 22:24:00 GMT, David Holmes wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update Runtime class doc re: other unexpected behaviors
>
> src/java.base/share/classes/java/lang/Runtime.java line 97:
>
>> 95: * no {@code finally} clause of any method is executed, and
>> try-with-resources
>> 96: * blocks do not {@linkplain AutoCloseable close} their resources.
>> 97: * Other unexpected behaviors may also result.
>
> No sorry that doesn't work for me. It sounds like you might get unexpected
> things happening, when in fact it is things not happening that might surprise
> someone (who doesn't understand what "immediately prevented from executing
> any further Java code" means). The list of examples is getting too long to
> handle in sentence structure so I suggest an actual list (not sure of list
> syntax). Suggestion:
>
>
> * Java code. This includes shutdown hooks as well as daemon and non-daemon
> threads. This
> * means, for example, that:
> * - threads' current methods do not complete normally or abruptly
> * - no {@code finally} clause of any method is executed
> * - no {@linkplain Thread.UncaughtExceptionHandler uncaught exception
> handler} is executed
> * - no try-with-resources blocks {@linkplain AutoCloseable close} their
> resources
I think it's reasonable to position these as examples and to put them into list
form.
(wordsmithing) For parallel structure I'd suggest "X does not occur" instead of
"no X occurs" in all but the first item, specifically (markup elided):
- threads' current methods do not complete normally or abruptly;
- finally clauses are not executed;
- uncaught exception handlers are not executed; and
- resources opened by try-with-resources statements are not closed.
-
PR: https://git.openjdk.org/jdk/pull/11218
Re: RFR: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt()) [v4]
On Tue, 22 Nov 2022 00:50:51 GMT, Brent Christian wrote:
>> [JDK-8290036](https://bugs.openjdk.org/browse/JDK-8290036) documented the
>> shutdown sequence, noting that calling Runtime.halt() skips the shutdown
>> sequence and immediately terminates the VM. Thus, "threads' current methods
>> do not complete normally or abruptly; no finally clause of any method is
>> executed".
>>
>> One ramification of this is that resources within try-with-resource blocks
>> will not be released. It would be good to state this explicitly.
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update Runtime class doc re: other unexpected behaviors
src/java.base/share/classes/java/lang/Runtime.java line 285:
> 283: * actions intended to be performed by shutdown hooks,
> 284: * {@linkplain Thread.UncaughtExceptionHandler uncaught exception
> handlers},
> 285: * finally blocks, or try-with-resources blocks. This can lead to
> data corruption.
Since the examples of the consequences of halting are covered above in "JVM
Termination" and are getting a bit involved, I'd suggest not trying to list the
concrete consequences here, and referring the reader (again) to the "JVM
Termination" section.
-
PR: https://git.openjdk.org/jdk/pull/11218
Re: RFR: 8297295: Remove ThreadGroup.allowThreadSuspension
On Fri, 25 Nov 2022 18:54:28 GMT, Alan Bateman wrote: > Another small step in the multi-release/multi-year effort to remove cruft > from Thread/ThreadGroup. > > java.lang.ThreadGroup.allowThreadSuspension(boolean) dates from JDK 1.1 and > the Classic VM. The method controlled whether threads were suspended when the > GC failed. It appears to have interacted with a callback mechanism that could > potentially free memory, allowing the GC to retry. The method was never > specified . > > The method was deprecated and changed to do nothing in JDK 1.2. It was > deprecated for removal in Java 14. > > A corpus analysis of 30M classes in 130k artifacts found 0 usages of this > method. > > It is time to finally remove this method. The compatibility impact should be > negligible. Joe, Stuart and I discussed briefly and think early in JDK 21 is > a good time to do this. Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11373
Re: RFR: 8297295: Remove ThreadGroup.allowThreadSuspension
On Mon, 28 Nov 2022 13:13:09 GMT, Jaikiran Pai wrote: >> Another small step in the multi-release/multi-year effort to remove cruft >> from Thread/ThreadGroup. >> >> java.lang.ThreadGroup.allowThreadSuspension(boolean) dates from JDK 1.1 and >> the Classic VM. The method controlled whether threads were suspended when >> the GC failed. It appears to have interacted with a callback mechanism that >> could potentially free memory, allowing the GC to retry. The method was >> never specified . >> >> The method was deprecated and changed to do nothing in JDK 1.2. It was >> deprecated for removal in Java 14. >> >> A corpus analysis of 30M classes in 130k artifacts found 0 usages of this >> method. >> >> It is time to finally remove this method. The compatibility impact should be >> negligible. Joe, Stuart and I discussed briefly and think early in JDK 21 is >> a good time to do this. > >> Joe, Stuart and I discussed briefly and think early in JDK 21 is a good time >> to do this. > > I guess, this would then mean this PR will be integrated into mainline after > 8th December when the branch for JDK 20 will be forked > https://openjdk.org/projects/jdk/20/#Schedule? @jaikiran wrote: > I guess, this would then mean this PR will be integrated into mainline after > 8th December when the branch for JDK 20 will be forked Yes, I'm sure this will go in soon after the JDK 20 fork. - PR: https://git.openjdk.org/jdk/pull/11373
Re: RFR: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt()) [v7]
On Wed, 7 Dec 2022 17:38:31 GMT, Brent Christian wrote: >> [JDK-8290036](https://bugs.openjdk.org/browse/JDK-8290036) documented the >> shutdown sequence, noting that calling Runtime.halt() skips the shutdown >> sequence and immediately terminates the VM. Thus, "threads' current methods >> do not complete normally or abruptly; no finally clause of any method is >> executed". >> >> One ramification of this is that resources within try-with-resource blocks >> will not be released. It would be good to state this explicitly. > > Brent Christian 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 10 additional > commits since the last revision: > > - Merge branch 'master' into 8295857 > - 'close' -> 'closed' in AutoCloseable link > - put examples into a list, in class doc only, not halt() > - Update Runtime class doc re: other unexpected behaviors > - Mentioned effects are not a complete list > - It's "try-with-resources" > - Merge branch 'master' into 8295857 > - update halt() @apiNote > - update doc changes > - Update doc for Runtime class and halt() method Looks good! - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/11218
RFR: 8299237: add ArraysSupport.newLength test to a test group
This test isn't part of any test group, so it isn't being run regularly! I'm adding it to the jdk_util_other test group. - Commit messages: - Add jdk/internal/util to jdk_util_other test group Changes: https://git.openjdk.org/jdk/pull/11769/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11769&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299237 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11769.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11769/head:pull/11769 PR: https://git.openjdk.org/jdk/pull/11769
Re: RFR: 8299237: add ArraysSupport.newLength test to a test group
On Thu, 22 Dec 2022 19:47:30 GMT, Roger Riggs wrote: >> This test isn't part of any test group, so it isn't being run regularly! I'm >> adding it to the jdk_util_other test group. > > LGTM; Test fixes are ok for JDK 20 backport. @RogerRiggs > LGTM; Test fixes are ok for JDK 20 backport. Thanks. Do you think I should push this into JDK 20 instead of 21? - PR: https://git.openjdk.org/jdk/pull/11769
Re: RFR: 8299237: add ArraysSupport.newLength test to a test group
On Thu, 22 Dec 2022 19:37:12 GMT, Stuart Marks wrote: > This test isn't part of any test group, so it isn't being run regularly! I'm > adding it to the jdk_util_other test group. OK, I guess I'll need to withdraw this PR and open a new one against JDK 20. - PR: https://git.openjdk.org/jdk/pull/11769
Re: RFR: 8299237: add ArraysSupport.newLength test to a test group
On Thu, 22 Dec 2022 19:37:12 GMT, Stuart Marks wrote: > This test isn't part of any test group, so it isn't being run regularly! I'm > adding it to the jdk_util_other test group. Yeah it would probably be simpler but it would clutter up the history. I've already started cloning jdk20 anyway, no big deal. - PR: https://git.openjdk.org/jdk/pull/11769
Withdrawn: 8299237: add ArraysSupport.newLength test to a test group
On Thu, 22 Dec 2022 19:37:12 GMT, Stuart Marks wrote: > This test isn't part of any test group, so it isn't being run regularly! I'm > adding it to the jdk_util_other test group. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/11769
Re: RFR: 8299237: add ArraysSupport.newLength test to a test group
On Thu, 22 Dec 2022 19:37:12 GMT, Stuart Marks wrote: > This test isn't part of any test group, so it isn't being run regularly! I'm > adding it to the jdk_util_other test group. Closing this in favor of PR https://github.com/openjdk/jdk20/pull/73 on JDK 20. - PR: https://git.openjdk.org/jdk/pull/11769
[jdk20] RFR: 8299237: add ArraysSupport.newLength test to a test group
It was running as part of tier4 (which is kind of a catch-all tier) but since it's (an internal) part of java.util, it should really be in tier1. This adds the test/jdk/jdk/internal/util directory to the :jdk_util_other test group. - Commit messages: - 8299237: add ArraysSupport.newLength test to a test group Changes: https://git.openjdk.org/jdk20/pull/73/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=73&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299237 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk20/pull/73.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/73/head:pull/73 PR: https://git.openjdk.org/jdk20/pull/73
[jdk20] Integrated: 8299237: add ArraysSupport.newLength test to a test group
On Thu, 22 Dec 2022 21:25:39 GMT, Stuart Marks wrote: > It was running as part of tier4 (which is kind of a catch-all tier) but since > it's (an internal) part of java.util, it should really be in tier1. This adds > the test/jdk/jdk/internal/util directory to the :jdk_util_other test group. This pull request has now been integrated. Changeset: 33042a49 Author: Stuart Marks URL: https://git.openjdk.org/jdk20/commit/33042a49d75011958e5030679433e6b2a779d90a Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8299237: add ArraysSupport.newLength test to a test group Reviewed-by: naoto - PR: https://git.openjdk.org/jdk20/pull/73
Re: RFR: 8189338: JMX RMI Remote Mbean server connection hangs if the server stops responding during a SSL Handshake
On Tue, 3 Jan 2023 13:35:32 GMT, Daniel Jeliński wrote: > This patch introduces a time limit for establishing a secure connection to a > RMI server. > > The existing implementation uses the configured > `sun.rmi.transport.tcp.handshakeTimeout` during RMI handshake only; there's > no time limit for SSL handshake. > With this patch, the configuration option > `sun.rmi.transport.tcp.handshakeTimeout` is also used as a socket read > timeout during the SSL handshake. > > I modified the existing `HandshakeTimeout` test to verify both non-SSL and > SSL connections; the test passes with my changes, fails without them. > > Existing tier1-3 tests continue to pass. > > While working on the patch I noticed that `conn.isReusable()` always returns > true. I can remove all references to `isReusable` here or in another PR; it > would simplify the code a bit. Thanks for taking this long-standing bug. Changes look good. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/11829
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections
On Wed, 4 Jan 2023 14:41:20 GMT, Viktor Klang wrote: > Currently Set.copyOf allocates both a HashSet and a new empty array when the > input collection is empty. > > This patch avoids allocating anything for the case where the parameter > collection's isEmpty returns true. There's no regression test. However, with the current code (prior to this change) a call to `Set.of(zeroLengthArray)` returns the same instance as `Set.of()`, so it's difficult to write a simple functional test for this change. The test would have to assert that "no HashSet is allocated along this code path" which is much harder to test and frankly probably isn't worth it. So, please add one of the `noreg-*` labels to the bug to indicate the rationale for omitting a regression test. https://openjdk.org/guide/#jbs-label-dictionary I'd probably add the `Map.copyOf` change to this PR to avoid some bug/PR/review overhead. Thanks for mentioning this @shipilev. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v3]
On Wed, 12 Oct 2022 13:26:29 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Fixes according to review > > 1. Reduce duplication in tests > 2. Use JumpableGenerator#copy() instead of create(1) in tests, as according > to the spec, seed can be ignored > 3. Simplify documentation for shuffle(List, Random) to avoid duplication. Sorry, this got derailed by the discussion of `SequencedCollection::replaceAll`. I think we should just set that issue aside. I don't think `SequencedCollection::replaceAll` makes sense, thus this PR shouldn't be held up on its account. The change is fine in concept. A couple adjustments needs to be made: javadoc since-tags need to be updated to 21, and copyrights updated to 2023. (Well, maybe an argument could be made that they should stay at 2022. I don't particularly care.) Also, the CSR should be updated to reflect the changes to the specifications. Other than those items, this should be fine to move forward. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v3]
On Sat, 3 Dec 2022 08:23:34 GMT, Tagir F. Valeev wrote: >> Tagir F. Valeev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixes according to review >> >> 1. Reduce duplication in tests >> 2. Use JumpableGenerator#copy() instead of create(1) in tests, as >> according to the spec, seed can be ignored >> 3. Simplify documentation for shuffle(List, Random) to avoid duplication. > > A gentle ping: please review the change and the CSR. Thanks. @amaembo a couple comments on the test. The test should probably have `@key randomness` added to it. On 2022-10-28, @bplb wrote: > jdk.test.lib.RandomFactory can be used to generate a reproducible sequence of > random numbers. An example of its use may be seen for example in > java/nio/file/Files/CopyAndMove.java This bit of the test library is useful if the test is testing a random subset of the state space. It prints out the random seed on each run so that if one of the test cases fails, it's possible to reproduce it by supplying the same seed. However, it's restricted to Random and SplittableRandom, and we want to test something like Xoshiro256PlusPlus that is a RandomGenerator but not a Random. So maybe this test library can't be applied. However, take a look and see if you think it might be useful to use it. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 08:33:09 GMT, Viktor Klang wrote: >> Currently Set.copyOf allocates both a HashSet and a new empty array when the >> input collection is empty. >> >> This patch avoids allocating anything for the case where the parameter >> collection's isEmpty returns true. > > Viktor Klang has updated the pull request incrementally with two additional > commits since the last revision: > > - 8299444: java.util.Set.copyOf allocates needlessly for empty input > collections > >Modifies ImmutableCollections.listCopy: >Introduces a check for isEmpty to avoid allocation in the case of an > empty input collection. > - 8299444: java.util.Set.copyOf allocates needlessly for empty input > collections > >Modifies Map.copyOf: >Introduces a check for isEmpty to avoid allocation in the case of an empty > input Map. Overall I think the cost of isEmpty() is likely to be relatively inexpensive, even on CHM. The CHM.isEmpty() implementation is O(n) where n is the number of counter cells, which is NOT proportional to the number of mappings contained in the map. Instead, the number of counter cells is proportional to the amount of contention there is over the map. It's hard to say what this is likely to be, but it doesn't seem obvious that this would be a pessimization. Of course it's also possible for isEmpty() to return an out-of-date value. If it returns true and the CHM later changes size, this is a race condition, and at some point in the past we believe the CHM actually was empty; so copyOf() returning an empty collection is not an error. If isEmpty() returns false and the CHM is cleared afterward, toArray() will return an empty array. We'll end up with an empty collection, and the only penalty is that we had to go through the slow path to do that. And yes, calling copyOf() on a collection that's being modified concurrently is a bit questionable. It'll return a snapshot of the contents at some time in the past, but if the application is aware of this, then we should be ok. - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Mon, 9 Jan 2023 08:33:09 GMT, Viktor Klang wrote:
>> Currently Set.copyOf allocates both a HashSet and a new empty array when the
>> input collection is empty.
>>
>> This patch avoids allocating anything for the case where the parameter
>> collection's isEmpty returns true.
>
> Viktor Klang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
> collections
>
>Modifies ImmutableCollections.listCopy:
>Introduces a check for isEmpty to avoid allocation in the case of an
> empty input collection.
> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
> collections
>
>Modifies Map.copyOf:
>Introduces a check for isEmpty to avoid allocation in the case of an empty
> input Map.
src/java.base/share/classes/java/util/ImmutableCollections.java line 169:
> 167: @SuppressWarnings("unchecked")
> 168: static List listCopy(Collection coll) {
> 169: if (coll instanceof List12 || (coll instanceof ListN && !
> ((ListN)coll).allowNulls)) {
Maybe replace the cast with an instanceof pattern here?
-
PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]
On Fri, 22 Jul 2022 20:51:59 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. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > remove some more tabs `Reference.reachabilityFence(this)` - PR: https://git.openjdk.org/jdk/pull/8311
Re: RFR: 8299513: Clean up java.io [v9]
On Wed, 11 Jan 2023 15:31:22 GMT, Per Minborg wrote: >> Code in java.io contains many legacy constructs and semantics not >> recommended including: >> >> * C-style array declaration >> * Unnecessary visibility >> * Redundant keywords in interfaces (e.g. public, static) >> * Non-standard naming for constants >> * Javadoc typos >> * Missing final declaration >> >> These should be fixed as a sanity effort. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Convert field to static and revert copyright year Some general comments on this PR. It contains a lot of different changes, all mixed together. Some of the changes have more value than others, but since they're all mixed together it can be hard to tease them apart. Some changes, like anachronistic array declarations (e.g., `int foo[]` instead of `int[] foo`) clearly need to be done. Fixing constants to have CAPITALIZED_SNAKE_CASE is also fairly straightforward and should be done too. Some renamings are a bit dubious, e.g. `U` => `UNSAFE` in one case. OK well sure `U` might look like a type variable, but in context it was quite clear that `U` was the Unsafe instance. However, renaming to `UNSAFE` isn't necessarily bad, so maybe this is ok. Some nested classes had `final` added. I'm not sure of the utility of this. Is there some general rule that "everything that can be declared final should be" ? This isn't quite as bad people who insist on declaring every local variable as final, but it seems to be edging in that direction -- specifically, adding clutter without adding value. In some cases removing `static final` from constants that are declared in interfaces seems like a step in the wrong directly. Formally, an interface that contains a field initialized to a constant expression is public, static, and final. (JLS 9.3.) I think most people accept that it's not necessary to declare everything (redundantly) public in an interface. But it seems to be conventional to have static and final present on the declaration even though they're arguably redundant as well. I'm not sure why this is nor why static and final aren't treated the same way as public. At least, I don't feel the same way about them. - PR: https://git.openjdk.org/jdk/pull/11848
RFR: 8038146: Clarify NavigableMap/TreeMap support for Entry.setValue
Also additional verbiage regarding Map.Entry objects in general and their possible connection to an underlying Map. - Commit messages: - 8038146: Clarify NavigableMap/TreeMap support for Entry.setValue Changes: https://git.openjdk.org/jdk/pull/11956/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11956&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8038146 Stats: 77 lines in 4 files changed: 41 ins; 3 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/11956.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11956/head:pull/11956 PR: https://git.openjdk.org/jdk/pull/11956
Re: RFR: 8038146: Clarify NavigableMap/TreeMap support for Entry.setValue
On Fri, 13 Jan 2023 15:02:00 GMT, Alan Bateman wrote:
>> Also additional verbiage regarding Map.Entry objects in general and their
>> possible connection to an underlying Map.
>
> src/java.base/share/classes/java/util/Map.java line 405:
>
>> 403: * {@link Iterator} or implicitly via the enhanced {@code for}
>> statement. This connection
>> 404: * to the backing map is valid only during iteration of the
>> entry-set view. During
>> 405: * the iteration, if supported by the backing map, a change to a
>> Entry's value via
>
> "a Entry's value" -> "an Entry's value" or "the value of an Entry" ?
Changed to "an Entry's Value".
> src/java.base/share/classes/java/util/NavigableMap.java line 80:
>
>> 78: * return {@link Map.Entry} instances that represent snapshots of
>> mappings as
>> 79: * of the time of the call, and they do not support mutation of
>> the
>> 80: * underlying map via the optional {@link Map.Entry#setValue setValue}
>> method.
>
> I think it would be a bit more readable to put to break after "as of the time
> of the call", either a full stop or "; they". Same thing in TreeMap.
Done.
-
PR: https://git.openjdk.org/jdk/pull/11956
Re: RFR: 8038146: Clarify NavigableMap/TreeMap support for Entry.setValue
On Fri, 13 Jan 2023 15:04:21 GMT, Alan Bateman wrote:
>> Also additional verbiage regarding Map.Entry objects in general and their
>> possible connection to an underlying Map.
>
> src/java.base/share/classes/java/util/Map.java line 421:
>
>> 419: * {@link Set#toArray toArray} overloads,
>> 420: * or by copying the entry-set view into another collection. It is
>> unspecified whether
>> 421: * the Entry instances thus obtained are connected to the
>> underlying map, whether
>
> It might be a bit clearer if tweaked to "whether the obtained Entry instances
> are connected".
OK. I also tweaked a bit of wording nearby, including changing "extracted" to
"obtained" at the start of this paragraph in order to strengthen the connection
to this clause.
-
PR: https://git.openjdk.org/jdk/pull/11956
Re: RFR: 8038146: Clarify NavigableMap/TreeMap support for Entry.setValue [v2]
> Also additional verbiage regarding Map.Entry objects in general and their > possible connection to an underlying Map. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Updates from Alan. - Changes: - all: https://git.openjdk.org/jdk/pull/11956/files - new: https://git.openjdk.org/jdk/pull/11956/files/5934dd01..8828110b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11956&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11956&range=00-01 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11956.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11956/head:pull/11956 PR: https://git.openjdk.org/jdk/pull/11956
Re: RFR: 8203035: Implement equals() and hashCode() for Throwable
On Sat, 10 Dec 2022 18:11:30 GMT, Victor Toni wrote: > Being able to compare instances of Throwable allows simple detection of > exceptions raised by the same circumstances. Comparison allows for reduction > of excessive logging e.g. in hotspots without requiring custom code to > compare Throwables. I've closed [JDK-8203035](https://bugs.openjdk.org/browse/JDK-8203035) as Won't Fix; see comments there. Based on that analysis I recommend that we not proceed with this PR. Briefly, I'd recommend that you implement your own logic for reducing logging messages based on "duplicate" exceptions. The logic for detecting duplicate exceptions is most likely application-specific and thus probably doesn't belong in the JDK. - PR: https://git.openjdk.org/jdk/pull/11624
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v4]
On Sun, 15 Jan 2023 07:52:53 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Copyright year and @since tag updated > - Fixes according to review > >1. Reduce duplication in tests >2. Use JumpableGenerator#copy() instead of create(1) in tests, as > according to the spec, seed can be ignored >3. Simplify documentation for shuffle(List, Random) to avoid duplication. > - Remove Random -> ThreadLocalRandom change > - 8294693: Add Collections.shuffle overload that accepts RandomGenerator > interface Marked as reviewed by smarks (Reviewer). Thanks for updates. I've tweaked the CSR a bit and I've marked it reviewed; please go ahead and move it to Finalized. (On rebase vs. merge, merge is definitely preferable. Merges preserve PR history more faithfully. Merge commits as well as intermediate commits all get squashed when the PR is integrated, so the result is a single commit in the mainline repo. Rebasing isn't a big deal with a small change like this, but rebasing would probably confuse things if this PR were more complicated or if it had a lot of comments on different versions.) Marking this Approved so it can be integrated after the CSR completes. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: JDK-8300133: Use generalized see and link tags in core libs [v2]
On Fri, 13 Jan 2023 22:54:47 GMT, Joe Darcy wrote: >> With generalized see and link tags that can refer to anchors (JDK-8200337), >> the see and link tags in core libraries should be updated to use this >> feature when possible. This PR covers such updates for java.base. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo found in code review. Changes in collections look good. I think there are a bunch of other places where this could be used, but fixing up optional-restrictions is a good start. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/12000
Re: RFR: JDK-8300594: Use generalized see and link tags in UnicastRemoteObject
On Wed, 18 Jan 2023 21:27:03 GMT, Joe Darcy wrote: > Use improved anchor syntax in UnicastRemoteObject. Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12083
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v4]
On Sun, 15 Jan 2023 07:52:53 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - Copyright year and @since tag updated > - Fixes according to review > >1. Reduce duplication in tests >2. Use JumpableGenerator#copy() instead of create(1) in tests, as > according to the spec, seed can be ignored >3. Simplify documentation for shuffle(List, Random) to avoid duplication. > - Remove Random -> ThreadLocalRandom change > - 8294693: Add Collections.shuffle overload that accepts RandomGenerator > interface The CSR still needs to be Approved. (Yes, unfortunately, Finalized is not a terminal state. It means the _writing of the proposal_ has been finalized and is ready to be evaluated for approval.) - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8038146: Clarify Map.Entry's connection to the underlying map [v2]
On Sat, 14 Jan 2023 03:56:33 GMT, Stuart Marks wrote: >> Also additional verbiage regarding Map.Entry objects in general and their >> possible connection to an underlying Map. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Updates from Alan. Please review CSR [JDK-8300622](https://bugs.openjdk.org/browse/JDK-8300622). - PR: https://git.openjdk.org/jdk/pull/11956
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Tue, 17 Jan 2023 12:23:25 GMT, Alan Bateman wrote: >> The choice of a `` paragraph rather than `@apiNote` is for consistency >> with similar commentary paragraphs in the specs of `CASE_INSENSITIVE`, >> `UNICODE_CASE`, and `UNICODE_CHARACTER_CLASS`. >> >> I have no problems in using `@apiNote` instead, but then it would be better >> to apply the same for the other mentioned flags as well. > > Okay, I see your point and to use apiNote consistently would require > "converting" some of the existing text to apiNote too. > > I'm still mulling over Pattern.compile throwing OOME. An implNote is probably > the right category for this, in which case it can start with "In the the JDK > Reference Implementation ...". I assume the static Pattern.matches needs > same, and also Pattern.matcher for the lazy compilation case. @AlanBateman I had previously discussed with @rgiulietti whether OOME or PatternSyntaxException was more appropriate. The issue is that you might try to compile a pattern that contains a character with N combining diacritics, and it might work fine. But if you change that character to have N+1 combining diacritics, it might throw OOME. There's no syntax difference, but rather the issue is hitting an internal implementation limit. There's a bit of a precedent for throwing OOME in such cases. Various places in the library try to grow arrays. If the required array size is greater than MAX_VALUE, the library pre-emptively throws OOME without even trying to allocate the array. - PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Thu, 19 Jan 2023 15:27:04 GMT, Raffaello Giulietti wrote: >> - Strengthen a computation that could overflow. >> - Specify that use of CANON_EQ could exhaust memory in the compilation phase. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8300207: Add a pre-check for the number of canonical equivalent > permutations in j.u.r.Pattern Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12027
Re: RFR: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern [v2]
On Fri, 20 Jan 2023 01:56:16 GMT, Stuart Marks wrote: >> Okay, I see your point and to use apiNote consistently would require >> "converting" some of the existing text to apiNote too. >> >> I'm still mulling over Pattern.compile throwing OOME. An implNote is >> probably the right category for this, in which case it can start with "In >> the the JDK Reference Implementation ...". I assume the static >> Pattern.matches needs same, and also Pattern.matcher for the lazy >> compilation case. > > @AlanBateman > > I had previously discussed with @rgiulietti whether OOME or > PatternSyntaxException was more appropriate. The issue is that you might try > to compile a pattern that contains a character with N combining diacritics, > and it might work fine. But if you change that character to have N+1 > combining diacritics, it might throw OOME. There's no syntax difference, but > rather the issue is hitting an internal implementation limit. > > There's a bit of a precedent for throwing OOME in such cases. Various places > in the library try to grow arrays. If the required array size is greater than > MAX_VALUE, the library pre-emptively throws OOME without even trying to > allocate the array. > "In the the JDK Reference Implementation ..." I'm still not sure of the right style for the JDK to refer to itself. This is really the "Java SE Reference Implementation". Or perhaps it should just be "OpenJDK". Regardless, this kind of wording would stick out in a funny way, as it's not used very frequently. I'm content not having such a preamble. Having the text within `@implNote` is probably sufficient. - PR: https://git.openjdk.org/jdk/pull/12027
Integrated: 8038146: Clarify Map.Entry's connection to the underlying map
On Thu, 12 Jan 2023 01:50:51 GMT, Stuart Marks wrote: > Also additional verbiage regarding Map.Entry objects in general and their > possible connection to an underlying Map. This pull request has now been integrated. Changeset: c6d56003 Author:Stuart Marks URL: https://git.openjdk.org/jdk/commit/c6d560039682ec52efa6fa7755d2aa86f20e1148 Stats: 77 lines in 4 files changed: 41 ins; 3 del; 33 mod 8038146: Clarify Map.Entry's connection to the underlying map Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/11956
Re: RFR: 8296935: Arrays.asList().set() with wrong types throws undocumented ArrayStoreException
On Mon, 23 Jan 2023 02:35:19 GMT, Tingjun Yuan wrote:
> Modify `java.util.Arrays.ArrayList.{set,replaceAll}` to throw a
> `ClassCastException` (as documented in `java.util.List`) when attempting to
> set an element with a wrong type.
We don't want to change long-standing exception-throwing behavior for these
cases. See my comments in
[JDK-8296935](https://bugs.openjdk.org/browse/JDK-8296935).
-
PR: https://git.openjdk.org/jdk/pull/12135
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v6]
On Sat, 21 Jan 2023 19:38:16 GMT, Sergey Bylokhov wrote: >> Tagir F. Valeev has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Whitespaces fixed >> - @implSpec added to shuffle(List) > > Filed, will create a PR soon. > https://bugs.openjdk.org/browse/JDK-8300817 @mrserb Thanks for noticing this and for cleaning up. Unfortunately the GitHub Actions are often broken spuriously, but this time it was a real issue. :-( - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Sun, 22 Jan 2023 15:20:18 GMT, Attila Szegedi wrote:
>> Viktor Klang has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
>> collections
>>
>>Modifies ImmutableCollections.listCopy:
>>Introduces a check for isEmpty to avoid allocation in the case of an
>> empty input collection.
>> - 8299444: java.util.Set.copyOf allocates needlessly for empty input
>> collections
>>
>>Modifies Map.copyOf:
>>Introduces a check for isEmpty to avoid allocation in the case of an
>> empty input Map.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 174:
>
>> 172: return List.of();
>> 173: } else {
>> 174: return (List)List.of(coll.toArray()); // implicit
>> nullcheck of coll
>
> The comment is no longer relevant here, as it now happens on line 171.
Thanks @szegedi for catching this and @viktorklang-ora for fixing it. I like
having comments like this in cases where we need to throw NPE for null and for
which there's no explicit `Objects.requireNonNull`. We've had cases in the past
where an apparently innocuous refactoring postponed an implicit nullcheck,
which opened the possibility of a side effect occuring before NPE was thrown
(violates failure idempotency). So I think maintaining such comments is
important.
With that in mind, for the Set and Map cases, could you (Viktor) add similar
comments there? Arguably they should have been there already, but, oh well,
they weren't. Thanks.
-
PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v5]
On Tue, 24 Jan 2023 19:43:14 GMT, Viktor Klang wrote: >> Currently Set.copyOf allocates both a HashSet and a new empty array when the >> input collection is empty. >> >> This patch avoids allocating anything for the case where the parameter >> collection's isEmpty returns true. > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Adding comment clarifying where implicit nullchecks are made in Set and Map > copyOf Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]
On Tue, 24 Jan 2023 19:34:48 GMT, Viktor Klang wrote: >> Thanks @szegedi for catching this and @viktorklang-ora for fixing it. I like >> having comments like this in cases where we need to throw NPE for null and >> for which there's no explicit `Objects.requireNonNull`. We've had cases in >> the past where an apparently innocuous refactoring postponed an implicit >> nullcheck, which opened the possibility of a side effect occuring before NPE >> was thrown (violates failure idempotency). So I think maintaining such >> comments is important. >> >> With that in mind, for the Set and Map cases, could you (Viktor) add similar >> comments there? Arguably they should have been there already, but, oh well, >> they weren't. Thanks. > > @stuart-marks Makes sense. I've added those comments to Set and Map copyOf() Great, thanks! - PR: https://git.openjdk.org/jdk/pull/11847
Re: RFR: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections [v2]
On Wed, 25 Jan 2023 22:23:26 GMT, Tagir F. Valeev wrote:
>> number of minor cleanups could be done in Arrays and Collections utility
>> classes.
>> In Arrays:
>> - Redundant import jdk.internal.misc.Unsafe;
>> - C-style array declaration is used in public static boolean equals(short[]
>> a, short a2[]) (that's the only place in the whole file)
>>
>> In Collections:
>> - A few obsolete "unchecked" and "rawtypes" suppressions
>> - Unnecessary local variable initializer in get() method
>> - Raw type can be avoided in a number of casts
>> - Explicit type parameters could be omitted or converted to diamonds
>> - A couple of javadoc links on private APIs are malformed
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Revert cast removal
src/java.base/share/classes/java/util/Collections.java line 1068:
> 1066:
> 1067: public Iterator iterator() {
> 1068: return new Iterator<>() {
Interesting, this whole family of changes is now possible because the original
version of diamond didn't allow it to be used in the context of an anonymous
inner class, because of non-denotability of types or some such. That
restriction has been relaxed, but we never went back to fix these up.
I also note think that Martin objected to this sort of change in
java.util.concurrent, but I'm OK with it here.
-
PR: https://git.openjdk.org/jdk/pull/12207
Re: RFR: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections [v2]
On Wed, 25 Jan 2023 22:23:26 GMT, Tagir F. Valeev wrote:
>> number of minor cleanups could be done in Arrays and Collections utility
>> classes.
>> In Arrays:
>> - Redundant import jdk.internal.misc.Unsafe;
>> - C-style array declaration is used in public static boolean equals(short[]
>> a, short a2[]) (that's the only place in the whole file)
>>
>> In Collections:
>> - A few obsolete "unchecked" and "rawtypes" suppressions
>> - Unnecessary local variable initializer in get() method
>> - Raw type can be avoided in a number of casts
>> - Explicit type parameters could be omitted or converted to diamonds
>> - A couple of javadoc links on private APIs are malformed
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Revert cast removal
Overall looks good, no changes necessary, comments are just observations.
I'll come back and approve this when my CI build finishes.
src/java.base/share/classes/java/util/Collections.java line 1636:
> 1634: @SuppressWarnings({"unchecked"})
> 1635: UnmodifiableEntrySet(Set extends V>> s) {
> 1636: super((Set>)s);
Huh, good catch, there doesn't appear to be a "limitation in the type system"
here, either currently or in older versions of javac going back to JDK 7.
src/java.base/share/classes/java/util/Collections.java line 5428:
> 5426: return (o == this) ||
> 5427: (o instanceof ReverseComparator2 that &&
> 5428: cmp.equals(that.cmp));
Nice, using an instanceof pattern where we had missed it previously.
-
PR: https://git.openjdk.org/jdk/pull/12207
Re: RFR: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections [v2]
On Wed, 25 Jan 2023 22:23:26 GMT, Tagir F. Valeev wrote: >> number of minor cleanups could be done in Arrays and Collections utility >> classes. >> In Arrays: >> - Redundant import jdk.internal.misc.Unsafe; >> - C-style array declaration is used in public static boolean equals(short[] >> a, short a2[]) (that's the only place in the whole file) >> >> In Collections: >> - A few obsolete "unchecked" and "rawtypes" suppressions >> - Unnecessary local variable initializer in get() method >> - Raw type can be avoided in a number of casts >> - Explicit type parameters could be omitted or converted to diamonds >> - A couple of javadoc links on private APIs are malformed > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Revert cast removal Test run looks good, ok to integrate. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.org/jdk/pull/12207
Re: RFR: 8301042: Need methods to check null elements in an array or a collection [v5]
On Sun, 29 Jan 2023 10:39:22 GMT, Tingjun Yuan wrote: >> Adding the following methods to check the nullity of elements of an array or >> a collection: >> >> >> java.util.Arrays: >> public static E[] requireNoNulls(E[] array) >> public static E[] requireNoNulls(E[] array, String message) >> public static E[] requireNoNulls(E[] array, IntFunction >> messageGenerator) >> public static E[] requireNoNullsElseReplace(E[] array, IntFunction> extends E> replaceFunction) >> public static E[] requireNoNullsCopied(E[] array) >> public static E[] requireNoNullsCopied(E[] array, IntFunction >> messageGenerator) >> public static E[] requireNoNullsCopied(E[] array, String message) >> public static E[] requireNoNullsCopiedElseReplace(E[] array, >> IntFunction replaceFunction) >> >> java.util.Collections: >> public static > C requireNoNulls(C collection) >> public static > M requireNoNulls(M map) >> public static > C requireNoNulls(C collection, String >> message) >> public static > M requireNoNulls(M map, String message) >> public static > C requireNoNulls(C collection, >> Supplier messageSupplier) >> public static > M requireNoNulls(M map, Supplier >> messageSupplier) >> public static > L requireNoNulls(L list, >> IntFunction messageGenerator) >> public static > L requireNoNullsElseReplace(L >> list, IntFunction replaceFunction) > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > `requireNoNullsCopiedElseReplace` should modify `result` I don't think we need these methods. Most of what they do can be done by the following: coll.forEach(Objects::requireNonNull) Arrays.asList(array).forEach(Objects::requireNonNull) - PR: https://git.openjdk.org/jdk/pull/12276
Re: NPE throwing behavior of immutable collections
In this reply I'll focus on the null handling issues in collections.
As you've noted, there are really (at least) two distinct issues here: whether a
collection permits nulls, and the behavior of contains(null) queries. There have
been continual complaints about both, and the issues are somewhat distinct.
The collection implementations in the JDK are all over the place with regard to
nulls. I believe this is because the original collections and the concurrent
collections up through JDK 1.6 or so were mostly done by Josh Bloch and Doug Lea,
who disagreed about null handling. They had this exchange in 2006:
Doug Lea wrote: [1]
The main reason that nulls aren't allowed in ConcurrentMaps
(ConcurrentHashMaps, ConcurrentSkipListMaps) is that
ambiguities that may be just barely tolerable in non-concurrent
maps can't be accommodated. The main one is that if
map.get(key) returns null, you can't detect whether the
key explicitly maps to null vs the key isn't mapped.
In a non-concurrent map, you can check this via map.contains(key),
but in a concurrent one, the map might have changed between calls.
Further digressing: I personally think that allowing
nulls in Maps (also Sets) is an open invitation for programs
to contain errors that remain undetected until
they break at just the wrong time. (Whether to allow nulls even
in non-concurrent Maps/Sets is one of the few design issues surrounding
Collections that Josh Bloch and I have long disagreed about.)
Josh Bloch wrote: [2]
I have moved towards your position over the years. It was probably a
mistake to allow null keys in Maps and null elements in Sets. I'm
still not sure about Map values and List elements.
In other words, Doug hates null more than I do, but over the years
I've come to see it as quite troublesome.
(I haven't talked to Josh or Doug about this particular issue recently, so I suppose
it's possible their opinions have changed in the intervening time.)
I had to decide what to do about nulls when I added the unmodifiable collections in
JDK 9. Given that Doug "hates" nulls, and Josh finds them at least quite
troublesome, I started from a position of disallowing null members in all the new
collections. Most of the unmodifiable collections are still this way (but see below).
What about contains(null)? Surely it should be ok to query for a null, and return
false, even if the collection itself can't contain null. However, a bunch of
collections and collection views in the JDK throw NPE on contains(null), so the
unmodifiable collections don't set a precedent here.
If you're dealing with all non-null values, and a null creeps in somehow, then
calling contains(null) might be an error, and it would be good for the library to
inform you of that instead of just returning false and letting the program continue
until it fails later (fail-fast). A similar issue arises with the non-generic
contains() method. If you have a Collection, then shouldn't contains("abc")
be an error? It's not, and it simply returns false, because the signature is
contains(Object). But there have been complaints that this should have been an error
because a Collection of Integer cannot possibly contain a String. Similar reasoning
applies to contains(null) on a collection that can't contain null. (Yes, the error
occurs at runtime instead of compile time, but better late than never.)
A meta-issue is: should a new thing correct a perceived design flaw in the older
thing, or should it be consistent with the old thing? Either way, you lose.
Another factor in my original decision was that it's much easier to start with a
restriction and relax it later than it is to go the other way. We have some
flexibility to allow nulls and contains(null) if we want; but it's essentially a
non-starter to disallow nulls once they've been allowed somewhere.
That's why my starting position for the design prohibited nulls everywhere.
Over time we've relaxed some restrictions around null handling in the unmodifiable
collections. Streams permit nulls, and the List returned from Stream.toList() also
permits nulls, and it also allows contains(null). So there's a different
unmodifiable List implementation under there, accessible only via streams. (Note
that the null-permitting unmodifiable lists lose the optimizations for sizes 0, 1,
and 2, which are in part based on nulls being disallowed.)
I'm fairly sympathetic to the case for changing the unmodifiable collections to
allow contains(null), given that the current NPE-throwing behavior has generated a
fair amount of complaints over the years. And the situation with having two flavors
of unmodifiable list is quite odd and is an uncomfortable point in the design space.
I should point out however that even if the unmodifiable collections were changed to
allow contains(null), it's still kind of unclear as to whether this code is safe:
public void addShoppingItems(Collection shoppingItems) {
if (shoppingI
Re: RFR: 8301958: Avoid Arrays.copyOfRange overhead in java.lang.String [v5]
On Tue, 7 Feb 2023 15:25:05 GMT, Claes Redestad wrote: >> This adds a local, specialized `copyBytes` method to `String` that avoids >> certain redundant range checks and clamping that JIT has issues removing >> fully. >> >> This has a small but statistically significant effect on `String` >> microbenchmarks, eg.: >> >> Baseline >> >> Benchmark(size) Mode Cnt >> Score Error Units >> StringConstructor.newStringFromArray 7 avgt 15 >> 16.817 ± 0.369 ns/op >> StringConstructor.newStringFromArrayWithCharset 7 avgt 15 >> 16.866 ± 0.449 ns/op >> StringConstructor.newStringFromArrayWithCharsetName 7 avgt 15 >> 22.198 ± 0.396 ns/op >> >> >> Patch: >> >> Benchmark(size) Mode Cnt >> Score Error Units >> StringConstructor.newStringFromArray 7 avgt 15 >> 15.477 ± 0.342 ns/op >> StringConstructor.newStringFromArrayWithCharset 7 avgt 15 >> 15.557 ± 0.352 ns/op >> StringConstructor.newStringFromArrayWithCharsetName 7 avgt 15 >> 21.272 ± 0.398 ns/op >> >> >> Care has to be taken to ensure preconditions have been checked when using >> `checkBytes`. In the case of `String(AbstractStringBuilder)` there's a >> possible pre-existing issue where the constructor might either throw an >> exception or truncate the buffer if the builder byte array and length is not >> in agreement (theoretically possible if you clear/remove and call >> `trimToSize()` concurrently). Adding an explicit check here seem to be the >> right thing to do regardless of this RFE. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > copyrights Marked as reviewed by smarks (Reviewer). src/java.base/share/classes/java/lang/StringLatin1.java line 763: > 761: return ""; > 762: } > 763: return new String(String.copyBytes(val, index, len), LATIN1); It might be worthwhile putting a comment at the top of this method saying that the caller is required to bounds-check the arguments. It's called from several other classes in this package, so it would be good to document this package-internal contract. I checked the callers and they seem fine. - PR: https://git.openjdk.org/jdk/pull/12453
Re: RFR: 8301958: Avoid Arrays.copyOfRange overhead in java.lang.String [v5]
On Tue, 7 Feb 2023 19:12:38 GMT, Claes Redestad wrote: > It might be that the redundant checks in Arrays.copyOfRange would be > eliminated properly with more inlining, and that the issue here is that the > affected String constructor is particularly gnarly. This gnarliness is due 1) > the need to construct the value and coder in one go and 2) the lack of > multiple return values. Give me a value-based record type so we can split > apart the constructor into charset-specific methods with zero overhead and we > might be able to split up the logic into better-sized chunks. I'm wondering if another contributing factor to the complexity of this code is the continued support of the non-compact-String codepaths. This means there are actually three code paths through every string computation. Do we need to continue to support the non-compact-string code paths? I'm concerned about maintainability too. - PR: https://git.openjdk.org/jdk/pull/12453
Re: RFR: 8296935: Arrays.asList().set() with wrong types throws undocumented ArrayStoreException [v7]
On Mon, 23 Jan 2023 07:33:21 GMT, Tingjun Yuan wrote: >> Document `java.util.Arrays.asList` that the list will throw an >> `ArrayStoreException` when attempting to set an element with a wrong type. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Update `implSpec` and remove the docs of the impl. I've added my recommended text for an apiNote as a comment in the bug report: https://bugs.openjdk.org/browse/JDK-8296935?focusedCommentId=14555728&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14555728 Even though this is non-normative text, I'd say it still requires a CSR to document the decision that this issue is NOT a conformance issue. That is, even though the various List methods are documented to throw certain runtime exceptions in various circumstances, an implementation is permitted to throw other runtime exceptions without violating the specification. The flip side is that it's good practice for method specifications to specify what exceptions are thrown in what circumstances, but it's not required for them to specify all exceptions in all circumstances. Indeed, in this specific case, it seems unreasonable to add throwing of ArrayStoreException to all the List methods because of this one edge case of this particular implementation. - PR: https://git.openjdk.org/jdk/pull/12135
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]
On Fri, 22 Jul 2022 20:51:59 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. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > remove some more tabs Oh darn, looks like `Reference.reachabilityFence(this)` is broken!! - PR: https://git.openjdk.org/jdk/pull/8311
Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v4]
On Tue, 14 Feb 2023 18:56:29 GMT, Roger Riggs wrote:
>> It can be difficult to find the cause of calls to
>> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java
>> runtime exits.
>> The status value and stack trace are logged using the System Logger named
>> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Correct System.getLogger link
src/java.base/share/classes/java/lang/Shutdown.java line 168:
> 166: Throwable throwable = new Throwable("Runtime.exit(" +
> status + ")");
> 167: log.log(System.Logger.Level.DEBUG, "Runtime.exit()
> called with status: " + status,
> 168: throwable);
I'd put a try/catch around the actual logging of the message, to avoid a
situation where an error in the logger handler prevents the system from being
shut down.
-
PR: https://git.openjdk.org/jdk/pull/12517
