Re: RFR: 8260221: java.util.Formatter throws wrong exception for mismatched flags in %% conversion
On Wed, 3 Feb 2021 22:42:00 GMT, Ian Graves wrote: > Updating the specification to reflect well-established behavior in Formatter > when incorrect flags used for `%`. Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2389
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v2]
On Fri, 12 Feb 2021 04:06:55 GMT, Naoto Sato wrote: >> Please review this doc fix to j.l.Character, which now includes the table of >> the history of supported Unicode versions. A corresponding CSR will be filed >> accordingly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Removed empty tag Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v2]
> Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed empty tag - Changes: - all: https://git.openjdk.java.net/jdk/pull/2538/files - new: https://git.openjdk.java.net/jdk/pull/2538/files/5560a4cf..b026b5eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2538=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2538=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2538.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2538/head:pull/2538 PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Fri, 12 Feb 2021 03:21:04 GMT, Brian Burkhalter wrote: > Did you run this through the usual CI tests in all tiers? Hello Brian, Do you mean other than the ones that have been automatically run and passed in the GitHub actions against this PR? I don't have a Windows box, but if there's some specific tests you want me to run locally, please do let me know and I'll run them. - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Fri, 12 Feb 2021 03:16:02 GMT, Jaikiran Pai wrote: >> I'd let it sit for a bit in case others want to comment. > > Ping. Anymore reviews/suggestions from anyone? Did you run this through the usual CI tests in all tiers? - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8260401: StackOverflowError on open WindowsPreferences
On Tue, 2 Feb 2021 02:41:10 GMT, Brian Burkhalter wrote: >>> > The code change looks all right. >>> >>> Should I go ahead and integrate this? >> >> Actually, I didn't notice that this PR wasn't marked as reviewed. I'll wait >> for the review(s) then. > > I'd let it sit for a bit in case others want to comment. Ping. Anymore reviews/suggestions from anyone? - PR: https://git.openjdk.java.net/jdk/pull/2326
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character
On Fri, 12 Feb 2021 02:50:35 GMT, Naoto Sato wrote: > Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. Looks fine to me. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2538
RFR: 8261621: Delegate Unicode history from JLS to j.l.Character
Please review this doc fix to j.l.Character, which now includes the table of the history of supported Unicode versions. A corresponding CSR will be filed accordingly. - Commit messages: - 8261621: Delegate Unicode history from JLS to j.l.Character Changes: https://git.openjdk.java.net/jdk/pull/2538/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2538=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261621 Stats: 37 lines in 1 file changed: 35 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2538.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2538/head:pull/2538 PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
On Thu, 11 Feb 2021 12:44:54 GMT, Claes Redestad wrote: >> This patch moves some sanity checking done in ClassLoader.java to the >> corresponding endpoints in native or VM code. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Consolidate verifyClassname and verifyFixClassname This more limited cleanup looks good. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]
> Please review this clarification of the specification of the method > `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the > behavior of the method is made clear for the case when the `Reader` is > already at the end of its stream when the method is invoked. A corresponding > CSR will be filed. Also, the change includes an update to an existing test in > order to verify that the specification change reflects actual behavior. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8247918: Change'ns' to 'n' in the skip doc - Changes: - all: https://git.openjdk.java.net/jdk/pull/2274/files - new: https://git.openjdk.java.net/jdk/pull/2274/files/0ebed6ea..2a2ceb41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2274=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2274=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2274.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2274/head:pull/2274 PR: https://git.openjdk.java.net/jdk/pull/2274
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v4]
> A follow-up of sorts to JDK-8257086, this change aims to improve the > discussion of the relationship between Object.equals and compareTo and > compare methods. The not-consistent-with-equals natural ordering of > BigDecimal get more explication too. While updating Object, I added some uses > of apiNote and implSpec, as appropriate. While a signum method did not exist > when Comparable was added, it has existed for many years so I removed > obsolete text that defined a "sgn" function to compute signum. > > Once the changes are agreed to, I'll reflow paragraphs and update the > copyright years. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2471/files - new: https://git.openjdk.java.net/jdk/pull/2471/files/2a8dd8ce..a7f1b28b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2471=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2471=02-03 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2471/head:pull/2471 PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8254001: [Metrics] Enhance parsing of cgroup interface files for version detection [v4]
On Tue, 9 Feb 2021 13:31:25 GMT, Severin Gehwolf wrote: >> This is an enhancement which solves two issues: >> >> 1. Multiple reads of relevant cgroup interface files. Now interface files >> are only read once per file (just like Hotspot). >> 2. Proxies creation of the impl specific subsystem via `determineType()` as >> before, but now reads all relevant interface files: `/proc/cgroups`, >> `/proc/self/mountinfo` and `/proc/self/cgroup`. Once read it passes the >> parsed information to the impl specific subsystem classes for instantiation. >> This allows for more flexibility of testing as interface files can be mocked >> and, thus, more cases can be tested that way without having access to these >> specific systems. For example, proper regression tests for JDK-8217766 and >> JDK-8253435 have been added now with this in place. >> >> * [x] Tested on Linux x86_64 on cgroups v1 and cgroups v2. Container tests >> pass. > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Fix jcheck > - Add documentation and reduce code running in the critical section > - Add some documentation > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - Merge branch 'master' into jdk-8254001-enhance-file-parsing-java-metrics > - 8254001: [Metrics] Enhance parsing of cgroup interface files for version > detection Hi Severin, Thanks for doing this! Sorry for taking so long to review this change. The change looks good. Before pushing it, could you add a comment explaining what the code in lines 185-194 of CgroupSubsystemFactory.java is doing? Also, please don't overwrite the fix for JDK-8257746. Thanks again! Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1393
Withdrawn: 8231436: Fix the applicability of a no-@Target annotation type
On Thu, 28 Jan 2021 22:33:53 GMT, Liam Miller-Cushon wrote: > Please review this fix to add `ElementType.MODULE` to the default list of > annotation targets, to allow annotations without an explicit `@Target` to be > used on module declarations. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/2303
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v3]
On Thu, 11 Feb 2021 19:23:55 GMT, Roger Riggs wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set Looks good. - Marked as reviewed by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v3]
On Thu, 11 Feb 2021 19:23:55 GMT, Roger Riggs wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v3]
> On Mac Os X, the OSVersionTest detected a difference in the version number > reported in the os.version property > and the version number provided by `sw_vers -productVersion`. > > When the java runtime is built with XCode 11.3, the os.version is reported as > 10.16 > though the current version numbering is 11.nnn. > > The workaround is to derive the os.version number from the > ProductBuildVersion. > When the toolchain is updated to XCode 12.nnn it can be removed. > The workaround is enabled only when the environment variable > SYSTEM_VERSION_COMPAT is unset. > When the SYSTEM_VERSION_COMPAT is set in the environment the version number > is reported as reported by Mac OS X. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set - Changes: - all: https://git.openjdk.java.net/jdk/pull/2530/files - new: https://git.openjdk.java.net/jdk/pull/2530/files/c7b05857..09f6fd0e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2530=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2530=01-02 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2530.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2530/head:pull/2530 PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v2]
On Thu, 11 Feb 2021 19:09:52 GMT, Kevin Rushforth wrote:
>> src/java.base/macosx/native/libjava/java_props_macosx.c line 262:
>>
>>> 260: // Copy out the char*
>>> 261: osVersionCStr = strdup([nsVerStr UTF8String]);
>>> 262: } else if (getenv("SYSTEM_VERSION_COMPAT") == NULL) {
>>
>> If version is 10.16 and `SYSTEM_VERSION_COMPAT` is set, you will fall
>> through to the pre-10.9 Mac OS code fallback. Just checking to see if that's
>> what you intended.
>
> FWIW, it seems to work OK using the legacy fallback path (reports 10.16 if I
> set `SYSTEM_VERSION_COMPAT=1`).
The same version string is available from both APIs, reading from the
SystemVersion.plist is a bit slower.
It would be clearer to move the checking of SYSTEM_VERSION_COMPAT to the first
test (line:252)
so the version info does not need to be read from the files.
-
PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:14:35 GMT, Roger Riggs wrote: > On Mac Os X, the OSVersionTest detected a difference in the version number > reported in the os.version property > and the version number provided by `sw_vers -productVersion`. > > When the java runtime is built with XCode 11.3, the os.version is reported as > 10.16 > though the current version numbering is 11.nnn. > > The workaround is to derive the os.version number from the > ProductBuildVersion. > When the toolchain is updated to XCode 12.nnn it can be removed. > The workaround is enabled only when the environment variable > SYSTEM_VERSION_COMPAT is unset. > When the SYSTEM_VERSION_COMPAT is set in the environment the version number > is reported as reported by Mac OS X. I tested this, and get the following behavior on macOS 11.0.1 with a JDK compiled with Xcode 11.3.1 and your patch: SYSTEM_VERSION_COMPAT not set : 11.0 SYSTEM_VERSION_COMPAT=1 : 10.16 SYSTEM_VERSION_COMPAT=0 : 11.0.1 So the fallback path reports what could be considered a more accurate version, at least on my laptop. Since I'm not sure what is expected, you can decide what to do with this information. - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v2]
On Thu, 11 Feb 2021 18:53:08 GMT, Kevin Rushforth wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Correct double-negative in 'other than 10.16'
>
> src/java.base/macosx/native/libjava/java_props_macosx.c line 262:
>
>> 260: // Copy out the char*
>> 261: osVersionCStr = strdup([nsVerStr UTF8String]);
>> 262: } else if (getenv("SYSTEM_VERSION_COMPAT") == NULL) {
>
> If version is 10.16 and `SYSTEM_VERSION_COMPAT` is set, you will fall through
> to the pre-10.9 Mac OS code fallback. Just checking to see if that's what you
> intended.
FWIW, it seems to work OK using the legacy fallback path (reports 10.16 if I
set `SYSTEM_VERSION_COMPAT=1`).
-
PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v2]
On Thu, 11 Feb 2021 18:34:54 GMT, Roger Riggs wrote:
>> On Mac Os X, the OSVersionTest detected a difference in the version number
>> reported in the os.version property
>> and the version number provided by `sw_vers -productVersion`.
>>
>> When the java runtime is built with XCode 11.3, the os.version is reported
>> as 10.16
>> though the current version numbering is 11.nnn.
>>
>> The workaround is to derive the os.version number from the
>> ProductBuildVersion.
>> When the toolchain is updated to XCode 12.nnn it can be removed.
>> The workaround is enabled only when the environment variable
>> SYSTEM_VERSION_COMPAT is unset.
>> When the SYSTEM_VERSION_COMPAT is set in the environment the version number
>> is reported as reported by Mac OS X.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Correct double-negative in 'other than 10.16'
src/java.base/macosx/native/libjava/java_props_macosx.c line 262:
> 260: // Copy out the char*
> 261: osVersionCStr = strdup([nsVerStr UTF8String]);
> 262: } else if (getenv("SYSTEM_VERSION_COMPAT") == NULL) {
If version is 10.16 and `SYSTEM_VERSION_COMPAT` is set, you will fall through
to the pre-10.9 Mac OS code fallback. Just checking to see if that's what you
intended.
-
PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 [v2]
> On Mac Os X, the OSVersionTest detected a difference in the version number > reported in the os.version property > and the version number provided by `sw_vers -productVersion`. > > When the java runtime is built with XCode 11.3, the os.version is reported as > 10.16 > though the current version numbering is 11.nnn. > > The workaround is to derive the os.version number from the > ProductBuildVersion. > When the toolchain is updated to XCode 12.nnn it can be removed. > The workaround is enabled only when the environment variable > SYSTEM_VERSION_COMPAT is unset. > When the SYSTEM_VERSION_COMPAT is set in the environment the version number > is reported as reported by Mac OS X. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Correct double-negative in 'other than 10.16' - Changes: - all: https://git.openjdk.java.net/jdk/pull/2530/files - new: https://git.openjdk.java.net/jdk/pull/2530/files/7ac2b6a6..c7b05857 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2530=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2530=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2530.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2530/head:pull/2530 PR: https://git.openjdk.java.net/jdk/pull/2530
Integrated: 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java
On Thu, 11 Feb 2021 17:55:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java > in order to reduce noise in the JDK17 CI. This pull request has now been integrated. Changeset: 75c8489c Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/75c8489c Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java Reviewed-by: hseigel - PR: https://git.openjdk.java.net/jdk/pull/2531
Re: RFR: 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java
On Thu, 11 Feb 2021 18:04:31 GMT, Harold Seigel wrote: >> A trivial fix to ProblemList >> jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java >> in order to reduce noise in the JDK17 CI. > > Looks good and trivial. > Thanks, Harold @hseigel - Thanks for the fast review! - PR: https://git.openjdk.java.net/jdk/pull/2531
Re: RFR: 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java
On Thu, 11 Feb 2021 17:55:13 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList > jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java > in order to reduce noise in the JDK17 CI. Looks good and trivial. Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2531
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:50:20 GMT, Brian Burkhalter wrote: >> It's a double negative, unless I'm reading it incorrectly: "other than >> pre-10.16" I interpret as "not pre-10.16" or "10.16". > > `// Copy out the char* if running on version 10.x[.y], where x < 16` ? It will also do it if running on `11.x[.y]` (once Xcode is upgraded), right? - PR: https://git.openjdk.java.net/jdk/pull/2530
RFR: 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java
A trivial fix to ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java in order to reduce noise in the JDK17 CI. - Commit messages: - 8261604: ProblemList jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java Changes: https://git.openjdk.java.net/jdk/pull/2531/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2531=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261604 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2531.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2531/head:pull/2531 PR: https://git.openjdk.java.net/jdk/pull/2531
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:37:34 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> src/java.base/share/classes/java/util/Comparator.java line 159:
>
>> 157: * and it imposes the same ordering as this comparator. Thus,
>> 158: * {@code comp1.equals(comp2)} implies that {@code
>> signum(comp1.compare(o1,
>> 159: * o2))==signum(comp2.compare(o1, o2))} for every object reference
>
> Maybe make "signum" be a link here, similar to other locations where it's
> used.
Okay -- I didn't want to go overboard making all the signum references links to
the method, but I can change the first occurrence in Comparator.equals to a
link.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:49:09 GMT, Kevin Rushforth wrote: >> So the words "other than" are too subtle? > > It's a double negative, unless I'm reading it incorrectly: "other than > pre-10.16" I interpret as "not pre-10.16" or "10.16". `// Copy out the char* if running on version 10.x, where x < 16` ? - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:46:06 GMT, Roger Riggs wrote: >> @kevinrushforth I think you are correct. This is actually mea culpa I think >> as I had provided in a Slack thread a failed attempt at a patch for this >> which contained this comment. > > So the words "other than" are too subtle? It's a double negative, unless I'm reading it incorrectly: "other than pre-10.16" I interpret as "not pre-10.16" or "10.16". - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:38:42 GMT, Brian Burkhalter wrote: >> src/java.base/macosx/native/libjava/java_props_macosx.c line 250: >> >>> 248: >>> 249: NSString *nsVerStr; >>> 250: // Copy out the char* if running on version other than >>> pre-10.16 Mac OS (10.16 == 11.x) >> >> Isn't the comment backwards from what the test does? I would think it should >> be "if running on version other than 10.16 Mac OS ...". Or am I missing >> something? > > @kevinrushforth I think you are correct. This is actually mea culpa I think > as I had provided in a Slack thread a failed attempt at a patch for this > which contained this comment. So the words "other than" are too subtle? - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:19:29 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> src/java.base/share/classes/java/lang/Object.java line 149:
>
>> 147: *
>> 148: * @apiNote
>> 149: * Note that it is generally necessary to override the {@link
>> hashCode hashCode}
>
> The `@apiNote` introduces the paragraph with a subhead "API Note:" so it's a
> bit redundant to say "Note that" Maybe just start with "It is generally
> necessary"
Agree.
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods [v3]
On Thu, 11 Feb 2021 04:14:08 GMT, Stuart Marks wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typos in javadoc tags found during review.
>
> src/java.base/share/classes/java/lang/Comparable.java line 110:
>
>> 108: * {@link Integer#signum signum}{@code (x.compareTo(y)) ==
>> -signum(y.compareTo(x))}
>> 109: * for all {@code x} and {@code y}. (This
>> 110: * implies that {@code x.compareTo(y)} must throw an exception iff
>
> I'd suggest replacing "iff" with "if and only if" because it looks like a
> typo, and writing out the long form emphasizes the bidirectional nature of
> the implication.
Sure; most of the terminology in the JDK docs aren't very "math-y".
-
PR: https://git.openjdk.java.net/jdk/pull/2471
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:31:34 GMT, Kevin Rushforth wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > src/java.base/macosx/native/libjava/java_props_macosx.c line 250: > >> 248: >> 249: NSString *nsVerStr; >> 250: // Copy out the char* if running on version other than >> pre-10.16 Mac OS (10.16 == 11.x) > > Isn't the comment backwards from what the test does? I would think it should > be "if running on version other than 10.16 Mac OS ...". Or am I missing > something? @kevinrushforth I think you are correct. This is actually mea culpa I think as I had provided in a Slack thread a failed attempt at a patch for this which contained this comment. - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Thu, 11 Feb 2021 17:14:35 GMT, Roger Riggs wrote: > On Mac Os X, the OSVersionTest detected a difference in the version number > reported in the os.version property > and the version number provided by `sw_vers -productVersion`. > > When the java runtime is built with XCode 11.3, the os.version is reported as > 10.16 > though the current version numbering is 11.nnn. > > The workaround is to derive the os.version number from the > ProductBuildVersion. > When the toolchain is updated to XCode 12.nnn it can be removed. > The workaround is enabled only when the environment variable > SYSTEM_VERSION_COMPAT is unset. > When the SYSTEM_VERSION_COMPAT is set in the environment the version number > is reported as reported by Mac OS X. src/java.base/macosx/native/libjava/java_props_macosx.c line 250: > 248: > 249: NSString *nsVerStr; > 250: // Copy out the char* if running on version other than pre-10.16 > Mac OS (10.16 == 11.x) Isn't the comment backwards from what the test does? I would think it should be "if running on version other than 10.16 Mac OS ...". Or am I missing something? - PR: https://git.openjdk.java.net/jdk/pull/2530
RFR: 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0
On Mac Os X, the OSVersionTest detected a difference in the version number reported in the os.version property and the version number provided by `sw_vers -productVersion`. When the java runtime is built with XCode 11.3, the os.version is reported as 10.16 though the current version numbering is 11.nnn. The workaround is to derive the os.version number from the ProductBuildVersion. When the toolchain is updated to XCode 12.nnn it can be removed. The workaround is enabled only when the environment variable SYSTEM_VERSION_COMPAT is unset. When the SYSTEM_VERSION_COMPAT is set in the environment the version number is reported as reported by Mac OS X. - Commit messages: - 8253702: BigSur java/lang/System/OsVersionTest.java: 10.16 != 11.0 Changes: https://git.openjdk.java.net/jdk/pull/2530/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2530=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253702 Stats: 30 lines in 1 file changed: 21 ins; 2 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/2530.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2530/head:pull/2530 PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8261160: Add a deserialization JFR event [v4]
On Thu, 11 Feb 2021 15:53:06 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Filter **C**onfigured Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
On Thu, 11 Feb 2021 15:53:06 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Filter **C**onfigured LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Added table of available algorithms. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/1c17ad35..f1e3699d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=15-16 Stats: 181 lines in 3 files changed: 140 ins; 0 del; 41 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:45:33 GMT, Daniel Fuchs wrote:
>> Chris Hegarty has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix failing test
>
> src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 42:
>
>> 40:
>> 41: @Label("Filter configured")
>> 42: public boolean filterConfigured;
>
> Should that be "Filter Configured" for consistency?
Good catch. Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:28:07 GMT, Chris Hegarty wrote:
>> This issue adds a new event to improve diagnostic information of Java
>> deserialization. The event captures the details of deserialization activity
>> from ObjectInputStream. The event details are similar to that of the serial
>> filter, but is agnostic of whether a filter is installed or not. The event
>> also captures the filter status, if there is one.
>
> Chris Hegarty has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix failing test
src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 42:
> 40:
> 41: @Label("Filter configured")
> 42: public boolean filterConfigured;
Should that be "Filter Configured" for consistency?
-
PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v4]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Filter **C**onfigured - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/5737ee7c..d764f75f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
On Thu, 11 Feb 2021 15:28:07 GMT, Chris Hegarty wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Chris Hegarty has updated the pull request incrementally with one additional > commit since the last revision: > > Fix failing test Marked as reviewed by coffeys (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v3]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Fix failing test - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/8ecf63ce..5737ee7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v2]
On Thu, 11 Feb 2021 14:27:19 GMT, Roger Riggs wrote: >> Marked as reviewed by rriggs (Reviewer). > > As proposed, events are only created if there is a serialFilter in effect > (and enabled by JFR configuration). > Being able to create the events without a serialFilter in effect would be > useful for monitoring, especially if it could be controlled by a separate JFR > configuration option. (always, never, serial-filter , etc.) I updated the PR and addressed all comments so far. Specifically: @RogerRiggs The generation of the event is independent of whether the filter is set or not. I also added a piece of state to determine if a filter is set or not. I think it could be useful to analyse all Deserialisation events to, say, ensure that there are none operating without a filter ( and the `filterStatus` state is ambitious in the `null` case ). @coffeys I would like GlobalFilterTest to run regardless of whether or not the jfr module is present, but of course running the test with jfr enabled is desirable too, so I added a separate at test tag for that case. @egahlin Excellent suggestions on the naming, etc. I adopted all. And added a test to ensure that the creation and generation of the event does not inadvertently trigger class initialization if the filter rejects the attempt ( thanks @dfuch ) @dfuch Thanks for the improved label suggestion, it is now merged in. - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event [v2]
> This issue adds a new event to improve diagnostic information of Java > deserialization. The event captures the details of deserialization activity > from ObjectInputStream. The event details are similar to that of the serial > filter, but is agnostic of whether a filter is installed or not. The event > also captures the filter status, if there is one. Chris Hegarty has updated the pull request incrementally with one additional commit since the last revision: Review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/2479/files - new: https://git.openjdk.java.net/jdk/pull/2479/files/ca0bcf44..8ecf63ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2479=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2479=00-01 Stats: 124 lines in 5 files changed: 76 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/2479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479 PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261160: Add a deserialization JFR event
On Wed, 10 Feb 2021 20:30:02 GMT, Roger Riggs wrote: >> This issue adds a new event to improve diagnostic information of Java >> deserialization. The event captures the details of deserialization activity >> from ObjectInputStream. The event details are similar to that of the serial >> filter, but is agnostic of whether a filter is installed or not. The event >> also captures the filter status, if there is one. > > Marked as reviewed by rriggs (Reviewer). As proposed, events are only created if there is a serialFilter in effect (and enabled by JFR configuration). Being able to create the events without a serialFilter in effect would be useful for monitoring, especially if it could be controlled by a separate JFR configuration option. (always, never, serial-filter , etc.) - PR: https://git.openjdk.java.net/jdk/pull/2479
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
On Thu, 4 Feb 2021 13:14:16 GMT, Coleen Phillimore wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Consolidate verifyClassname and verifyFixClassname > > Changes requested by coleenp (Reviewer). I tried to consolidate all name checking into the native layer for the remaining methods, but there are places where we are calling the JNI code with internalized names directly through `JavaLangAccess.defineClass`, so we'd need a way to differentiate these. Seems simpler to leave the `checkName` in `preDefineClass` for now. For the JNI code consolidating verifyFixClassname and verifyClassname into a single method seems to be the most straightforward simplification possible, since these are currently called back to back. Since ASCII like `/` is never a component of a multibyte character in UTF-8 we can do the fix-up pass without validation, then do the full verification. This simplifies the code and might speed it up marginally. Also added some cleanup to the cleanup code as suggested by @tstuefe in #2407 - PR: https://git.openjdk.java.net/jdk/pull/2378
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]
> This patch moves some sanity checking done in ClassLoader.java to the > corresponding endpoints in native or VM code. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Consolidate verifyClassname and verifyFixClassname - Changes: - all: https://git.openjdk.java.net/jdk/pull/2378/files - new: https://git.openjdk.java.net/jdk/pull/2378/files/727b2b37..6b8305e9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2378=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2378=01-02 Stats: 77 lines in 4 files changed: 13 ins; 40 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/2378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2378/head:pull/2378 PR: https://git.openjdk.java.net/jdk/pull/2378
Integrated: 8261300: jpackage: rewrite while(0)/while(false) to proper blocks
On Mon, 8 Feb 2021 09:17:46 GMT, Aleksey Shipilev wrote:
> After JDK-8254702, SonarCloud instance complains about blocks like these:
> "Change this loop body so that it can be executed more than once."
>
> int initJvmlLauncherData(JvmlLauncherData* ptr) const {
> // Store path to JLI library just behind JvmlLauncherData header.
> char* curPtr = reinterpret_cast(ptr + 1);
> do {
> const size_t count = sizeof(char)
> * (jliLibPath.size() + 1 /* trailing zero */);
> if (ptr) {
> std::memcpy(curPtr, jliLibPath.c_str(), count);
> ptr->jliLibPath = curPtr;
> }
> curPtr += count;
> } while (false); // < here
>
> There is no sense in having `while(false)` here, where the syntactic `{}`
> block would do. There are also other uses in the jpackage code that employ
> `while(0)` for this, and then they also trigger the inspection about the
> implicit conversion of zero int to boolean.
>
> Additional testing:
> - [x] Linux x86_64 (Ubuntu) tools/jpackage
This pull request has now been integrated.
Changeset: 9fed6048
Author:Aleksey Shipilev
URL: https://git.openjdk.java.net/jdk/commit/9fed6048
Stats: 74 lines in 2 files changed: 0 ins; 4 del; 70 mod
8261300: jpackage: rewrite while(0)/while(false) to proper blocks
Reviewed-by: herrick, asemenyuk, almatvee
-
PR: https://git.openjdk.java.net/jdk/pull/2454
Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v2]
> This patch moves some sanity checking done in ClassLoader.java to the > corresponding endpoints in native or VM code. Claes Redestad 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 four additional commits since the last revision: - Merge branch 'master' into checkName - Merge branch 'master' into checkName - Copyrights - Move class name checking for findBootstrapClass/findLoadedClass into native/VM - Changes: - all: https://git.openjdk.java.net/jdk/pull/2378/files - new: https://git.openjdk.java.net/jdk/pull/2378/files/f2fd1d1c..727b2b37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2378=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2378=00-01 Stats: 28701 lines in 954 files changed: 16489 ins; 8085 del; 4127 mod Patch: https://git.openjdk.java.net/jdk/pull/2378.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2378/head:pull/2378 PR: https://git.openjdk.java.net/jdk/pull/2378
Integrated: 8261418: Reduce decoder creation overheads for sun.nio.cs.ext Charsets
On Tue, 9 Feb 2021 12:54:12 GMT, Claes Redestad wrote: > This refactor some `sun.nio.cs.ext` charsets, such as ISO-2022-CN-GB, > ISO-2022-CN-CNS, ISO-2022-KR and a few others to use static rather than > per-instance auxiliary decoders. Doing so reduce overheads of calling > `charset.newDecoder()`. This reduce or eliminate regressions on `new > String(byte[], String)` operations due the removal of thread-local decoder > caching in [JDK-8259842](https://bugs.openjdk.java.net/browse/JDK-8259842) > > Most ISO-2022 Charsets define a specialized decoder already. The > `ISO2022.Decoder` class was only used by `ISO2022_KR`, so folding it into > that implementation and simplifying the code brings a rather significant > speed-up, both to decoder creation and on actual decoding. > > Testing: tier1-3, manual runs of sun.nio.cs tests This pull request has now been integrated. Changeset: 8b6ab31d Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/8b6ab31d Stats: 761 lines in 15 files changed: 257 ins; 404 del; 100 mod 8261418: Reduce decoder creation overheads for sun.nio.cs.ext Charsets Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/2480
Re: RFR: 8261418: Reduce decoder creation overheads for sun.nio.cs.ext Charsets [v3]
On Wed, 10 Feb 2021 23:38:51 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add comment about removing the generic ISO2022.Decoder > > Thanks. The newly added comment to ISO2022 is helpful. Thanks Naoto! - PR: https://git.openjdk.java.net/jdk/pull/2480
Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v9]
> Could I get the following change reviewed please? It fixes a problem (in > JEP380) on Windows where some file operations on Unix domain sockets were not > working and led to the feature being disabled on Windows 2019 Server in JDK > 16. So, the fix re-enables the feature on all versions of Windows that > support it. > > The test checks all the file APIs that were affected by the problem. The > change touches the code that handles symbolic links in Windows (since they > are implemented as NTFS reparse points, like Unix sockets), but I didn't add > any specific testing in this area, as I assume the existing unit tests for > NIO symbolic links should cover that. If I should add more tests here, then I > can do that. > > Thanks, > Michael Michael McMahon 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 17 additional commits since the last revision: - Merge branch 'master' into 8252971-socket-attributes - Rearrange test in WindowsPath. Change to Security.java test to explicitly delete socket files - Merge branch 'master' into 8252971-socket-attributes - update - fix mistake in last push - update - Merge branch 'master' into 8252971-socket-attributes - add specific check for unix domain socket - Merge branch 'master' into 8252971-socket-attributes - update - ... and 7 more: https://git.openjdk.java.net/jdk/compare/3bc0384d...11ae5e72 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2424/files - new: https://git.openjdk.java.net/jdk/pull/2424/files/70832057..11ae5e72 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2424=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2424=07-08 Stats: 2988 lines in 123 files changed: 1583 ins; 865 del; 540 mod Patch: https://git.openjdk.java.net/jdk/pull/2424.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424 PR: https://git.openjdk.java.net/jdk/pull/2424
Integrated: 8261449: Micro-optimize JVM_LatestUserDefinedLoader
On Tue, 9 Feb 2021 15:40:03 GMT, Aleksey Shipilev wrote: > `JVM_LatestUserDefinedLoader` is called normally from > `ObjectInputStream.resolveClass` -> `VM.latestUserDefinedLoader0`. And it > takes a measurable time to walk the stack. There is JDK-8173368 that wants to > replace it with `StackWalker`, but we can tune up the > `JVM_LatestUserDefinedLoader` itself without changing the semantics of it > (thus providing the backportability, including the releases that do not have > `StackWalker`) and improving performance (thus providing a more aggressive > baseline for `StackWalker` rewrite). > > The key is to recognize that out of two checks: 1) checking for two special > subclasses; 2) checking for user classloader -- the first one usually passes, > and second one fails much more frequently. First check also requires > traversing the superclasses upwards looking for match. Reversing the order of > the checks, plus inlining the helper method improves performance without > changing the semantics. > > Out of curiosity, my previous patch dropped the first check completely, > replacing it by asserts, and we definitely run into situation where that > check is needed on some tests. > > On my machine, `VM.latestUserDefinedLoader` invocation time drops from 115 to > 100 ns/op. Single-threaded SPECjvm2008:serial improves about 3% with this > patch. > > Additional testing: > - [x] Ad-hoc benchmarks > - [x] Linux x86_64 fastdebug, `tier1`, `tier2`, `tier3` > > - > ### Progress > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > - [ ] Change must be properly reviewed > > > > ### Download > `$ git fetch https://git.openjdk.java.net/jdk pull/2485/head:pull/2485` > `$ git checkout pull/2485` This pull request has now been integrated. Changeset: 49cf13d2 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/49cf13d2 Stats: 20 lines in 3 files changed: 4 ins; 13 del; 3 mod 8261449: Micro-optimize JVM_LatestUserDefinedLoader Reviewed-by: dholmes, stuefe, alanb - PR: https://git.openjdk.java.net/jdk/pull/2485
Re: RFR: 8261449: Micro-optimize JVM_LatestUserDefinedLoader [v2]
On Thu, 11 Feb 2021 08:04:55 GMT, Alan Bateman wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added a comment > > Marked as reviewed by alanb (Reviewer). Thanks! - PR: https://git.openjdk.java.net/jdk/pull/2485
Re: RFR: 8261449: Micro-optimize JVM_LatestUserDefinedLoader [v2]
On Wed, 10 Feb 2021 07:34:59 GMT, Aleksey Shipilev wrote:
>> `JVM_LatestUserDefinedLoader` is called normally from
>> `ObjectInputStream.resolveClass` -> `VM.latestUserDefinedLoader0`. And it
>> takes a measurable time to walk the stack. There is JDK-8173368 that wants
>> to replace it with `StackWalker`, but we can tune up the
>> `JVM_LatestUserDefinedLoader` itself without changing the semantics of it
>> (thus providing the backportability, including the releases that do not have
>> `StackWalker`) and improving performance (thus providing a more aggressive
>> baseline for `StackWalker` rewrite).
>>
>> The key is to recognize that out of two checks: 1) checking for two special
>> subclasses; 2) checking for user classloader -- the first one usually
>> passes, and second one fails much more frequently. First check also requires
>> traversing the superclasses upwards looking for match. Reversing the order
>> of the checks, plus inlining the helper method improves performance without
>> changing the semantics.
>>
>> Out of curiosity, my previous patch dropped the first check completely,
>> replacing it by asserts, and we definitely run into situation where that
>> check is needed on some tests.
>>
>> On my machine, `VM.latestUserDefinedLoader` invocation time drops from 115
>> to 100 ns/op. Single-threaded SPECjvm2008:serial improves about 3% with this
>> patch.
>>
>> Additional testing:
>> - [x] Ad-hoc benchmarks
>> - [x] Linux x86_64 fastdebug, `tier1`, `tier2`, `tier3`
>>
>> -
>> ### Progress
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> - [ ] Change must be properly reviewed
>>
>>
>>
>> ### Download
>> `$ git fetch https://git.openjdk.java.net/jdk pull/2485/head:pull/2485`
>> `$ git checkout pull/2485`
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added a comment
Marked as reviewed by alanb (Reviewer).
src/hotspot/share/prims/jvm.cpp line 3297:
> 3295:
> !ik->is_subclass_of(vmClasses::reflect_ConstructorAccessorImpl_klass())) {
> 3296: return JNIHandles::make_local(THREAD, loader);
> 3297: }
This okay looks but surprised it has a measurable (or micro) difference.
There has been several proposals over the years to improve
latestUserDefinedLoader in the common case that OIS.resolveClass is not
overridden. It may need to be looked at again. Ideally
JVM_LastestUSerDefinedLoader would go away and there would be a solution based
on StackWalker (but work would be required there to match the current
performance).
-
PR: https://git.openjdk.java.net/jdk/pull/2485
