Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 16:28:14 GMT, Maxim Kartashev
wrote:
>> src/hotspot/os/windows/os_windows.cpp line 1541:
>>
>>> 1539: void * os::dll_load(const char *utf8_name, char *ebuf, int ebuflen) {
>>> 1540: LPWSTR utf16_name = NULL;
>>> 1541: errno_t err = convert_UTF8_to_UTF16(utf8_name, _name);
>>
>> Do you have any figures on the cost of this additional conversion in
>> relation to startup time?
>>
>> I'm already concerned to see that we have to perform each conversion twice
>> via MultiByteToWideChar/WideCharToMultiByte, once to get the size and then
>> to actually get the characters! This seems potentially very inefficient.
>
> I measured time spend converting the library path name relative to the
> overall time of a (successful) `os::dll_load()` call. It varies between a
> fraction of a percent to ~3% (see below for the actual data from a `release`
> build). I'll defer to your expertise wrt to these numbers.
>
> What can be done here (without changing os::dll_load() API) is to have a
> "fast path" for ascii-only path names, in which case no conversion is
> necessary, and a "slow path" for all the rest. This will complicate things a
> bit, but not by much, I believe. I am certainly willing to give that a try.
> Let me know what do you think.
>
>
>
> ./build/windows-x86_64-server-release/images/jdk/bin/java -jar
> ./build/windows-x86_64-server-fastdebug/images/jdk/demo/jfc/SwingSet2/SwingSet2.jar
> 0.050% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\jimage.dll
> 2.273% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.dll
> 0.177% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\net.dll
> 0.541% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\nio.dll
> 0.359% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\zip.dll
> 3.186% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\jimage.dll
> 0.075% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\awt.dll
> 0.232% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\freetype.dll
> 0.136% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\fontmanager.dll
> 0.170% for
> C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\javajpeg.dll
The core-libs folks have the experience/expertise with these character encoding
issues so I will defer to them. But someone should run this through our startup
benchmarks to see if it makes a difference.
Thanks,
David
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 28 May 2021 03:12:35 GMT, Phil Race wrote: >> There *is* a tiny refactoring here: a new variable `s2` is introduced so the >> 2nd `doPrivileged` call on line 636 is now also in a declaration statement >> (for `s2`) and therefore annotatable. Without this I cannot add the >> annotation on line 635. > > Ok. But I will quote you > "This happens when a deprecated method is called inside a static block. The > annotation can only be added to a declaration and here it must be the whole > class" > > So the way you explained this before made it sound like any time there was > any SM API usage in a static block, the entire class needed to be annotated. > > Why has the explanation changed ? I should have been more precise. An annotation can only be added on a declaration, whether it's a variable, a method, or a class. Static block is not a declaration and the only one covers it is the class. But then if it's on a local variable declaration inside a static block, we certainly can annotate on that variable. - PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 28 May 2021 02:50:55 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Component.java line 630:
>>
>>> 628: }
>>> 629:
>>> 630: @SuppressWarnings("removal")
>>
>> I'm confused. I thought the reason this wasn't done in the JEP
>> implementation PR is because of refactoring
>> that was needed because of the usage in this static block and you could not
>> apply the annotation here.
>> Yet it seems you are doing exactly what was supposed to be impossible with
>> no refactoring.
>> Can you explain ?
>
> There *is* a tiny refactoring here: a new variable `s2` is introduced so the
> 2nd `doPrivileged` call on line 636 is now also in a declaration statement
> (for `s2`) and therefore annotatable. Without this I cannot add the
> annotation on line 635.
Ok. But I will quote you
"This happens when a deprecated method is called inside a static block. The
annotation can only be added to a declaration and here it must be the whole
class"
So the way you explained this before made it sound like any time there was any
SM API usage in a static block, the entire class needed to be annotated.
Why has the explanation changed ?
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Thu, 27 May 2021 17:42:56 GMT, Phil Race wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update FtpClient.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 630:
>
>> 628: }
>> 629:
>> 630: @SuppressWarnings("removal")
>
> I'm confused. I thought the reason this wasn't done in the JEP implementation
> PR is because of refactoring
> that was needed because of the usage in this static block and you could not
> apply the annotation here.
> Yet it seems you are doing exactly what was supposed to be impossible with no
> refactoring.
> Can you explain ?
There *is* a tiny refactoring here: a new variable `s2` is introduced so the
2nd `doPrivileged` call on line 636 is now also in a declaration statement (for
`s2`) and therefore annotatable. Without this I cannot add the annotation on
line 635.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes
On Thu, 27 May 2021 22:05:55 GMT, Leonid Mesnik wrote:
> EFH is improved to process cores and get mixed stack traces with jhsdb and
> native stack traces with gdb/lldb. It might be useful because hs_err doesn't
> contain info about all threads, sometimes it is even not generated.
@lmesnik , how has this been tested?
test/failure_handler/Makefile line 41:
> 39: CONF_DIR = src/share/conf
> 40:
> 41: JAVA_RELEASE = 7
could you please update `DEPENDENCES` section in `test/failure_handler/README`?
test/failure_handler/src/share/classes/jdk/test/failurehandler/ToolKit.java
line 69:
> 67: }
> 68: } catch (IOException ioe) {
> 69: // just silently skip gzipped core processing
we don't silently ignore exceptions in `failure_handler`, we tend to print them
so we at least have some echoes of what happened.
test/failure_handler/src/share/classes/jdk/test/failurehandler/ToolKit.java
line 71:
> 69: // just silently skip gzipped core processing
> 70: }
> 71: unpackedCore.toFile().delete();
Suggestion:
Paths.delete(unpackedCore);
``` ?
test/failure_handler/src/share/classes/jdk/test/failurehandler/Utils.java line
73:
> 71: InputStream stream =
> Utils.class.getResourceAsStream(resourceName);
> 72:
> 73: // EFH_CONF_DIR might re-defined to load custom configs for
> development purposes
this also seems to be unrelated to the subject and does require documentation
in `test/failure_handler/README`.
test/failure_handler/src/share/classes/jdk/test/failurehandler/action/PatternAction.java
line 63:
> 61: }
> 62: for (int i = 0, n = args.length; i < n; ++i) {
> 63: args[i] = args[i].replace("%java",
> helper.findApp("java").getAbsolutePath());
are we sure that `java` from `PATH` (which is what `findApp` returns) is the
right `java`?
test/failure_handler/src/share/conf/common.properties line 38:
> 36: jcmd.vm.system_properties \
> 37: jcmd.gc.heap_info jcmd.gc.class_histogram jcmd.gc.finalizer_info \
> 38: jcmd.vm.info \
this seems to be unrelated to the RFE you are working on.
test/failure_handler/src/share/conf/common.properties line 67:
> 65: cores=jhsdb.jstack
> 66: jhsdb.jstack.app=jhsdb
> 67: jhsdb.jstack.args=jstack --mixed --core %p --exe %java
strictly speaking, we can't guarantee that the executable is always `bin/java`,
but it's the most common and the most interesting case, so this assumption is
good enough for our pourposes.
could you please add a comment explaining this so the further reading won't be
puzzled?
test/failure_handler/src/share/conf/mac.properties line 68:
> 66: native.jhsdb.app=bash
> 67: native.jhsdb.jstack.delimiter=\0
> 68: native.jhsdb.jstack.args=-c\0DevToolsSecurity --status | grep -q enabled
> && jhsdb jstack --mixed --pid %p
AFAICS `jhsdb` does check "Developer mode" on macos before trying to attach and
will just report 'lack of privileges' (as opposed to hanging with a modal
window asking for permission), so you can move `jshdb.jstack` to
`common.properties`.
-
Changes requested by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4234
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
On Thu, 27 May 2021 20:16:25 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - move one annotation to new method > - merge from master, two files removed, one needs merge > - keep only one systemProperty tag > - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java > - feedback from Sean, Phil and Alan > - add supresswarnings annotations automatically > - manual change before automatic annotating > - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal Two files were removed by JEP 403 and JEP 407, respectively. One method in `XMLSchemaFactory.java` got [its own](https://github.com/openjdk/jdk/commit/8c4719a58834dddcea39d69b199abf1aabf780e2#diff-593a224979eaff03e2a3df1863fcaf865364a31a2212cc0d1fe67a8458057857R429) `@SuppressWarnings` and have to be merged with the one here. Another file `ResourceBundle.java` had a portion of a method extracted into a [new method](https://github.com/openjdk/jdk/commit/a4c46e1e4f4f2f05c8002b2af683a390fc46b424#diff-59caf1a68085064b4b3eb4f6e33e440bb85ea93719f34660970e2d4eaf8ce469R3175) and the annotation must be moved there. - PR: https://git.openjdk.java.net/jdk/pull/4073
Integrated: 8265029: Preserve SIZED characteristics on slice operations (skip, limit)
On Sat, 10 Apr 2021 06:30:33 GMT, Tagir F. Valeev wrote: > With the introduction of `toList()`, preserving the SIZED characteristics in > more cases becomes more important. This patch preserves SIZED on `skip()` and > `limit()` operations, so now every combination of > `map/mapToX/boxed/asXyzStream/skip/limit/sorted` preserves size, and > `toList()`, `toArray()` and `count()` may benefit from this. E. g., > `LongStream.range(0, 10_000_000_000L).skip(1).count()` returns result > instantly with this patch. > > Some microbenchmarks added that confirm the reduced memory allocation in > `toList()` and `toArray()` cases. Before patch: > > ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 > thrpt 10 40235,534 ± 0,984B/op > ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 > thrpt 10 106431,101 ± 0,198B/op > ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 > thrpt 10 106544,977 ± 1,983B/op > value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 > thrpt 10 40121,878 ± 0,247B/op > value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 > thrpt 10 106317,693 ± 1,083B/op > value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 > thrpt 10 106430,954 ± 0,136B/op > > > After patch: > > ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 > thrpt 10 40235,648 ± 1,354B/op > ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 > thrpt 10 40355,784 ± 1,288B/op > ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 > thrpt 10 40476,032 ± 2,855B/op > value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 > thrpt 10 40121,830 ± 0,308B/op > value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 > thrpt 10 40242,554 ± 0,443B/op > value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 > thrpt 10 40363,674 ± 1,576B/op > > > Time improvements are less exciting. It's likely that inlining and > vectorizing dominate in these tests over array allocations and unnecessary > copying. Still, I notice a significant improvement in SliceToArray.seq_limit > case (2x) and mild improvement (+12..16%) in other slice tests. No > significant change in parallel execution time, though its performance is much > less stable and I didn't run enough tests. > > Before patch: > > Benchmark (size) Mode Cnt Score Error > Units > ref.SliceToList.par_baseline 1 thrpt 30 14876,723 ± 99,770 > ops/s > ref.SliceToList.par_limit 1 thrpt 30 14856,841 ± 215,089 > ops/s > ref.SliceToList.par_skipLimit 1 thrpt 30 9555,818 ± 991,335 > ops/s > ref.SliceToList.seq_baseline 1 thrpt 30 23732,290 ± 444,162 > ops/s > ref.SliceToList.seq_limit 1 thrpt 30 14894,040 ± 176,496 > ops/s > ref.SliceToList.seq_skipLimit 1 thrpt 30 10646,929 ± 36,469 > ops/s > value.SliceToArray.par_baseline1 thrpt 30 25093,141 ± 376,402 > ops/s > value.SliceToArray.par_limit 1 thrpt 30 24798,889 ± 760,762 > ops/s > value.SliceToArray.par_skipLimit 1 thrpt 30 16456,310 ± 926,882 > ops/s > value.SliceToArray.seq_baseline1 thrpt 30 69669,787 ± 494,562 > ops/s > value.SliceToArray.seq_limit 1 thrpt 30 21097,081 ± 117,338 > ops/s > value.SliceToArray.seq_skipLimit 1 thrpt 30 15522,871 ± 112,557 > ops/s > > > After patch: > > Benchmark (size) Mode Cnt Score Error > Units > ref.SliceToList.par_baseline 1 thrpt 30 14793,373 ± 64,905 > ops/s > ref.SliceToList.par_limit 1 thrpt 30 13301,024 ± 1300,431 > ops/s > ref.SliceToList.par_skipLimit 1 thrpt 30 11131,698 ± 1769,932 > ops/s > ref.SliceToList.seq_baseline 1 thrpt 30 24101,048 ± 263,528 > ops/s > ref.SliceToList.seq_limit 1 thrpt 30 16872,168 ± 76,696 > ops/s > ref.SliceToList.seq_skipLimit 1 thrpt 30 11953,253 ± 105,231 > ops/s > value.SliceToArray.par_baseline1 thrpt 30 25442,442 ± 455,554 > ops/s > value.SliceToArray.par_limit 1 thrpt 30 23111,730 ± 2246,086 > ops/s > value.SliceToArray.par_skipLimit 1 thrpt 30 17980,750 ± 2329,077 > ops/s > value.SliceToArray.seq_baseline1 thrpt 30 66512,898 ± 1001,042 > ops/s > value.SliceToArray.seq_limit 1 thrpt 30 41792,549 ± 1085,547 > ops/s > value.SliceToArray.seq_skipLimit 1 thrpt 30 18007,613 ± 141,716 > ops/s > > > I also modernized SliceOps a little bit, using switch expression (with no > explicit default!) and diamonds on anonymous
Re: RFR: 8265029: Preserve SIZED characteristics on slice operations (skip, limit) [v9]
> With the introduction of `toList()`, preserving the SIZED characteristics in > more cases becomes more important. This patch preserves SIZED on `skip()` and > `limit()` operations, so now every combination of > `map/mapToX/boxed/asXyzStream/skip/limit/sorted` preserves size, and > `toList()`, `toArray()` and `count()` may benefit from this. E. g., > `LongStream.range(0, 10_000_000_000L).skip(1).count()` returns result > instantly with this patch. > > Some microbenchmarks added that confirm the reduced memory allocation in > `toList()` and `toArray()` cases. Before patch: > > ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 > thrpt 10 40235,534 ± 0,984B/op > ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 > thrpt 10 106431,101 ± 0,198B/op > ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 > thrpt 10 106544,977 ± 1,983B/op > value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 > thrpt 10 40121,878 ± 0,247B/op > value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 > thrpt 10 106317,693 ± 1,083B/op > value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 > thrpt 10 106430,954 ± 0,136B/op > > > After patch: > > ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 > thrpt 10 40235,648 ± 1,354B/op > ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 > thrpt 10 40355,784 ± 1,288B/op > ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 > thrpt 10 40476,032 ± 2,855B/op > value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 > thrpt 10 40121,830 ± 0,308B/op > value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 > thrpt 10 40242,554 ± 0,443B/op > value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 > thrpt 10 40363,674 ± 1,576B/op > > > Time improvements are less exciting. It's likely that inlining and > vectorizing dominate in these tests over array allocations and unnecessary > copying. Still, I notice a significant improvement in SliceToArray.seq_limit > case (2x) and mild improvement (+12..16%) in other slice tests. No > significant change in parallel execution time, though its performance is much > less stable and I didn't run enough tests. > > Before patch: > > Benchmark (size) Mode Cnt Score Error > Units > ref.SliceToList.par_baseline 1 thrpt 30 14876,723 ± 99,770 > ops/s > ref.SliceToList.par_limit 1 thrpt 30 14856,841 ± 215,089 > ops/s > ref.SliceToList.par_skipLimit 1 thrpt 30 9555,818 ± 991,335 > ops/s > ref.SliceToList.seq_baseline 1 thrpt 30 23732,290 ± 444,162 > ops/s > ref.SliceToList.seq_limit 1 thrpt 30 14894,040 ± 176,496 > ops/s > ref.SliceToList.seq_skipLimit 1 thrpt 30 10646,929 ± 36,469 > ops/s > value.SliceToArray.par_baseline1 thrpt 30 25093,141 ± 376,402 > ops/s > value.SliceToArray.par_limit 1 thrpt 30 24798,889 ± 760,762 > ops/s > value.SliceToArray.par_skipLimit 1 thrpt 30 16456,310 ± 926,882 > ops/s > value.SliceToArray.seq_baseline1 thrpt 30 69669,787 ± 494,562 > ops/s > value.SliceToArray.seq_limit 1 thrpt 30 21097,081 ± 117,338 > ops/s > value.SliceToArray.seq_skipLimit 1 thrpt 30 15522,871 ± 112,557 > ops/s > > > After patch: > > Benchmark (size) Mode Cnt Score Error > Units > ref.SliceToList.par_baseline 1 thrpt 30 14793,373 ± 64,905 > ops/s > ref.SliceToList.par_limit 1 thrpt 30 13301,024 ± 1300,431 > ops/s > ref.SliceToList.par_skipLimit 1 thrpt 30 11131,698 ± 1769,932 > ops/s > ref.SliceToList.seq_baseline 1 thrpt 30 24101,048 ± 263,528 > ops/s > ref.SliceToList.seq_limit 1 thrpt 30 16872,168 ± 76,696 > ops/s > ref.SliceToList.seq_skipLimit 1 thrpt 30 11953,253 ± 105,231 > ops/s > value.SliceToArray.par_baseline1 thrpt 30 25442,442 ± 455,554 > ops/s > value.SliceToArray.par_limit 1 thrpt 30 23111,730 ± 2246,086 > ops/s > value.SliceToArray.par_skipLimit 1 thrpt 30 17980,750 ± 2329,077 > ops/s > value.SliceToArray.seq_baseline1 thrpt 30 66512,898 ± 1001,042 > ops/s > value.SliceToArray.seq_limit 1 thrpt 30 41792,549 ± 1085,547 > ops/s > value.SliceToArray.seq_skipLimit 1 thrpt 30 18007,613 ± 141,716 > ops/s > > > I also modernized SliceOps a little bit, using switch expression (with no > explicit default!) and diamonds on anonymous classes. Tagir F. Valeev has updated the pull request
Re: RFR: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes
On Thu, 27 May 2021 23:38:48 GMT, David Holmes wrote: > Hi Leonid, > > What is EFH? Please update the bug and PR to explain. > > Thanks, > David Updated summary to "Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes". - PR: https://git.openjdk.java.net/jdk/pull/4234
Re: Proposal for new interface: TimeSource
On 5/27/2021 5:03 PM, Stephen Colebourne wrote: Hi all, Is there anything I need to do to progress the CSR and/or PR? The CSR is in Provisional state. To request the second phase of CSR review, the assignee can Finalize the CSR; for more details see https://wiki.openjdk.java.net/display/csr/Main HTH, -Joe
Re: Proposal for new interface: TimeSource
Hi all, Is there anything I need to do to progress the CSR and/or PR? thanks Stephen On Thu, 13 May 2021 at 22:05, Stephen Colebourne wrote: > > On Wed, 12 May 2021 at 18:41, Roger Riggs wrote: > > Will you be posting a PR for the implementation? > > It is frequently helpful to evaluate the CSR and the implementation > > concurrently. > > PR: https://github.com/openjdk/jdk/pull/4016 > Issue: https://bugs.openjdk.java.net/browse/JDK-8266846 > CSR: https://bugs.openjdk.java.net/browse/JDK-8266847 > > The PR takes a middle ground approach to the implementation. > > It is not practical to remove the existing package-scoped Clock > implementation classes (SystemClock, TickClock, FixedClock, > OffsetClock) despite the fact that these would be better expressed as > classes that only implement `InstantSource`. However, given that > "system" is the 99%+ use case, I do believe it is worth adding a > dedicated `SystemInstantSource` class, as per the PR. > > To achieve this I moved the actual logic using > `VM.getNanoAdjustment()` into a static method which can then be called > directly from three places - Instant.now(), SystemClock and > SystemInstantSource. Previously, every instance of SystemClock > performed the VM/offset calculations separately. The new logic > performs them once based on a single shared static variable. I have no > reason to believe this changes the memory model or performance, but I > must flag it up for reviewers. > > When initially discussing the proposal, I planned to add a new static > method `Clock.of(InstantSource, ZoneId)`. When implementing the change > I found that the method was adding no value as the instance method > `InstantSource.withZone(ZoneId)` achieves the same outcome, so I > omitted it. > > The Mac test failure appears to be unconnected to the change. > > Thanks for any and all reviews! > Stephen
Re: RFR: 8267893: Improve EFH do get native/mixed stack traces for cores and live processes
On Thu, 27 May 2021 22:05:55 GMT, Leonid Mesnik wrote: > EFH is improved to process cores and get mixed stack traces with jhsdb and > native stack traces with gdb/lldb. It might be useful because hs_err doesn't > contain info about all threads, sometimes it is even not generated. Hi Leonid, What is EFH? Please update the bug and PR to explain. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4234
Re: RFR: 8267529: StringJoiner can create a String that breaks String::equals [v2]
On Thu, 27 May 2021 20:25:35 GMT, Claes Redestad wrote: >> This patch fixes a regression caused by >> https://bugs.openjdk.java.net/browse/JDK-8265237 where the result of >> String.join always get a UTF-16 coder if the delimiter is UTF-16, even when >> no delimiter is emitted. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add join of zero strings, clarify comment Thanks for reviewing, Naoto - PR: https://git.openjdk.java.net/jdk/pull/4228
Integrated: 8267529: StringJoiner can create a String that breaks String::equals
On Thu, 27 May 2021 17:38:42 GMT, Claes Redestad wrote: > This patch fixes a regression caused by > https://bugs.openjdk.java.net/browse/JDK-8265237 where the result of > String.join always get a UTF-16 coder if the delimiter is UTF-16, even when > no delimiter is emitted. This pull request has now been integrated. Changeset: 95b1fa7a Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/95b1fa7a88ec3c017734c9d0a6b6b6117f74a610 Stats: 20 lines in 2 files changed: 16 ins; 0 del; 4 mod 8267529: StringJoiner can create a String that breaks String::equals Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/4228
RFR: 8267893: Improve EFH do get native/mixed stack traces for cores and live processes
EFH is improved to process cores and get mixed stack traces with jhsdb and native stack traces with gdb/lldb. It might be useful because hs_err doesn't contain info about all threads, sometimes it is even not generated. - Commit messages: - Merge branch 'master' of https://github.com/openjdk/jdk into efh - EFH improvements Changes: https://git.openjdk.java.net/jdk/pull/4234/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4234=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267893 Stats: 188 lines in 13 files changed: 158 ins; 6 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/4234.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4234/head:pull/4234 PR: https://git.openjdk.java.net/jdk/pull/4234
RFR: 8267569: java.io.File.equals contains misleading Javadoc
Modify the specification of `java.io.File.equals` to clarify that equality is based only on a comparison of abstract pathnames as opposed to the file system objects that the `File`s represent. - Commit messages: - 8267569: java.io.File.equals contains misleading Javadoc Changes: https://git.openjdk.java.net/jdk/pull/4232/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4232=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267569 Stats: 9 lines in 1 file changed: 4 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4232.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4232/head:pull/4232 PR: https://git.openjdk.java.net/jdk/pull/4232
Integrated: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
On Thu, 27 May 2021 20:17:59 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > javax/management/remote/mandatory/connection/RMIConnector_NPETest.java. This pull request has now been integrated. Changeset: 8a31c075 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/8a31c07598cd5ea1305a9706d80b0251fd3a1e6d Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java Reviewed-by: smarks - PR: https://git.openjdk.java.net/jdk/pull/4231
Re: Integrated: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
On Thu, 27 May 2021 20:22:31 GMT, Stuart Marks wrote: >> A trivial fix to ProblemList >> javax/management/remote/mandatory/connection/RMIConnector_NPETest.java. > > Marked as reviewed by smarks (Reviewer). @stuart-marks - Thanks for the fast review. - PR: https://git.openjdk.java.net/jdk/pull/4231
Re: Integrated: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
On Thu, 27 May 2021 20:17:59 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > javax/management/remote/mandatory/connection/RMIConnector_NPETest.java. Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4231
Integrated: 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java
A trivial fix to ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java. - Commit messages: - 8267886: ProblemList javax/management/remote/mandatory/connection/RMIConnector_NPETest.java Changes: https://git.openjdk.java.net/jdk/pull/4231/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4231=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267886 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4231/head:pull/4231 PR: https://git.openjdk.java.net/jdk/pull/4231
Re: RFR: 8267529: StringJoiner can create a String that breaks String::equals [v2]
> This patch fixes a regression caused by > https://bugs.openjdk.java.net/browse/JDK-8265237 where the result of > String.join always get a UTF-16 coder if the delimiter is UTF-16, even when > no delimiter is emitted. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add join of zero strings, clarify comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/4228/files - new: https://git.openjdk.java.net/jdk/pull/4228/files/0e2ca7cd..b4b9d14d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4228=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4228=00-01 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4228.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4228/head:pull/4228 PR: https://git.openjdk.java.net/jdk/pull/4228
Integrated: 8265418: Clean-up redundant null-checks of Class.getPackageName()
On Mon, 19 Apr 2021 14:05:31 GMT, Сергей Цыпанов wrote: > As discussed in https://github.com/openjdk/jdk/pull/3464 we can clean-up > null-checks remaining after > [8142968](https://bugs.openjdk.java.net/browse/JDK-8142968) as > Class.getPackageName() never returns null. This pull request has now been integrated. Changeset: ae258f1e Author:Сергей Цыпанов Committer: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/ae258f1e6a6335585190aaa9358a4290a453fdbf Stats: 20 lines in 8 files changed: 1 ins; 7 del; 12 mod 8265418: Clean-up redundant null-checks of Class.getPackageName() Reviewed-by: redestad - PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]
> Please review this implementation of [JEP > 411](https://openjdk.java.net/jeps/411). > > The code change is divided into 3 commits. Please review them one by one. > > 1. > https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 > The essential change for this JEP, including the `@Deprecate` annotations > and spec change. It also update the default value of the > `java.security.manager` system property to "disallow", and necessary test > change following this update. > 2. > https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 > Manual changes to several files so that the next commit can be generated > programatically. > 3. > https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 > Automatic changes to other source files to avoid javac warnings on > deprecation for removal > > The 1st and 2nd commits should be reviewed carefully. The 3rd one is > generated programmatically, see the comment below for more details. If you > are only interested in a portion of the 3rd commit and would like to review > it as a separate file, please comment here and I'll generate an individual > webrev. > > Due to the size of this PR, no attempt is made to update copyright years for > any file to minimize unnecessary merge conflict. > > Furthermore, since the default value of `java.security.manager` system > property is now "disallow", most of the tests calling > `System.setSecurityManager()` need to launched with > `-Djava.security.manager=allow`. This is covered in a different PR at > https://github.com/openjdk/jdk/pull/4071. > > Update: the deprecation annotations and javadoc tags, build, compiler, > core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are > reviewed. Rest are 2d, awt, beans, sound, and swing. Weijun Wang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - move one annotation to new method - merge from master, two files removed, one needs merge - keep only one systemProperty tag - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java - feedback from Sean, Phil and Alan - add supresswarnings annotations automatically - manual change before automatic annotating - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal - Changes: https://git.openjdk.java.net/jdk/pull/4073/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4073=04 Stats: 2022 lines in 825 files changed: 1884 ins; 10 del; 128 mod Patch: https://git.openjdk.java.net/jdk/pull/4073.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073 PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267529: StringJoiner can create a String that breaks String::equals
On Thu, 27 May 2021 17:38:42 GMT, Claes Redestad wrote: > This patch fixes a regression caused by > https://bugs.openjdk.java.net/browse/JDK-8265237 where the result of > String.join always get a UTF-16 coder if the delimiter is UTF-16, even when > no delimiter is emitted. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4228
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Thu, 27 May 2021 18:19:38 GMT, Rémi Forax wrote:
> in your example, there is no guard so no backward goto
@forax btw this example is not about switch pattern matching - it is about
already existing string switch, where indy not involed
void example(java.lang.String);
descriptor: (Ljava/lang/String;)V
flags: (0x)
Code:
stack=2, locals=4, args_size=2
0: aload_1
1: astore_2
2: iconst_m1
3: istore_3
4: aload_2
5: invokevirtual #7 // Method
java/lang/String.hashCode:()I
8: lookupswitch { // 1
97: 28
default: 39
}
28: aload_2
29: ldc #13 // String a
31: invokevirtual #15 // Method
java/lang/String.equals:(Ljava/lang/Object;)Z
34: ifeq 39
37: iconst_0
38: istore_3
39: iload_3
40: lookupswitch { // 1
0: 60
default: 68
}
60: getstatic #19 // Field
java/lang/System.out:Ljava/io/PrintStream;
63: ldc #13 // String a
65: invokevirtual #25 // Method
java/io/PrintStream.println:(Ljava/lang/String;)V
68: return
LineNumberTable:
line 3: 0
line 5: 60
line 7: 68
StackMapTable: number_of_entries = 5
frame_type = 253 /* append */
offset_delta = 4
locals = [ class java/lang/String, int ]
frame_type = 23 /* same */
frame_type = 10 /* same */
frame_type = 20 /* same */
frame_type = 249 /* chop */
offset_delta = 7
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - Post-merge fix - need to include jdk.internal.javac in the list of > packages used by jdk.compiler again, as we now (again) have a preview feature > in javac. > - Correcting LineNumberTable for rule switches. > - Merging master into JDK-8262891 > - Fixing various error-related bugs. > - Avoiding fall-through from the total case to a synthetic default but > changing total patterns to default. > - Reflecting recent spec changes. > - Reflecting review comments. > - Reflecting review comments on SwitchBootstraps. > - Trailing whitespaces. > - Cleanup, reflecting review comments. > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/083416d3...fd748501 > > I've updated the code to produce better/more useful LineNumberTable for > > rule switches. > > @lahodaj I re-tested previous example and tested few others. Now everything > seems to be good with `LineNumberTable` entries +1 > [...] > Don't know about importance of this, and whether this was expected, but > decided to mention. Look likes a mistake for me, you only need a StackFrame in front of the call to invokedynamic if it's the target of a goto and in your example, there is no guard so no backward goto. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov wrote: >> 8267706: bin/idea.sh tries to use cygpath on WSL > > Nikita Gubarkov has updated the pull request incrementally with one > additional commit since the last revision: > > 8267706: Break long line in make/ide/idea/jdk/idea.gmk Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 12 commits:
>
> - Post-merge fix - need to include jdk.internal.javac in the list of
> packages used by jdk.compiler again, as we now (again) have a preview feature
> in javac.
> - Correcting LineNumberTable for rule switches.
> - Merging master into JDK-8262891
> - Fixing various error-related bugs.
> - Avoiding fall-through from the total case to a synthetic default but
> changing total patterns to default.
> - Reflecting recent spec changes.
> - Reflecting review comments.
> - Reflecting review comments on SwitchBootstraps.
> - Trailing whitespaces.
> - Cleanup, reflecting review comments.
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/083416d3...fd748501
> I've updated the code to produce better/more useful LineNumberTable for rule
> switches.
@lahodaj I re-tested previous example and tested few others. Now everything
seems to be good with `LineNumberTable` entries
However I also noticed that for
class Example {
void example(String s) {
switch (s) {
case "a":
System.out.println("a");
}
}
}
there is difference in frames before and after this PR
javap -v -p Example.class
diff is
line 3: 0
line 5: 60
line 7: 68
- StackMapTable: number_of_entries = 4
+ StackMapTable: number_of_entries = 5
frame_type = 253 /* append */
- offset_delta = 28
+ offset_delta = 4
locals = [ class java/lang/String, int ]
+frame_type = 23 /* same */
frame_type = 10 /* same */
frame_type = 20 /* same */
frame_type = 249 /* chop */
and
java -cp asm-util-9.1.jar:asm-9.1.jar org.objectweb.asm.util.Textifier
Example.class
diff is
+ L1
+ FRAME APPEND [java/lang/String I]
ALOAD 2
INVOKEVIRTUAL java/lang/String.hashCode ()I
LOOKUPSWITCH
- 97: L1
- default: L2
- L1
- FRAME APPEND [java/lang/String I]
+ 97: L2
+ default: L3
+ L2
+ FRAME SAME
Don't know about importance of this, and whether this was expected, but decided
to mention.
-
Marked as reviewed by godin (Author).
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang wrote:
>> The code change refactors classes that have a `SuppressWarnings("removal")`
>> annotation that covers more than 50KB of code. The big annotation is often
>> quite faraway from the actual deprecated API usage it is suppressing, and
>> with the annotation covering too big a portion it's easy to call other
>> deprecated methods without being noticed.
>>
>> The code change shows some common solutions to avoid such too wide
>> annotations:
>>
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local
>> variable and add annotation on the declaration, and then assign that value
>> to the original l-value if not void. The local variable will be called `tmp`
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>>
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update FtpClient.java
src/java.desktop/share/classes/java/awt/Component.java line 630:
> 628: }
> 629:
> 630: @SuppressWarnings("removal")
I'm confused. I thought the reason this wasn't done in the JEP implementation
PR is because of refactoring
that was needed because of the usage in this static block and you could not
apply the annotation here.
Yet it seems you are doing exactly what was supposed to be impossible with no
refactoring.
Can you explain ?
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
> 8267706: bin/idea.sh tries to use cygpath on WSL Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision: 8267706: Break long line in make/ide/idea/jdk/idea.gmk - Changes: - all: https://git.openjdk.java.net/jdk/pull/4190/files - new: https://git.openjdk.java.net/jdk/pull/4190/files/e1617757..f8d6c405 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4190=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4190=00-01 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4190/head:pull/4190 PR: https://git.openjdk.java.net/jdk/pull/4190
RFR: 8267529: StringJoiner can create a String that breaks String::equals
This patch fixes a regression caused by https://bugs.openjdk.java.net/browse/JDK-8265237 where the result of String.join always get a UTF-16 coder if the delimiter is UTF-16, even when no delimiter is emitted. - Commit messages: - Fix copyright, @bug - 8267529: StringJoiner can create a String that breaks String::equals Changes: https://git.openjdk.java.net/jdk/pull/4228/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4228=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267529 Stats: 18 lines in 2 files changed: 14 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4228.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4228/head:pull/4228 PR: https://git.openjdk.java.net/jdk/pull/4228
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
On Mon, 24 May 2021 13:53:34 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > keep only one systemProperty tag Marked as reviewed by prr (Reviewer). I'm OK with this now given that the refactoring is already underway at https://github.com/openjdk/jdk/pull/4138 - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL
On Tue, 25 May 2021 16:37:30 GMT, Nikita Gubarkov wrote: > 8267706: bin/idea.sh tries to use cygpath on WSL I think this looks ok, though I'm not very familiar with the details of this code. I also very rarely use the idea projects. It would be good if some frequent users could take this for a spin. make/ide/idea/jdk/idea.gmk line 50: > 48: idea: > 49: $(ECHO) "SUPPORT=$(SUPPORT_OUTPUTDIR)" > $(OUT) > 50: $(ECHO) "MODULES=\"$(foreach mod, $(SEL_MODULES), module='$(mod)' > moduleSrcDirs='$(call FindModuleSrcDirs,$(mod))' moduleDependencies='$(call > FindTransitiveDepsForModule,$(mod))' #)\"" >> $(OUT) If you can find a way to break this line, it would be appreciated. We try to keep line length "reasonable" within the build files. (Reasonable meaning, not strict 80, but with future 3-way merges on a normal screen in mind) - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 05:13:44 GMT, David Holmes wrote:
>> Maxim Kartashev has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR. The pull request
>> contains one new commit since the last revision:
>>
>> 8195129: System.load() fails to load from unicode paths
>
> src/hotspot/os/windows/os_windows.cpp line 1541:
>
>> 1539: void * os::dll_load(const char *utf8_name, char *ebuf, int ebuflen) {
>> 1540: LPWSTR utf16_name = NULL;
>> 1541: errno_t err = convert_UTF8_to_UTF16(utf8_name, _name);
>
> Do you have any figures on the cost of this additional conversion in relation
> to startup time?
>
> I'm already concerned to see that we have to perform each conversion twice
> via MultiByteToWideChar/WideCharToMultiByte, once to get the size and then to
> actually get the characters! This seems potentially very inefficient.
I measured time spend converting the library path name relative to the overall
time of a (successful) `os::dll_load()` call. It varies between a fraction of a
percent to ~3% (see below for the actual data from a `release` build). I'll
defer to your expertise wrt to these numbers.
What can be done here (without changing os::dll_load() API) is to have a "fast
path" for ascii-only path names, in which case no conversion is necessary, and
a "slow path" for all the rest. This will complicate things a bit, but not by
much, I believe. I am certainly willing to give that a try. Let me know what do
you think.
./build/windows-x86_64-server-release/images/jdk/bin/java -jar
./build/windows-x86_64-server-fastdebug/images/jdk/demo/jfc/SwingSet2/SwingSet2.jar
0.050% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\jimage.dll
2.273% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.dll
0.177% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\net.dll
0.541% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\nio.dll
0.359% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\zip.dll
3.186% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\jimage.dll
0.075% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\awt.dll
0.232% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\freetype.dll
0.136% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\fontmanager.dll
0.170% for
C:\cygwin64\home\maxim\work\OpenJDK\jdk\build\windows-x86_64-server-release\images\jdk\bin\javajpeg.dll
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 05:18:50 GMT, David Holmes wrote:
>> Maxim Kartashev has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR. The pull request
>> contains one new commit since the last revision:
>>
>> 8195129: System.load() fails to load from unicode paths
>
> test/hotspot/jtreg/runtime/jni/loadLibraryUnicode/LoadLibraryUnicode.java
> line 48:
>
>> 46: } else {
>> 47: throw new Error("Unsupported OS");
>> 48: }
>
> Please use the test library function `Platform.sharedLibraryExt()` to get the
> library file extension.
Thanks for the suggestion. Rewrote this piece.
(Side note: since the libraries' prefix differs between platforms also, it'd be
nice to have something like `Platform.sharedLibraryName(name)`; this is the way
all the code that uses `Platform.sharedLibraryExt()` is structured anyway. But
I guess it's best not to conflate things).
-
PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]
On Thu, 27 May 2021 04:23:16 GMT, David Holmes wrote: >> Maxim Kartashev has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. The pull request >> contains one new commit since the last revision: >> >> 8195129: System.load() fails to load from unicode paths > > src/hotspot/os/windows/os_windows.cpp line 1462: > >> 1460: const int flag_source_str_is_null_terminated = -1; >> 1461: const int flag_estimate_chars_count = 0; >> 1462: int utf16_chars_count_estimated = >> MultiByteToWideChar(source_encoding, > > Your local naming style is somewhat excessive. You could just comment the > values of the flags when you pass them eg: > > MultiByteToWideChar(source_encoding, > MB_ERR_INVALID_CHARS, >source_str, >-1, //source is null-terminated > NULL, // no output buffer > 0); // calculate required buffer size > > Or you could just add a comment before the call: > > // Perform a dummy conversion so that we can get the required size of the > buffer to > // allocate. The source is null-terminated. > > Trying to document parameter semantics by variable naming doesn't work in my > opinion - at some point if you want to know you have to RTFM for the API. > > And utf16_len is perfectly adequate for the returned size. Fair enough. Corrected. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
> Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 Maxim Kartashev has updated the pull request incrementally with two additional commits since the last revision: - Coding style-related corrections. - Corrected the test to use Platform.sharedLibraryExt() - Changes: - all: https://git.openjdk.java.net/jdk/pull/4169/files - new: https://git.openjdk.java.net/jdk/pull/4169/files/fe661dff..c8ef8b64 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4169=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4169=01-02 Stats: 43 lines in 2 files changed: 7 ins; 9 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException [v2]
On Thu, 27 May 2021 15:57:27 GMT, Aleksei Efimov wrote: >> Hi, >> >> `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently >> with "Address already in use" `BindException`. The reason of this failure is >> that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram >> socket to the original port during `disconnect` call (the failure is in >> `DatagramChannel::repairSocket`). >> Since Datagram socket is not reused and closed after the failure `finally` >> block with the `disconnect` call can be removed. >> >> The fix was tested with all available JNDI/DNS tests, and no failures >> related to the change were observed. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Add bug id to Timeout.java jtreg header Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException [v2]
On Thu, 27 May 2021 15:33:00 GMT, Daniel Fuchs wrote: > LGTM. I really wish git had a better `diff` ! > Can you add `@bug 8265309` to com/sun/jndi/dns/ConfigTests/Timeout.java since > this is a product change? Thanks for the review and the suggestion @dfuch . Bug ID has been added. - PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException [v2]
> Hi, > > `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently > with "Address already in use" `BindException`. The reason of this failure is > that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram > socket to the original port during `disconnect` call (the failure is in > `DatagramChannel::repairSocket`). > Since Datagram socket is not reused and closed after the failure `finally` > block with the `disconnect` call can be removed. > > The fix was tested with all available JNDI/DNS tests, and no failures related > to the change were observed. Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: Add bug id to Timeout.java jtreg header - Changes: - all: https://git.openjdk.java.net/jdk/pull/4227/files - new: https://git.openjdk.java.net/jdk/pull/4227/files/f3671fc5..82d026db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4227=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4227=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4227/head:pull/4227 PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v8]
On Thu, 27 May 2021 15:33:36 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.io`, >> `java.math`, and `java.text` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon 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 ten additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Added missing brace > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Removed trailing whitespace > - 8267670: Remove redundant code from switch > - 8267670: Updated code to use yield > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Update java.io, java.math, and java.text to use switch expressions Looks good Patrick - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8265029: Preserve SIZED characteristics on slice operations (skip, limit) [v8]
On Thu, 27 May 2021 06:13:40 GMT, Tagir F. Valeev wrote: >> With the introduction of `toList()`, preserving the SIZED characteristics in >> more cases becomes more important. This patch preserves SIZED on `skip()` >> and `limit()` operations, so now every combination of >> `map/mapToX/boxed/asXyzStream/skip/limit/sorted` preserves size, and >> `toList()`, `toArray()` and `count()` may benefit from this. E. g., >> `LongStream.range(0, 10_000_000_000L).skip(1).count()` returns result >> instantly with this patch. >> >> Some microbenchmarks added that confirm the reduced memory allocation in >> `toList()` and `toArray()` cases. Before patch: >> >> ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 >> thrpt 10 40235,534 ± 0,984B/op >> ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 >> thrpt 10 106431,101 ± 0,198B/op >> ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 >> thrpt 10 106544,977 ± 1,983B/op >> value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 >> thrpt 10 40121,878 ± 0,247B/op >> value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 >> thrpt 10 106317,693 ± 1,083B/op >> value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 >> thrpt 10 106430,954 ± 0,136B/op >> >> >> After patch: >> >> ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 >> thrpt 10 40235,648 ± 1,354B/op >> ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 >> thrpt 10 40355,784 ± 1,288B/op >> ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 >> thrpt 10 40476,032 ± 2,855B/op >> value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 >> thrpt 10 40121,830 ± 0,308B/op >> value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 >> thrpt 10 40242,554 ± 0,443B/op >> value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 >> thrpt 10 40363,674 ± 1,576B/op >> >> >> Time improvements are less exciting. It's likely that inlining and >> vectorizing dominate in these tests over array allocations and unnecessary >> copying. Still, I notice a significant improvement in SliceToArray.seq_limit >> case (2x) and mild improvement (+12..16%) in other slice tests. No >> significant change in parallel execution time, though its performance is >> much less stable and I didn't run enough tests. >> >> Before patch: >> >> Benchmark (size) Mode Cnt Score Error >> Units >> ref.SliceToList.par_baseline 1 thrpt 30 14876,723 ± 99,770 >> ops/s >> ref.SliceToList.par_limit 1 thrpt 30 14856,841 ± 215,089 >> ops/s >> ref.SliceToList.par_skipLimit 1 thrpt 30 9555,818 ± 991,335 >> ops/s >> ref.SliceToList.seq_baseline 1 thrpt 30 23732,290 ± 444,162 >> ops/s >> ref.SliceToList.seq_limit 1 thrpt 30 14894,040 ± 176,496 >> ops/s >> ref.SliceToList.seq_skipLimit 1 thrpt 30 10646,929 ± 36,469 >> ops/s >> value.SliceToArray.par_baseline1 thrpt 30 25093,141 ± 376,402 >> ops/s >> value.SliceToArray.par_limit 1 thrpt 30 24798,889 ± 760,762 >> ops/s >> value.SliceToArray.par_skipLimit 1 thrpt 30 16456,310 ± 926,882 >> ops/s >> value.SliceToArray.seq_baseline1 thrpt 30 69669,787 ± 494,562 >> ops/s >> value.SliceToArray.seq_limit 1 thrpt 30 21097,081 ± 117,338 >> ops/s >> value.SliceToArray.seq_skipLimit 1 thrpt 30 15522,871 ± 112,557 >> ops/s >> >> >> After patch: >> >> Benchmark (size) Mode Cnt Score Error >> Units >> ref.SliceToList.par_baseline 1 thrpt 30 14793,373 ± 64,905 >> ops/s >> ref.SliceToList.par_limit 1 thrpt 30 13301,024 ± 1300,431 >> ops/s >> ref.SliceToList.par_skipLimit 1 thrpt 30 11131,698 ± 1769,932 >> ops/s >> ref.SliceToList.seq_baseline 1 thrpt 30 24101,048 ± 263,528 >> ops/s >> ref.SliceToList.seq_limit 1 thrpt 30 16872,168 ± 76,696 >> ops/s >> ref.SliceToList.seq_skipLimit 1 thrpt 30 11953,253 ± 105,231 >> ops/s >> value.SliceToArray.par_baseline1 thrpt 30 25442,442 ± 455,554 >> ops/s >> value.SliceToArray.par_limit 1 thrpt 30 23111,730 ± 2246,086 >> ops/s >> value.SliceToArray.par_skipLimit 1 thrpt 30 17980,750 ± 2329,077 >> ops/s >> value.SliceToArray.seq_baseline1 thrpt 30 66512,898 ± 1001,042 >> ops/s >> value.SliceToArray.seq_limit 1 thrpt 30 41792,549 ± 1085,547 >> ops/s >> value.SliceToArray.seq_skipLimit 1 thrpt 30 18007,613 ± 141,716 >> ops/s >> >> >> I also
Re: RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException
On Thu, 27 May 2021 15:02:01 GMT, Aleksei Efimov wrote: > Hi, > > `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently > with "Address already in use" `BindException`. The reason of this failure is > that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram > socket to the original port during `disconnect` call (the failure is in > `DatagramChannel::repairSocket`). > Since Datagram socket is not reused and closed after the failure `finally` > block with the `disconnect` call can be removed. > > The fix was tested with all available JNDI/DNS tests, and no failures related > to the change were observed. LGTM. I really wish git had a better `diff` ! Can you add `@bug 8265309` to com/sun/jndi/dns/ConfigTests/Timeout.java since this is a product change? - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4227
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v8]
> Hi, > > Could someone please review my code for updating the code in the `java.io`, > `java.math`, and `java.text` packages to make use of the switch expressions? > > Kind regards, > Patrick Patrick Concannon 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 ten additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8267670 - Merge remote-tracking branch 'origin/master' into JDK-8267670 - 8267670: Added missing brace - Merge remote-tracking branch 'origin/master' into JDK-8267670 - 8267670: Removed trailing whitespace - 8267670: Remove redundant code from switch - 8267670: Updated code to use yield - Merge remote-tracking branch 'origin/master' into JDK-8267670 - 8267670: Update java.io, java.math, and java.text to use switch expressions - Changes: - all: https://git.openjdk.java.net/jdk/pull/4182/files - new: https://git.openjdk.java.net/jdk/pull/4182/files/57184fbf..933e59e9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4182=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4182=06-07 Stats: 29913 lines in 434 files changed: 3355 ins; 25561 del; 997 mod Patch: https://git.openjdk.java.net/jdk/pull/4182.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4182/head:pull/4182 PR: https://git.openjdk.java.net/jdk/pull/4182
Integrated: 8267123: Remove RMI Activation
On Tue, 25 May 2021 18:04:45 GMT, Stuart Marks wrote: > This is the implementation of [JEP 407](https://openjdk.java.net/jeps/407). > > This is a fairly straightforward removal of this component. > - Activation implementation classes removed > - Activation tests removed > - adjustments to various comments to remove references to Activation > - adjustments to some code to remove special-case support for Activation > from core RMI > - adjustments to several tests to remove testing and support for, and > mentions of Activation > - remove `rmid` tool > > (Personally, I found that reviewing using the webrev was easier than > navigating github's diff viewer.) This pull request has now been integrated. Changeset: 7c85f351 Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/7c85f3510cb84881ff232548fbcc933ef4b34972 Stats: 21982 lines in 243 files changed: 0 ins; 21930 del; 52 mod 8267123: Remove RMI Activation Reviewed-by: erikj, rriggs, alanb - PR: https://git.openjdk.java.net/jdk/pull/4194
RFR: 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException
Hi, `com/sun/jndi/dns/ConfigTests/Timeout.java ` was seen failing intermittently with "Address already in use" `BindException`. The reason of this failure is that `disconnect` call in JNDI `DnsClient` fails to rebind the datagram socket to the original port during `disconnect` call (the failure is in `DatagramChannel::repairSocket`). Since Datagram socket is not reused and closed after the failure `finally` block with the `disconnect` call can be removed. The fix was tested with all available JNDI/DNS tests, and no failures related to the change were observed. - Commit messages: - 8265309: com/sun/jndi/dns/ConfigTests/Timeout.java fails with "Address already in use" BindException Changes: https://git.openjdk.java.net/jdk/pull/4227/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4227=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265309 Stats: 30 lines in 1 file changed: 0 ins; 5 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/4227.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4227/head:pull/4227 PR: https://git.openjdk.java.net/jdk/pull/4227
RFR: 8267706: bin/idea.sh tries to use cygpath on WSL
8267706: bin/idea.sh tries to use cygpath on WSL - Commit messages: - 8267706: Added shell run configurations instead of Ant for IDEA project - 8267706: Improved IDEA project setup - 8267706: Fix bin/idea.sh cygpath usage on WSL Changes: https://git.openjdk.java.net/jdk/pull/4190/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4190=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267706 Stats: 668 lines in 14 files changed: 67 ins; 498 del; 103 mod Patch: https://git.openjdk.java.net/jdk/pull/4190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4190/head:pull/4190 PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8266310: deadlock while loading the JNI code [v5]
On Fri, 21 May 2021 15:49:09 GMT, Aleksei Voitylov wrote: >> Please review this PR which fixes the deadlock in ClassLoader between the >> two lock objects - a lock object associated with the class being loaded, and >> the ClassLoader.loadedLibraryNames hash map, locked during the native >> library load operation. >> >> Problem being fixed: >> >> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile >> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is >> removed because there's another lock object in the class loader, associated >> with the name of the class being loaded. Such objects are stored in >> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads >> exactly the same class, whose signature is being verified in another thread. >> >> Proposed fix: >> >> The proposed patch suggests to get rid of locking loadedLibraryNames hash >> map and synchronize on each entry name, as it's done with class names in see >> ClassLoader.getClassLoadingLock(name) method. >> >> The patch introduces nativeLibraryLockMap which holds the lock objects for >> each library name, and the getNativeLibraryLock() private method is used to >> lazily initialize the corresponding lock object. nativeLibraryContext was >> changed to ThreadLocal, so that in any concurrent thread it would have a >> NativeLibrary object on top of the stack, that's being currently >> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names >> of all native libraries loaded - in line with class loading code, it is not >> explicitly cleared. >> >> Testing: jtreg and jck testing with no regressions. A new regression test >> was developed. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > fix whitespace Thanks Mandy, that would be great. Waiting on your and other core-libs members reviews! - PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8266310: deadlock while loading the JNI code [v5]
On Wed, 26 May 2021 07:30:10 GMT, Peter Levart wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix whitespace
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 481:
>
>> 479: throw new Error("Maximum lock count exceeded");
>> 480: }
>> 481:
>
> Hi Aleksei,
> I know in practice this counter will never overflow, but just to be pedantic,
> it would be better that you either decrement back the counter before throwing
> Error or you check for overflow without incrementing it. Otherwise the lock
> is left in a corrupted state since you use it like this:
>
>
> acquireNativeLibraryLock(name);
> try {
> ...
> } finally {
> releaseNativeLibraryLock(name);
> }
>
> ...you see, if acquire fails, it is not paired with release, which is correct
> since the ReentrantLock has not been acquired, but the counter *HAS* been
> incremented...
Peter, the updated version checks if counter hits MAX_VALUE before
incrementing. It also means the counter can't underflow, so that check is
removed.
-
PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8266310: deadlock while loading the JNI code [v5]
On Wed, 26 May 2021 02:36:34 GMT, David Holmes wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix whitespace
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 497:
>
>> 495:
>> 496: private static void acquireNativeLibraryLock(String libraryName) {
>> 497: nativeLibraryLockMap.compute(libraryName, (name, lock) -> {
>
> This would be clearer if lock were named currentLock as for releaseNLLock
David, lock is now renamed to currentLock
-
PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8266310: deadlock while loading the JNI code [v6]
> Please review this PR which fixes the deadlock in ClassLoader between the two > lock objects - a lock object associated with the class being loaded, and the > ClassLoader.loadedLibraryNames hash map, locked during the native library > load operation. > > Problem being fixed: > > The initial reproducer demonstrated a deadlock between the JarFile/ZipFile > and the hash map. That deadlock exists even when the ZipFile/JarFile lock is > removed because there's another lock object in the class loader, associated > with the name of the class being loaded. Such objects are stored in > ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads > exactly the same class, whose signature is being verified in another thread. > > Proposed fix: > > The proposed patch suggests to get rid of locking loadedLibraryNames hash map > and synchronize on each entry name, as it's done with class names in see > ClassLoader.getClassLoadingLock(name) method. > > The patch introduces nativeLibraryLockMap which holds the lock objects for > each library name, and the getNativeLibraryLock() private method is used to > lazily initialize the corresponding lock object. nativeLibraryContext was > changed to ThreadLocal, so that in any concurrent thread it would have a > NativeLibrary object on top of the stack, that's being currently > loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names of > all native libraries loaded - in line with class loading code, it is not > explicitly cleared. > > Testing: jtreg and jck testing with no regressions. A new regression test > was developed. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/3976/files - new: https://git.openjdk.java.net/jdk/pull/3976/files/8e735117..85005d57 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3976=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3976=04-05 Stats: 11 lines in 1 file changed: 3 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/3976.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3976/head:pull/3976 PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8267587: Update java.util to use enhanced switch [v6]
On Wed, 26 May 2021 02:22:42 GMT, Tagir F. Valeev wrote:
>> Inspired by PR#4088. Most of the changes are done automatically using
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed,
>> including indentations, moving comments, extracting common cast out of
>> switch expression branches, etc.
>>
>> I also noticed that there are some switches having one branch only in
>> JapaneseImperialCalendar.java. Probably it would be better to replace them
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - JapaneseImperialCalendar: use switch expressions
> - Use yield in java.util.Calendar.Builder.build
Globally looks good.
I haven't reviewed `src/java.base/share/classes/java/util/regex/Pattern.java`
due to formatting issues. See also my other remark about `java.util.concurrent`.
src/java.base/share/classes/java/util/concurrent/FutureTask.java line 495:
> 493: * @return a string representation of this FutureTask
> 494: */
> 495: public String toString() {
Classes in java.util.concurrent are handled upstream. It would probably be
better to leave them out of this patch. Or synchronize with @DougLea to see how
to bring these changes in the upstream repo.
-
PR: https://git.openjdk.java.net/jdk/pull/4161
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName() [v3]
On Thu, 27 May 2021 12:00:46 GMT, Claes Redestad wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8265418: Compare package names with == instead of equals()
>
> src/java.base/share/classes/sun/reflect/misc/ReflectUtil.java line 129:
>
>> 127: while (clazz.isArray()) {
>> 128: clazz = clazz.getComponentType();
>> 129: }
>
> Pre-existing issue (so consider filing a follow-up) but this loop is
> redundant since `getPackageName` will already return the package name of the
> innermost component type.
Seems to be true, I'll file another ticket for this
-
PR: https://git.openjdk.java.net/jdk/pull/3571
Integrated: 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar
On Tue, 25 May 2021 16:40:53 GMT, Naoto Sato wrote: > Please review the fix. The issue was informed yesterday by @amaembo that it > offends some code analysis tools. > Although the fix is to change the condition error, it turned out that > `JapaneseImperialCalendar.roll()` did not work for `WEEK_OF_MONTH` and > `DAY_OF_MONTH`. There was an inherent issue where `GregorianCalendar` does > not follow the `roll()` spec, i.e., "The MONTH must not change when the > WEEK_OF_MONTH is rolled." during the Julian/Gregorian transition. JDK-6191841 > seems to have tried to fix it but it was closed as `Future Project`... > So I simply fixed the `roll()` issue in `JapaneseImperialCalendar` to follow > the spec here, which seems to be the intent of the original author. This pull request has now been integrated. Changeset: bea4109e Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/bea4109ef75a6536af4296db56e6ec90ab0f30fc Stats: 46 lines in 3 files changed: 36 ins; 0 del; 10 mod 8187649: ArrayIndexOutOfBoundsException in java.util.JapaneseImperialCalendar Reviewed-by: joehw, rriggs - PR: https://git.openjdk.java.net/jdk/pull/4191
Integrated: 8240347: remove undocumented options from jlink --help message
On Thu, 27 May 2021 09:23:50 GMT, Athijegannathan Sundararajan wrote: > fixed constructor argument passing issue. This pull request has now been integrated. Changeset: ec65cf83 Author:Athijegannathan Sundararajan URL: https://git.openjdk.java.net/jdk/commit/ec65cf833294e21e9dc59dfe014148d3e1210b53 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8240347: remove undocumented options from jlink --help message Reviewed-by: alanb, redestad - PR: https://git.openjdk.java.net/jdk/pull/4221
Integrated: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
On Thu, 8 Apr 2021 18:51:21 GMT, Jorn Vernee wrote:
> This patch adds a `tableSwitch` combinator that can be used to switch over a
> set of method handles given an index, with a fallback in case the index is
> out of bounds, much like the `tableswitch` bytecode. Here is a description of
> how it works (copied from the javadoc):
>
> Creates a table switch method handle, which can be used to switch over a
> set of target
> method handles, based on a given target index, called selector.
>
> For a selector value of {@code n}, where {@code n} falls in the range
> {@code [0, N)},
> and where {@code N} is the number of target method handles, the table
> switch method
> handle will invoke the n-th target method handle from the list of target
> method handles.
>
> For a selector value that does not fall in the range {@code [0, N)}, the
> table switch
> method handle will invoke the given fallback method handle.
>
> All method handles passed to this method must have the same type, with
> the additional
> requirement that the leading parameter be of type {@code int}. The
> leading parameter
> represents the selector.
>
> Any trailing parameters present in the type will appear on the returned
> table switch
> method handle as well. Any arguments assigned to these parameters will
> be forwarded,
> together with the selector value, to the selected method handle when
> invoking it.
>
> The combinator does not support specifying the starting index, so the switch
> cases always run from 0 to however many target handles are specified. A
> starting index can be added manually with another combination step that
> filters the input index by adding or subtracting a constant from it, which
> does not affect performance. One of the reasons for not supporting a starting
> index is that it allows for more lambda form sharing, but also simplifies the
> implementation somewhat. I guess an open question is if a convenience
> overload should be added for that case?
>
> Lookup switch can also be simulated by filtering the input through an
> injection function that translates it into a case index, which has also
> proven to have the ability to have comparable performance to, or even better
> performance than, a bytecode-native `lookupswitch` instruction. I plan to add
> such an injection function to the runtime libraries in the future as well.
> Maybe at that point it could be evaluated if it's worth it to add a lookup
> switch combinator as well, but I don't see an immediate need to include it in
> this patch.
>
> The current bytecode intrinsification generates a call for each switch case,
> which guarantees full inlining of the target method handles. Alternatively we
> could only have 1 callsite at the end of the switch, where each case just
> loads the target method handle, but currently this does not allow for
> inlining of the handles, since they are not constant.
>
> Maybe a future C2 optimization could look at the receiver input for
> invokeBasic call sites, and if the input is a phi node, clone the call for
> each constant input of the phi. I believe that would allow simplifying the
> bytecode without giving up on inlining.
>
> Some numbers from the added benchmarks:
>
> Benchmark(numCases) (offset)
> (sorted) Mode Cnt Score Error Units
> MethodHandlesTableSwitchConstant.testSwitch 5 0
> N/A avgt 30 4.186 � 0.054 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 5 150
> N/A avgt 30 4.164 � 0.057 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 10 0
> N/A avgt 30 4.124 � 0.023 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 10 150
> N/A avgt 30 4.126 � 0.025 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 25 0
> N/A avgt 30 4.137 � 0.042 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 25 150
> N/A avgt 30 4.113 � 0.016 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 50 0
> N/A avgt 30 4.118 � 0.028 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 50 150
> N/A avgt 30 4.127 � 0.019 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 0
> N/A avgt 30 4.116 � 0.013 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 150
> N/A avgt 30 4.121 � 0.020 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 5 0
> N/A avgt 30 4.113 � 0.009 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 5 150
> N/A avgt 30 4.149 � 0.041 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 10 0
> N/A avgt 30 4.121 � 0.026 ms/op
>
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName() [v3]
On Thu, 27 May 2021 11:19:24 GMT, Сергей Цыпанов
wrote:
>> As discussed in https://github.com/openjdk/jdk/pull/3464 we can clean-up
>> null-checks remaining after
>> [8142968](https://bugs.openjdk.java.net/browse/JDK-8142968) as
>> Class.getPackageName() never returns null.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8265418: Compare package names with == instead of equals()
LGTM
src/java.base/share/classes/sun/reflect/misc/ReflectUtil.java line 129:
> 127: while (clazz.isArray()) {
> 128: clazz = clazz.getComponentType();
> 129: }
Pre-existing issue (so consider filing a follow-up) but this loop is redundant
since `getPackageName` will already return the package name of the innermost
component type.
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName() [v3]
On Thu, 27 May 2021 10:08:17 GMT, Claes Redestad wrote:
>> One more thing I'm thinking about (not to be done in this PR of course) is
>> to move call to `String.intern()` from where it is now in
>> `Class.getPackageName()`
>>
>> public String getPackageName() {
>> String pn = this.packageName;
>> if (pn == null) {
>> Class c = this;
>> while (c.isArray()) {
>> c = c.getComponentType();
>> }
>> if (c.isPrimitive()) {
>> pn = "java.lang";
>> } else {
>> String cn = c.getName();
>> int dot = cn.lastIndexOf('.');
>> pn = (dot != -1) ? cn.substring(0, dot).intern() : "";// <---
>> }
>> this.packageName = pn;
>> }
>> return pn;
>> }
>>
>> to `packageName` field assignement like
>>
>> this.packageName = pn.intern();
>>
>> this would add two more Strings (`""` and `"java.lang"`) into string table
>> and allow to avoid `String.equals()` in favour of `==` call when comparing
>> package names. What do you think?
>
> String literals are implicitly interned:
>
> System.out.println("" == new String("")); // false
> System.out.println("" == new String("").intern()); // true
> System.out.println("java.lang" == new String("java.lang")); // false
> System.out.println("java.lang" == new String("java.lang").intern()); // true
>
>
> So we could already use `==` instead of `equals` when evaluating the result
> of `getPackageName`, which might be a good optimization in some places
> (`ReflectionFactory::isSamePackage` shows up in profiles of some
> reflection-related benchmarks).
Done!
-
PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName() [v3]
> As discussed in https://github.com/openjdk/jdk/pull/3464 we can clean-up > null-checks remaining after > [8142968](https://bugs.openjdk.java.net/browse/JDK-8142968) as > Class.getPackageName() never returns null. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8265418: Compare package names with == instead of equals() - Changes: - all: https://git.openjdk.java.net/jdk/pull/3571/files - new: https://git.openjdk.java.net/jdk/pull/3571/files/7d52a13d..dc91091b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3571=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3571=01-02 Stats: 7 lines in 6 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/3571.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3571/head:pull/3571 PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 12 commits:
>
> - Post-merge fix - need to include jdk.internal.javac in the list of
> packages used by jdk.compiler again, as we now (again) have a preview feature
> in javac.
> - Correcting LineNumberTable for rule switches.
> - Merging master into JDK-8262891
> - Fixing various error-related bugs.
> - Avoiding fall-through from the total case to a synthetic default but
> changing total patterns to default.
> - Reflecting recent spec changes.
> - Reflecting review comments.
> - Reflecting review comments on SwitchBootstraps.
> - Trailing whitespaces.
> - Cleanup, reflecting review comments.
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/083416d3...fd748501
Compiler changes look good
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1657:
> 1655:
> 1656: try {
> 1657: boolean enumSwitch = (seltype.tsym.flags() & Flags.ENUM) !=
> 0;
This is getting convoluted enough that an enum on the AST (e.g. SwitchKind)
might be helpful to save some of these classification properties - which I
imagine we have to redo at some point later (e.g. in Lower/TransPattern). I'm
ok with fixing in a followup issue.
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v26]
> This PR contains the API and implementation changes for JEP-412 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/412 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 37 commits: - Merge branch 'master' into JEP-412 - * Add missing `final` in some static fields * Downgrade native methods in ProgrammableUpcallHandler to package-private - Add sealed modifiers in morally sealed API interfaces - Merge branch 'master' into JEP-412 - Fix VaList test Remove unused code in Utils - Merge pull request #11 from JornVernee/JEP-412-MXCSR Add MXCSR save and restore to upcall stubs for non-windows platforms - Add MXCSR save and restore to upcall stubs for non-windows platforms - Merge branch 'master' into JEP-412 - Fix issue with bounded arena allocator - Rename is_entry_blob to is_optimized_entry_blob Rename as_entry_blob to as_optimized_entry_blob Fix misc typos and indentation issues - ... and 27 more: https://git.openjdk.java.net/jdk/compare/7278f56b...8bbddfc2 - Changes: https://git.openjdk.java.net/jdk/pull/3699/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3699=25 Stats: 14501 lines in 219 files changed: 8847 ins; 3642 del; 2012 mod Patch: https://git.openjdk.java.net/jdk/pull/3699.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3699/head:pull/3699 PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName() [v2]
On Mon, 26 Apr 2021 11:24:23 GMT, Сергей Цыпанов
wrote:
>> That should be fine, the null check in Objects.equals is benign with these
>> usages.
>
> One more thing I'm thinking about (not to be done in this PR of course) is to
> move call to `String.intern()` from where it is now in
> `Class.getPackageName()`
>
> public String getPackageName() {
> String pn = this.packageName;
> if (pn == null) {
> Class c = this;
> while (c.isArray()) {
> c = c.getComponentType();
> }
> if (c.isPrimitive()) {
> pn = "java.lang";
> } else {
> String cn = c.getName();
> int dot = cn.lastIndexOf('.');
> pn = (dot != -1) ? cn.substring(0, dot).intern() : "";// <---
> }
> this.packageName = pn;
> }
> return pn;
> }
>
> to `packageName` field assignement like
>
> this.packageName = pn.intern();
>
> this would add two more Strings (`""` and `"java.lang"`) into string table
> and allow to avoid `String.equals()` in favour of `==` call when comparing
> package names. What do you think?
String literals are implicitly interned:
System.out.println("" == new String("")); // false
System.out.println("" == new String("").intern()); // true
System.out.println("java.lang" == new String("java.lang")); // false
System.out.println("java.lang" == new String("java.lang").intern()); // true
So we could already use `==` instead of `equals` when evaluating the result of
`getPackageName`, which might be a good optimization in some places
(`ReflectionFactory::isSamePackage` shows up in profiles of some
reflection-related benchmarks).
-
PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v25]
> This PR contains the API and implementation changes for JEP-412 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/412 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: * Add missing `final` in some static fields * Downgrade native methods in ProgrammableUpcallHandler to package-private - Changes: - all: https://git.openjdk.java.net/jdk/pull/3699/files - new: https://git.openjdk.java.net/jdk/pull/3699/files/e927c235..e1fcd2d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3699=24 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3699=23-24 Stats: 5 lines in 3 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/3699.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3699/head:pull/3699 PR: https://git.openjdk.java.net/jdk/pull/3699
Re: RFR: 8240347: remove undocumented options from jlink --help message
On Thu, 27 May 2021 09:23:50 GMT, Athijegannathan Sundararajan wrote: > fixed constructor argument passing issue. Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4221
Re: RFR: 8240347: remove undocumented options from jlink --help message
On Thu, 27 May 2021 09:23:50 GMT, Athijegannathan Sundararajan wrote: > fixed constructor argument passing issue. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4221
RFR: 8240347: remove undocumented options from jlink --help message
fixed constructor argument passing issue. - Commit messages: - 8240347: remove undocumented options from jlink --help message Changes: https://git.openjdk.java.net/jdk/pull/4221/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4221=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8240347 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4221.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4221/head:pull/4221 PR: https://git.openjdk.java.net/jdk/pull/4221
Re: RFR: 8265418: Clean-up redundant null-checks of Class.getPackageName() [v2]
> As discussed in https://github.com/openjdk/jdk/pull/3464 we can clean-up > null-checks remaining after > [8142968](https://bugs.openjdk.java.net/browse/JDK-8142968) as > Class.getPackageName() never returns null. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' into 8265418 # Conflicts: #src/java.base/share/classes/java/lang/Class.java - 8265418: Clean-up redundant null-checks of Class.getPackageName() - Changes: https://git.openjdk.java.net/jdk/pull/3571/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3571=01 Stats: 18 lines in 7 files changed: 1 ins; 7 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3571.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3571/head:pull/3571 PR: https://git.openjdk.java.net/jdk/pull/3571
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Wed, 26 May 2021 17:58:06 GMT, Roger Riggs wrote: > Process is abstract. Is there any use for these new methods to be overridden? > Perhaps they should be final. It's not clear to me that it is useful to extend Process outside of the JDK. Testing, wrapping, ...? It feels like this class wants to be sealed. Maybe we should look at deprecating the no-arg constructor, like Joe did recently with AccessibleObject. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8265029: Preserve SIZED characteristics on slice operations (skip, limit) [v8]
> With the introduction of `toList()`, preserving the SIZED characteristics in > more cases becomes more important. This patch preserves SIZED on `skip()` and > `limit()` operations, so now every combination of > `map/mapToX/boxed/asXyzStream/skip/limit/sorted` preserves size, and > `toList()`, `toArray()` and `count()` may benefit from this. E. g., > `LongStream.range(0, 10_000_000_000L).skip(1).count()` returns result > instantly with this patch. > > Some microbenchmarks added that confirm the reduced memory allocation in > `toList()` and `toArray()` cases. Before patch: > > ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 > thrpt 10 40235,534 ± 0,984B/op > ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 > thrpt 10 106431,101 ± 0,198B/op > ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 > thrpt 10 106544,977 ± 1,983B/op > value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 > thrpt 10 40121,878 ± 0,247B/op > value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 > thrpt 10 106317,693 ± 1,083B/op > value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 > thrpt 10 106430,954 ± 0,136B/op > > > After patch: > > ref.SliceToList.seq_baseline:·gc.alloc.rate.norm1 > thrpt 10 40235,648 ± 1,354B/op > ref.SliceToList.seq_limit:·gc.alloc.rate.norm 1 > thrpt 10 40355,784 ± 1,288B/op > ref.SliceToList.seq_skipLimit:·gc.alloc.rate.norm 1 > thrpt 10 40476,032 ± 2,855B/op > value.SliceToArray.seq_baseline:·gc.alloc.rate.norm 1 > thrpt 10 40121,830 ± 0,308B/op > value.SliceToArray.seq_limit:·gc.alloc.rate.norm1 > thrpt 10 40242,554 ± 0,443B/op > value.SliceToArray.seq_skipLimit:·gc.alloc.rate.norm1 > thrpt 10 40363,674 ± 1,576B/op > > > Time improvements are less exciting. It's likely that inlining and > vectorizing dominate in these tests over array allocations and unnecessary > copying. Still, I notice a significant improvement in SliceToArray.seq_limit > case (2x) and mild improvement (+12..16%) in other slice tests. No > significant change in parallel execution time, though its performance is much > less stable and I didn't run enough tests. > > Before patch: > > Benchmark (size) Mode Cnt Score Error > Units > ref.SliceToList.par_baseline 1 thrpt 30 14876,723 ± 99,770 > ops/s > ref.SliceToList.par_limit 1 thrpt 30 14856,841 ± 215,089 > ops/s > ref.SliceToList.par_skipLimit 1 thrpt 30 9555,818 ± 991,335 > ops/s > ref.SliceToList.seq_baseline 1 thrpt 30 23732,290 ± 444,162 > ops/s > ref.SliceToList.seq_limit 1 thrpt 30 14894,040 ± 176,496 > ops/s > ref.SliceToList.seq_skipLimit 1 thrpt 30 10646,929 ± 36,469 > ops/s > value.SliceToArray.par_baseline1 thrpt 30 25093,141 ± 376,402 > ops/s > value.SliceToArray.par_limit 1 thrpt 30 24798,889 ± 760,762 > ops/s > value.SliceToArray.par_skipLimit 1 thrpt 30 16456,310 ± 926,882 > ops/s > value.SliceToArray.seq_baseline1 thrpt 30 69669,787 ± 494,562 > ops/s > value.SliceToArray.seq_limit 1 thrpt 30 21097,081 ± 117,338 > ops/s > value.SliceToArray.seq_skipLimit 1 thrpt 30 15522,871 ± 112,557 > ops/s > > > After patch: > > Benchmark (size) Mode Cnt Score Error > Units > ref.SliceToList.par_baseline 1 thrpt 30 14793,373 ± 64,905 > ops/s > ref.SliceToList.par_limit 1 thrpt 30 13301,024 ± 1300,431 > ops/s > ref.SliceToList.par_skipLimit 1 thrpt 30 11131,698 ± 1769,932 > ops/s > ref.SliceToList.seq_baseline 1 thrpt 30 24101,048 ± 263,528 > ops/s > ref.SliceToList.seq_limit 1 thrpt 30 16872,168 ± 76,696 > ops/s > ref.SliceToList.seq_skipLimit 1 thrpt 30 11953,253 ± 105,231 > ops/s > value.SliceToArray.par_baseline1 thrpt 30 25442,442 ± 455,554 > ops/s > value.SliceToArray.par_limit 1 thrpt 30 23111,730 ± 2246,086 > ops/s > value.SliceToArray.par_skipLimit 1 thrpt 30 17980,750 ± 2329,077 > ops/s > value.SliceToArray.seq_baseline1 thrpt 30 66512,898 ± 1001,042 > ops/s > value.SliceToArray.seq_limit 1 thrpt 30 41792,549 ± 1085,547 > ops/s > value.SliceToArray.seq_skipLimit 1 thrpt 30 18007,613 ± 141,716 > ops/s > > > I also modernized SliceOps a little bit, using switch expression (with no > explicit default!) and diamonds on anonymous classes. Tagir F. Valeev has updated the pull request
Re: RFR: 8265029: Preserve SIZED characteristics on slice operations (skip, limit) [v7]
On Mon, 24 May 2021 19:51:04 GMT, Paul Sandoz wrote:
>> Tagir F. Valeev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Trailing whitespace removed
>
> src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 471:
>
>> 469: int flags = getStreamAndOpFlags();
>> 470: long size = StreamOpFlag.SIZED.isKnown(flags) ?
>> spliterator.getExactSizeIfKnown() : -1;
>> 471: if (size != -1 && StreamOpFlag.SIZE_ADJUSTING.isKnown(flags) &&
>> !isParallel()) {
>
> Very nice. It's a good compromise to support only for sequential streams,
> since we have no size adjusting intermediate stateless op. If that was the
> case we would need to step back through the pipeline until the depth was
> zero, then step forward. I think it worth a comment here to inform our future
> selves if we ever add such an operation.
>
> Strictly speaking we only need to call `exactOutputSize` if the stage is size
> adjusting. Not sure it really matters perf-wise. If we leave as is maybe add
> a comment.
It's hard to imagine SIZE_ADJUSTING stateless intermediate op (probably the op
that duplicates every stream element like `flatMap(x -> Stream.of(x, x))` as a
single op?). Nevertheless, I added the comment. Please check if it's clear
enough. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/3427
