Re: RFR: 8260265: UTF-8 by Default [v9]

[Alan Bateman]

On Fri, 27 Aug 2021 23:14:58 GMT, Naoto Sato  wrote:

>> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of 
>> the changes is `Charset.defaultCharset()` returning `UTF-8` and 
>> `file.encoding` system property being added in the spec, but another notable 
>> modification is in `java.io.PrintStream` where it continues to use the 
>> `Console` encoding as the default charset instead of `UTF-8`. Other changes 
>> are mostly clarification of the term "default charset" and their links. 
>> Corresponding CSR has also been drafted.
>> 
>> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed a failing test

Marked as reviewed by alanb (Reviewer).

-

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

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Jaikiran Pai]

Hello Roger,

On 28/08/21 12:16 am, Roger Riggs wrote:

Hi,

I'm finding the idea of removing the hardcoded timestamp and adding a 
property to restore compatibility
strangely attractive.  I don't think we've yet found a case where the 
timestamp was needed (but need to keep looking).
(Adding a timestamp to the comment by the caller of store() is already 
possible)


It will reveal where the timestamp is needed (via some kind of 
failure, though perhaps not a timely one)

and includes a fallback mechanism when needed.

It will generally cleanup up the behavior of an old API.
The other approaches make new work for developers based on unclear 
requirements.


So this is essentially the proposal 1d that I listed in one of my mails, 
with the added advantage of allowing users to switch back to the old 
behaviour with a system property setting. I hadn't considered the system 
property approach to switch to old behaviour in my proposals, so this is 
a very good input and I personally think the most logical proposals so 
far. One question that however remains is, how long (how many releases) 
do we support this new system property? The --illegal-access option 
(although not a system property) seems to be one such example where 
after a few releases, that option will no longer be supported and won't 
play any role. Perhaps this system property too will follow a similar 
lifetime?


One other thing - I believe this new system property must be "set once 
at launch time" kind of property, whose value can be set at launch time 
and cannot be changed dynamically in the runtime. That would provide 
consistency in how the Properties class behaves globally within that 
runtime, instead of potentially behaving differently in different parts 
of the code, depending on how the callers set/reset the system property 
value before calling the "store(...)" APIs.


-Jaikiran

RFR: 8273092: Sort classlist in JDK image

[Ioi Lam]

When the classlist is generated using build.tools.classlist.HelloClasslist, its 
contents may be non-deterministic due to Java thread execution order.

We should sort the generated classlist to make the JDK image's contents more 
deterministic.

Tested with Mach5 tier1, tier2, builds-tier5

-

Commit messages:
 - 8273092: Sort classlist in JDK image

Changes: https://git.openjdk.java.net/jdk/pull/5288/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5288&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8273092
  Stats: 114 lines in 3 files changed: 102 ins; 6 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5288.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5288/head:pull/5288

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

Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]

[Roger Riggs]

> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified 
> unexpected messages from a child Java VM
> as the cause of the test failure.  Attempts to control the output of the 
> child VM have failed, the VM is unrepentant .
> 
> There is no functionality in the child except to wait long enough for the 
> test to finish and the child is destroyed.
> The fix is to switch from using a Java child to using a native child; a new 
> executable `sleepmillis`.

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Revised to use native /bin/sleep program on Unix* (non-Windows).
  For Windows, a native "BasicSleep" program is used.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5239/files
  - new: https://git.openjdk.java.net/jdk/pull/5239/files/7ab6c934..05d009de

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5239&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5239&range=01-02

  Stats: 44 lines in 2 files changed: 30 ins; 6 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5239.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5239/head:pull/5239

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

Re: RFR: 8271302: Regex Test Refresh [v4]

[Stuart Marks]

On Fri, 20 Aug 2021 21:17:50 GMT, Ian Graves  wrote:

>> 8271302: Regex Test Refresh
>
> Ian Graves has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Additional cleanup

Marked as reviewed by smarks (Reviewer).

Whew! Changes to GraphemeTest.java and NegativeArraySize.java look fine.

I finally figured out how to look at RegExTest.java effectively -- none of the 
diff views were helpful at all. I just brought up the old and new versions in 
windows side by side and just scrolled through them.

Overall it looks good, mostly a replacement of the ad hoc internal framework 
(failCount++) with assertX from testng, and some minor reorganizations here and 
there, such as the splitting of `stringBufferSubstitute` into several tests. 
Overall it looks like most things here got about 15% shorter, which is pretty 
good since it reduces the overall bulk. (However, there's so much more to 
do)

One observation about this technique is that using the "failCount++" approach, 
if there is a failure, the tests in the same method will continue to run. With 
the assertX approach, the test will stop at that point, and any assertions 
later in the same method won't get run at all. Since we expect all the tests to 
pass all the time, this has no effect in the normal case. If there is a bug, 
though, the assertX approach might result in a single failure whereas the 
failCount++ approach might result in several failures. While this bothers some 
people, it doesn't bother me; it's likely only to occur at development time, 
and it's like to be only a minor impediment to development. Indeed, it's 
commonly viewed as a testing antipattern to have a single test method test 
multiple cases. The solution is to fix that, and not worry about failCount++ vs 
assertX.

I think the long run solution here is to continue cleaning up these tests, 
possibly breaking down test methods further, eventually driving things into a 
purely table-driven or data generator/provider approach.

test/jdk/java/util/regex/RegExTest.java line 85:

> 83: import static org.testng.Assert.fail;
> 84: import static org.testng.Assert.expectThrows; //Replaced by assertThrows 
> in JUnit5
> 85: 

Slightly odd to mention JUnit 5 here, since this is being converted to a TestNG 
test. Are these notes for yourself for future work?

-

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

Re: RFR: 8260265: UTF-8 by Default [v9]

[Naoto Sato]

> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of 
> the changes is `Charset.defaultCharset()` returning `UTF-8` and 
> `file.encoding` system property being added in the spec, but another notable 
> modification is in `java.io.PrintStream` where it continues to use the 
> `Console` encoding as the default charset instead of `UTF-8`. Other changes 
> are mostly clarification of the term "default charset" and their links. 
> Corresponding CSR has also been drafted.
> 
> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266

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

  Fixed a failing test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4733/files
  - new: https://git.openjdk.java.net/jdk/pull/4733/files/67e1d4a9..28e79d4e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4733&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4733&range=07-08

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

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

Integrated: 8273091: Doc of [Strict]Math.floorDiv(long, int) erroneously documents int in @return tag

[Raffaello Giulietti]

On Fri, 27 Aug 2021 20:02:48 GMT, Raffaello Giulietti 
 wrote:

> This PR corrects a typo in the JavaDoc of `[Strict]Math.floorDiv(long,int)`.

This pull request has now been integrated.

Changeset: 51167846
Author:Raffaello Giulietti 
Committer: Brian Burkhalter 
URL:   
https://git.openjdk.java.net/jdk/commit/51167846cb5a60dfb31b4f8dfa214ba26640175c
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod

8273091: Doc of [Strict]Math.floorDiv(long,int) erroneously documents int in 
@return tag

Reviewed-by: darcy, bpb

-

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

Re: RFR: 8260265: UTF-8 by Default [v8]

[Naoto Sato]

> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of 
> the changes is `Charset.defaultCharset()` returning `UTF-8` and 
> `file.encoding` system property being added in the spec, but another notable 
> modification is in `java.io.PrintStream` where it continues to use the 
> `Console` encoding as the default charset instead of `UTF-8`. Other changes 
> are mostly clarification of the term "default charset" and their links. 
> Corresponding CSR has also been drafted.
> 
> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266

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

 - Removed "default" alias
 - Merge branch 'master' into JDK-8260265
 - Merge branch 'master' into JDK-8260265
 - Merge branch 'master' into JDK-8260265
 - Merge branch 'master' into JDK-8260265
 - Merge branch 'master' into JDK-8260265
 - Merge branch 'master' into JDK-8260265
 - Refined `file.encoding` description
 - Merge branch 'master' into JDK-8260265
 - Reflects comments
 - ... and 5 more: https://git.openjdk.java.net/jdk/compare/66c3531f...67e1d4a9

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4733/files
  - new: https://git.openjdk.java.net/jdk/pull/4733/files/7d5137d3..67e1d4a9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4733&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4733&range=06-07

  Stats: 15450 lines in 595 files changed: 11498 ins; 2067 del; 1885 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4733.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733

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

Re: RFR: 8268788: Annotations with lambda expressions can still cause AnnotationFormatError [v4]

[Joe Darcy]

On Wed, 25 Aug 2021 17:53:52 GMT, Sergei Ustimenko 
 wrote:

>> Change #3294 helps to avoid `AnnotaionFormatException` in 
>> `sun.reflect.annotation.AnnotationInvocationHandler.validateAnnotationMethods`.
>>  While it fixes the case with e.g. `Runnable`  that generates the synthetic 
>> method without parameters, validation still fails on synthetic methods that 
>> have parameters e.g. `Function`, `BiFunction`, etc.
>> 
>> This change removes the restriction on parameters count to be zero i.e. 
>> lambdas with parameters
>> would be skipped from validation.
>
> Sergei Ustimenko 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 two additional 
> commits since the last revision:
> 
>  - 8268788: Filter out synthetic methods from the member methods computation
>  - 8268788: Annotations with lambda expressions can still cause 
> AnnotationFormatError

Hello,

Thanks for the refined PR. Doing another round of review remain on my to-do 
list.

-Joe

-

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

Re: RFR: 8273091: Doc of [Strict]Math.floorDiv(long, int) erroneously documents int in @return tag

[Brian Burkhalter]

On Fri, 27 Aug 2021 20:02:48 GMT, Raffaello Giulietti 
 wrote:

> This PR corrects a typo in the JavaDoc of `[Strict]Math.floorDiv(long,int)`.

Marked as reviewed by bpb (Reviewer).

-

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

Re: RFR: 8273091: Doc of [Strict]Math.floorDiv(long, int) erroneously documents int in @return tag

[Joe Darcy]

On Fri, 27 Aug 2021 20:02:48 GMT, Raffaello Giulietti 
 wrote:

> This PR corrects a typo in the JavaDoc of `[Strict]Math.floorDiv(long,int)`.

Marked as reviewed by darcy (Reviewer).

-

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

RFR: 8273091: Doc of [Strict]Math.floorDiv(long, int) erroneously documents int in @return tag

[Raffaello Giulietti]

This PR corrects a typo in the JavaDoc of `[Strict]Math.floorDiv(long,int)`.

-

Commit messages:
 - 8273091: Doc of [Strict]Math.floorDiv(long,int) erroneously documents int in 
@return tag

Changes: https://git.openjdk.java.net/jdk/pull/5287/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5287&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8273091
  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5287.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5287/head:pull/5287

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

RFR: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family

[Raffaello Giulietti]

Please review this PR to add officially endorsed `ceilDiv()` and `ceilMod()` 
methods do `j.l.[Strict]Math`.

Beside adding fresh tests to `test/jdk/java/lang/Math/DivModTests.java`, this 
PR also corrects small typos in it and exercises tests that were already 
present but which were never invoked.
Let me know if this is acceptable for a test or if there's a need of a separate 
issue in the JBS.

-

Commit messages:
 - 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family

Changes: https://git.openjdk.java.net/jdk/pull/5285/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5285&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8271602
  Stats: 871 lines in 3 files changed: 850 ins; 0 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5285.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5285/head:pull/5285

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

Integrated: JDK-8272866 java.util.random package summary contains incorrect mixing function in table

[Jim Laskey]

On Fri, 27 Aug 2021 14:20:32 GMT, Jim Laskey  wrote:

> The table 'LXM Multipliers' at the end of the javadoc for the 
> java.util.random package states that the mixing function is mixLea32 for all 
> the 64/128-bit LCG based generators. This should be updated to mixLea64 for 
> the 64/128-bit LCG generators. Only the L32X64MixRandom which uses a 32-bit 
> LCG will use the mixLea32 mixing function.

This pull request has now been integrated.

Changeset: e66c8afb
Author:Jim Laskey 
URL:   
https://git.openjdk.java.net/jdk/commit/e66c8afb59b57c4546656efa97f723f084964330
Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod

8272866: java.util.random package summary contains incorrect mixing function in 
table

Reviewed-by: rriggs

-

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

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Roger Riggs]

Hi,

I'm finding the idea of removing the hardcoded timestamp and adding a 
property to restore compatibility
strangely attractive.  I don't think we've yet found a case where the 
timestamp was needed (but need to keep looking).
(Adding a timestamp to the comment by the caller of store() is already 
possible)


It will reveal where the timestamp is needed (via some kind of failure, 
though perhaps not a timely one)

and includes a fallback mechanism when needed.

It will generally cleanup up the behavior of an old API.
The other approaches make new work for developers based on unclear 
requirements.


$.02, Roger


On 8/27/21 1:38 PM, Robert Scholte wrote:

I've dropped this topic on the Maven Dev mailinglist as well[1].
The responses are mixed, so I'd prefer to refer to the mailinglist 
instead of summarizing the responses here.

This will prevent that those opinions are considered my opinions.

Thanks,
Robert

[1] 
https://lists.apache.org/thread.html/r687bca66bb548d3a13ea665fb7202daf9710564802fa497a4b34d81e%40%3Cdev.maven.apache.org%3E

Re: Replace StringBuffers to StringBuilders in tests

[Sergei Ustimenko]

Hi Daniel and David,

Thanks for your opinions and for sharing more context - it is
definitely clearer now! I am glad that I reached out to the
mailing list first instead of going ahead with the changes! :)

Regards,
Sergei

Sent with ProtonMail Secure Email.

‐‐‐ Original Message ‐‐‐

On Friday, August 27th, 2021 at 15:14, David Holmes  
wrote:

> On 27/08/2021 8:42 pm, Daniel Fuchs wrote:
>
> > Hi Sergei,
> >
> > I wouldn't bother replacing StringBuffers with StringDuilders in tests.
> >
> > It seems a bit gratuitous - and possibly could complicate future
> >
> > tests backports.
> >
> > But that's my personal opinion. Others might disagree.
>
> I agree with you. Complete waste of time and effort for zero benefit
>
> IMO. Sorry Sergei.
>
> Cheers,
>
> David
>
> > best regards,
> >
> > -- daniel
> >
> > On 27/08/2021 11:00, Sergei Ustimenko wrote:
> >
> > > Hi all,
> > >
> > > Some tests use StringBuffers instead of StringBuilders where
> > >
> > > additional thread-safety
> > >
> > > is not required as e.g. in
> > >
> > > test/jdk/sun/util/resources/TimeZone/Bug4640234.java:82 :
> > >
> > > ...
> > >
> > > StringBuffer errors
> > >
> > > =
> > >
> > > new
> > >
> > > StringBuffer(
> > >
> > > ""
> > >
> > > );
> > >
> > > StringBuffer warnings
> > >
> > > =
> > >
> > > new
> > >
> > > StringBuffer(
> > >
> > > ""
> > >
> > > );
> > >
> > > ...
> > >
> > > There were some efforts to clean up core libs (e.g. java.base module in
> > >
> > > https://github.com/openjdk/jdk/pull/2922) and I've noticed some tests
> > >
> > > that could be
> > >
> > > improved as well.
> > >
> > > Now there are about 300 tests for different modules that in general
> > >
> > > use StringBuffers
> > >
> > > (most probably some of them not without a reason) so is it something
> > >
> > > worth looking
> > >
> > > into? What you think about it?
> > >
> > > Regards,
> > >
> > > Sergei

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Robert Scholte]

I've dropped this topic on the Maven Dev mailinglist as well[1].
The responses are mixed, so I'd prefer to refer to the mailinglist 
instead of summarizing the responses here.

This will prevent that those opinions are considered my opinions.

Thanks,
Robert

[1] 
https://lists.apache.org/thread.html/r687bca66bb548d3a13ea665fb7202daf9710564802fa497a4b34d81e%40%3Cdev.maven.apache.org%3E

Integrated: 8272541: Incorrect overflow test in Toom-Cook branch of BigInteger multiplication

[Brian Burkhalter]

On Mon, 16 Aug 2021 21:00:00 GMT, Brian Burkhalter  wrote:

> Please consider this change which would modify a conditional `a + b > c` 
> where the left side variables are `int`s and the right side is 
> `(long)Integer.MAX_VALUE + 1`. The change is to cast the left side variables 
> to `long`s.

This pull request has now been integrated.

Changeset: d1aeca11
Author:Brian Burkhalter 
URL:   
https://git.openjdk.java.net/jdk/commit/d1aeca117ccc4334d67b2692ff087a9f8d808a59
Stats: 30 lines in 2 files changed: 18 ins; 3 del; 9 mod

8272541: Incorrect overflow test in Toom-Cook branch of BigInteger 
multiplication

Reviewed-by: darcy

-

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

Re: RFR: JDK-8272866 java.util.random package summary contains incorrect mixing function in table

[Roger Riggs]

On Fri, 27 Aug 2021 14:20:32 GMT, Jim Laskey  wrote:

> The table 'LXM Multipliers' at the end of the javadoc for the 
> java.util.random package states that the mixing function is mixLea32 for all 
> the 64/128-bit LCG based generators. This should be updated to mixLea64 for 
> the 64/128-bit LCG generators. Only the L32X64MixRandom which uses a 32-bit 
> LCG will use the mixLea32 mixing function.

Marked as reviewed by rriggs (Reviewer).

-

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

Re: Possible ClassCastException in java.util.regex.Pattern.BmpCharPredicate#union(java.util.regex.Pattern.CharPredicate...)

[Pavel Rappo]

Has that method been ever used? If nothing else its name seems strange. To me, 
a union has OR semantics, not AND.

> On 27 Aug 2021, at 15:37, Andrey Turbanov  wrote:
> 
> Hello.
> I found suspicious code in the method
> "java.util.regex.Pattern.BmpCharPredicate#union(java.util.regex.Pattern.CharPredicate...)"
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/regex/Pattern.java#L5639
> 
> static CharPredicate union(CharPredicate... predicates) {
>CharPredicate cp = ch -> {
>for (CharPredicate p : predicates) {
>if (!p.is(ch))
>return false;
>}
>return true;
>};
>for (CharPredicate p : predicates) {
>if (! (p instanceof BmpCharPredicate))
>return cp;
>}
>return (BmpCharPredicate)cp;
> }
> 
> Variable `cp` has type CharPredicate initially. And then it's casted
> to BmpCharPredicate. This cast always fails with ClassCastException
> when reached.
> 
> Can reproduced in small sample class:
> 
>public static void main(String[] args) {
>CharPredicate result = BmpCharPredicate.union();
>System.out.println(result);
>}
> 
>interface CharPredicate {
>boolean is(int ch);
>}
> 
>interface BmpCharPredicate extends CharPredicate {
>static CharPredicate union(CharPredicate... predicates) {
>CharPredicate cp = ch -> true;
>for (CharPredicate p : predicates) {
>if (! (p instanceof BmpCharPredicate))
>return cp;
>}
>return (BmpCharPredicate)cp;
>}
>}
> 
> 
> Exception in thread "main" java.lang.ClassCastException: class
> org.RegexpBug$BmpCharPredicate$$Lambda$14/0x000800c028f0 cannot be
> cast to class org.RegexpBug$BmpCharPredicate
> (org.RegexpBug$BmpCharPredicate$$Lambda$14/0x000800c028f0 and
> org.RegexpBug$BmpCharPredicate are in unnamed module of loader 'app')
>at org.RegexpBug$BmpCharPredicate.union(RegexpBug.java:20)
>at org.RegexpBug.main(RegexpBug.java:5)
> 
> As I can see this method is never used. Perhaps it should be removed?
> 
> 
> Andrey Turbanov

Re: RFR: 8266936: Add a finalization JFR event [v9]

[Markus Grönlund]

On Tue, 24 Aug 2021 12:58:29 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Model as finalizerService

Sorry for the wide distribution, but this became necessary as the PR touches 
many component areas, if only some with minor parts. Below is a high-level 
description of this suggested PR.

// Design
Per class statistics about finalizers is implemented by 
services/finalizerServices. The concept of a "service" is modelled after other 
management components, such as ClassLoadingService and RuntimeService.

Allocation of an object with a class overriding finalize() with a non-empty 
finalize() method, is hooked by the runtime using bytecode 
"_return_register_finalizer", which lets it enter 
InstanceKlass::register_finalizer().
A hook is added to InstanceKlass::register_finalizer() to notify 
FinalizerService which increments the number of "objects_on_heap" for the 
corresponding InstanceKlass.

The dedicated finalizer thread is responsible for running the finalizer methods 
for referent objects whose java.lang.ref.Finalizers have been enqueued for 
finalization by the GC.
It will get a native method to report the completion state of a finalizer back 
to the VM. FinalizerService will then increase the total number of finalizers 
run and decrease the number of outstanding objects on the heap for the 
corresponding InstanceKlass.

Class Unloading support is in place by adding a hook into 
SystemDictionary::do_unloading(). The existing JFR class unloading support is 
extended to also report a FinalizerStatistic event for the class unloading 
before its corresponding FinalizerEntry is purged.

-Xlog:finalize is added for Unified Logging support.

// JFR event output jdk.FinalizerStatistics:
Event:jdk.FinalizerStatistics {
  startTime = 14:22:06.683
  finalizableClass = 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize 
(classLoader = app)
  codeSource = 
"file:///D:/utilities/jtreg/runtime_artifacts/work/classes/jdk/jfr/event/runtime/TestFinalizerStatisticsEvent.d/"
  objects = 0
  totalFinalizersRun = 2
}


Event:jdk.FinalizerStatistics {
  startTime = 14:22:07.244
  finalizableClass = jdk.jfr.internal.RepositoryChunk (classLoader = bootstrap)
  codeSource = N/A
  objects = 3
  totalFinalizersRun = 0
}


// Xlog:finalizer output:
[4.517s][info][finalizer] Registered object (0x73fbf594) of class 
jdk.jfr.internal.RepositoryChunk as finalizable
[4.737s][info][finalizer] Registered object (0x7ee4f130) of class 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize 
as finalizable
[5.002s][info][finalizer] Finalizer was run for object (0x7ee4f130) of 
class 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize
[5.004s][info][finalizer] Registered object (0x56bf003b) of class 
jdk.jfr.internal.RepositoryChunk as finalizable
[5.127s][info][finalizer] Registered object (0x14d51169) of class 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize 
as finalizable
[5.325s][info][finalizer] Finalizer was run for object (0x14d51169) of 
class 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize
[5.691s][info][finalizer] Registered object (0x2baaa366) of class 
jdk.jfr.internal.RepositoryChunk as finalizable
[5.696s][info][finalizer] Registered object (0x75b10e10) of class 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize 
as finalizable
[5.891s][info][finalizer] Finalizer was run for object (0x75b10e10) of 
class 
jdk.jfr.event.runtime.TestFinalizerStatisticsEvent$TestClassOverridingFinalize
[6.121s][info][finalizer] Registered object (0x3c036ecb) of class 
jdk.jfr.internal.ChunksChannel as finalizable
[6.342s][info][finalizer] Finalizer was run for object (0x3c036ecb) of 
class jdk.jfr.internal.ChunksChannel

// Misc
JfrSymbolTable - an existing JFR internal component repackaged and published to 
let other JFR native components re-use the symbol ids (more specifically for 
the CodeSource in this case).

ClassLoadingService - some parts needed to have better conditional compilation 
and runtime support for running the JVM when excluding JVM feature 'management'.

jmm.h - is a private interface between the JVM and the JDK, so a CSR should not 
be necessary for the update and version change.

//

Re: RFR: 8266936: Add a finalization JFR event [v10]

[Markus Grönlund]

> Greetings,
> 
> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
> 
> We also like to assist users in replacing and mitigating uses in non-JDK code.
> 
> Hence, this changeset adds a periodic JFR event to help identify which 
> classes are overriding Object.finalize().
> 
> Thanks
> Markus

Markus Grönlund has updated the pull request incrementally with three 
additional commits since the last revision:

 - localize
 - cleanup
 - FinalizerStatistics

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4731/files
  - new: https://git.openjdk.java.net/jdk/pull/4731/files/e6b786f1..fd86899f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=08-09

  Stats: 457 lines in 17 files changed: 190 ins; 222 del; 45 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4731.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731

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

Re: what does the spec say about file paths that are too long?

[Alan Snyder]

> On Aug 26, 2021, at 8:57 AM, Alan Bateman  wrote:
> 
> On 26/08/2021 15:43, Alan Snyder wrote:
>> As I said, I think it's great that we agree an exception should be thrown 
>> when an over-length path is used in an actual file operation.
>> 
>> However, would it not be better to have that in the specification, rather 
>> than relying on the opinions of individual engineers expressed in a thread 
>> on a mailing list?
>> 
>> And would it not also be better if there were test cases?
> You are welcome to propose additional tests if you want.
> 


Thanks. However, I think the specification issue should be addressed first.

One writes test programs to find cases where the code does not obey the 
specification. It’s hard to do that if the specification does not address the 
behavior of interest.



>> Granted that path operations cannot in general predict when a path will be 
>> of usable length in a file operation, the question remains whether path 
>> operations should report over-length paths in those cases where it can be 
>> determined. Is it not the case that some OS APIs have hard limits on path 
>> length (unrelated to limits imposed by the file system itself)? For a file 
>> provider using such an API, would throwing an exception when that limit is 
>> exceeded be a good idea or a bad idea?
> I don't think this is feasible or a compatible change.

It seems this case is already covered, at least in part. From the spec for 
FileSystem.getPath:

An implementation may choose to reject path strings that contain names that 
are longer than those allowed by any file store.

All that is missing is the case where the entire path exceeds some hard limit.

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Roger Riggs]

Hi,

It would seem useful in the long term to move the inclusion of the date 
to be part of the comment
and it would be up to the caller of store() to include a date (as a 
string) where ever appropriate.
And drop the writing hardcoded date; but its not clear how to get there 
given compatibility concerns.



One more option would be to add a separate method on a Properties 
instance to set or omit the date written by store().


It would not have a global affect on all Properties.store calls but 
would be specific to an instance.
It would still require callers to have access to the instance but not 
have to modify the immediate caller of store().


Without a better understanding of the uses that affect reproducible 
builds, this might not be able to address them all.


There is likely no good (clean) solution since the reproducible build 
goal implies a global modification of behavior and all the alternatives 
that are specific to Properties have only a local effect.


$.02, Roger



On 8/27/21 10:16 AM, Jaikiran Pai wrote:


On 26/08/21 5:06 pm, Alan Bateman wrote:

On 25/08/2021 15:57, Jaikiran Pai wrote:

:

Introducing an overloaded "store" which takes the timestamp or 
implicitly using the SOURCE_DATE_EPOCH environment variable would 
mean that the Properties.store(...) APIs will continue to always 
write out a Date representation into the outputstream. That 
effectively means that there will continue to be no way to disable 
writing out a (date) comment. More specifically, even if a user 
doesn't explicitly specify a comment, we would end up writing a 
default (date) comment. Do we want that? Or while we are doing these 
changes, should we introduce this ability of disabling writing out 
these date comments by default? That, IMO, can only be done by 
introducing new APIs instead of trying to slightly change the spec 
and the implementation of the current "store" APIs. After all, if 
any callers do want a date (and a reproducible one at that), they 
could always do so by passing that as a value for the "comment" 
parameter of these (new) "storeXXX" APIs.


Properties save/store has always emitted a comment line with the 
current date/time, it goes back to JDK 1.0. It's unfortunate that 
newer store methods didn't re-examine this, also unfortunate that it 
continued with 8859-1. In the discussion on jdk-dev about 
reproducibility then I think we concluded that we don't want to 
change the behavior of existing methods, hence the discussion on 
introducing a new method.


An new overload of store would give the maximum flexibility to new 
code but it would require programs used in builds to use it 
consistently. It's possible that libraries or tools that are using 
Properties::store have no idea that they will ultimately be used in a 
system that is trying to produce reproducible builds. So I have some 
sympathy to the argument that there should a way to omit the date or 
emit it as the Unix epoch time or a fixed value. This would mean 
changing the spec to allow for some implementation means to do this, 
and maybe an implNote that documents that it reads SOURCE_DATE_EPOCH. 
I think Roger has misgivings about this.


So let's list the options and the pros/cons and see if we can 
converge on an approach.


Keeping aside the discussion about whether to introduce a new API or 
change the spec of the existing API, just for a moment, I think the 
main question is whether the current date comment that gets added by 
the store(...) is being used by anything out there, other than maybe 
for visual aspects while reading the properties file. From the 
discussions I have seen so far in the threads on openjdk and other 
mailing lists, I don't think anyone is using that comment for 
anything. So if we are indeed willing to change the spec of the 
store(...) API would it be too big/unacceptable a change to disable 
writing this date comment, at least in certain specific cases? With my 
limited usage of this API, my guess is that it's higly unlikely that 
it will break anything - after all it's a comment that was being 
added. The only potential breakages perhaps would be some scripts? But 
even those, I don't know how it would break them since anyway the date 
comment wasn't a constant value. Having said that, I looked at the 
patch that you pointed out[1] that got integrated into the JDK shipped 
in Ubuntu, for introducing reproducibility. I'm a bit surprised that 
the patched implementation decided to write out a formatted 
reproducible date instead of patching it to just not write the date. 
After all that IMO would have been much simpler and it would anyway 
have changed (removed) the exact same line in the code that was 
patched. So maybe I'm underestimating the usage of this date comment.


So now coming to the API part of it. The potential ways to tackle 
this, that I could think of are as follows:


1. No new APIs to be added. Instead, the existing "store(...)" APIs 
specification will be changed to allow for

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Jaikiran Pai]

On 27/08/21 8:12 pm, Jaikiran Pai wrote:

Hello Daniel,

On 27/08/21 8:05 pm, Daniel Fuchs wrote:

Hi Jaikiran,

What about writing the keys in natural ordering?
Has that been abandoned, or will the implementation silently do it,
or will it require a new API?

In the discussion so far, there has been no opposition to changing the 
implementation of these existing store(...) APIs to use natural 
sorting order for writing out the keys.


Just to avoid any kind of confusion - when I say natural sorting order 
of keys, I mean the implementation will be changed to lexicographically 
compare the keys as noted in the compareTo[1] method of java.lang.String.



[1] 
https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/lang/String.html#compareTo(java.lang.String)



-Jaikiran

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Jaikiran Pai]

Hello Daniel,

On 27/08/21 8:05 pm, Daniel Fuchs wrote:

Hi Jaikiran,

What about writing the keys in natural ordering?
Has that been abandoned, or will the implementation silently do it,
or will it require a new API?

In the discussion so far, there has been no opposition to changing the 
implementation of these existing store(...) APIs to use natural sorting 
order for writing out the keys. In fact, there seems to be an agreement 
that the current 2 store(...) API implementations and any new API that 
we might introduce to tackle the date comment issue, must use natural 
key ordering for writing out the properties. Whether or not this 
implementation detail will be noted/mentioned in the spec of these APIs 
hasn't yet been decided.


So it's just the date comment which requires some kind of agreement on 
how we deal with it.


-Jaikiran

Possible ClassCastException in java.util.regex.Pattern.BmpCharPredicate#union(java.util.regex.Pattern.CharPredicate...)

[Andrey Turbanov]

Hello.
I found suspicious code in the method
"java.util.regex.Pattern.BmpCharPredicate#union(java.util.regex.Pattern.CharPredicate...)"
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/regex/Pattern.java#L5639

static CharPredicate union(CharPredicate... predicates) {
CharPredicate cp = ch -> {
for (CharPredicate p : predicates) {
if (!p.is(ch))
return false;
}
return true;
};
for (CharPredicate p : predicates) {
if (! (p instanceof BmpCharPredicate))
return cp;
}
return (BmpCharPredicate)cp;
}

Variable `cp` has type CharPredicate initially. And then it's casted
to BmpCharPredicate. This cast always fails with ClassCastException
when reached.

Can reproduced in small sample class:

public static void main(String[] args) {
CharPredicate result = BmpCharPredicate.union();
System.out.println(result);
}

interface CharPredicate {
boolean is(int ch);
}

interface BmpCharPredicate extends CharPredicate {
static CharPredicate union(CharPredicate... predicates) {
CharPredicate cp = ch -> true;
for (CharPredicate p : predicates) {
if (! (p instanceof BmpCharPredicate))
return cp;
}
return (BmpCharPredicate)cp;
}
}


Exception in thread "main" java.lang.ClassCastException: class
org.RegexpBug$BmpCharPredicate$$Lambda$14/0x000800c028f0 cannot be
cast to class org.RegexpBug$BmpCharPredicate
(org.RegexpBug$BmpCharPredicate$$Lambda$14/0x000800c028f0 and
org.RegexpBug$BmpCharPredicate are in unnamed module of loader 'app')
at org.RegexpBug$BmpCharPredicate.union(RegexpBug.java:20)
at org.RegexpBug.main(RegexpBug.java:5)

As I can see this method is never used. Perhaps it should be removed?


Andrey Turbanov

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Daniel Fuchs]

Hi Jaikiran,

What about writing the keys in natural ordering?
Has that been abandoned, or will the implementation silently do it,
or will it require a new API?

AFAIU there are two problems that are hostile to reproducible builds:
  - date comments differ from store to store
  - property ordering may change from VM to VM due to differences
in hashing

best regards,

-- daniel

On 27/08/2021 15:16, Jaikiran Pai wrote:


On 26/08/21 5:06 pm, Alan Bateman wrote:

On 25/08/2021 15:57, Jaikiran Pai wrote:

:

Introducing an overloaded "store" which takes the timestamp or 
implicitly using the SOURCE_DATE_EPOCH environment variable would 
mean that the Properties.store(...) APIs will continue to always 
write out a Date representation into the outputstream. That 
effectively means that there will continue to be no way to disable 
writing out a (date) comment. More specifically, even if a user 
doesn't explicitly specify a comment, we would end up writing a 
default (date) comment. Do we want that? Or while we are doing these 
changes, should we introduce this ability of disabling writing out 
these date comments by default? That, IMO, can only be done by 
introducing new APIs instead of trying to slightly change the spec 
and the implementation of the current "store" APIs. After all, if any 
callers do want a date (and a reproducible one at that), they could 
always do so by passing that as a value for the "comment" parameter 
of these (new) "storeXXX" APIs.


Properties save/store has always emitted a comment line with the 
current date/time, it goes back to JDK 1.0. It's unfortunate that 
newer store methods didn't re-examine this, also unfortunate that it 
continued with 8859-1. In the discussion on jdk-dev about 
reproducibility then I think we concluded that we don't want to change 
the behavior of existing methods, hence the discussion on introducing 
a new method.


An new overload of store would give the maximum flexibility to new 
code but it would require programs used in builds to use it 
consistently. It's possible that libraries or tools that are using 
Properties::store have no idea that they will ultimately be used in a 
system that is trying to produce reproducible builds. So I have some 
sympathy to the argument that there should a way to omit the date or 
emit it as the Unix epoch time or a fixed value. This would mean 
changing the spec to allow for some implementation means to do this, 
and maybe an implNote that documents that it reads SOURCE_DATE_EPOCH. 
I think Roger has misgivings about this.


So let's list the options and the pros/cons and see if we can converge 
on an approach.


Keeping aside the discussion about whether to introduce a new API or 
change the spec of the existing API, just for a moment, I think the main 
question is whether the current date comment that gets added by the 
store(...) is being used by anything out there, other than maybe for 
visual aspects while reading the properties file. From the discussions I 
have seen so far in the threads on openjdk and other mailing lists, I 
don't think anyone is using that comment for anything. So if we are 
indeed willing to change the spec of the store(...) API would it be too 
big/unacceptable a change to disable writing this date comment, at least 
in certain specific cases? With my limited usage of this API, my guess 
is that it's higly unlikely that it will break anything - after all it's 
a comment that was being added. The only potential breakages perhaps 
would be some scripts? But even those, I don't know how it would break 
them since anyway the date comment wasn't a constant value. Having said 
that, I looked at the patch that you pointed out[1] that got integrated 
into the JDK shipped in Ubuntu, for introducing reproducibility. I'm a 
bit surprised that the patched implementation decided to write out a 
formatted reproducible date instead of patching it to just not write the 
date. After all that IMO would have been much simpler and it would 
anyway have changed (removed) the exact same line in the code that was 
patched. So maybe I'm underestimating the usage of this date comment.


So now coming to the API part of it. The potential ways to tackle this, 
that I could think of are as follows:


1. No new APIs to be added. Instead, the existing "store(...)" APIs 
specification will be changed to allow for reproducibility of the 
content that gets stored. The possible specification changes that we 
could attempt are:


     1a. The store(...) APIs by default will continue to write out a 
date comment, which represents the current date. In the case where 
SOURCE_DATE_EPOCH environment variable is set, the store(...) APIs will 
use that value to construct a (UTC) Date and write out the date comment.


         Pros:
         
         - Existing callers of these APIs won't have to change their 
code to use this new semantics.
         - Tools/environments where reproducibility is required, can 
configure th

RFR: JDK-8272866 java.util.random package summary contains incorrect mixing function in table

[Jim Laskey]

The table 'LXM Multipliers' at the end of the javadoc for the java.util.random 
package states that the mixing function is mixLea32 for all the 64/128-bit LCG 
based generators. This should be updated to mixLea64 for the 64/128-bit LCG 
generators. Only the L32X64MixRandom which uses a 32-bit LCG will use the 
mixLea32 mixing function.

-

Commit messages:
 - Correct table entries

Changes: https://git.openjdk.java.net/jdk/pull/5279/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5279&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8272866
  Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5279.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5279/head:pull/5279

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

Re: Proposal: JDK-8231640 - (prop) Canonical property storage

[Jaikiran Pai]

On 26/08/21 5:06 pm, Alan Bateman wrote:

On 25/08/2021 15:57, Jaikiran Pai wrote:

:

Introducing an overloaded "store" which takes the timestamp or 
implicitly using the SOURCE_DATE_EPOCH environment variable would 
mean that the Properties.store(...) APIs will continue to always 
write out a Date representation into the outputstream. That 
effectively means that there will continue to be no way to disable 
writing out a (date) comment. More specifically, even if a user 
doesn't explicitly specify a comment, we would end up writing a 
default (date) comment. Do we want that? Or while we are doing these 
changes, should we introduce this ability of disabling writing out 
these date comments by default? That, IMO, can only be done by 
introducing new APIs instead of trying to slightly change the spec 
and the implementation of the current "store" APIs. After all, if any 
callers do want a date (and a reproducible one at that), they could 
always do so by passing that as a value for the "comment" parameter 
of these (new) "storeXXX" APIs.


Properties save/store has always emitted a comment line with the 
current date/time, it goes back to JDK 1.0. It's unfortunate that 
newer store methods didn't re-examine this, also unfortunate that it 
continued with 8859-1. In the discussion on jdk-dev about 
reproducibility then I think we concluded that we don't want to change 
the behavior of existing methods, hence the discussion on introducing 
a new method.


An new overload of store would give the maximum flexibility to new 
code but it would require programs used in builds to use it 
consistently. It's possible that libraries or tools that are using 
Properties::store have no idea that they will ultimately be used in a 
system that is trying to produce reproducible builds. So I have some 
sympathy to the argument that there should a way to omit the date or 
emit it as the Unix epoch time or a fixed value. This would mean 
changing the spec to allow for some implementation means to do this, 
and maybe an implNote that documents that it reads SOURCE_DATE_EPOCH. 
I think Roger has misgivings about this.


So let's list the options and the pros/cons and see if we can converge 
on an approach.


Keeping aside the discussion about whether to introduce a new API or 
change the spec of the existing API, just for a moment, I think the main 
question is whether the current date comment that gets added by the 
store(...) is being used by anything out there, other than maybe for 
visual aspects while reading the properties file. From the discussions I 
have seen so far in the threads on openjdk and other mailing lists, I 
don't think anyone is using that comment for anything. So if we are 
indeed willing to change the spec of the store(...) API would it be too 
big/unacceptable a change to disable writing this date comment, at least 
in certain specific cases? With my limited usage of this API, my guess 
is that it's higly unlikely that it will break anything - after all it's 
a comment that was being added. The only potential breakages perhaps 
would be some scripts? But even those, I don't know how it would break 
them since anyway the date comment wasn't a constant value. Having said 
that, I looked at the patch that you pointed out[1] that got integrated 
into the JDK shipped in Ubuntu, for introducing reproducibility. I'm a 
bit surprised that the patched implementation decided to write out a 
formatted reproducible date instead of patching it to just not write the 
date. After all that IMO would have been much simpler and it would 
anyway have changed (removed) the exact same line in the code that was 
patched. So maybe I'm underestimating the usage of this date comment.


So now coming to the API part of it. The potential ways to tackle this, 
that I could think of are as follows:


1. No new APIs to be added. Instead, the existing "store(...)" APIs 
specification will be changed to allow for reproducibility of the 
content that gets stored. The possible specification changes that we 
could attempt are:


    1a. The store(...) APIs by default will continue to write out a 
date comment, which represents the current date. In the case where 
SOURCE_DATE_EPOCH environment variable is set, the store(...) APIs will 
use that value to construct a (UTC) Date and write out the date comment.


        Pros:
        
        - Existing callers of these APIs won't have to change their 
code to use this new semantics.
        - Tools/environments where reproducibility is required, can 
configure their environment (and they probably already would have) to 
set the SOURCE_DATE_EPOCH value.


        Cons:
        
        - Behaviour of the same API will change dynamically based on 
the current environment of the process. However, this change in 
behaviour will not have functional impact on the generated output, since 
it's just a comment that changes.
        - There is no way to disable writing out comments to the 
o

Replace StringBuffers to StringBuilders in tests

[Sergei Ustimenko]

Hi all,

Some tests use StringBuffers instead of StringBuilders where additional 
thread-safety
is not required as e.g. in 
test/jdk/sun/util/resources/TimeZone/Bug4640234.java:82 :
...

StringBuffer errors

=

new

StringBuffer(

""

);

StringBuffer warnings

=

new

StringBuffer(

""

);

...
There were some efforts to clean up core libs (e.g. java.base module in
https://github.com/openjdk/jdk/pull/2922) and I've noticed some tests that 
could be
improved as well.

Now there are about 300 tests for different modules that in general use 
StringBuffers
(most probably some of them not without a reason) so is it something worth 
looking
into? What you think about it?

Regards,
Sergei

Re: StringBuilder OOMs earlier with JRE 11 compared to JRE 8

[Remi Forax]

- Original Message -
> From: "David Holmes" 
> To: "S" , "core-libs-dev" 
> 
> Cc: "Andrey Loskutov" 
> Sent: Vendredi 27 Août 2021 15:25:25
> Subject: Re: StringBuilder OOMs earlier with JRE 11 compared to JRE 8

> Hi Simeon,
> 
> Redirecting this to core-libs-dev as it is not a serviceability issue.
> (bcc serviceabillity-dev)

Hi Simeon,
in Java 9, the String representation was changed to use less memory if the 
characters can be encoded in ISO Latin 1,
a side effect is that StringBuilder now uses a byte[] instead of a char[], 
hence the maximum size being half the one it was previously.

> 
> Thanks,
> David

[1] 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/AbstractStringBuilder.java#L63

> 
> On 27/08/2021 8:53 pm, S A wrote:
>> Hi all,
>> 
>> while working on https://bugs.eclipse.org/bugs/show_bug.cgi?id=575641
>> , I noticed that
>> StringBuilder throws an OOM "half as early" in OpenJDK 11 (and 17 early
>> access), when compared to OpenJDK 8.
>> 
>> In particular, I ran the following snippet to see where the limit of
>> appending to a StringBuilder is:
>> 
>> StringBuilder sb = new StringBuilder();
>> long n = 8L * 1024L * 1024L;
>> for (long i = 0; i < n; ++i) {
>> int m = 1024;
>> for (int j = 0; j < m; ++j) {
>> int length = sb.length();
>> try {
>> sb.append((char) j);
>> } catch (Error e) {
>> System.out.println("Error occurred at length=" + length + " [i=" + i +
>> ", j=" + j + "]");
>> throw e;
>> }
>> 
>> }
>> }
>> 
>> JRE 8:
>> 
>> Error occurred at length=2147483645 [i=2097151, j=1021]
>> Exception in thread "main" java.lang.OutOfMemoryError: Requested array
>> size exceeds VM limit
>> at java.util.Arrays.copyOf(Arrays.java:3332)
>> at
>> java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:124)
>> at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:649)
>> at java.lang.StringBuilder.append(StringBuilder.java:202)
>> at
>> test.stringbuilder.TestStringbuilderOOM.main(TestStringbuilderOOM.java:13)
>> 
>> JRE 11:
>> 
>> Error occurred at length=1073741822 [i=1048575, j=1022]
>> Exception in thread "main" java.lang.OutOfMemoryError: Requested array
>> size exceeds VM limit
>> at java.base/java.util.Arrays.copyOf(Arrays.java:3745)
>> at
>> java.base/java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:172)
>> at
>> java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:748)
>> at java.base/java.lang.StringBuilder.append(StringBuilder.java:241)
>> at
>> TestJava11/test.stringbuilder.TestStringbuilderOOM.main(TestStringbuilderOOM.java:13)
>> 
>> While StringBuilder is not a good choice for holding GBs of text, I
>> wonder that no effort is made to clamp the capacity to its limit when (I
>> assume) it is being doubled upon array expansion.
>> 
>> Is this something that should be looked into or it can be safely ignored
>> (from JDK users POV)?
>> 
>> Best regards,
> > Simeon

Re: StringBuilder OOMs earlier with JRE 11 compared to JRE 8

[Aleksey Shipilev]

On 8/27/21 3:25 PM, David Holmes wrote:

Is this something that should be looked into or it can be safely ignored
(from JDK users POV)?


That is the unfortunate but expected trade-off from the Compact Strings work in JDK 9. That reworked 
the String storage to hold byte[] instead of char[]. When String is in non-Latin1 mode, then 1 
character is placed in 2 bytes. But since both arrays can only hold ~2^32 elements, it means that 
the effective size for a String is twice lower than it used to.


$ jdk11u-dev/bin/java TestSB
Error occurred at length=1073741822 [i=1048575, j=1022]
Exception in thread "main" java.lang.OutOfMemoryError: Requested array size 
exceeds VM limit

If you keep the String* in Latin1, then the limit is the same at it "used to be", because every 
character takes 1 byte:

 - sb.append((char) j);
 + sb.append((char) (j & 0xFF));

$ jdk11u-dev/bin/java TestSB
Error occurred at length=2147483645 [i=2097151, j=1021]
Exception in thread "main" java.lang.OutOfMemoryError: Requested array size 
exceeds VM limit

--
Thanks,
-Aleksey

Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]

[Maurizio Cimadamore]

On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung  wrote:

>> This reimplements core reflection with method handles.
>> 
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation 
>> uses `MethodHandle`.  For `Field` accessor, the new implementation uses 
>> `VarHandle`.For the first few invocations of one of these reflective 
>> methods on a specific reflective object we invoke the corresponding method 
>> handle directly. After that we spin a dynamic bytecode stub defined in a 
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its 
>> class data as a dynamically computed constant. Loading the method handle 
>> from a constant allows JIT to inline the method-handle invocation in order 
>> to achieve good performance.
>> 
>> The VM's native reflection methods are needed during early startup, before 
>> the method-handle mechanism is initialized. That happens soon after 
>> System::initPhase1 and before System::initPhase2, after which we switch to 
>> using method handles exclusively.
>> 
>> The core reflection and method handle implementation are updated to handle 
>> chained caller-sensitive method calls [1] properly. A caller-sensitive 
>> method can define with a caller-sensitive adapter method that will take an 
>> additional caller class parameter and the adapter method will be annotated 
>> with `@CallerSensitiveAdapter` for better auditing.   See the detailed 
>> description from [2].
>> 
>> Ran tier1-tier8 tests.   
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2] 
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove separate accessor for static vs instance method
>   
>   There is no effective difference when using MethodHandle::dropArgument for 
> static method.   Removing Static*Accessor and Instance*Accessor simplifies 
> the implementation.

Overall, seems like a solid piece of work. I did not review in full the 
intricacies with caller sensitive (as I don't know that area too much), but the 
general rewrite seems solid.

 One thing I had trouble with was the naming of the various method accessors - 
for instance, DirectMethodAccessorImpl vs. NativeMethodAccessorImpl vs. 
DelegatingMethodAccessorImpl vs. AdaptiveMethodAccessor (and there's more). 
It's not always easy to grasp from the method name which one is new and which 
one is old. Maybe sprinkling the `Handle` word on any accessor impl would help 
a bit with that.

Similarly, I found the `ClassByteBuilder` name a bit weak, since that is meant 
to only create instance of custom MHInvokers. A more apt name will help there 
too.

I also had some issues parsing the flow in 
`ReflectionFactory::newMethodAccessor` (and constructor, has similar issue):


   if (useNativeAccessor(method)) {
return DirectMethodAccessorImpl.nativeAccessor(method, 
callerSensitive);
}
return MethodHandleAccessorFactory.newMethodAccessor(method, 
callerSensitive);
 ```
 
 Why can't logic like this be encapsulated in a single factory call? E.g. it 
seems like the code is would like to abstract the differences between the 
various accessor implementations, but it doesn't seem to get all the way there 
(it's also possible I'm missing some constraint here).

src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java
 line 106:

> 104: Object invokeImpl(Object[] args) throws Throwable {
> 105: var mhInvoker = mhInvoker();
> 106: return switch (paramCount) {

As @plevart observed, I'm also a bit surprised to see this, but I note your 
comments regarding performance - especially in cold case. Every adaptation 
counts, I guess, if you're not in the hot path. But let's make sure that the 
pay off to add the extra hand-specialized cases is worth it - I'd be surprised 
if spreading arguments is that expensive.

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 
200:

> 198: return MethodHandleAccessorFactory.newMethodAccessor(method, 
> callerSensitive);
> 199: } else {
> 200: if (!useDirectMethodHandle && noInflation

seems to me that the `!useDirectMethodHandle` is useless here (always false) ?

src/java.base/share/classes/jdk/internal/reflect/VarHandleBooleanFieldAccessorImpl.java
 line 32:

> 30: import java.lang.reflect.Modifier;
> 31: 
> 32: abstract class VarHandleBooleanFieldAccessorImpl extends 
> VarHandleFieldAccessorImpl {

I wondered if we could avoid these hand specialized versions of VH accessors 
(one for each carrier type) by instead getting the getter/setter method handle 
out of the var handle, and adapt that so that its type matches Object... but 
then I realized that the `Field` API has sharp types in the gett

Re: StringBuilder OOMs earlier with JRE 11 compared to JRE 8

[David Holmes]

Hi Simeon,

Redirecting this to core-libs-dev as it is not a serviceability issue. 
(bcc serviceabillity-dev)


Thanks,
David

On 27/08/2021 8:53 pm, S A wrote:

Hi all,

while working on https://bugs.eclipse.org/bugs/show_bug.cgi?id=575641 
, I noticed that 
StringBuilder throws an OOM "half as early" in OpenJDK 11 (and 17 early 
access), when compared to OpenJDK 8.


In particular, I ran the following snippet to see where the limit of 
appending to a StringBuilder is:


StringBuilder sb = new StringBuilder();
long n = 8L * 1024L * 1024L;
for (long i = 0; i < n; ++i) {
int m = 1024;
for (int j = 0; j < m; ++j) {
int length = sb.length();
try {
sb.append((char) j);
} catch (Error e) {
System.out.println("Error occurred at length=" + length + " [i=" + i + 
", j=" + j + "]");

throw e;
}

}
}

JRE 8:

Error occurred at length=2147483645 [i=2097151, j=1021]
Exception in thread "main" java.lang.OutOfMemoryError: Requested array 
size exceeds VM limit

at java.util.Arrays.copyOf(Arrays.java:3332)
at 
java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:124)

at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:649)
at java.lang.StringBuilder.append(StringBuilder.java:202)
at 
test.stringbuilder.TestStringbuilderOOM.main(TestStringbuilderOOM.java:13)


JRE 11:

Error occurred at length=1073741822 [i=1048575, j=1022]
Exception in thread "main" java.lang.OutOfMemoryError: Requested array 
size exceeds VM limit

at java.base/java.util.Arrays.copyOf(Arrays.java:3745)
at 
java.base/java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:172)
at 
java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:748)

at java.base/java.lang.StringBuilder.append(StringBuilder.java:241)
at 
TestJava11/test.stringbuilder.TestStringbuilderOOM.main(TestStringbuilderOOM.java:13)


While StringBuilder is not a good choice for holding GBs of text, I 
wonder that no effort is made to clamp the capacity to its limit when (I 
assume) it is being doubled upon array expansion.


Is this something that should be looked into or it can be safely ignored 
(from JDK users POV)?


Best regards,
Simeon

Re: Replace StringBuffers to StringBuilders in tests

[David Holmes]

On 27/08/2021 8:42 pm, Daniel Fuchs wrote:

Hi Sergei,

I wouldn't bother replacing StringBuffers with StringDuilders in tests.
It seems a bit gratuitous - and possibly could complicate future
tests backports.

But that's my personal opinion. Others might disagree.


I agree with you. Complete waste of time and effort for zero benefit 
IMO. Sorry Sergei.


Cheers,
David


best regards,

-- daniel

On 27/08/2021 11:00, Sergei Ustimenko wrote:

Hi all,

Some tests use StringBuffers instead of StringBuilders where 
additional thread-safety
is not required as e.g. in 
test/jdk/sun/util/resources/TimeZone/Bug4640234.java:82 :

...

StringBuffer errors

=

new

StringBuffer(

""

);

StringBuffer warnings

=

new

StringBuffer(

""

);

...
There were some efforts to clean up core libs (e.g. java.base module in
https://github.com/openjdk/jdk/pull/2922) and I've noticed some tests 
that could be

improved as well.

Now there are about 300 tests for different modules that in general 
use StringBuffers
(most probably some of them not without a reason) so is it something 
worth looking

into? What you think about it?

Regards,
Sergei

Re: Replace StringBuffers to StringBuilders in tests

[Daniel Fuchs]

Hi Sergei,

I wouldn't bother replacing StringBuffers with StringDuilders in tests.
It seems a bit gratuitous - and possibly could complicate future
tests backports.

But that's my personal opinion. Others might disagree.

best regards,

-- daniel

On 27/08/2021 11:00, Sergei Ustimenko wrote:

Hi all,

Some tests use StringBuffers instead of StringBuilders where additional 
thread-safety
is not required as e.g. in 
test/jdk/sun/util/resources/TimeZone/Bug4640234.java:82 :
...

StringBuffer errors

=

new

StringBuffer(

""

);

StringBuffer warnings

=

new

StringBuffer(

""

);

...
There were some efforts to clean up core libs (e.g. java.base module in
https://github.com/openjdk/jdk/pull/2922) and I've noticed some tests that 
could be
improved as well.

Now there are about 300 tests for different modules that in general use 
StringBuffers
(most probably some of them not without a reason) so is it something worth 
looking
into? What you think about it?

Regards,
Sergei