Re: RFR: 8265173: [test] divert spurious log output away from stream under test in ProcessBuilder Basic test

[David Holmes]

On Wed, 14 Apr 2021 14:08:50 GMT, Roger Riggs  wrote:

> The most recent intermittent failure showed that the error occurred during VM 
> initialization.
> Only the tty output was diverted, but not log output.
> Add diversion of log output as well tty output.
> 
> Add `-Xlog:all=warning:stderr` and `-Xlog:all=warning:stdout` to correspond 
> to 
> `-XX:+DisplayVMOutputToStderr` and `-XX:+DisplayVMOutputToStdout`

Hi Roger,

The fix looks good, but it strikes me that any test using 
-XX:+DisplayVMOutputToXXX can potentially suffer from a similar problem e.g. 
(in this space):

test/jdk/java/lang/ProcessHandle/JavaChild.java

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3492

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]

[Naoto Sato]

On Thu, 15 Apr 2021 01:57:01 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>> 
>> As noted in a comment in that issue, the bug relates to the return value of 
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The 
>> implementation has started returning invalid values for the `AM_PM` field 
>> after the "day period" support was added recently in the JDK as part of 
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>> 
>> The commit here adds a check in the internal implementation of the display 
>> name handling logic, to special case the `AM_PM` field and properly convert 
>> the display name array indexes (which is an internal detail) to valid values 
>> that represent the `AM_PM` calendar field.
>> 
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
>> reproduces this issue and verifies the fix.
>> 
>> After this fix was introduced, I ran the test in 
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
>> issue in the first place? To fix this, I have added an additional commit 
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - update existing testcase based on review comment
>  - Improve code comment to be clear it's only applicable for 
> java.util.Calendar
>  - Remove irrelevant setLenient from new testcase

Changes requested by naoto (Reviewer).

test/jdk/java/util/Calendar/NarrowNamesTest.java line 115:

> 113: } else {
> 114: testMap(US, AM_PM, ALL_STYLES,
> 115: "AM", "PM",

What I meant was there is no need to check the providers and introduce the 
am/pm specific test method. Both `CLDR` and `COMPAT` (i.e., no "if" statement) 
should be tested with `testMap()` method as other tests do.

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v3]

[Jaikiran Pai]

On Wed, 14 Apr 2021 18:01:10 GMT, Naoto Sato  wrote:

>> Jaikiran Pai has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Update existing NarrowNamesTest to match expectations
>>  - Naoto's review suggestion
>
> src/java.base/share/classes/sun/util/locale/provider/CalendarNameProviderImpl.java
>  line 193:
> 
>> 191: }
>> 192: if (field == AM_PM && !javatime && i > PM) {
>> 193: // when dealing with calendar fields, 
>> don't set AM_PM field value
> 
> Make it explicit that this block only serves for `java.util.Calendar`, not 
> `java.time.format.DateTimeFormatter(Builder)`.

Hello Naoto, I've now updated the PR to be more explicit in this code comment.

> test/jdk/java/util/Calendar/CalendarDisplayNamesTest.java line 53:
> 
>> 51: for (final int style : styles) {
>> 52: final Calendar cal = Calendar.getInstance();
>> 53: cal.setLenient(false);
> 
> Don't think leniency is relevant here.

Removed in the latest update of this PR. This was a leftover from my 
experimental testing.

> test/jdk/java/util/Calendar/NarrowNamesTest.java line 119:
> 
>> 117: expectedFieldValues.put("a", Calendar.AM);
>> 118: expectedFieldValues.put("p", Calendar.PM);
>> 119: testAM_PM(US, ALL_STYLES, expectedFieldValues);
> 
> I believe this can be reverted to the original, as the original code compares 
> the exact map (including the length of the map).  , i.e., 
> 
>testMap(US, AM_PM, ALL_STYLES,
> "AM", "PM",
> RESET_INDEX,
> "a", "p");

Done. Reverted this specific part to what it was originally. Test continues to 
pass with this change and the latest PR update.

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]

[Jaikiran Pai]

> Can I please get a review for this proposed fix for 
> https://bugs.openjdk.java.net/browse/JDK-8262108?
> 
> As noted in a comment in that issue, the bug relates to the return value of 
> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation 
> has started returning invalid values for the `AM_PM` field after the "day 
> period" support was added recently in the JDK as part of 
> https://bugs.openjdk.java.net/browse/JDK-8262108.
> 
> The commit here adds a check in the internal implementation of the display 
> name handling logic, to special case the `AM_PM` field and properly convert 
> the display name array indexes (which is an internal detail) to valid values 
> that represent the `AM_PM` calendar field.
> 
> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
> reproduces this issue and verifies the fix.
> 
> After this fix was introduced, I ran the test in 
> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
> issue in the first place? To fix this, I have added an additional commit 
> which updates this test case to properly test the `AM_PM` field values.

Jaikiran Pai has updated the pull request incrementally with three additional 
commits since the last revision:

 - update existing testcase based on review comment
 - Improve code comment to be clear it's only applicable for java.util.Calendar
 - Remove irrelevant setLenient from new testcase

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3463/files
  - new: https://git.openjdk.java.net/jdk/pull/3463/files/e7194733..607776b3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3463=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3463=02-03

  Stats: 9 lines in 3 files changed: 1 ins; 3 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3463.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3463/head:pull/3463

PR: https://git.openjdk.java.net/jdk/pull/3463

RFR: 8263154: [macos] DMG builds have finder errors

[Alexander Matveev]

- Issue was reproducible when install-dir points to some invalid location.
 - Fixed by defaulting DMG drag and drop location to /Applications folder and 
--install-dir will be ignored with warning for DMG.
 - I do not see any way to support other valid, but uncommon locations for drag 
and drop. For example: /Users/USERNAME/Applications is not possible to support 
since user name is not known. /usr/bin requires root privileges and should 
contain symbolic links. Locations which does not exist also not possible to 
support, since DMG cannot create paths. So /Applications/MyCompany is not 
possible for DMG.

-

Commit messages:
 - 8263154: [macos] DMG builds have finder errors

Changes: https://git.openjdk.java.net/jdk/pull/3505/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3505=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263154
  Stats: 18 lines in 6 files changed: 7 ins; 0 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3505.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3505/head:pull/3505

PR: https://git.openjdk.java.net/jdk/pull/3505

Re: Garbage Free Check

[Dalibor Topic]

Hi Ralph,

I've added an apache-log4j-interest label to the issue.

cheers,
dalibor topic

On 14.04.2021 19:00, Ralph Goers wrote:

I don’t have an account in the bug tracking system. Could someone possibly 
update the issue noted below to indicate that Apache Log4j 2 would also like 
that enhancement?

Thanks,

Ralph


On Apr 5, 2021, at 1:26 PM, Roger Riggs  wrote:

Hi,

Java does not have a data type with enough resolution to hold a full nanosecond 
value.
Hence the implementation of Instant holding seconds and nanos.

There is an long dormant enhancement request to return micro-seconds as a long.
8196003  java.time Instant 
and Duration methods for microseconds

That might be useful if the application gets enough resolution from 
microseconds.
There might be some clever interpolation between System.currentTimeMillis()
and adjusting with System.nanoTime().
Though it would likely not be exactly synchronized with the values from Instant.

Regards, Roger


On 4/5/21 3:56 PM, Brian Goetz wrote:

Project Valhalla will allow Instant to be migrated to a primitive class, which 
would address your problem.

On 4/2/2021 7:47 PM, Ralph Goers wrote:

Log4j 2 supports the notion of a PreciseClock - one that can be initialized to 
something more precise than a millisecond. At the same time it also supports 
running with no heap allocations in certain circumstances. I am in the process 
of moving our master branch to require Java 11 as the minimum. In doing so I am 
encountering unit test errors while verifying that logging is garbage free. 
They all occur allocating an Instant.

The code we have simply does

public void init(MutableInstant mutableInstant) {
  Instant instant = java.time.Clock.systemUTC().instant();
mutableInstant.initFromEpochSecond(instant.getEpochSecond(), instant.getNano());
}
In our previous tests we had thought the allocation was being eliminated due to 
escape analysis since the data is being extracted from the Instant and not 
passed along. However, after upgrading the Google test library and the JDK 
version it appears that is not the case.
Ideally we would really like something like

public void init(MutableInstant mutableInstant) {
 java.time.Clock.systemUTC().initInstant(mutableInstant);
}

where Mutable instant would implement an interface that has the two set 
methods.The method would execute the same logic that is in the instant() method 
but instead of creating a new Instant it would call the set methods for the 
provided object.

This would allow us to either have the MutableInstants in ThreadLocals or some 
other mechanism to ensure they are thread safe and have no heap allocations. As 
it stands now I see no way to gain access to the higher precision clock without 
memory allocation.

Do you know of another way to do this? Am I missing something?

Ralph










--
 Dalibor Topic
Consulting Product Manager
Phone: +494089091214 , Mobile: +491737185961
, Video: dalibor.to...@oracle.com


Oracle Global Services Germany GmbH
Hauptverwaltung: Riesstr. 25, D-80992 München
Registergericht: Amtsgericht München, HRB 246209
Geschäftsführer: Ralf Herrmann

Re: RFR: 8265079: Implement VarHandle invoker caching [v3]

[Mandy Chung]

On Wed, 14 Apr 2021 11:38:15 GMT, Jorn Vernee  wrote:

>> This patch implements 2 leftover TODOs for implementing var handle invoker 
>> MH caching (lambda forms for those were already shared/cached).
>> 
>> This piggybacks on the existing mechanism for method handle invoker caching.
>> 
>> Testing: Local testing `java/lang/invoke` tests. Tier 1-3
>> 
>> Thanks,
>> Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comment: simplify test

Marked as reviewed by mchung (Reviewer).

test/jdk/java/lang/invoke/TestVHInvokerCaching.java line 26:

> 24: /* @test
> 25:  * @bug 8265079
> 26:  * @run testng/othervm -Xverify:all -ea -esa TestVHInvokerCaching

Nit: the makefile to run jtreg set `-ea -esa`.   It's not strictly necessary in 
`@run`.  If you want to run it standalone, you can run with `jtreg -ea -esa` 
option.

-

PR: https://git.openjdk.java.net/jdk/pull/3439

Re: RFR: 8265135: Reduce work initializing VarForms [v5]

[Mandy Chung]

On Wed, 14 Apr 2021 21:26:52 GMT, Claes Redestad  wrote:

>> This patch reduces work done initializing VarForms - mostly observed when 
>> loading each VarHandle implementation class.
>> 
>> - Lazily resolve MemberNames.
>> - Streamline MethodType creation. This reduces the number of MethodTypes 
>> created. 
>> 
>> Net effect is a reduction in bytecode executed per VH class by 50-60%.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add resolveOrNull for exploded MemberName

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: RFR: 8258794: Support for CLDR version 39

[Erik Joelsson]

On Wed, 14 Apr 2021 21:13:51 GMT, Naoto Sato  wrote:

> Please review the changes to support CLDR version 39. The vast majority of 
> the changes are purely data changes from Unicode. The only change affected in 
> logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with 
> CLDR's Norwegian language code switch 
> (https://unicode-org.atlassian.net/browse/CLDR-2698)

Marked as reviewed by erikj (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3502

Re: RFR: 8265237: String.join and StringJoiner can be improved further

[Peter Levart]

On Wed, 14 Apr 2021 19:54:26 GMT, Florent Guillaume 
 wrote:

>> While JDK-8148937 improved StringJoiner class by replacing internal use of 
>> getChars that copies out characters from String elements into a char[] array 
>> with StringBuilder which is somehow more optimal, the improvement was 
>> marginal in speed (0% ... 10%) and mainly for smaller strings, while GC was 
>> reduced by about 50% in average per operation.
>> Initial attempt to tackle that issue was more involved, but was later 
>> discarded because it was apparently using too much internal String details 
>> in code that lives outside String and outside java.lang package.
>> But there is another way to package such "intimate" code - we can put it 
>> into String itself and just call it from StringJoiner.
>> This PR is an attempt at doing just that. It introduces new package-private 
>> method in `java.lang.String` which is then used from both pubic static 
>> `String.join` methods as well as from `java.util.StringJoiner` (via 
>> SharedSecrets). The improvements can be seen by running the following JMH 
>> benchmark:
>> 
>> https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce
>> 
>> The comparative results are here:
>> 
>> https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15
>> 
>> The jmh-result.json files are here:
>> 
>> https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15
>> 
>> Improvement in speed ranges from 8% (for small strings) to 200% (for long 
>> strings), while creation of garbage has been further reduced to an almost 
>> garbage-free operation.
>> 
>> So WDYT?
>
> src/java.base/share/classes/java/lang/String.java line 3230:
> 
>> 3228: 
>> 3229: /**
>> 3230:  * Designated join routine.
> 
> Did you mean "dedicated"?

No, I meant designated. It is the routine that all other public API entry 
points call at the end to do the job. Would some other word more accurately 
describe that? I definitely didn't mean "dedicated".

-

PR: https://git.openjdk.java.net/jdk/pull/3501

Re: RFR: 8258794: Support for CLDR version 39

[Naoto Sato]

On Wed, 14 Apr 2021 21:13:51 GMT, Naoto Sato  wrote:

> Please review the changes to support CLDR version 39. The vast majority of 
> the changes are purely data changes from Unicode. The only change affected in 
> logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with 
> CLDR's Norwegian language code switch 
> (https://unicode-org.atlassian.net/browse/CLDR-2698)

Thanks, Joe.

> Naoto, are you testing GitHub ('s ability to handle a large number of files) 
> ;-)

CLDR itself has been hosted on GitHub too, so it shouldn't be a problem 

-

PR: https://git.openjdk.java.net/jdk/pull/3502

Re: RFR: 8258794: Support for CLDR version 39

[Joe Wang]

On Wed, 14 Apr 2021 21:13:51 GMT, Naoto Sato  wrote:

> Please review the changes to support CLDR version 39. The vast majority of 
> the changes are purely data changes from Unicode. The only change affected in 
> logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with 
> CLDR's Norwegian language code switch 
> (https://unicode-org.atlassian.net/browse/CLDR-2698)

Naoto, are you testing GitHub ('s ability to handle a large number of files) ;-)

Indeed, the majority changes were version and copyright. If you hadn't 
mentioned the changed class, it would be almost impossible to find it :-)

-

Marked as reviewed by joehw (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3502

Re: RFR: 8265135: Reduce work initializing VarForms [v5]

[Claes Redestad]

> This patch reduces work done initializing VarForms - mostly observed when 
> loading each VarHandle implementation class.
> 
> - Lazily resolve MemberNames.
> - Streamline MethodType creation. This reduces the number of MethodTypes 
> created. 
> 
> Net effect is a reduction in bytecode executed per VH class by 50-60%.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Add resolveOrNull for exploded MemberName

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3472/files
  - new: https://git.openjdk.java.net/jdk/pull/3472/files/f84f5194..1c2208c1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3472=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3472=03-04

  Stats: 24 lines in 2 files changed: 13 ins; 7 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3472.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3472/head:pull/3472

PR: https://git.openjdk.java.net/jdk/pull/3472

RFR: 8258794: Support for CLDR version 39

[Naoto Sato]

Please review the changes to support CLDR version 39. The vast majority of the 
changes are purely data changes from Unicode. The only change affected in logic 
was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with CLDR's 
Norwegian language code switch 
(https://unicode-org.atlassian.net/browse/CLDR-2698)

-

Commit messages:
 - 8258794: Support for CLDR version 39
 - CLDR 38.1

Changes: https://git.openjdk.java.net/jdk/pull/3502/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3502=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258794
  Stats: 26326 lines in 815 files changed: 761 ins; 23140 del; 2425 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3502.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3502/head:pull/3502

PR: https://git.openjdk.java.net/jdk/pull/3502

Re: RFR: 8265237: String.join and StringJoiner can be improved further

[Claes Redestad]

On Wed, 14 Apr 2021 18:58:57 GMT, Peter Levart  wrote:

> While JDK-8148937 improved StringJoiner class by replacing internal use of 
> getChars that copies out characters from String elements into a char[] array 
> with StringBuilder which is somehow more optimal, the improvement was 
> marginal in speed (0% ... 10%) and mainly for smaller strings, while GC was 
> reduced by about 50% in average per operation.
> Initial attempt to tackle that issue was more involved, but was later 
> discarded because it was apparently using too much internal String details in 
> code that lives outside String and outside java.lang package.
> But there is another way to package such "intimate" code - we can put it into 
> String itself and just call it from StringJoiner.
> This PR is an attempt at doing just that. It introduces new package-private 
> method in `java.lang.String` which is then used from both pubic static 
> `String.join` methods as well as from `java.util.StringJoiner` (via 
> SharedSecrets). The improvements can be seen by running the following JMH 
> benchmark:
> 
> https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce
> 
> The comparative results are here:
> 
> https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15
> 
> The jmh-result.json files are here:
> 
> https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15
> 
> Improvement in speed ranges from 8% (for small strings) to 200% (for long 
> strings), while creation of garbage has been further reduced to an almost 
> garbage-free operation.
> 
> So WDYT?

I think this seems reasonable. The String encoding details doesn't leak outside 
of java.lang, and numbers do look convincing.

There's a StringJoinerBenchmark micro added by JDK-8148937 which could perhaps 
be expanded with the scenarios you've experimented with here?

-

PR: https://git.openjdk.java.net/jdk/pull/3501

Integrated: JDK-8265078: jpackage tests on Windows leave large temp files

[Andy Herrick]

On Tue, 13 Apr 2021 18:57:20 GMT, Andy Herrick  wrote:

> two changes:
> One to jpackage, when recursively removing directory, when IOException 
> occurs, record it and continue (deleting as much as possible) before throwing 
> the exception.
> The other to tests, when running jpackage via ProcessBuilder.execute(), set 
> the "TMP" environment variable to the current value of System Property 
> "java.io.tmpdir".  This causes the sub-process (jpackage) to output tmp files 
> to the tmp file location used by the test. (So the test harness can clean up 
> after test exits).

This pull request has now been integrated.

Changeset: e1675778
Author:Andy Herrick 
URL:   https://git.openjdk.java.net/jdk/commit/e1675778
Stats: 30 lines in 3 files changed: 28 ins; 0 del; 2 mod

8265078: jpackage tests on Windows leave large temp files

Reviewed-by: asemenyuk, kcr

-

PR: https://git.openjdk.java.net/jdk/pull/3473

Re: RFR: 8265237: String.join and StringJoiner can be improved further

[Florent Guillaume]

On Wed, 14 Apr 2021 18:58:57 GMT, Peter Levart  wrote:

> While JDK-8148937 improved StringJoiner class by replacing internal use of 
> getChars that copies out characters from String elements into a char[] array 
> with StringBuilder which is somehow more optimal, the improvement was 
> marginal in speed (0% ... 10%) and mainly for smaller strings, while GC was 
> reduced by about 50% in average per operation.
> Initial attempt to tackle that issue was more involved, but was later 
> discarded because it was apparently using too much internal String details in 
> code that lives outside String and outside java.lang package.
> But there is another way to package such "intimate" code - we can put it into 
> String itself and just call it from StringJoiner.
> This PR is an attempt at doing just that. It introduces new package-private 
> method in `java.lang.String` which is then used from both pubic static 
> `String.join` methods as well as from `java.util.StringJoiner` (via 
> SharedSecrets). The improvements can be seen by running the following JMH 
> benchmark:
> 
> https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce
> 
> The comparative results are here:
> 
> https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15
> 
> The jmh-result.json files are here:
> 
> https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15
> 
> Improvement in speed ranges from 8% (for small strings) to 200% (for long 
> strings), while creation of garbage has been further reduced to an almost 
> garbage-free operation.
> 
> So WDYT?

src/java.base/share/classes/java/lang/String.java line 3230:

> 3228: 
> 3229: /**
> 3230:  * Designated join routine.

Did you mean "dedicated"?

-

PR: https://git.openjdk.java.net/jdk/pull/3501

Re: RFR: 8265237: String.join and StringJoiner can be improved further

[Peter Levart]

On Wed, 14 Apr 2021 18:58:57 GMT, Peter Levart  wrote:

> While JDK-8148937 improved StringJoiner class by replacing internal use of 
> getChars that copies out characters from String elements into a char[] array 
> with StringBuilder which is somehow more optimal, the improvement was 
> marginal in speed (0% ... 10%) and mainly for smaller strings, while GC was 
> reduced by about 50% in average per operation.
> Initial attempt to tackle that issue was more involved, but was later 
> discarded because it was apparently using too much internal String details in 
> code that lives outside String and outside java.lang package.
> But there is another way to package such "intimate" code - we can put it into 
> String itself and just call it from StringJoiner.
> This PR is an attempt at doing just that. It introduces new package-private 
> method in `java.lang.String` which is then used from both pubic static 
> `String.join` methods as well as from `java.util.StringJoiner` (via 
> SharedSecrets). The improvements can be seen by running the following JMH 
> benchmark:
> 
> https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce
> 
> The comparative results are here:
> 
> https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15
> 
> The jmh-result.json files are here:
> 
> https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15
> 
> Improvement in speed ranges from 8% (for small strings) to 200% (for long 
> strings), while creation of garbage has been further reduced to an almost 
> garbage-free operation.
> 
> So WDYT?

Some background: This change was motivated by Sergey Tsypanov's attempt to 
replace usage of StringBuilder with string concatenation in JDK-8265075 only to 
find out that string concatenation in java.base module is compiled down to 
inline usage of StringBuilder, so no improvement was possible.
StringJoiner API and String.join static utility methods lend itself to a better 
implementation, but in last incarnation they are implemented with StringBuilder 
internally: String.join -> StringJoiner -> StringBuilder. A lot of JDK internal 
usages of StringBuilder were already replaced with StringJoiner (JDK-8054714) 
but under the hood the StringBuilder is still used. There were also lots of 
incremental attempts to improve StringJoiner. I think this one is the last one 
for some time to come... :-)

-

PR: https://git.openjdk.java.net/jdk/pull/3501

RFR: 8265237: String.join and StringJoiner can be improved further

[Peter Levart]

While JDK-8148937 improved StringJoiner class by replacing internal use of 
getChars that copies out characters from String elements into a char[] array 
with StringBuilder which is somehow more optimal, the improvement was marginal 
in speed (0% ... 10%) and mainly for smaller strings, while GC was reduced by 
about 50% in average per operation.
Initial attempt to tackle that issue was more involved, but was later discarded 
because it was apparently using too much internal String details in code that 
lives outside String and outside java.lang package.
But there is another way to package such "intimate" code - we can put it into 
String itself and just call it from StringJoiner.
This PR is an attempt at doing just that. It introduces new package-private 
method in `java.lang.String` which is then used from both pubic static 
`String.join` methods as well as from `java.util.StringJoiner` (via 
SharedSecrets). The improvements can be seen by running the following JMH 
benchmark:

https://gist.github.com/plevart/86ac7fc6d4541dbc08256cde544019ce

The comparative results are here:

https://jmh.morethan.io/?gist=7eb421cf7982456a2962269137f71c15

The jmh-result.json files are here:

https://gist.github.com/plevart/7eb421cf7982456a2962269137f71c15

Improvement in speed ranges from 8% (for small strings) to 200% (for long 
strings), while creation of garbage has been further reduced to an almost 
garbage-free operation.

So WDYT?

-

Commit messages:
 - Make JavaLangAccess JLA field private static final
 - Alternative String.join

Changes: https://git.openjdk.java.net/jdk/pull/3501/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3501=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265237
  Stats: 97 lines in 4 files changed: 61 ins; 18 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3501.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3501/head:pull/3501

PR: https://git.openjdk.java.net/jdk/pull/3501

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v3]

[Naoto Sato]

On Wed, 14 Apr 2021 17:14:55 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>> 
>> As noted in a comment in that issue, the bug relates to the return value of 
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The 
>> implementation has started returning invalid values for the `AM_PM` field 
>> after the "day period" support was added recently in the JDK as part of 
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>> 
>> The commit here adds a check in the internal implementation of the display 
>> name handling logic, to special case the `AM_PM` field and properly convert 
>> the display name array indexes (which is an internal detail) to valid values 
>> that represent the `AM_PM` calendar field.
>> 
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
>> reproduces this issue and verifies the fix.
>> 
>> After this fix was introduced, I ran the test in 
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
>> issue in the first place? To fix this, I have added an additional commit 
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update existing NarrowNamesTest to match expectations
>  - Naoto's review suggestion

Changes requested by naoto (Reviewer).

src/java.base/share/classes/sun/util/locale/provider/CalendarNameProviderImpl.java
 line 193:

> 191: }
> 192: if (field == AM_PM && !javatime && i > PM) {
> 193: // when dealing with calendar fields, 
> don't set AM_PM field value

Make it explicit that this block only serves for `java.util.Calendar`, not 
`java.time.format.DateTimeFormatter(Builder)`.

test/jdk/java/util/Calendar/CalendarDisplayNamesTest.java line 53:

> 51: for (final int style : styles) {
> 52: final Calendar cal = Calendar.getInstance();
> 53: cal.setLenient(false);

Don't think leniency is relevant here.

test/jdk/java/util/Calendar/NarrowNamesTest.java line 119:

> 117: expectedFieldValues.put("a", Calendar.AM);
> 118: expectedFieldValues.put("p", Calendar.PM);
> 119: testAM_PM(US, ALL_STYLES, expectedFieldValues);

I believe this can be reverted to the original, as the original code compares 
the exact map (including the length of the map).  , i.e., 

   testMap(US, AM_PM, ALL_STYLES,
"AM", "PM",
RESET_INDEX,
"a", "p");

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v2]

[Kevin Rushforth]

On Wed, 14 Apr 2021 17:36:21 GMT, Alexey Semenyuk  wrote:

> postVisitDirectory() and visitFile() methods can be potentially called 
> concurrently by walkFileTree()

I don't think they can.

> Javadoc ... doesn't say anything about synchronization, so I think it is fair 
> to assume it is not guaranteed.

Given that the `Files` class says nothing about running its various file 
walking operations in parallel, I think you can be confident that the visitor 
methods are only ever run on the same thread that calls walkFileTree. I would 
consider it a bug if the thread used to callback into the visitor were 
different from the calling thread.

Still, I think using `AtomicReference` is fine here, so this is a moot point 
for this PR.

-

PR: https://git.openjdk.java.net/jdk/pull/3473

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v3]

[Jim Laskey]

> Move makeXXXSpilterator from public (@hidden) to protected. No API ch

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  change static final from 'proxy' to 'PROXY'

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3469/files
  - new: https://git.openjdk.java.net/jdk/pull/3469/files/a6e35c6b..c6b5da30

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3469=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3469=01-02

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3469.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3469/head:pull/3469

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v2]

[Alexey Semenyuk]

On Tue, 13 Apr 2021 21:10:56 GMT, Kevin Rushforth  wrote:

> Are you sure you need an `AtomicReference` to set the exception? I don't see 
> any use of multiple threads, but I might be missing something. If you do need 
> it, you need to fix the order of arguments.

`postVisitDirectory()` and `visitFile()` methods can be potentially called 
concurrently by `walkFileTree()`. Javadoc 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#walkFileTree(java.nio.file.Path,java.util.Set,int,java.nio.file.FileVisitor)
 doesn't say anything about synchronization, so I think it is fair to assume it 
is not guaranteed.

-

PR: https://git.openjdk.java.net/jdk/pull/3473

Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v3]

[Alexey Semenyuk]

On Tue, 13 Apr 2021 22:50:12 GMT, Andy Herrick  wrote:

>> two changes:
>> One to jpackage, when recursively removing directory, when IOException 
>> occurs, record it and continue (deleting as much as possible) before 
>> throwing the exception.
>> The other to tests, when running jpackage via ProcessBuilder.execute(), set 
>> the "TMP" environment variable to the current value of System Property 
>> "java.io.tmpdir".  This causes the sub-process (jpackage) to output tmp 
>> files to the tmp file location used by the test. (So the test harness can 
>> clean up after test exits).
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8265078: jpackage tests on Windows leave large temp files

Marked as reviewed by asemenyuk (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3473

RFR: 8265208: [JEP-356] : SplittableRandom and SplittableGenerators - splits() methods does not throw NullPointerException when source is null

[Jim Laskey]

Add check for null.

-

Commit messages:
 - Check for null source
 - Make makeXXXSpliterator final
 - Move makeXXXSpilterator from public (@hidden) to protected. No API change.

Changes: https://git.openjdk.java.net/jdk/pull/3495/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3495=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265208
  Stats: 178 lines in 4 files changed: 28 ins; 131 del; 19 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3495.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3495/head:pull/3495

PR: https://git.openjdk.java.net/jdk/pull/3495

Re: RFR: 8265135: Reduce work initializing VarForms [v4]

[Claes Redestad]

> This patch reduces work done initializing VarForms - mostly observed when 
> loading each VarHandle implementation class.
> 
> - Lazily resolve MemberNames.
> - Streamline MethodType creation. This reduces the number of MethodTypes 
> created. 
> 
> Net effect is a reduction in bytecode executed per VH class by 50-60%.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Simplify resolveMemberName

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3472/files
  - new: https://git.openjdk.java.net/jdk/pull/3472/files/dc56c25b..f84f5194

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3472=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3472=02-03

  Stats: 9 lines in 1 file changed: 0 ins; 4 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3472.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3472/head:pull/3472

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: RFR: 8265135: Reduce work initializing VarForms [v3]

[Mandy Chung]

On Wed, 14 Apr 2021 11:35:14 GMT, Claes Redestad  wrote:

>> This patch reduces work done initializing VarForms - mostly observed when 
>> loading each VarHandle implementation class.
>> 
>> - Lazily resolve MemberNames.
>> - Streamline MethodType creation. This reduces the number of MethodTypes 
>> created. 
>> 
>> Net effect is a reduction in bytecode executed per VH class by 50-60%.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revert UOE change, ensure VarHandle.isAccessModeSupported behaves well

src/java.base/share/classes/java/lang/invoke/VarForm.java line 136:

> 134: return memberName_table[mode]
> 135: = MethodHandles.Lookup.IMPL_LOOKUP
> 136: .resolveOrFail(REF_invokeStatic, implClass, 
> methodName, type);

An alternative way can be:


final MemberName getMemberName(int mode) {
 MemberName mn = getMemberNameOrNull(mode);
 if (mn == null) {
   throw new UnsupportedOperationException();
 }
 }   


This way `resolveMemberName(int mode)` can simply call 
IMPL_LOOKUP.resolveOrNull`.

-

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v2]

[Rémi Forax]

On Wed, 14 Apr 2021 12:27:55 GMT, Jim Laskey  wrote:

>> Move makeXXXSpilterator from public (@hidden) to protected. No API ch
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Make makeXXXSpliterator final

I don't see the point of having those methods virtual, they can be public 
static in RandomSupport given that there is only one implementation (the one 
with an if ... instanceof).
Also the static final proxy should be spelled PROXY.

-

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v3]

[Jaikiran Pai]

On Wed, 14 Apr 2021 17:14:55 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>> 
>> As noted in a comment in that issue, the bug relates to the return value of 
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The 
>> implementation has started returning invalid values for the `AM_PM` field 
>> after the "day period" support was added recently in the JDK as part of 
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>> 
>> The commit here adds a check in the internal implementation of the display 
>> name handling logic, to special case the `AM_PM` field and properly convert 
>> the display name array indexes (which is an internal detail) to valid values 
>> that represent the `AM_PM` calendar field.
>> 
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
>> reproduces this issue and verifies the fix.
>> 
>> After this fix was introduced, I ran the test in 
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
>> issue in the first place? To fix this, I have added an additional commit 
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update existing NarrowNamesTest to match expectations
>  - Naoto's review suggestion

Hello Naoto,

> CalendarNameProviderImpl is an implementation detail, which is shared between 
> j.u.Calendar and j.t.f.DateTimeFormatter(Builder) class, where it provides 
> localized names for Calendar fields. For the field Calendar.AM_PM, it 
> includes not only am/pm translations, but also day periods translations as 
> you see.

Thank you for those details. That's very helpful.

>I'd rather modify the code like:

if (field == AM_PM && !javatime && i > PM) {
continue;
} else {
map.put(name, base + i);
}


I've taken your suggestion and updated the code to match this. In the update, 
I've included a code comment to explain what this block does. Please do let me 
know if that code comment isn't accurate and needs any updates.

 The new `CalendarDisplayNamesTest` continues to pass with this change. I 
updated the existing `NarrowNamesTest` to match the expectations of the 
`Calendar.getDisplayName()` API.

All jtreg tests in `test/jdk/java/util/Calendar` and `test/jdk/java/time/` 
continue to pass too. I have triggered a local run of `tier1` tests and will 
check back the results early morning tomorrow.

Thank you for your reviews so far.

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: RFR: 8264208: Console charset API [v8]

[Naoto Sato]

> Please review the changes for the subject issue.  This has been suggested in 
> a recent discussion thread for the JEP 400 
> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>  A CSR has also been drafted, and comments are welcome 
> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Added @see links.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3419/files
  - new: https://git.openjdk.java.net/jdk/pull/3419/files/45ab77b3..5988f600

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=06-07

  Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3419.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v3]

[Jaikiran Pai]

> Can I please get a review for this proposed fix for 
> https://bugs.openjdk.java.net/browse/JDK-8262108?
> 
> As noted in a comment in that issue, the bug relates to the return value of 
> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation 
> has started returning invalid values for the `AM_PM` field after the "day 
> period" support was added recently in the JDK as part of 
> https://bugs.openjdk.java.net/browse/JDK-8262108.
> 
> The commit here adds a check in the internal implementation of the display 
> name handling logic, to special case the `AM_PM` field and properly convert 
> the display name array indexes (which is an internal detail) to valid values 
> that represent the `AM_PM` calendar field.
> 
> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
> reproduces this issue and verifies the fix.
> 
> After this fix was introduced, I ran the test in 
> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
> issue in the first place? To fix this, I have added an additional commit 
> which updates this test case to properly test the `AM_PM` field values.

Jaikiran Pai has updated the pull request incrementally with two additional 
commits since the last revision:

 - Update existing NarrowNamesTest to match expectations
 - Naoto's review suggestion

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3463/files
  - new: https://git.openjdk.java.net/jdk/pull/3463/files/84b11879..e7194733

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3463=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3463=01-02

  Stats: 33 lines in 2 files changed: 12 ins; 6 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3463.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3463/head:pull/3463

PR: https://git.openjdk.java.net/jdk/pull/3463

Re: Garbage Free Check

[Ralph Goers]

I don’t have an account in the bug tracking system. Could someone possibly 
update the issue noted below to indicate that Apache Log4j 2 would also like 
that enhancement?

Thanks,

Ralph

> On Apr 5, 2021, at 1:26 PM, Roger Riggs  wrote:
> 
> Hi,
> 
> Java does not have a data type with enough resolution to hold a full 
> nanosecond value.
> Hence the implementation of Instant holding seconds and nanos.
> 
> There is an long dormant enhancement request to return micro-seconds as a 
> long.
> 8196003  java.time Instant 
> and Duration methods for microseconds
> 
> That might be useful if the application gets enough resolution from 
> microseconds.
> There might be some clever interpolation between System.currentTimeMillis()
> and adjusting with System.nanoTime().
> Though it would likely not be exactly synchronized with the values from 
> Instant.
> 
> Regards, Roger
> 
> 
> On 4/5/21 3:56 PM, Brian Goetz wrote:
>> Project Valhalla will allow Instant to be migrated to a primitive class, 
>> which would address your problem.
>> 
>> On 4/2/2021 7:47 PM, Ralph Goers wrote:
>>> Log4j 2 supports the notion of a PreciseClock - one that can be initialized 
>>> to something more precise than a millisecond. At the same time it also 
>>> supports running with no heap allocations in certain circumstances. I am in 
>>> the process of moving our master branch to require Java 11 as the minimum. 
>>> In doing so I am encountering unit test errors while verifying that logging 
>>> is garbage free. They all occur allocating an Instant.
>>> 
>>> The code we have simply does
>>> 
>>> public void init(MutableInstant mutableInstant) {
>>>  Instant instant = java.time.Clock.systemUTC().instant();
>>> mutableInstant.initFromEpochSecond(instant.getEpochSecond(), 
>>> instant.getNano());
>>> }
>>> In our previous tests we had thought the allocation was being eliminated 
>>> due to escape analysis since the data is being extracted from the Instant 
>>> and not passed along. However, after upgrading the Google test library and 
>>> the JDK version it appears that is not the case.
>>> Ideally we would really like something like
>>> 
>>> public void init(MutableInstant mutableInstant) {
>>> java.time.Clock.systemUTC().initInstant(mutableInstant);
>>> }
>>> 
>>> where Mutable instant would implement an interface that has the two set 
>>> methods.The method would execute the same logic that is in the instant() 
>>> method but instead of creating a new Instant it would call the set methods 
>>> for the provided object.
>>> 
>>> This would allow us to either have the MutableInstants in ThreadLocals or 
>>> some other mechanism to ensure they are thread safe and have no heap 
>>> allocations. As it stands now I see no way to gain access to the higher 
>>> precision clock without memory allocation.
>>> 
>>> Do you know of another way to do this? Am I missing something?
>>> 
>>> Ralph
>> 
> 
> 

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v2]

[Uwe Schindler]

On Wed, 14 Apr 2021 12:27:55 GMT, Jim Laskey  wrote:

>> Move makeXXXSpilterator from public (@hidden) to protected. No API ch
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Make makeXXXSpliterator final

To me this looks fine. It is all a bit of hack, but this makes sure the public 
API of java.util.Random does not change in comparison to JDK 16.
The final protected method may be visible by IDEs but it cannot be called und 
is not overrideable as it is final.

-

Marked as reviewed by uschindler (Author).

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 8264208: Console charset API [v6]

[Naoto Sato]

On Wed, 14 Apr 2021 15:03:19 GMT, Alan Bateman  wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added comment to System.out/err init.
>
> src/java.base/share/classes/java/lang/System.java line 166:
> 
>> 164:  * exists, {@link Charset#defaultCharset()} otherwise.
>> 165:  */
>> 166: public static final PrintStream err = null;
> 
> I still think we need something in System.in to link to Console::charset. If 
> someone creates an InputStreamReader(System.in) then it will use the default 
> charset, they should be using Console::reader for these cases.

Added some explanation to it.

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8264208: Console charset API [v7]

[Naoto Sato]

> Please review the changes for the subject issue.  This has been suggested in 
> a recent discussion thread for the JEP 400 
> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>  A CSR has also been drafted, and comments are welcome 
> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Added Console::charset() relation with System.in

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3419/files
  - new: https://git.openjdk.java.net/jdk/pull/3419/files/9e323145..45ab77b3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=05-06

  Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3419.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419

PR: https://git.openjdk.java.net/jdk/pull/3419

Re: RFR: 8262883: doccheck: Broken links in java.base

[Michael McMahon]

On Wed, 14 Apr 2021 15:27:36 GMT, Alan Bateman  wrote:

>> Hi,
>> 
>> Could I get the following trivial doc changes reviewed please, caused by:
>> 
>> - broken  tags in MethodHandles referring to package.html instead of 
>> package-summary.html
>> 
>> - references to a package level #unixdomain anchor that no longer exists.
>> 
>> - a  tag missing a "../" in SocketChannel
>> 
>> Thanks,
>> 
>> Michael
>
> src/java.base/share/classes/java/nio/channels/DatagramChannel.java line 175:
> 
>> 173:  * connected.
>> 174:  *
>> 175:  * @apiNote {@linkplain java.net.UnixDomainSocketAddress Unix 
>> domain} sockets
> 
> The parameter to this method is a ProtocolFamily and maybe this note should 
> be say that StandardProtocolFamily#UNIX is not supported.

Okay, reasonable suggestion. I was a bit quick integrating. I will file a new 
issue to update the spec as suggested.

-

PR: https://git.openjdk.java.net/jdk/pull/3491

Integrated: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

[Aleksey Shipilev]

On Fri, 9 Apr 2021 09:41:19 GMT, Aleksey Shipilev  wrote:

> SonarCloud reports:
>   Cast one of the operands of this subtraction operation to a "long".
> 
> Here:
> 
> Spliterator makeSplitsSpliterator(long index, 
> long fence, SplittableGenerator source) {
> ...
> long multiplier = (1 << SALT_SHIFT) - 1; // < here
> 
> 
> The shift is integer, and the implicit cast to `long` happens too late. 
> `SALT_SHIFT` is currently 4, so this is not the problem yet. But it would 
> become a problem if `SALT_SHIFT` ever becomes 32 or larger. The shift operand 
> should be `1L` for safety. Observe:
> 
> 
> jshell> Long.toHexString((1 << 31) - 1)
> $2 ==> "7fff"
> 
> jshell> Long.toHexString((1 << 32) - 1)
> $3 ==> "0"
> 
> jshell> Long.toHexString((1L << 32) - 1)
> $4 ==> ""
> 
> 
> Additional testing: 
>  - [x] Linux x86_64 fastdebug, `jdk/utils/Random`

This pull request has now been integrated.

Changeset: 94067446
Author:Aleksey Shipilev 
URL:   https://git.openjdk.java.net/jdk/commit/94067446
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8264976: Minor numeric bug in 
AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

Reviewed-by: psandoz, jlaskey

-

PR: https://git.openjdk.java.net/jdk/pull/3409

Integrated: 8265174: Update Class.getDeclaredMethods to discuss synthetic and bridge methods

[Joe Darcy]

On Tue, 13 Apr 2021 22:18:58 GMT, Joe Darcy  wrote:

> The results from Class.getDeclaredMethods can include bridge and other 
> synthetic methods, which can be unexpected by users (JDK-6815786, 
> JDK-8142904) and appear to be inherited methods. The javadoc for 
> Class.getDeclaredMethods should be updated to explicitly mention the 
> possibility of synthetic methods appearing.

This pull request has now been integrated.

Changeset: 80026d81
Author:Joe Darcy 
URL:   https://git.openjdk.java.net/jdk/commit/80026d81
Stats: 28 lines in 5 files changed: 26 ins; 1 del; 1 mod

8265174: Update Class.getDeclaredMethods to discuss synthetic and bridge methods

Reviewed-by: jfranck

-

PR: https://git.openjdk.java.net/jdk/pull/3477

Re: RFR: 8262883: doccheck: Broken links in java.base

[Roger Riggs]

On Wed, 14 Apr 2021 14:03:01 GMT, Michael McMahon  wrote:

> Hi,
> 
> Could I get the following trivial doc changes reviewed please, caused by:
> 
> - broken  tags in MethodHandles referring to package.html instead of 
> package-summary.html
> 
> - references to a package level #unixdomain anchor that no longer exists.
> 
> - a  tag missing a "../" in SocketChannel
> 
> Thanks,
> 
> Michael

Integrating less than an hour after the PR is started seems to be a bit too 
quick to allow useful input.
Perhaps even for 'trivial' changes.

-

PR: https://git.openjdk.java.net/jdk/pull/3491

Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v2]

[Naoto Sato]

On Tue, 13 Apr 2021 15:03:28 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this proposed fix for 
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>> 
>> As noted in a comment in that issue, the bug relates to the return value of 
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The 
>> implementation has started returning invalid values for the `AM_PM` field 
>> after the "day period" support was added recently in the JDK as part of 
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>> 
>> The commit here adds a check in the internal implementation of the display 
>> name handling logic, to special case the `AM_PM` field and properly convert 
>> the display name array indexes (which is an internal detail) to valid values 
>> that represent the `AM_PM` calendar field.
>> 
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest` 
>> reproduces this issue and verifies the fix.
>> 
>> After this fix was introduced, I ran the test in 
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing 
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current 
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this 
>> issue in the first place? To fix this, I have added an additional commit 
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - copyright year and @bug id reference in existing test
>  - copyright year on source

Thanks for checking.
`CalendarNameProviderImpl` is an implementation detail, which is shared between 
`j.u.Calendar` and `j.t.f.DateTimeFormatter(Builder)` class, where it provides 
localized names for Calendar fields. For the field `Calendar.AM_PM`, it 
includes not only am/pm translations, but also day periods translations as you 
see. So blindly mapping day period translations to Calendar.AM/PM index is 
incorrect. I'd rather modify the code like:

if (field == AM_PM && !javatime && i > PM) {
continue;
} else {
map.put(name, base + i);
}

-

PR: https://git.openjdk.java.net/jdk/pull/3463

Integrated: 8265019 : Update tests for additional TestNG test permissions

[Lance Andersen]

On Tue, 13 Apr 2021 18:01:49 GMT, Lance Andersen  wrote:

> Hi all,
> 
> Please review the following patch which adds additional permissions needed 
> for when JTREG upgrades to a newer version of TestNG.
> 
> Best,
> Lance

This pull request has now been integrated.

Changeset: ffb37718
Author:Lance Andersen 
URL:   https://git.openjdk.java.net/jdk/commit/ffb37718
Stats: 22 lines in 2 files changed: 16 ins; 4 del; 2 mod

8265019: Update tests for additional TestNG test permissions

Reviewed-by: naoto, bpb, alanb

-

PR: https://git.openjdk.java.net/jdk/pull/3471

Re: RFR: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

[Jim Laskey]

On Fri, 9 Apr 2021 09:41:19 GMT, Aleksey Shipilev  wrote:

> SonarCloud reports:
>   Cast one of the operands of this subtraction operation to a "long".
> 
> Here:
> 
> Spliterator makeSplitsSpliterator(long index, 
> long fence, SplittableGenerator source) {
> ...
> long multiplier = (1 << SALT_SHIFT) - 1; // < here
> 
> 
> The shift is integer, and the implicit cast to `long` happens too late. 
> `SALT_SHIFT` is currently 4, so this is not the problem yet. But it would 
> become a problem if `SALT_SHIFT` ever becomes 32 or larger. The shift operand 
> should be `1L` for safety. Observe:
> 
> 
> jshell> Long.toHexString((1 << 31) - 1)
> $2 ==> "7fff"
> 
> jshell> Long.toHexString((1 << 32) - 1)
> $3 ==> "0"
> 
> jshell> Long.toHexString((1L << 32) - 1)
> $4 ==> ""
> 
> 
> Additional testing: 
>  - [x] Linux x86_64 fastdebug, `jdk/utils/Random`

Marked as reviewed by jlaskey (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3409

Re: RFR: 8262883: doccheck: Broken links in java.base

[Alan Bateman]

On Wed, 14 Apr 2021 14:03:01 GMT, Michael McMahon  wrote:

> Hi,
> 
> Could I get the following trivial doc changes reviewed please, caused by:
> 
> - broken  tags in MethodHandles referring to package.html instead of 
> package-summary.html
> 
> - references to a package level #unixdomain anchor that no longer exists.
> 
> - a  tag missing a "../" in SocketChannel
> 
> Thanks,
> 
> Michael

src/java.base/share/classes/java/nio/channels/DatagramChannel.java line 175:

> 173:  * connected.
> 174:  *
> 175:  * @apiNote {@linkplain java.net.UnixDomainSocketAddress Unix 
> domain} sockets

The parameter to this method is a ProtocolFamily and maybe this note should be 
say that StandardProtocolFamily#UNIX is not supported.

-

PR: https://git.openjdk.java.net/jdk/pull/3491

Re: RFR: 8265079: Implement VarHandle invoker caching [v3]

[Paul Sandoz]

On Wed, 14 Apr 2021 11:38:15 GMT, Jorn Vernee  wrote:

>> This patch implements 2 leftover TODOs for implementing var handle invoker 
>> MH caching (lambda forms for those were already shared/cached).
>> 
>> This piggybacks on the existing mechanism for method handle invoker caching.
>> 
>> Testing: Local testing `java/lang/invoke` tests. Tier 1-3
>> 
>> Thanks,
>> Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comment: simplify test

Marked as reviewed by psandoz (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3439

Re: RFR: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

[Paul Sandoz]

On Fri, 9 Apr 2021 09:41:19 GMT, Aleksey Shipilev  wrote:

> SonarCloud reports:
>   Cast one of the operands of this subtraction operation to a "long".
> 
> Here:
> 
> Spliterator makeSplitsSpliterator(long index, 
> long fence, SplittableGenerator source) {
> ...
> long multiplier = (1 << SALT_SHIFT) - 1; // < here
> 
> 
> The shift is integer, and the implicit cast to `long` happens too late. 
> `SALT_SHIFT` is currently 4, so this is not the problem yet. But it would 
> become a problem if `SALT_SHIFT` ever becomes 32 or larger. The shift operand 
> should be `1L` for safety. Observe:
> 
> 
> jshell> Long.toHexString((1 << 31) - 1)
> $2 ==> "7fff"
> 
> jshell> Long.toHexString((1 << 32) - 1)
> $3 ==> "0"
> 
> jshell> Long.toHexString((1L << 32) - 1)
> $4 ==> ""
> 
> 
> Additional testing: 
>  - [x] Linux x86_64 fastdebug, `jdk/utils/Random`

Marked as reviewed by psandoz (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3409

Re: RFR: 8265135: Reduce work initializing VarForms [v3]

[Paul Sandoz]

On Wed, 14 Apr 2021 11:31:55 GMT, Claes Redestad  wrote:

>> No, since VarHandles are not publicly extensible, the exception should not 
>> occur unless something has gone very wrong (the correspondence between 
>> access mode and implementing method is broken).
>
> Unfortunately the change to InternalError breaks a number of tests, since the 
> UOE does bubble up through the public API. I also found a few failing tests I 
> had overlooked due VarHandle.isAccessModeSupported throwing rather than 
> returning false, so I had to slightly rework the patch.

Oops, my bad. I got confused and forgot that VH implementations can avoid 
implementing access mode methods that would just throw UOE. This slightly 
complicates lazily resolution.

We don't cache the result of a failed method resolution, which would require a 
non-null sentinel value, probably does not matter.

Using `resolveOrNull` seems a better fit rather than catching and dropping ROE, 
less work performed too.

-

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: RFR: 8264208: Console charset API [v6]

[Alan Bateman]

On Tue, 13 Apr 2021 19:59:30 GMT, Naoto Sato  wrote:

>> Please review the changes for the subject issue.  This has been suggested in 
>> a recent discussion thread for the JEP 400 
>> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)].
>>  A CSR has also been drafted, and comments are welcome 
>> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)].
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comment to System.out/err init.

src/java.base/share/classes/java/lang/System.java line 166:

> 164:  * exists, {@link Charset#defaultCharset()} otherwise.
> 165:  */
> 166: public static final PrintStream err = null;

I still think we need something in System.in to link to Console::charset. If 
someone creates an InputStreamReader(System.in) then it will use the default 
charset, they should be using Console::reader for these cases.

-

PR: https://git.openjdk.java.net/jdk/pull/3419

Integrated: 8262883: doccheck: Broken links in java.base

[Michael McMahon]

On Wed, 14 Apr 2021 14:03:01 GMT, Michael McMahon  wrote:

> Hi,
> 
> Could I get the following trivial doc changes reviewed please, caused by:
> 
> - broken  tags in MethodHandles referring to package.html instead of 
> package-summary.html
> 
> - references to a package level #unixdomain anchor that no longer exists.
> 
> - a  tag missing a "../" in SocketChannel
> 
> Thanks,
> 
> Michael

This pull request has now been integrated.

Changeset: 46616909
Author:Michael McMahon 
URL:   https://git.openjdk.java.net/jdk/commit/46616909
Stats: 5 lines in 4 files changed: 0 ins; 0 del; 5 mod

8262883: doccheck: Broken links in java.base

Reviewed-by: lancea

-

PR: https://git.openjdk.java.net/jdk/pull/3491

Re: RFR: 8203359: Container level resources events [v8]

[Severin Gehwolf]

On Wed, 14 Apr 2021 12:03:12 GMT, Jaroslav Bachorik  
wrote:

>> src/jdk.jfr/share/classes/jdk/jfr/events/ContainerMemoryUsageEvent.java line 
>> 46:
>> 
>>> 44: public final class ContainerMemoryUsageEvent extends AbstractJDKEvent {
>>> 45: @Label("Memory Pressure")
>>> 46: @Description("(attempts per second * 1000), if enabled, that the 
>>> operating system tries to satisfy a memory request for any " +
>> 
>> This unit seems a bit strange. Do we really need to multiply by 1000?
>
> This is taken as reported by cgroups - I didn't want to change the semantics 
> so it does not confuse people familiar with cgroups.

I don't see this event field being set anywhere? Which Metrics API call is this 
using?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8262883: doccheck: Broken links in java.base

[Lance Andersen]

On Wed, 14 Apr 2021 14:03:01 GMT, Michael McMahon  wrote:

> Hi,
> 
> Could I get the following trivial doc changes reviewed please, caused by:
> 
> - broken  tags in MethodHandles referring to package.html instead of 
> package-summary.html
> 
> - references to a package level #unixdomain anchor that no longer exists.
> 
> - a  tag missing a "../" in SocketChannel
> 
> Thanks,
> 
> Michael

Marked as reviewed by lancea (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3491

RFR: 8265173: [test] divert spurious log output away from stream under test in ProcessBuilder Basic test

[Roger Riggs]

The most recent intermittent failure showed that the error occurred during VM 
initialization.
Only the tty output was diverted, but not log output.
Add diversion of log output as well tty output.

Add `-Xlog:all=warning:stderr` and `-Xlog:all=warning:stdout` to correspond to 
`-XX:+DisplayVMOutputToStderr` and `-XX:+DisplayVMOutputToStdout`

-

Commit messages:
 - 8265173: [test] divert spurious log output away from stream under test in 
ProcessBuilder Basic test

Changes: https://git.openjdk.java.net/jdk/pull/3492/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3492=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265173
  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3492.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3492/head:pull/3492

PR: https://git.openjdk.java.net/jdk/pull/3492

RFR: 8262883: doccheck: Broken links in java.base

[Michael McMahon]

Hi,

Could I get the following trivial doc changes reviewed please, caused by:

- broken  tags in MethodHandles referring to package.html instead of 
package-summary.html

- references to a package level #unixdomain anchor that no longer exists.

- a  tag missing a "../" in SocketChannel

Thanks,

Michael

-

Commit messages:
 - Fixed SocketChannel
 - fixed links in apidoc

Changes: https://git.openjdk.java.net/jdk/pull/3491/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3491=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262883
  Stats: 5 lines in 4 files changed: 0 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3491.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3491/head:pull/3491

PR: https://git.openjdk.java.net/jdk/pull/3491

Re: RFR: 8265075: Improve and simplify Class.resolveName()

[Сергей Цыпанов]

On Tue, 13 Apr 2021 12:47:50 GMT, Сергей Цыпанов 
 wrote:

> In mentioned method this code snippet
> 
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
> 
> 
> can be simplified with performance improvement as
> 
> name = baseName.replace('.', '/') + '/' + name;
> 
> Also null check of `baseName` can be removed as Class.getPackageName() never 
> returns null.
> 
> This benchmark
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ResolveNameBenchmark {
> 
>   private final Class klazz = getClass();
> 
>   @Benchmark
>   public Object original() {
> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   @Benchmark
>   public Object patched() {
> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   private String original(String name) {
> if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (baseName != null && !baseName.isEmpty()) {
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
>   }
> } else {
>   name = name.substring(1);
> }
> return name;
>   }
> 
>   private String patched(String name) {
>   if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (!baseName.isEmpty()) {
>   return baseName.replace('.', '/') + '/' + name;
>   }
>   return name;
>   }
>   return name.substring(1);
>   }
> 
>   private String getPackageName() {
> return klazz.getPackageName();
>   }
> }
> 
> demonstrates good improvement, especially as of memory consumption:
> 
> Mode  Cnt Score Error   
> Units
> 
> originalavgt   5057.974 ±   0.365   
> ns/op
> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
> MB/sec
> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
> B/op
> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
> MB/sec
> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
> B/op
> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
> MB/sec
> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
> B/op
> original:·gc.count  avgt   50   208.000
> counts
> original:·gc.time   avgt   50   224.000   
>  ms
> 
> patched avgt   5044.367 ±   0.162   
> ns/op
> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
> MB/sec
> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
> B/op
> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
> MB/sec
> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
> B/op
> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
> MB/sec
> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
> B/op
> patched:·gc.count   avgt   50   167.000
> counts
> patched:·gc.timeavgt   50   181.000   
>  ms

Ok, then I close this one

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Withdrawn: 8265075: Improve and simplify Class.resolveName()

[Сергей Цыпанов]

On Tue, 13 Apr 2021 12:47:50 GMT, Сергей Цыпанов 
 wrote:

> In mentioned method this code snippet
> 
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
> 
> 
> can be simplified with performance improvement as
> 
> name = baseName.replace('.', '/') + '/' + name;
> 
> Also null check of `baseName` can be removed as Class.getPackageName() never 
> returns null.
> 
> This benchmark
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ResolveNameBenchmark {
> 
>   private final Class klazz = getClass();
> 
>   @Benchmark
>   public Object original() {
> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   @Benchmark
>   public Object patched() {
> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   private String original(String name) {
> if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (baseName != null && !baseName.isEmpty()) {
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
>   }
> } else {
>   name = name.substring(1);
> }
> return name;
>   }
> 
>   private String patched(String name) {
>   if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (!baseName.isEmpty()) {
>   return baseName.replace('.', '/') + '/' + name;
>   }
>   return name;
>   }
>   return name.substring(1);
>   }
> 
>   private String getPackageName() {
> return klazz.getPackageName();
>   }
> }
> 
> demonstrates good improvement, especially as of memory consumption:
> 
> Mode  Cnt Score Error   
> Units
> 
> originalavgt   5057.974 ±   0.365   
> ns/op
> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
> MB/sec
> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
> B/op
> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
> MB/sec
> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
> B/op
> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
> MB/sec
> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
> B/op
> original:·gc.count  avgt   50   208.000
> counts
> original:·gc.time   avgt   50   224.000   
>  ms
> 
> patched avgt   5044.367 ±   0.162   
> ns/op
> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
> MB/sec
> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
> B/op
> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
> MB/sec
> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
> B/op
> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
> MB/sec
> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
> B/op
> patched:·gc.count   avgt   50   167.000
> counts
> patched:·gc.timeavgt   50   181.000   
>  ms

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Re: RFR: 8265075: Improve and simplify Class.resolveName()

[Alan Bateman]

On Tue, 13 Apr 2021 12:47:50 GMT, Сергей Цыпанов 
 wrote:

> In mentioned method this code snippet
> 
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
> 
> 
> can be simplified with performance improvement as
> 
> name = baseName.replace('.', '/') + '/' + name;
> 
> Also null check of `baseName` can be removed as Class.getPackageName() never 
> returns null.
> 
> This benchmark
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ResolveNameBenchmark {
> 
>   private final Class klazz = getClass();
> 
>   @Benchmark
>   public Object original() {
> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   @Benchmark
>   public Object patched() {
> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   private String original(String name) {
> if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (baseName != null && !baseName.isEmpty()) {
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
>   }
> } else {
>   name = name.substring(1);
> }
> return name;
>   }
> 
>   private String patched(String name) {
>   if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (!baseName.isEmpty()) {
>   return baseName.replace('.', '/') + '/' + name;
>   }
>   return name;
>   }
>   return name.substring(1);
>   }
> 
>   private String getPackageName() {
> return klazz.getPackageName();
>   }
> }
> 
> demonstrates good improvement, especially as of memory consumption:
> 
> Mode  Cnt Score Error   
> Units
> 
> originalavgt   5057.974 ±   0.365   
> ns/op
> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
> MB/sec
> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
> B/op
> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
> MB/sec
> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
> B/op
> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
> MB/sec
> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
> B/op
> original:·gc.count  avgt   50   208.000
> counts
> original:·gc.time   avgt   50   224.000   
>  ms
> 
> patched avgt   5044.367 ±   0.162   
> ns/op
> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
> MB/sec
> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
> B/op
> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
> MB/sec
> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
> B/op
> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
> MB/sec
> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
> B/op
> patched:·gc.count   avgt   50   167.000
> counts
> patched:·gc.timeavgt   50   181.000   
>  ms

If you have the time, then looking for usages of getPackageName where it checks 
for null would be useful. We can close this PR and open a new issue/PR for that 
cleanup.

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v3]

[Andy Herrick]

On Tue, 13 Apr 2021 22:50:12 GMT, Andy Herrick  wrote:

>> two changes:
>> One to jpackage, when recursively removing directory, when IOException 
>> occurs, record it and continue (deleting as much as possible) before 
>> throwing the exception.
>> The other to tests, when running jpackage via ProcessBuilder.execute(), set 
>> the "TMP" environment variable to the current value of System Property 
>> "java.io.tmpdir".  This causes the sub-process (jpackage) to output tmp 
>> files to the tmp file location used by the test. (So the test harness can 
>> clean up after test exits).
>
> Andy Herrick has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8265078: jpackage tests on Windows leave large temp files

Although jpackage failing to clean up has only been seen as an issue on 
Windows, allowing it to write outside the test directory (in default tmp dir) 
still exists on mac and linux - I will look into enhancing (what is now 
setWindowsTmpDir()) to be usefull on all platforms.

-

PR: https://git.openjdk.java.net/jdk/pull/3473

Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v2]

[Jim Laskey]

> Move makeXXXSpilterator from public (@hidden) to protected. No API ch

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Make makeXXXSpliterator final

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3469/files
  - new: https://git.openjdk.java.net/jdk/pull/3469/files/4bde44fc..a6e35c6b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3469=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3469=00-01

  Stats: 79 lines in 2 files changed: 29 ins; 43 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3469.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3469/head:pull/3469

PR: https://git.openjdk.java.net/jdk/pull/3469

Re: RFR: 8203359: Container level resources events [v8]

[Jaroslav Bachorik]

On Wed, 14 Apr 2021 10:25:09 GMT, Erik Gahlin  wrote:

>> Jaroslav Bachorik has updated the pull request with a new target base due to 
>> a merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 11 additional 
>> commits since the last revision:
>> 
>>  - Roll back conditional registration of container events
>>  - Remove container events flag
>>  - Remove trailing spaces
>>  - Doh
>>  - Report container type and register events conditionally
>>  - Remove unused test files
>>  - Initial test support for JFR container events
>>  - Update the JFR control files
>>  - Split off the CPU throttling metrics
>>  - Formatting spaces
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/19aad098...67a61bd7
>
> src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUThrottlingEvent.java 
> line 44:
> 
>> 42: @Category({"Operating System", "Container", "Processor"})
>> 43: @Description("Container CPU throttling related information.")
>> 44: public class ContainerCPUThrottlingEvent extends AbstractJDKEvent {
> 
> I wonder if we should put all the container events in the same category 
> {"Operating System", "Container"}, Or possibly add them under the already 
> existing categories under "Operating System"?

I guess we could fit those events under `Operating System/Memory` and 
`Operating System/Processor` categories.
What about I/O? Currently, there is only `Operating System/Network` category. 
The options are:
* Add `Operating System/IO` category and move `Network` to `Operating 
System/IO/Network`
* Add `Operation System/FileSystem` category and move the container IO event 
there

What would you prefer?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v8]

[Jaroslav Bachorik]

On Wed, 14 Apr 2021 11:17:14 GMT, Erik Gahlin  wrote:

>> Jaroslav Bachorik has updated the pull request with a new target base due to 
>> a merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 11 additional 
>> commits since the last revision:
>> 
>>  - Roll back conditional registration of container events
>>  - Remove container events flag
>>  - Remove trailing spaces
>>  - Doh
>>  - Report container type and register events conditionally
>>  - Remove unused test files
>>  - Initial test support for JFR container events
>>  - Update the JFR control files
>>  - Split off the CPU throttling metrics
>>  - Formatting spaces
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/b72abe91...67a61bd7
>
> src/jdk.jfr/share/classes/jdk/jfr/events/ContainerMemoryUsageEvent.java line 
> 46:
> 
>> 44: public final class ContainerMemoryUsageEvent extends AbstractJDKEvent {
>> 45: @Label("Memory Pressure")
>> 46: @Description("(attempts per second * 1000), if enabled, that the 
>> operating system tries to satisfy a memory request for any " +
> 
> This unit seems a bit strange. Do we really need to multiply by 1000?

This is taken as reported by cgroups - I didn't want to change the semantics so 
it does not confuse people familiar with cgroups.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8265079: Implement VarHandle invoker caching [v3]

[Claes Redestad]

On Wed, 14 Apr 2021 11:38:15 GMT, Jorn Vernee  wrote:

>> This patch implements 2 leftover TODOs for implementing var handle invoker 
>> MH caching (lambda forms for those were already shared/cached).
>> 
>> This piggybacks on the existing mechanism for method handle invoker caching.
>> 
>> Testing: Local testing `java/lang/invoke` tests. Tier 1-3
>> 
>> Thanks,
>> Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comment: simplify test

Marked as reviewed by redestad (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3439

Re: RFR: 8265075: Improve and simplify Class.resolveName()

[Сергей Цыпанов]

On Wed, 14 Apr 2021 11:41:09 GMT, Alan Bateman  wrote:

>> In mentioned method this code snippet
>> 
>> int len = baseName.length() + 1 + name.length();
>> StringBuilder sb = new StringBuilder(len);
>> name = sb.append(baseName.replace('.', '/'))
>> .append('/')
>> .append(name)
>> .toString();
>> 
>> 
>> can be simplified with performance improvement as
>> 
>> name = baseName.replace('.', '/') + '/' + name;
>> 
>> Also null check of `baseName` can be removed as Class.getPackageName() never 
>> returns null.
>> 
>> This benchmark
>> 
>> @State(Scope.Thread)
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class ResolveNameBenchmark {
>> 
>>   private final Class klazz = getClass();
>> 
>>   @Benchmark
>>   public Object original() {
>> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>>   }
>> 
>>   @Benchmark
>>   public Object patched() {
>> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>>   }
>> 
>>   private String original(String name) {
>> if (!name.startsWith("/")) {
>>   String baseName = getPackageName();
>>   if (baseName != null && !baseName.isEmpty()) {
>> int len = baseName.length() + 1 + name.length();
>> StringBuilder sb = new StringBuilder(len);
>> name = sb.append(baseName.replace('.', '/'))
>> .append('/')
>> .append(name)
>> .toString();
>>   }
>> } else {
>>   name = name.substring(1);
>> }
>> return name;
>>   }
>> 
>>   private String patched(String name) {
>>   if (!name.startsWith("/")) {
>>   String baseName = getPackageName();
>>   if (!baseName.isEmpty()) {
>>   return baseName.replace('.', '/') + '/' + name;
>>   }
>>   return name;
>>   }
>>   return name.substring(1);
>>   }
>> 
>>   private String getPackageName() {
>> return klazz.getPackageName();
>>   }
>> }
>> 
>> demonstrates good improvement, especially as of memory consumption:
>> 
>> Mode  Cnt Score Error   
>> Units
>> 
>> originalavgt   5057.974 ±   0.365   
>> ns/op
>> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
>> MB/sec
>> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
>> B/op
>> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
>> MB/sec
>> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
>> B/op
>> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
>> MB/sec
>> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
>> B/op
>> original:·gc.count  avgt   50   208.000
>> counts
>> original:·gc.time   avgt   50   224.000  
>>   ms
>> 
>> patched avgt   5044.367 ±   0.162   
>> ns/op
>> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
>> MB/sec
>> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
>> B/op
>> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
>> MB/sec
>> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
>> B/op
>> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
>> MB/sec
>> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
>> B/op
>> patched:·gc.count   avgt   50   167.000
>> counts
>> patched:·gc.timeavgt   50   181.000  
>>   ms
>
> We can remove the check for baseName being null, that's a left over from 
> iteration during JDK 9.

@AlanBateman should I then do it only for `Class.resolveName()` or everywhere 
we call `Class.getPackageName()`?

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Re: RFR: 8265075: Improve and simplify Class.resolveName()

[Сергей Цыпанов]

On Tue, 13 Apr 2021 14:36:39 GMT, Alan Bateman  wrote:

>> In mentioned method this code snippet
>> 
>> int len = baseName.length() + 1 + name.length();
>> StringBuilder sb = new StringBuilder(len);
>> name = sb.append(baseName.replace('.', '/'))
>> .append('/')
>> .append(name)
>> .toString();
>> 
>> 
>> can be simplified with performance improvement as
>> 
>> name = baseName.replace('.', '/') + '/' + name;
>> 
>> Also null check of `baseName` can be removed as Class.getPackageName() never 
>> returns null.
>> 
>> This benchmark
>> 
>> @State(Scope.Thread)
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class ResolveNameBenchmark {
>> 
>>   private final Class klazz = getClass();
>> 
>>   @Benchmark
>>   public Object original() {
>> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>>   }
>> 
>>   @Benchmark
>>   public Object patched() {
>> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>>   }
>> 
>>   private String original(String name) {
>> if (!name.startsWith("/")) {
>>   String baseName = getPackageName();
>>   if (baseName != null && !baseName.isEmpty()) {
>> int len = baseName.length() + 1 + name.length();
>> StringBuilder sb = new StringBuilder(len);
>> name = sb.append(baseName.replace('.', '/'))
>> .append('/')
>> .append(name)
>> .toString();
>>   }
>> } else {
>>   name = name.substring(1);
>> }
>> return name;
>>   }
>> 
>>   private String patched(String name) {
>>   if (!name.startsWith("/")) {
>>   String baseName = getPackageName();
>>   if (!baseName.isEmpty()) {
>>   return baseName.replace('.', '/') + '/' + name;
>>   }
>>   return name;
>>   }
>>   return name.substring(1);
>>   }
>> 
>>   private String getPackageName() {
>> return klazz.getPackageName();
>>   }
>> }
>> 
>> demonstrates good improvement, especially as of memory consumption:
>> 
>> Mode  Cnt Score Error   
>> Units
>> 
>> originalavgt   5057.974 ±   0.365   
>> ns/op
>> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
>> MB/sec
>> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
>> B/op
>> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
>> MB/sec
>> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
>> B/op
>> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
>> MB/sec
>> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
>> B/op
>> original:·gc.count  avgt   50   208.000
>> counts
>> original:·gc.time   avgt   50   224.000  
>>   ms
>> 
>> patched avgt   5044.367 ±   0.162   
>> ns/op
>> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
>> MB/sec
>> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
>> B/op
>> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
>> MB/sec
>> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
>> B/op
>> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
>> MB/sec
>> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
>> B/op
>> patched:·gc.count   avgt   50   167.000
>> counts
>> patched:·gc.timeavgt   50   181.000  
>>   ms
>
> src/java.base/share/classes/java/lang/Class.java line 3067:
> 
>> 3065:  */
>> 3066: private String resolveName(String name) {
>> 3067: if (!name.startsWith("/")) {
> 
> I expect this would be more more readable if you invert it, i.e. "if 
> (name.startsWith("/") { return name.substring(1); } else { ... }.
> 
> Note that the baseName == null check was needed when it was originally 
> created because getPackageName could return null in the initial version.

@AlanBateman As Peter commented below there's likely to be no improvement when 
the code is called from `j.l.Class` itself, and indeed there's no any. 

However, there's still unnecessary null check here and in other places, so is 
it reasonable to reword the ticker and this PR to rid that check, or create 
another ticket, or it's not worth the effort? What do you thinl?

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Re: RFR: 8265075: Improve and simplify Class.resolveName()

[Alan Bateman]

On Tue, 13 Apr 2021 12:47:50 GMT, Сергей Цыпанов 
 wrote:

> In mentioned method this code snippet
> 
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
> 
> 
> can be simplified with performance improvement as
> 
> name = baseName.replace('.', '/') + '/' + name;
> 
> Also null check of `baseName` can be removed as Class.getPackageName() never 
> returns null.
> 
> This benchmark
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ResolveNameBenchmark {
> 
>   private final Class klazz = getClass();
> 
>   @Benchmark
>   public Object original() {
> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   @Benchmark
>   public Object patched() {
> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   private String original(String name) {
> if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (baseName != null && !baseName.isEmpty()) {
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
>   }
> } else {
>   name = name.substring(1);
> }
> return name;
>   }
> 
>   private String patched(String name) {
>   if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (!baseName.isEmpty()) {
>   return baseName.replace('.', '/') + '/' + name;
>   }
>   return name;
>   }
>   return name.substring(1);
>   }
> 
>   private String getPackageName() {
> return klazz.getPackageName();
>   }
> }
> 
> demonstrates good improvement, especially as of memory consumption:
> 
> Mode  Cnt Score Error   
> Units
> 
> originalavgt   5057.974 ±   0.365   
> ns/op
> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
> MB/sec
> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
> B/op
> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
> MB/sec
> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
> B/op
> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
> MB/sec
> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
> B/op
> original:·gc.count  avgt   50   208.000
> counts
> original:·gc.time   avgt   50   224.000   
>  ms
> 
> patched avgt   5044.367 ±   0.162   
> ns/op
> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
> MB/sec
> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
> B/op
> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
> MB/sec
> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
> B/op
> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
> MB/sec
> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
> B/op
> patched:·gc.count   avgt   50   167.000
> counts
> patched:·gc.timeavgt   50   181.000   
>  ms

We can remove the check for baseName being null, that's a left over from 
iteration during JDK 9.

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Re: RFR: 8265079: Implement VarHandle invoker caching [v2]

[Jorn Vernee]

On Tue, 13 Apr 2021 15:24:13 GMT, Paul Sandoz  wrote:

>> Jorn Vernee has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments:
>>   - Use boolean instead of index for var handle cache
>
> test/jdk/java/lang/invoke/TestVHInvokerCaching.java line 88:
> 
>> 86: MethodHandles.Lookup lookup = lookup();
>> 87: 
>> 88: for (Class type : TEST_TYPES) {
> 
> A simpler approach would be to iterate over the fields of `Holder` and use 
> `unreflectVarHandle`, then you can remove `TEST_TYPES`.

Yeah, that's a good idea. Fixed

-

PR: https://git.openjdk.java.net/jdk/pull/3439

Re: RFR: 8265079: Implement VarHandle invoker caching [v3]

[Jorn Vernee]

> This patch implements 2 leftover TODOs for implementing var handle invoker MH 
> caching (lambda forms for those were already shared/cached).
> 
> This piggybacks on the existing mechanism for method handle invoker caching.
> 
> Testing: Local testing `java/lang/invoke` tests. Tier 1-3
> 
> Thanks,
> Jorn

Jorn Vernee has updated the pull request incrementally with one additional 
commit since the last revision:

  Review comment: simplify test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3439/files
  - new: https://git.openjdk.java.net/jdk/pull/3439/files/93681f77..fa5a721f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3439=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3439=01-02

  Stats: 19 lines in 1 file changed: 4 ins; 12 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3439.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3439/head:pull/3439

PR: https://git.openjdk.java.net/jdk/pull/3439

Re: RFR: 8265075: Improve and simplify Class.resolveName()

[Сергей Цыпанов]

On Tue, 13 Apr 2021 12:47:50 GMT, Сергей Цыпанов 
 wrote:

> In mentioned method this code snippet
> 
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
> 
> 
> can be simplified with performance improvement as
> 
> name = baseName.replace('.', '/') + '/' + name;
> 
> Also null check of `baseName` can be removed as Class.getPackageName() never 
> returns null.
> 
> This benchmark
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ResolveNameBenchmark {
> 
>   private final Class klazz = getClass();
> 
>   @Benchmark
>   public Object original() {
> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   @Benchmark
>   public Object patched() {
> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
>   }
> 
>   private String original(String name) {
> if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (baseName != null && !baseName.isEmpty()) {
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
>   }
> } else {
>   name = name.substring(1);
> }
> return name;
>   }
> 
>   private String patched(String name) {
>   if (!name.startsWith("/")) {
>   String baseName = getPackageName();
>   if (!baseName.isEmpty()) {
>   return baseName.replace('.', '/') + '/' + name;
>   }
>   return name;
>   }
>   return name.substring(1);
>   }
> 
>   private String getPackageName() {
> return klazz.getPackageName();
>   }
> }
> 
> demonstrates good improvement, especially as of memory consumption:
> 
> Mode  Cnt Score Error   
> Units
> 
> originalavgt   5057.974 ±   0.365   
> ns/op
> original:·gc.alloc.rate avgt   50  3419.447 ±  21.154  
> MB/sec
> original:·gc.alloc.rate.normavgt   50   312.006 ±   0.001
> B/op
> original:·gc.churn.G1_Eden_Spaceavgt   50  3399.396 ± 149.836  
> MB/sec
> original:·gc.churn.G1_Eden_Space.norm   avgt   50   310.198 ±  13.628
> B/op
> original:·gc.churn.G1_Survivor_Spaceavgt   50 0.004 ±   0.001  
> MB/sec
> original:·gc.churn.G1_Survivor_Space.norm   avgt   50≈ 10⁻³  
> B/op
> original:·gc.count  avgt   50   208.000
> counts
> original:·gc.time   avgt   50   224.000   
>  ms
> 
> patched avgt   5044.367 ±   0.162   
> ns/op
> patched:·gc.alloc.rate  avgt   50  2749.265 ±  10.014  
> MB/sec
> patched:·gc.alloc.rate.norm avgt   50   192.004 ±   0.001
> B/op
> patched:·gc.churn.G1_Eden_Space avgt   50  2729.221 ± 193.552  
> MB/sec
> patched:·gc.churn.G1_Eden_Space.normavgt   50   190.615 ±  13.539
> B/op
> patched:·gc.churn.G1_Survivor_Space avgt   50 0.003 ±   0.001  
> MB/sec
> patched:·gc.churn.G1_Survivor_Space.normavgt   50≈ 10⁻⁴  
> B/op
> patched:·gc.count   avgt   50   167.000
> counts
> patched:·gc.timeavgt   50   181.000   
>  ms

@plevart you are right, when the method is called from `java.lang.Class` 
there's no improvement observed. I'll close the PR then along with the ticket

-

PR: https://git.openjdk.java.net/jdk/pull/3464

Re: RFR: 8265135: Reduce work initializing VarForms [v3]

[Claes Redestad]

> This patch reduces work done initializing VarForms - mostly observed when 
> loading each VarHandle implementation class.
> 
> - Lazily resolve MemberNames.
> - Streamline MethodType creation. This reduces the number of MethodTypes 
> created. 
> 
> Net effect is a reduction in bytecode executed per VH class by 50-60%.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert UOE change, ensure VarHandle.isAccessModeSupported behaves well

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3472/files
  - new: https://git.openjdk.java.net/jdk/pull/3472/files/708c0e28..dc56c25b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3472=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3472=01-02

  Stats: 25 lines in 2 files changed: 13 ins; 6 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3472.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3472/head:pull/3472

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: RFR: 8265135: Reduce work initializing VarForms [v3]

[Claes Redestad]

On Wed, 14 Apr 2021 00:46:36 GMT, Paul Sandoz  wrote:

>> Thanks for reviewing!
>> 
>> Is there's a way to provoke this exception through the public API? If not 
>> then the suggested behavior change seems reasonable.
>
> No, since VarHandles are not publicly extensible, the exception should not 
> occur unless something has gone very wrong (the correspondence between access 
> mode and implementing method is broken).

Unfortunately the change to InternalError breaks a number of tests, since the 
UOE does bubble up through the public API. I also found a few failing tests I 
had overlooked due VarHandle.isAccessModeSupported throwing rather than 
returning false, so I had to slightly rework the patch.

-

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: RFR: 8203359: Container level resources events [v8]

[Erik Gahlin]

On Wed, 14 Apr 2021 08:31:55 GMT, Jaroslav Bachorik  
wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 11 additional 
> commits since the last revision:
> 
>  - Roll back conditional registration of container events
>  - Remove container events flag
>  - Remove trailing spaces
>  - Doh
>  - Report container type and register events conditionally
>  - Remove unused test files
>  - Initial test support for JFR container events
>  - Update the JFR control files
>  - Split off the CPU throttling metrics
>  - Formatting spaces
>  - ... and 1 more: 
> https://git.openjdk.java.net/jdk/compare/1f93be70...67a61bd7

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUThrottlingEvent.java line 
44:

> 42: @Category({"Operating System", "Container", "Processor"})
> 43: @Description("Container CPU throttling related information.")
> 44: public class ContainerCPUThrottlingEvent extends AbstractJDKEvent {

I wonder if we should put all the container events in the same category 
{"Operating System", "Container"}, Or possibly add them under the already 
existing categories under "Operating System"?

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUThrottlingEvent.java line 
46:

> 44: public class ContainerCPUThrottlingEvent extends AbstractJDKEvent {
> 45:   @Label("CPU Elapsed Slices")
> 46:   @Description("Number of time-slice periods that have elapsed if a CPU 
> quota has been setup for the container.")

If the description is one sentence, period should not be included.

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUUsageEvent.java line 46:

> 44: public class ContainerCPUUsageEvent extends AbstractJDKEvent {
> 45:   @Label("CPU Time")
> 46:   @Description("Aggregate time, in nanoseconds, consumed by all tasks in 
> the container.")

We usually skip the unit "nanoseconds" in descriptions when the field has a 
content type that describes the unit.

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerConfigurationEvent.java line 
45:

> 43: @Description("A set of container specific attributes.")
> 44: public final class ContainerConfigurationEvent extends AbstractJDKEvent {
> 45: @Label("Container type")

Capitalize "type" in the label

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerConfigurationEvent.java line 
78:

> 76: 
> 77: @Label("Memory and Swap Limit")
> 78: @Description("Maximum amount of physical memory and swap space, in 
> bytes, that can be allocated in the container.")

No need to mention bytes in the description when the field has DataAmount 
annotation.

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerIOUsageEvent.java line 47:

> 45: public class ContainerIOUsageEvent extends AbstractJDKEvent {
> 46: 
> 47:   @Label("BlkIO Request Count")

Change to "Block IO"

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerMemoryUsageEvent.java line 46:

> 44: public final class ContainerMemoryUsageEvent extends AbstractJDKEvent {
> 45: @Label("Memory Pressure")
> 46: @Description("(attempts per second * 1000), if enabled, that the 
> operating system tries to satisfy a memory request for any " +

This unit seems a bit strange. Do we really need to multiply by 1000?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v3]

[Lance Andersen]

> Hi all,
> 
> Please review the following patch which adds additional permissions needed 
> for when JTREG upgrades to a newer version of TestNG.
> 
> Best,
> Lance

Lance Andersen has updated the pull request incrementally with one additional 
commit since the last revision:

  Add back individual security imports

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3471/files
  - new: https://git.openjdk.java.net/jdk/pull/3471/files/20ef2bd0..31edda8e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3471=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3471=01-02

  Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3471.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3471/head:pull/3471

PR: https://git.openjdk.java.net/jdk/pull/3471

Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]

[Lance Andersen]

On Apr 14, 2021, at 2:13 AM, Alan Bateman 
mailto:al...@openjdk.java.net>> wrote:

On Tue, 13 Apr 2021 18:56:22 GMT, Lance Andersen 
mailto:lan...@openjdk.org>> wrote:

Hi all,

Please review the following patch which adds additional permissions needed for 
when JTREG upgrades to a newer version of TestNG.

Best,
Lance

Lance Andersen has updated the pull request incrementally with one additional 
commit since the last revision:

 TestNG updates Permission re-organization

Marked as reviewed by alanb (Reviewer).

test/jdk/java/sql/testng/util/TestPolicy.java line 27:

25: import java.io.FilePermission;
26: import java.lang.reflect.ReflectPermission;
27: import java.security.*;

My guess is that you didn't mean the change the import.

Intellij did that automagically.  I just reverted the change for consistency

-

PR: https://git.openjdk.java.net/jdk/pull/3471

[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8265135: Reduce work initializing VarForms [v2]

[Claes Redestad]

> This patch reduces work done initializing VarForms - mostly observed when 
> loading each VarHandle implementation class.
> 
> - Lazily resolve MemberNames.
> - Streamline MethodType creation. This reduces the number of MethodTypes 
> created. 
> 
> Net effect is a reduction in bytecode executed per VH class by 50-60%.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Change from UOE to InternalError

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3472/files
  - new: https://git.openjdk.java.net/jdk/pull/3472/files/f2067686..708c0e28

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3472=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3472=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3472.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3472/head:pull/3472

PR: https://git.openjdk.java.net/jdk/pull/3472

Re: ObjectMethods seems generating wrong methods for array-type field

[Remi Forax]

- Mail original -
> De: "Kengo TODA" 
> À: "core-libs-dev" 
> Envoyé: Mercredi 14 Avril 2021 11:03:14
> Objet: Re: ObjectMethods seems generating wrong methods for array-type field

Hello,

> I found that the JLS 16 does not cover the array type record component:
> https://docs.oracle.com/javase/specs/jls/se16/html/jls-8.html#jls-8.10.3
> 
> So it could be not a core-lib issue but a JLS issue.

Nope, the implementation of a record is covered in the javadoc of 
java.lang.Record
for equals, see 
https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/lang/Record.html#equals(java.lang.Object)

As you can see the spec clearly says that for any fields which is an object 
(arrays are objects), it should be semantically equivalent to calling 
java.util.Objects.equals() on those fields.

So it's not a bug, it's how record.equals() works, as Raffaello said, records 
are dumb containers, this is how we have specified them.

Records have been specified as part of the project Amber, you can take a look 
to the mailing list [1] for more if you want.

> 
> Do I need to forward this thread to another mailing list?
> If so, please let me know which is the preferred one.
> I've checked https://mail.openjdk.java.net/mailman/listinfo but not so sure.
> 
> 
> Thank you very much.

regards,
Rémi

[1] https://mail.openjdk.java.net/mailman/listinfo/amber-spec-experts

> 
> ***
> Kengo TODA
> skypen...@gmail.com
> 
> 
> On Wed, Apr 14, 2021 at 9:04 AM Kengo TODA  wrote:
> 
>> Hello there,
>>
>>
>> I'm Kengo TODA, an engineer learning about the Record feature.
>> Today I found a nonintentional behavior, and it seems that the bug
>> database has no info about it.
>> Let me ask here to confirm it's by-design or not. If it's a bug, I want to
>> try to send a patch to OpenJDK.
>>
>> Here is the code reproducing the nonintentional behavior:
>> https://gist.github.com/KengoTODA/4d7ef6a5226d347ad9385241fee6bc63
>>
>> In short, the ObjectMethods class in OpenJDK v16 generates code
>> that invokes the fields' method, even when the field is an array.
>>
>> Please help me to understand this behavior, or
>> make an entry in the bug database to propose a patch.
>> Thanks in advance! :)
>>
>> ***
>> Kengo TODA
>> skypen...@gmail.com

Re: ObjectMethods seems generating wrong methods for array-type field

[Kengo TODA]

I found that the JLS 16 does not cover the array type record component:
https://docs.oracle.com/javase/specs/jls/se16/html/jls-8.html#jls-8.10.3

So it could be not a core-lib issue but a JLS issue.

Do I need to forward this thread to another mailing list?
If so, please let me know which is the preferred one.
I've checked https://mail.openjdk.java.net/mailman/listinfo but not so sure.


Thank you very much.

***
Kengo TODA
skypen...@gmail.com


On Wed, Apr 14, 2021 at 9:04 AM Kengo TODA  wrote:

> Hello there,
>
>
> I'm Kengo TODA, an engineer learning about the Record feature.
> Today I found a nonintentional behavior, and it seems that the bug
> database has no info about it.
> Let me ask here to confirm it's by-design or not. If it's a bug, I want to
> try to send a patch to OpenJDK.
>
> Here is the code reproducing the nonintentional behavior:
> https://gist.github.com/KengoTODA/4d7ef6a5226d347ad9385241fee6bc63
>
> In short, the ObjectMethods class in OpenJDK v16 generates code
> that invokes the fields' method, even when the field is an array.
>
> Please help me to understand this behavior, or
> make an entry in the bug database to propose a patch.
> Thanks in advance! :)
>
> ***
> Kengo TODA
> skypen...@gmail.com
>

Re: ObjectMethods seems generating wrong methods for array-type field

[Raffaello Giulietti]

Hi Kengo,

I don't think this is a bug, as records are designed to "act as 
transparent carriers for immutable data". A record having an array-typed 
component is far from being immutable because everyone can change the 
array contents. So you probably shouldn't opt for records for such use 
cases, since you would loose the semantic benefits of records.


But if you need deeper hashing and comparison over array-typed 
components of records, as you are probably expecting in your example, 
you have to override the record hashCode() and equals() yourself. But 
then (http://openjdk.java.net/jeps/395):
"Any explicit implementation of accessors or the equals or hashCode 
methods should be careful to preserve the semantic invariants of the 
record class."



Greetings
Raffaello




Hello there,


I'm Kengo TODA, an engineer learning about the Record feature.
Today I found a nonintentional behavior, and it seems that the bug database
has no info about it.
Let me ask here to confirm it's by-design or not. If it's a bug, I want to
try to send a patch to OpenJDK.

Here is the code reproducing the nonintentional behavior:
https://gist.github.com/KengoTODA/4d7ef6a5226d347ad9385241fee6bc63

In short, the ObjectMethods class in OpenJDK v16 generates code
that invokes the fields' method, even when the field is an array.

Please help me to understand this behavior, or
make an entry in the bug database to propose a patch.
Thanks in advance! :)

***
Kengo TODA
skypencil at gmail.com

Re: RFR: 8203359: Container level resources events [v7]

[Erik Gahlin]

On Wed, 14 Apr 2021 08:28:37 GMT, Jaroslav Bachorik  
wrote:

>> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 
>> 163:
>> 
>>> 161: private static void initializeContainerEvents() {
>>> 162: containerMetrics = Container.metrics();
>>> 163: if (containerMetrics != null) {
>> 
>> I understand this will reduce startup time, but it's contrary to how we 
>> treat other events. 
>> 
>> We register events, even if they can't be used. We want users to see what 
>> events are available (and their metadata) and use JMC recording wizard or 
>> other means to configure a .jfc file without actually being connected to a 
>> containerized process. We want the same events to minimize (subtle) platform 
>> dependent bugs.
>> 
>> I think we should try to find other means to reduce the startup time. It's 
>> better to have consistent behaviour, but an initial implementation than 
>> isn't as performant, than inconsistent behavior and somewhat faster 
>> implementation.
>> 
>> At some point we will need to address the startup cost of registering Java 
>> events anyway. For example, we could generate metadata at build time in a 
>> binary format, similar to what we already do with native events. Could even 
>> be the same file. Then we can have hundreds of Java events without the cost 
>> of reflection and unnecessary class loading at startup. We could add a 
>> simple check so that bytecode for the container events (commit() etc) are 
>> not generated unless in a container environment. A couple of (cached) checks 
>> in JVMUpcalls may be sufficient to prevent instrumentation cost.
>
> Right. So, for the initial drop I will just leave the container events 
> registered unconditionally.

I think that is fine.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v8]

[Jaroslav Bachorik]

> With this change it becomes possible to surface various cgroup level metrics 
> (available via `jdk.internal.platform.Metrics`) as JFR events.
> 
> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
> turned into JFR events to start with.
> * CPU related metrics
> * Memory related metrics
> * I/O related metrics
> 
> For each of those subsystems a configuration data will be emitted as well. 
> The initial proposal is to emit the configuration data events at least once 
> per chunk and the metrics values at 30 seconds interval. 
> By using these values the emitted events seem to contain useful information 
> without increasing overhead (the metrics values are read from `/proc` 
> filesystem so that should not be done too frequently).

Jaroslav Bachorik has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 11 additional commits 
since the last revision:

 - Roll back conditional registration of container events
 - Remove container events flag
 - Remove trailing spaces
 - Doh
 - Report container type and register events conditionally
 - Remove unused test files
 - Initial test support for JFR container events
 - Update the JFR control files
 - Split off the CPU throttling metrics
 - Formatting spaces
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/faed1f5b...67a61bd7

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3126/files
  - new: https://git.openjdk.java.net/jdk/pull/3126/files/f4372e23..67a61bd7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3126=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3126=06-07

  Stats: 76531 lines in 2761 files changed: 49167 ins; 17238 del; 10126 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3126.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3126/head:pull/3126

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v7]

[Jaroslav Bachorik]

On Mon, 12 Apr 2021 18:53:07 GMT, Erik Gahlin  wrote:

>> Jaroslav Bachorik has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove trailing spaces
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 163:
> 
>> 161: private static void initializeContainerEvents() {
>> 162: containerMetrics = Container.metrics();
>> 163: if (containerMetrics != null) {
> 
> I understand this will reduce startup time, but it's contrary to how we treat 
> other events. 
> 
> We register events, even if they can't be used. We want users to see what 
> events are available (and their metadata) and use JMC recording wizard or 
> other means to configure a .jfc file without actually being connected to a 
> containerized process. We want the same events to minimize (subtle) platform 
> dependent bugs.
> 
> I think we should try to find other means to reduce the startup time. It's 
> better to have consistent behaviour, but an initial implementation than isn't 
> as performant, than inconsistent behavior and somewhat faster implementation.
> 
> At some point we will need to address the startup cost of registering Java 
> events anyway. For example, we could generate metadata at build time in a 
> binary format, similar to what we already do with native events. Could even 
> be the same file. Then we can have hundreds of Java events without the cost 
> of reflection and unnecessary class loading at startup. We could add a simple 
> check so that bytecode for the container events (commit() etc) are not 
> generated unless in a container environment. A couple of (cached) checks in 
> JVMUpcalls may be sufficient to prevent instrumentation cost.

Right. So, for the initial drop I will just leave the container events 
registered unconditionally.

> src/jdk.jfr/share/conf/jfr/default.jfc line 1051:
> 
>> 1049:   false
>> 1050: 
>> 1051:   true
> 
> I don't think we should create "flag" for "Container Events". Instead we 
> should treat them like CPU and other OS events, always on. Since JFR can be 
> used outside a container, it seems wrong to have this as an option.

Ok. So what would be a good rule-of-thumb for when one should introduce a flag?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8265174: Update Class.getDeclaredMethods to discuss synthetic and bridge methods [v2]

[Joel Borggrén-Franck]

On Wed, 14 Apr 2021 02:56:11 GMT, Joe Darcy  wrote:

>> The results from Class.getDeclaredMethods can include bridge and other 
>> synthetic methods, which can be unexpected by users (JDK-6815786, 
>> JDK-8142904) and appear to be inherited methods. The javadoc for 
>> Class.getDeclaredMethods should be updated to explicitly mention the 
>> possibility of synthetic methods appearing.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add links to discussion in java.lang.reflect package javadoc.

Marked as reviewed by jfranck (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3477

RFR: 8265136: ZGC: Expose GarbageCollectorMXBeans for both pauses and cycles

[Per Liden]

JDK-8240679 introduced a change where the information exposed via the 
GarbageCollectorMXBean went from being related to the GC pauses to instead be 
related to the GC cycles. This helped provide more accurate heap usage 
information. However, some users have noticed that that you no longer get 
timing information related to the GC pauses, only related to the GC cycle.

Shenandoah has opted for having two distinct memory managers to provide timing 
information about both pauses and cycles. To align how concurent GCs report 
this information, ZGC should probably do the same.

Testing:
 * Tier1-7 with ZGC
 * Manual testing with relevant jtreg tests

-

Commit messages:
 - 8265136: ZGC: Expose GarbageCollectorMXBeans for both pauses and cycles

Changes: https://git.openjdk.java.net/jdk/pull/3483/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3483=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265136
  Stats: 179 lines in 9 files changed: 107 ins; 25 del; 47 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3483.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3483/head:pull/3483

PR: https://git.openjdk.java.net/jdk/pull/3483

Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]

[Alan Bateman]

On Tue, 13 Apr 2021 18:56:22 GMT, Lance Andersen  wrote:

>> Hi all,
>> 
>> Please review the following patch which adds additional permissions needed 
>> for when JTREG upgrades to a newer version of TestNG.
>> 
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   TestNG updates Permission re-organization

Marked as reviewed by alanb (Reviewer).

test/jdk/java/sql/testng/util/TestPolicy.java line 27:

> 25: import java.io.FilePermission;
> 26: import java.lang.reflect.ReflectPermission;
> 27: import java.security.*;

My guess is that you didn't mean the change the import.

-

PR: https://git.openjdk.java.net/jdk/pull/3471

Re: RFR: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator

[Aleksey Shipilev]

On Fri, 9 Apr 2021 09:41:19 GMT, Aleksey Shipilev  wrote:

> SonarCloud reports:
>   Cast one of the operands of this subtraction operation to a "long".
> 
> Here:
> 
> Spliterator makeSplitsSpliterator(long index, 
> long fence, SplittableGenerator source) {
> ...
> long multiplier = (1 << SALT_SHIFT) - 1; // < here
> 
> 
> The shift is integer, and the implicit cast to `long` happens too late. 
> `SALT_SHIFT` is currently 4, so this is not the problem yet. But it would 
> become a problem if `SALT_SHIFT` ever becomes 32 or larger. The shift operand 
> should be `1L` for safety. Observe:
> 
> 
> jshell> Long.toHexString((1 << 31) - 1)
> $2 ==> "7fff"
> 
> jshell> Long.toHexString((1 << 32) - 1)
> $3 ==> "0"
> 
> jshell> Long.toHexString((1L << 32) - 1)
> $4 ==> ""
> 
> 
> Additional testing: 
>  - [x] Linux x86_64 fastdebug, `jdk/utils/Random`

Anyone? :)

-

PR: https://git.openjdk.java.net/jdk/pull/3409