Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v5]
> This is a followup to simplify Shutdown.exit after the removal of > finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement > on the approach has been reached in this PR, a CSR will be filed to > propose the spec change to Runtime.exit. Ryan Ernst 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: - Merge branch 'master' into shutdown_cleanup - iter text - iterate on wording - better clarify multiple threads behavior - 8288984: Simplification in Shutdown.exit This is a followup to simplify Shutdown.exit after the removal of finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement on the approach has been reached in this PR, a CSR will be filed to propose the spec change to Runtime.exit. - Changes: - all: https://git.openjdk.org/jdk/pull/9351/files - new: https://git.openjdk.org/jdk/pull/9351/files/fccd85ba..75a2651e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9351=04 - incr: https://webrevs.openjdk.org/?repo=jdk=9351=03-04 Stats: 2278 lines in 110 files changed: 1285 ins; 604 del; 389 mod Patch: https://git.openjdk.org/jdk/pull/9351.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9351/head:pull/9351 PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time [v2]
On Sun, 3 Jul 2022 22:06:58 GMT, Attila Szegedi wrote:
>> src/java.base/share/classes/java/time/format/DateTimeTextProvider.java line
>> 312:
>>
>>> 310: private Object findStore(TemporalField field, Locale locale) {
>>> 311: Entry key = createEntry(field, locale);
>>> 312: return CACHE.computeIfAbsent(key, e -> createStore(e.getKey(),
>>> e.getValue()));
>>
>> If `createStore` can be static, the call site will only have to construct a
>> constant lambda than having to pass `this` to construct a new instance on
>> every call
>
> Well, if you _really_ want to noodle this for performance, you can also store
> a `this`-bound lambda in a `private final` instance field, so then it's only
> created once too. I wouldn't put it past `javac` to do this, but I'd have to
> disassemble the bytecode of a little test program to see whether it's the
> case.
Can there be some JMH tests to confirm the performance?
The value domain of the keys is pretty limited (7 * 7) max; and I'm not sure
that the combination of creating a new record and the hashcode and equals
methods would be faster than string concat of a constant and a single digit
integer.
-
PR: https://git.openjdk.org/jdk/pull/9208
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v2]
On Mon, 4 Jul 2022 12:19:27 GMT, David Holmes wrote: >> Ryan Ernst has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better clarify multiple threads behavior > >> Let's say we've run shutdown so run all the hooks but not halted. Then >> someone calls exit(0). That seems to suggest the call will block >> indefinitely, which is neither desirable nor what was actually implemented. > > If the hook threads do not halt then the exiting thread (which holds the > lock) blocks forever in the join(). Any other call to exit blocks trying to > acquire the lock. That has always been the way this works - if hook threads > don't terminate then the VM doesn't (unless someone calls halt() directly). > That is one of the things the window you suggested be closed, allowed - a > call to exit(non-zero) could force a call to halt(). I appreciate all the feedback and the many opinions expressed here! This has been a learning exercise for me in finding a balance between implementation and specification. While I still think mentioning the possibility of signals is beneficial to a developer trying to understand that the passed status code could be ignored, the text suggested by @dholmes-ora is better than was previously there, so I have updated this PR with that. See [fccd85b](https://github.com/openjdk/jdk/pull/9351/commits/fccd85ba106ff651c00479446ac3207ed60698e8). - PR: https://git.openjdk.org/jdk/pull/9351
RFR: 8289768: Clean up unused code
This patch removes many unused variables and one unused label reported by the compilers when relevant warnings are enabled. The unused code was found by compiling after removing `unused` from the list of disabled warnings for [gcc](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L190) and [clang](https://github.com/openjdk/jdk/blob/master/make/autoconf/flags-cflags.m4#L203), and enabling [C4189](https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4189?view=msvc-170) MSVC warning. I only removed variables that were uninitialized or initialized without side effects. I verified that the removed variables were not used in any `#ifdef`'d code. I checked that the changed code still compiles on Windows, Linux and Mac, both in release and debug versions. - Commit messages: - Remove unused variables (windows) - Remove unused variables (macos) - Remove unused variables (linux) Changes: https://git.openjdk.org/jdk/pull/9383/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9383=00 Issue: https://bugs.openjdk.org/browse/JDK-8289768 Stats: 69 lines in 45 files changed: 0 ins; 65 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/9383.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9383/head:pull/9383 PR: https://git.openjdk.org/jdk/pull/9383
RFR: JDK-8289730 : Deprecated code in src/java.base/share/classes/java/lang/ClassCastException.java
- Correct a deprecated code. - Update Copyright. - More wide question : How are you dealing with deprecated codes in snippets ? ``` @deprecated(since="9") public Integer(int value) ``` - Commit messages: - Merge branch 'openjdk:master' into patch-2 - Merge branch 'openjdk:master' into patch-2 - Merge branch 'openjdk:master' into patch-2 - Merge branch 'openjdk:master' into patch-2 - Update Copyright date. - Merge branch 'openjdk:master' into patch-2 - Use deprecated Constructor. Changes: https://git.openjdk.org/jdk/pull/9359/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9359=00 Issue: https://bugs.openjdk.org/browse/JDK-8289730 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/9359.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9359/head:pull/9359 PR: https://git.openjdk.org/jdk/pull/9359
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v5]
On Tue, 5 Jul 2022 18:43:26 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst 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: > > - Merge branch 'master' into shutdown_cleanup > - iter text > - iterate on wording > - better clarify multiple threads behavior > - 8288984: Simplification in Shutdown.exit > >This is a followup to simplify Shutdown.exit after the removal of >finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >on the approach has been reached in this PR, a CSR will be filed to >propose the spec change to Runtime.exit. Version 75a2651e looks fine. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v3]
On Mon, 4 Jul 2022 16:17:27 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > iterate on wording src/java.base/share/classes/java/lang/Runtime.java line 90: > 88: * invocation may be initiated via platform specific signal handlers. > All > 89: * other invocations will block indefinitely, and their supplied exit > 90: * statuses will be ignored. If this method is invoked from a > shutdown hook If they block indefinitely then it is implicit that nothing happens with their exit status. I think you are trying to say too much. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time [v2]
On Tue, 5 Jul 2022 15:26:06 GMT, Roger Riggs wrote: >> Well, if you _really_ want to noodle this for performance, you can also >> store a `this`-bound lambda in a `private final` instance field, so then >> it's only created once too. I wouldn't put it past `javac` to do this, but >> I'd have to disassemble the bytecode of a little test program to see whether >> it's the case. > > Can there be some JMH tests to confirm the performance? > The value domain of the keys is pretty limited (7 * 7) max; and I'm not sure > that the combination of creating a new record and the hashcode and equals > methods would be faster than string concat of a constant and a single digit > integer. @RogerRiggs is you comment about `DateTimeTextProvider.findStore` or `WeekFields.of`? There is no record creation here, in `DateTimeTextProvider.findStore`. - PR: https://git.openjdk.org/jdk/pull/9208
Re: RFR: JDK-8289741 : Remove unused imports from DTDBuilder.java
On Tue, 5 Jul 2022 03:40:51 GMT, Julian Waters wrote: >> This is tracked in JBS as >> >> [JDK-8289741](https://bugs.openjdk.org/browse/JDK-8289741) >> >>> **Remove unused imports from DTDBuilder.java** >> Some imports are no more used. >> >> - java.io.FileNotFoundException; >> - java.io.BufferedInputStream; >> - java.io.OutputStream; >> - java.util.BitSet; >> - java.util.StringTokenizer; >> - java.util.Properties; >> - java.util.zip.DeflaterOutputStream; >> - java.util.zip.Deflater; >> - java.net.URL; > > Alright, was asking since I can help create one for you, but it seems that > others already have it covered > @TheShermanTanker , thank you for asking and helping. > > > it seems that others already have it covered > > In fact, I dont known. Yesterday, I posted 3 reports in the Java Bug Database > and I have no feedback at this moment. Well, in that case, wait no longer! Your issue ID is [8289741](https://bugs.openjdk.org/browse/JDK-8289741), just change your title to "8289741" and the automated tooling should take care of the rest and send this PR to the appropriate teams for review. All the best! - PR: https://git.openjdk.org/jdk/pull/9360
Re: RFR: JDK-8289741 : Remove unused imports from DTDBuilder.java
On Tue, 5 Jul 2022 01:53:33 GMT, Julian Waters wrote: >> This is tracked in JBS as >> >> [JDK-8289741](https://bugs.openjdk.org/browse/JDK-8289741) >> >>> **Remove unused imports from DTDBuilder.java** >> Some imports are no more used. >> >> - java.io.FileNotFoundException; >> - java.io.BufferedInputStream; >> - java.io.OutputStream; >> - java.util.BitSet; >> - java.util.StringTokenizer; >> - java.util.Properties; >> - java.util.zip.DeflaterOutputStream; >> - java.util.zip.Deflater; >> - java.net.URL; > > @scientificware Do you need an entry in the JBS for this PR? Hi @TheShermanTanker, Yes I need it. I'm waiting for the Issue Id. Regards > Alright, was asking since I can help create one for you, but it seems that > others already have it covered @TheShermanTanker , thank you for asking and helping. > it seems that others already have it covered In fact, I don't known. Yesterday, I posted 3 reports in the Java Bug Database and I have no feedback at this moment. - PR: https://git.openjdk.org/jdk/pull/9360
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v7]
On Tue, 5 Jul 2022 19:17:27 GMT, Lance Andersen wrote: >> Hi, >> >> Please review the following patch which will: >> >> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include >> the methods >> >> - public boolean exists(Path path, LinkOption... options) >> - public A readAttributesIfExists(Path >> path, Class type, LinkOption... options) >> >> >> This change allows for providers to provide optimizations when the file's >> attributes are not needed. >> >> Mach5 tiers 1 - 3 run clean with this change >> >> The CSR may be viewed at >> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) >> >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Add @BeforeMethod and rename fields I think all my comments have been addressed so I think this is good to go. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v7]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add @BeforeMethod and rename fields - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/240c38b8..74436951 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=06 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=05-06 Stats: 36 lines in 1 file changed: 9 ins; 3 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: JDK-8289730 : Deprecated code in src/java.base/share/classes/java/lang/ClassCastException.java
On Mon, 4 Jul 2022 06:57:12 GMT, ScientificWare wrote: > - Correct a deprecated code. > - Update Copyright. > - More wide question : How are you dealing with deprecated codes in snippets ? > ``` > @deprecated(since="9") > public Integer(int value) > ``` Looks fine. There is as of yet no systemic way of dealing with compiler warnings with formal or informal snippet samples, but JEP 413: "Code Snippets in Java API Documentation" is meant to enable that. - Marked as reviewed by darcy (Reviewer). PR: https://git.openjdk.org/jdk/pull/9359
RFR: 8288706: Unused parameter 'boolean newln' in method java.lang.VersionProps#print(boolean, boolean)
Hi all, This PR cleans up `VersionProps::print` removing the unused parameter `newln`. Mach5 tiers1-3 are currently running. Best, Lance - Commit messages: - Address unused parameter in VersionProps::print Changes: https://git.openjdk.org/jdk/pull/9382/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9382=00 Issue: https://bugs.openjdk.org/browse/JDK-8288706 Stats: 19 lines in 2 files changed: 1 ins; 13 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/9382.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9382/head:pull/9382 PR: https://git.openjdk.org/jdk/pull/9382
Re: RFR: JDK-8289730 : Deprecated code sample in java.lang.ClassCastException
On Tue, 5 Jul 2022 17:27:03 GMT, Joe Darcy wrote: >> - Correct a deprecated code. >> - Update Copyright. >> - More wide question : How are you dealing with deprecated codes in snippets >> ? >> ``` >> @deprecated(since="9") >> public Integer(int value) >> ``` > > Looks fine. > > There is as of yet no systemic way of dealing with compiler warnings with > formal or informal snippet samples, but JEP 413: "Code Snippets in Java API > Documentation" is meant to enable that. @jddarcy, please could you sponsor this PR ? - PR: https://git.openjdk.org/jdk/pull/9359
Re: [jdk19] RFR: 8289601: SegmentAllocator::allocateUtf8String(String str) should be clarified for strings containing \0
On Mon, 4 Jul 2022 13:01:34 GMT, Jorn Vernee wrote: > This PR updates the spec and implementation to throw an > `IllegalArgumentException` when an attempt is made to convert a Java string > containing null characters to a C string. > > Testing: local run of the `jdk_foreign` test suite. It's tempting to allow null/0 characters to pass through, truncating the string, given the rarity of such characters, but it could hide a nasty bug. If performance becomes a really important issue we can make intrinsic and vectorize. I think it is better to refer to the characters in the String, rather than refer to bytes of the UTF-8 encoding of that String e.g. throws if the string contains a null character (same for the exception message). We can explain in an API note why. - PR: https://git.openjdk.org/jdk19/pull/107
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
On Tue, 5 Jul 2022 10:56:45 GMT, Alan Bateman wrote: > > Unless you feel this is a must, I would prefer to keep the DataProviders. > > The benefit I see is the test code does not need to be duplicated per > > parameter, each test scenario can be run as an individual test so that you > > do not need extra plumbing to run each test scenario in the unlikely event > > of a failure. > > Okay, but there are a few other things to mention: > > One issue is the reset method is called at the end of each test. I think it > needs to be at the beginning of the method, moved to a finally block of a > try-finally, or maybe @BeforeMethod to reset before each test. The reason is > that one test failing will cause the tests that follow to fail too. Good catch, added `@BeforeMethod` > > The fields aren't constants so looks a bit strange (to me anyway) to be in > uppercase. If you rename them then I think the tests would be a bit more > readable. Ah, sometimes you see what you want. I must have had fields on my mind when I did that as I completely agree with you. Both of the above are addressed in the latest update to the PR. Thank you for the feedback. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: JDK-8289730 : Deprecated code in src/java.base/share/classes/java/lang/ClassCastException.java
On Tue, 5 Jul 2022 17:27:03 GMT, Joe Darcy wrote:
>> - Correct a deprecated code.
>> - Update Copyright.
>> - More wide question : How are you dealing with deprecated codes in snippets
>> ?
>> ```
>> @deprecated(since="9")
>> public Integer(int value)
>> ```
>
> Looks fine.
>
> There is as of yet no systemic way of dealing with compiler warnings with
> formal or informal snippet samples, but JEP 413: "Code Snippets in Java API
> Documentation" is meant to enable that.
@jddarcy Thanks.
In fact this Issue is related to JEP 413, I had a short informal exchange about
this with Jonathan Giles one year ago. It seems that Microsoft already
validates snippets in its AzureSDK :
https://twitter.com/JonathanGiles/status/1387550356524081154
How JEP 413 process will react with this kind of code ?
/**
* Thrown to indicate that the code has attempted to cast an object
* to a subclass of which it is not an instance. For example, the
* following code generates a {@code ClassCastException}:
*
* Object x = Integer.valueOf(0);
* System.out.println((String)x);
*
*
* @since 1.0
*/
In another word, is there a way to specifiy witch result the author expected.
-
PR: https://git.openjdk.org/jdk/pull/9359
Integrated: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider
On Wed, 22 Jun 2022 19:05:41 GMT, Lance Andersen wrote: > Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance This pull request has now been integrated. Changeset: d48694d0 Author:Lance Andersen URL: https://git.openjdk.org/jdk/commit/d48694d0f3865c1b205acdfa2e6c6d032a39959d Stats: 712 lines in 13 files changed: 537 ins; 135 del; 40 mod 8283335: Add exists and readAttributesIfExists methods to FileSystemProvider Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/9249
Re: [jdk19] RFR: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM [v3]
> This patch changes all VaList implementations to throw > `NoSuchElementException` when out of bounds reads occur on a VaList that is > created using the Java builder API. The docs are updated accordingly. > > For VaLists that are created from native addresses, we don't know their > bounds, so we can not throw exceptions in that case. > > Testing: `jdk_foreign` test suite on all platforms that implement VaList. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: hmtl -> html - Changes: - all: https://git.openjdk.org/jdk19/pull/91/files - new: https://git.openjdk.org/jdk19/pull/91/files/e7d7b367..ba132af5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk19=91=02 - incr: https://webrevs.openjdk.org/?repo=jdk19=91=01-02 Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk19/pull/91.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/91/head:pull/91 PR: https://git.openjdk.org/jdk19/pull/91
Re: [jdk19] RFR: 8289486: Improve XSLT XPath operators count efficiency
On Fri, 1 Jul 2022 17:04:10 GMT, Joe Wang wrote: > To improve efficiency, this patch moves the limit check to within the Lexer > and reports any overlimit situation as soon as it happens. > > Note the change in XPathParser: diff (and also webrevs) showed the whole > error-report block was changed, the actual change was only placing the block > to within the isOverLimit statement. > > Test: java.xml tests passed. Thanks for the explanation. Looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.org/jdk19/pull/101
Re: [jdk19] RFR: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM [v2]
> This patch changes all VaList implementations to throw > `NoSuchElementException` when out of bounds reads occur on a VaList that is > created using the Java builder API. The docs are updated accordingly. > > For VaLists that are created from native addresses, we don't know their > bounds, so we can not throw exceptions in that case. > > Testing: `jdk_foreign` test suite on all platforms that implement VaList. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.org/jdk19/pull/91/files - new: https://git.openjdk.org/jdk19/pull/91/files/e1c757c6..e7d7b367 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk19=91=01 - incr: https://webrevs.openjdk.org/?repo=jdk19=91=00-01 Stats: 19 lines in 2 files changed: 2 ins; 0 del; 17 mod Patch: https://git.openjdk.org/jdk19/pull/91.diff Fetch: git fetch https://git.openjdk.org/jdk19 pull/91/head:pull/91 PR: https://git.openjdk.org/jdk19/pull/91
Re: [jdk19] RFR: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM [v2]
On Tue, 5 Jul 2022 15:14:20 GMT, Jorn Vernee wrote:
>> This seems a bit too much.
>>
>> The class javadoc further up already describes a va list as "a stateful
>> cursor used to iterate over a set of arguments". If that description is
>> insufficient, I think it should be amended at that point.
>>
>> Also, I don't think we have to go into describing how va lists returned by
>> different factories are implemented (in fact, I think we should avoid that
>> in order not to make accidental false promises when things change). It seems
>> like noise next to the safety concerns (if someone really wants to know how
>> the specified semantics are implemented, they can look at the code, or ask
>> on the mailing list).
>>
>> I'd suggest something simpler like this:
>>
>> Suggestion:
>>
>> * It is possible for clients to access elements outside the spatial bounds
>> of a variable argument list. Variable argument list
>> * implementations will try to detect out-of-bounds reads on a best-effort
>> basis.
>> *
>> * Whether this detection succeeds depends on the factory method used to
>> create the variable argument list:
>> *
>> * Variable argument lists created safely, using {@link
>> #make(Consumer, MemorySession)} are capable of detecting out-of-bounds
>> reads;
>> * Variable argument lists created unsafely, using {@link
>> #ofAddress(MemoryAddress, MemorySession)} are not capable of detecting
>> out-of-bounds reads
>> *
>> *
>>
>>
>> I'm also fine with changing the section title to "Safety considerations".
>
> i.e. I'd like to just specify the behavior, and avoid explaining why we have
> that behavior.
I've updated the PR with this suggestion for now. Please let me know if it
looks OK to you as well.
-
PR: https://git.openjdk.org/jdk19/pull/91
Re: RFR: 8288984: Simplification in Shutdown.exit [v3]
On Mon, 4 Jul 2022 16:17:27 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > iterate on wording src/java.base/share/classes/java/lang/Runtime.java line 88: > 86: * Shutdown is serialized such that only one invocation will run > 87: * shutdown hooks and terminate the VM with the given status code. > That > 88: * invocation may be initiated via platform specific signal handlers. > All Why are we mentioning signal handlers here? How is that relevant? - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288838: jpackage: file association additional arguments [v4]
On Mon, 4 Jul 2022 23:38:08 GMT, Alex Kasko wrote: >> jpackage implementation of file association on Windows currently passes a >> selected filename as an only argument to associated executable. >> >> It is proposed to introduce additional option in file association property >> file to allow optionally support additional arguments using `%*` batch >> wildcard. >> >> Note, current implementation, while fully functional, is only a **DRAFT** >> one, it is not ready for integration in this form. I would appreciate any >> guidance on the following points: >> >> - option naming inside a properties file, currently `pass-all-args` is used >> - option naming in a bundler parameter implementation, it is not clear if >> it should introduce a new group of "file association windows specific >> options" next to the existing "file association mac specific options" group >> - test organization to cover the new option: currently it is included >> inside `FileAssociationTest` and piggybacks on the existing (and unrelated) >> `includeDescription` parameter; it is not clear whether it should be done in >> a separate test and whether to include runs for every parameter combination >> - test run implementation: currently arguments are checked when a file with >> associated extension is invoked from command line; it is not clear whether >> it would be more appropriate instead to create a desktop shortcut with the >> same command as a target and to invoke it with `java.awt.Desktop` >> >> Also please note, that full install/uninstall run is currently enabled in >> `FileAssociationTest`, it is intended to be used only in a draft code during >> the development and to be removed (to use the same "install or unpack" logic >> as other tests) in a final version. >> >> Testing: >> >> - [x] test to cover new logic is included >> - [x] ran jtreg:jdk/tools/jpackage with no new failures > > Alex Kasko has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains three commits: > > - enable passing args, add test coverage > - Added input params validation > - Proposed test changes to make FA testing of jpackage more flexible I've rebased the changes on top of PR 9331 and rebased the result on top of recent master (to have JDK-8288961 to be able to run MSI test). I think the patch if ready for review now. Note on non-ASCII arguments support: I've found that Windows system locale needs to be changed to support specific language. Checked manually that such args are passed successfully with both command-line and LNK shortcut arguments. Left a note on this in test. - PR: https://git.openjdk.org/jdk/pull/9224
Re: RFR: 8288838: jpackage: file association additional arguments [v4]
> jpackage implementation of file association on Windows currently passes a > selected filename as an only argument to associated executable. > > It is proposed to introduce additional option in file association property > file to allow optionally support additional arguments using `%*` batch > wildcard. > > Note, current implementation, while fully functional, is only a **DRAFT** > one, it is not ready for integration in this form. I would appreciate any > guidance on the following points: > > - option naming inside a properties file, currently `pass-all-args` is used > - option naming in a bundler parameter implementation, it is not clear if it > should introduce a new group of "file association windows specific options" > next to the existing "file association mac specific options" group > - test organization to cover the new option: currently it is included inside > `FileAssociationTest` and piggybacks on the existing (and unrelated) > `includeDescription` parameter; it is not clear whether it should be done in > a separate test and whether to include runs for every parameter combination > - test run implementation: currently arguments are checked when a file with > associated extension is invoked from command line; it is not clear whether it > would be more appropriate instead to create a desktop shortcut with the same > command as a target and to invoke it with `java.awt.Desktop` > > Also please note, that full install/uninstall run is currently enabled in > `FileAssociationTest`, it is intended to be used only in a draft code during > the development and to be removed (to use the same "install or unpack" logic > as other tests) in a final version. > > Testing: > > - [x] test to cover new logic is included > - [x] ran jtreg:jdk/tools/jpackage with no new failures Alex Kasko has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - enable passing args, add test coverage - Added input params validation - Proposed test changes to make FA testing of jpackage more flexible - Changes: https://git.openjdk.org/jdk/pull/9224/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9224=03 Stats: 194 lines in 5 files changed: 167 ins; 9 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/9224.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9224/head:pull/9224 PR: https://git.openjdk.org/jdk/pull/9224
Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]
On Thu, 30 Jun 2022 12:08:19 GMT, Сергей Цыпанов wrote: >> If there are two threads calling `Executable.hasRealParameterData()` under >> race and the first one writes into volatile `Executable.parameters` field >> (doing _releasing store_) and the second thread reads non-null value from >> the same field (doing acquiring read) then it must read exactly the same >> value written into `hasRealParameterData` within `privateGetParameters()`. >> The reason for this is that we assign `hasRealParameterData` before >> _releasing store_. >> >> In the opposite case (_acquiring read_ reads null) the second thread writes >> the value itself and returns it from the method so there is no change in >> behavior. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8288327: Inline privateGetParameters() This looks good to me. Thanks for doing that. - Marked as reviewed by plevart (Reviewer). PR: https://git.openjdk.org/jdk/pull/9143
Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]
On Mon, 4 Jul 2022 10:06:12 GMT, Сергей Цыпанов wrote:
>> A more realistic use case would be something in the lines of checking
>> whether the method is an expected one:
>>
>>
>> @Benchmark
>> public boolean isFoo() {
>> return matches(method, "foo", int.class, String.class);
>> }
>>
>> private boolean matches(Method method, String name, Class
>> ...parameterTypes) {
>> if (method.getName().equals(name) &&
>> method.getParameterCount() == parameterTypes.length) {
>> var params = method.getParameters();
>> for (int i = 0; i < parameterTypes.length; i++) {
>> if (params[i].getType() != parameterTypes[i]) {
>> return false;
>> }
>> }
>> return true;
>> } else {
>> return false;
>> }
>> }
>
> Without `@Stable` your benchmark yields
>
> BenchmarkMode Cnt Score Error Units
> AccessParamsBenchmark.isFoo avgt 40 1,110 ± 0,062 ns/op
>
> and with `@Stable` it yields
>
> BenchmarkMode Cnt Score Error Units
> AccessParamsBenchmark.isFoo avgt 40 0,836 ± 0,015 ns/op
>
> Java 18:
>
> BenchmarkMode Cnt Score Error Units
> AccessParamsBenchmark.isFoo avgt 40 6,105 ± 0,107 ns/op
So, it looks like @Stable should stay.
-
PR: https://git.openjdk.org/jdk/pull/9143
Re: RFR: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time [v2]
On Sun, 3 Jul 2022 19:47:16 GMT, Attila Szegedi wrote: >> Andrey Turbanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8288723: Avoid redundant ConcurrentHashMap.get call in java.time >> use computeIfAbsent where lambda could be short and static > > src/java.base/share/classes/java/time/ZoneOffset.java line 435: > >> 433: result = prev; >> 434: } >> 435: ID_CACHE.putIfAbsent(result.getId(), result); > > Feel free to ignore this one, but you could still have this be a > `computeIfAbsent` and put the ID_CACHE.putIfAbsent part in the lambda. Yeah, > there'll be more work done inside of `computeIfAbsent` but I think it's an > acceptable tradeoff for a more comprehensible code. Implemented - PR: https://git.openjdk.org/jdk/pull/9208
Integrated: 8289484: Cleanup unnecessary null comparison before instanceof check in java.rmi
On Wed, 29 Jun 2022 21:11:09 GMT, Andrey Turbanov wrote:
> Update code checks both non-null and instance of a class in jdk.hotspot.agent
> module classes.
> The checks and explicit casts could also be replaced with pattern matching
> for the instanceof operator.
>
> For example, the following code:
>
> if ((obj != null) && (obj instanceof TCPEndpoint)) {
> TCPEndpoint ep = (TCPEndpoint) obj;
> if (port != ep.port || !host.equals(ep.host))
>
> Can be simplified to:
>
> if (obj instanceof TCPEndpoint ep) {
> if (port != ep.port || !host.equals(ep.host))
>
>
> See similar cleanup in java.base -
> [JDK-8258422](https://bugs.openjdk.java.net/browse/JDK-8258422)
This pull request has now been integrated.
Changeset: df063f7d
Author:Andrey Turbanov
URL:
https://git.openjdk.org/jdk/commit/df063f7db18a40ea7325fe608b3206a6dff812c1
Stats: 9 lines in 3 files changed: 0 ins; 4 del; 5 mod
8289484: Cleanup unnecessary null comparison before instanceof check in java.rmi
Reviewed-by: jpai, attila
-
PR: https://git.openjdk.org/jdk/pull/9332
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
On Mon, 4 Jul 2022 19:30:40 GMT, Lance Andersen wrote: >> Hi, >> >> Please review the following patch which will: >> >> - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include >> the methods >> >> - public boolean exists(Path path, LinkOption... options) >> - public A readAttributesIfExists(Path >> path, Class type, LinkOption... options) >> >> >> This change allows for providers to provide optimizations when the file's >> attributes are not needed. >> >> Mach5 tiers 1 - 3 run clean with this change >> >> The CSR may be viewed at >> [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) >> >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Address whitespace issue The updated TestDelegation test is looking a bit better now but I think it would be simplified a lot more by getting rid of the data providers, just aren't needed in this test. - PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v6]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Address whitespace issue - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/a51010aa..240c38b8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=05 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8283335 : Add exists and readAttributesIfExists methods to FileSystemProvider [v5]
> Hi, > > Please review the following patch which will: > > - Enhance the java.nio.file.spi.FileSystemProvider abstract class to include > the methods > > - public boolean exists(Path path, LinkOption... options) > - public A readAttributesIfExists(Path > path, Class type, LinkOption... options) > > > This change allows for providers to provide optimizations when the file's > attributes are not needed. > > Mach5 tiers 1 - 3 run clean with this change > > The CSR may be viewed at > [JDK-8283336](https://bugs.openjdk.org/browse/JDK-8283336) > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Separated test cases out and added benchmark - Changes: - all: https://git.openjdk.org/jdk/pull/9249/files - new: https://git.openjdk.org/jdk/pull/9249/files/63b97ce3..a51010aa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=9249=04 - incr: https://webrevs.openjdk.org/?repo=jdk=9249=03-04 Stats: 212 lines in 2 files changed: 198 ins; 3 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/9249.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9249/head:pull/9249 PR: https://git.openjdk.org/jdk/pull/9249
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 16:31:22 GMT, Alan Bateman wrote: >> I reworded with the suggested text in mind. I also added another sentence >> referencing native signal handlers, which may also initiate shutdown. I >> think this clarification is important so that it is clear the status code of >> all invocations from Java may still be ignored. See >> [2253259](https://github.com/openjdk/jdk/pull/9351/commits/2253259c82b13e7b2186429a80cc42497235b035). > > I think the wording in the latest commit (9d972b) is problematic. One reason > is that you've changed it to "will run shutdown hooks" but it doesn't run the > hooks, it starts them, and so conflicts with the previous paragraph. I also > think "That invocation may be initiated via platform specific signal > handlers" is confusing here as there is no notion of "signal handler" to > reference. There may be scope for text elsewhere, maybe with an implNote for > signals such as HUP, but I don't think this is the PR for this. So I think it > better to try the wording that David suggested and see if it needs any > improvement. YAO (Yet Another Opinion)! But I think we're actually converging. David's wording: >Invocations of this method are serialized such that only one invocation will >actually proceed with the shutdown sequence and terminate the VM with the >given status code. ... gives the impression that an invocation of this method WILL terminate the VM with the given status code - which is not actually true, given the potential for signals. This is already alluded to in `Runtime::addShutdownHook`. I agree that this could be resolved separately, but we should _allow_ for it. The original wording: > If this method is invoked after all shutdown hooks have already been run ... ... seems quite clever (and allows for signals). Maybe we could: > If this method is invoked after shutdown hooks have already been started it > will block indefinitely. If this method is invoked from a shutdown hook the > system will deadlock. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Mon, 4 Jul 2022 16:13:10 GMT, Ryan Ernst wrote: >> David's refinement looks good. The sentence on deadlock is accurate as >> shutdown hooks run on other threads, not the thread calling System.ext. > > I reworded with the suggested text in mind. I also added another sentence > referencing native signal handlers, which may also initiate shutdown. I think > this clarification is important so that it is clear the status code of all > invocations from Java may still be ignored. See > [2253259](https://github.com/openjdk/jdk/pull/9351/commits/2253259c82b13e7b2186429a80cc42497235b035). I think the wording in the latest commit (9d972b) is problematic. One reason is that you've changed it to "will run shutdown hooks" but it doesn't run the hooks, it starts them, and so conflicts with the previous paragraph. I also think "That invocation may be initiated via platform specific signal handlers" is confusing here as there is no notion of "signal handler" to reference. There may be scope for text elsewhere, maybe with an implNote for signals such as HUP, but I don't think this is the PR for this. So I think it better to try the wording that David suggested and see if it needs any improvement. - PR: https://git.openjdk.org/jdk/pull/9351
