Integrated: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller

[Ioi Lam]

On Mon, 23 May 2022 22:11:47 GMT, Ioi Lam  wrote:

> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

This pull request has now been integrated.

Changeset: 704b9a66
Author:Ioi Lam 
URL:   
https://git.openjdk.java.net/jdk/commit/704b9a66bba0dc8adb62be80fd62864b9c687c3f
Stats: 117 lines in 3 files changed: 114 ins; 0 del; 3 mod

8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller

Reviewed-by: mseledtsov, sgehwolf

-

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

[Ioi Lam]

On Tue, 24 May 2022 19:36:57 GMT, Severin Gehwolf  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   @jerboaa comments
>
> Looks good. Thanks for the thorough analysis.

Thanks @jerboaa and @mseledts for the review.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v9]

[Kim Barrett]

On Wed, 25 May 2022 09:16:43 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Change Array::data() implementation
>  - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp

Mostly good, but I missed a problem with an earlier part of the change.  Sorry 
I didn't notice sooner.

src/java.base/unix/native/libjli/java_md_common.c line 133:

> 131: 
> 132: snprintf_result = JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, 
> FILE_SEPARATOR, cmd);
> 133: if ((snprintf_result < 0) && (snprintf_result >= (int)sizeof(name))) 
> {

That should be `||` rather than `&&`.

src/java.base/unix/native/libjli/java_md_common.c line 135:

> 133: if ((snprintf_result < 0) && (snprintf_result >= (int)sizeof(name))) 
> {
> 134:   return 0;
> 135: }

Pre-existing: It seems odd that this returns `0` above and below, rather than 
returning `NULL`.  I think there are one or two other places in this file that 
are similarly using literal `0` for a null pointer, though others are using 
`NULL`.  Something to report and clean up separately from this change.

-

Changes requested by kbarrett (Reviewer).

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v4]

[Ioi Lam]

> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  fixed typo in comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8858/files
  - new: https://git.openjdk.java.net/jdk/pull/8858/files/9134182e..a13afe47

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

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

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

Re: RFR: 8284638: store skip buffers in InputStream Object [v15]

[Sergey Kuksenko]

On Tue, 24 May 2022 20:40:51 GMT, XenoAmess  wrote:

>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   invert if; refine javadoc.

Is there any practical scenario where the current code (skip buffer allocation 
on each invocation) creates issues?
Most leaf InputStreams have their own skip implementation. And the same 
question for Reader class.

-

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

RFR: 8287003: InputStreamReader::read() can return zero despite writing a char in the buffer

[Brian Burkhalter]

If only a leftover `char` remains in the stream, do not add `-1` to the return 
value in `lockedRead()`.

-

Commit messages:
 - 8287003: InputStreamReader::read() can return zero despite writing a char in 
the buffer

Changes: https://git.openjdk.java.net/jdk/pull/8895/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8895=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287003
  Stats: 33 lines in 2 files changed: 24 ins; 1 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8895.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8895/head:pull/8895

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

Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]

[XenoAmess]

On Wed, 25 May 2022 05:22:44 GMT, Stuart Marks  wrote:

> Yes, this test fails with IllegalAccessException. Probably it's easiest to 
> use a VarHandle to get private fields, similar to other usage already in this 
> test.
> 
> This test case is a bit odd though in that it's supposed to test HashSet and 
> LinkedHashSet but it mostly actually tests HashMap. It creates the Set 
> instance and immediately extracts the HashMap, which is then passed to the 
> actual test, which operates directly on the HashMap.

Considering about it being a whitebox test, it is not very weird, but still 
weird enough.
A better way might be wrap it into another layer.
Will try it when I have time.

-

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

Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]

[Naoto Sato]

On Wed, 25 May 2022 05:42:22 GMT, Stuart Marks  wrote:

> (Also, I haven't seen `StringTokenizer` in a long time)

That's some old code lingering in locale-related stuff. Will fix them after 
this PR gets integrated.

-

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

Re: RFR: 8287187: Utilize HashMap.newHashMap() in CLDRConverter [v2]

[Joe Wang]

On Wed, 25 May 2022 17:15:18 GMT, Naoto Sato  wrote:

>> Refactoring the leftover self-calculations of the optimized `HashMap` 
>> initial value with `newHashMap()` method. Also replaced some string literals 
>> using text blocks for better readability. Confirmed that the output resource 
>> bundle sources are effectively (sans indentation) the same as the original 
>> ones.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   minor fixup

Marked as reviewed by joehw (Reviewer).

-

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

Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v2]

[Mandy Chung]

On Tue, 17 May 2022 20:16:48 GMT, Tim Prinzing  wrote:

>> The Class::forName behavior change to match JNI FindClass is a compatible 
>> change and seems pretty attractive as it would be expected that 
>> Class::forName would give the same behavior as FindClass which uses the 
>> system classloader.  The test for 8281006 was enhanced to test for this 
>> change.  Merged master to pick up fixes to unrelated test failures to reduce 
>> noise.
>
> Tim Prinzing has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added javadoc comment

src/java.base/share/classes/java/lang/Class.java line 361:

> 359:  *
> 360:  * 
> 361:  * This method is caller sensitive.  In cases where this method is 
> called

You can drop "This method is caller sensitive." sentence for consistency with 
other caller-sensitive methods that do not state that explicitly.   The javadoc 
already specifies that it uses the class loader of the current class.

-

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]

[Roger Riggs]

On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy  wrote:

>> This is an early review of changes to better model JVM access flags, that is 
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>> 
>> Language level modifiers and JVM level access flags are closely related, but 
>> distinct. There are concepts that overlap in the two domains (public, 
>> private, etc.), others that only have a language-level modifier (sealed), 
>> and still others that only have an access flag (synthetic).
>> 
>> The existing java.lang.reflect.Modifier class is inadequate to model these 
>> subtleties. For example, the bit positions used by access flags on different 
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
>> methods. Just having a raw integer does not provide sufficient context to 
>> decode the corresponding language-level string. Methods like 
>> Modifier.methodModifiers() were introduced to cope with this situation.
>> 
>> With additional modifiers and flags on the horizon with projects like 
>> Valhalla, addressing the existent modeling deficiency now ahead of time is 
>> reasonable before further strain is introduced.
>> 
>> This PR in its current form is meant to give the overall shape of the API. 
>> It is missing implementations to map from, say, method modifiers to access 
>> flags, taking into account overlaps in bit positions.
>> 
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
>> once the API is further along.
>
> Joe Darcy 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 32 additional commits since 
> the last revision:
> 
>  - Target JDK 20 rather than 19.
>  - Merge branch 'master' into JDK-8266670
>  - Add mask values to constants' javadoc.
>  - Implement review feedback from mlchung.
>  - Fix type in @throws tag.
>  - Merge branch 'master' into JDK-8266670
>  - Respond to review feedback.
>  - Merge branch 'master' into JDK-8266670
>  - Make workding changes suggested in review feedback.
>  - Merge branch 'master' into JDK-8266670
>  - ... and 22 more: 
> https://git.openjdk.java.net/jdk/compare/65e59633...05cf2d8b

AccessFlags.SUPER can/should be removed; it is unused and will be redefined in 
the [Value Objects JEP](https://openjdk.java.net/jeps/8277163).
It will be a cleaner transition if there is no opportunity to create a 
dependency on the obsolete flag.

-

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

Re: RFR: [CSR] 8287026: Changes related to overflow handling in Random.doubles

[Raffaello Giulietti]

Sorry, it's the other way round!
To bring the spec in sync with the extended behavior of the PR [2]


From: core-libs-dev  on behalf of 
Raffaello Giulietti 
Date: Wednesday, 25 May 2022 at 18:57
To: core-libs-dev@openjdk.java.net 
Subject: RFR: [CSR] 8287026: Changes related to overflow handling in 
Random.doubles
Please review this CSR [1], whose aim is to bring the extended behavior of the 
PR [2] in sync with the spec.

Greetings
Raffaello



[1] https://bugs.openjdk.java.net/browse/JDK-8287026
[2] https://git.openjdk.java.net/jdk/pull/8791

Re: RFR: 8287206: Use WrongThreadException for confinement errors

[Uwe Schindler]

On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore  
wrote:

> This patch tweaks the foreign API to use the newly added 
> `WrongThreadException` instead of `IllegalStateException` to report 
> confinement errors.

I changed the PR on our side, `IllegalStateException` is fine, if you catch it 
as close as possible to the code calling varhandles and so on (which is the 
case for Lucene). Thanks for this PR, makes life much easier!

-

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

Re: RFR: 8287187: Utilize HashMap.newHashMap() in CLDRConverter [v2]

[Naoto Sato]

> Refactoring the leftover self-calculations of the optimized `HashMap` initial 
> value with `newHashMap()` method. Also replaced some string literals using 
> text blocks for better readability. Confirmed that the output resource bundle 
> sources are effectively (sans indentation) the same as the original ones.

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

  minor fixup

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8887/files
  - new: https://git.openjdk.java.net/jdk/pull/8887/files/faab3ea1..c2cc3391

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

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

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

[Ioi Lam]

On Wed, 25 May 2022 15:50:32 GMT, Severin Gehwolf  wrote:

>> It confused me, fwiw. Anyway up to you. It's not super important.
>
> works for me. +1. Note the typo 
> `anyCgroupsV1Controller/anyCgroupsV2Controller` not **V1** twice.

Oops, I'll fixed that. Thanks!

-

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

RFR: [CSR] 8287026: Changes related to overflow handling in Random.doubles

[Raffaello Giulietti]

Please review this CSR [1], whose aim is to bring the extended behavior of the 
PR [2] in sync with the spec.

Greetings
Raffaello



[1] https://bugs.openjdk.java.net/browse/JDK-8287026
[2] https://git.openjdk.java.net/jdk/pull/8791

RFR: 8287187: Utilize HashMap.newHashMap() in CLDRConverter

[Naoto Sato]

Refactoring the leftover self-calculations of the optimized `HashMap` initial 
value with `newHashMap()` method. Also replaced some string literals using text 
blocks for better readability. Confirmed that the output resource bundle 
sources are effectively (sans indentation) the same as the original ones.

-

Commit messages:
 - 8287187: Utilize HashMap.newHashMap() in CLDRConverter

Changes: https://git.openjdk.java.net/jdk/pull/8887/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8887=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287187
  Stats: 65 lines in 1 file changed: 8 ins; 1 del; 56 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8887.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8887/head:pull/8887

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v9]

[Vladimir Kozlov]

On Wed, 25 May 2022 06:29:06 GMT, Jatin Bhateja  wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8284960: Review comments resolved.
>
> Hi @vnkozlov , Your comments have been addressed.

@jatin-bhateja something wrong with merge. `vpadd()` is removed. It was added 
by #8778 and still is used in `x86.ad`.

-

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

[Severin Gehwolf]

On Wed, 25 May 2022 15:51:04 GMT, Ioi Lam  wrote:

>> This PR fixes a bug found on an Ubuntu host that's mostly running with 
>> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 
>> mode.
>> 
>> The container support code in the VM and JDK checks if we have 
>> simultaneously mounted v1 and v2 containers. If so, we revert to "hybrid" 
>> mode (which is essentially v1).
>> 
>> However, the "hybrid checks" only considers the following controllers that 
>> Java cares about:
>> 
>> - cpu
>> - cpuacct
>> - cpuset
>> - blkio
>> - memory
>> - pids
>> 
>> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
>> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
>> into the assert.
>> 
>> 
>> $ cat /proc/self/cgroup
>> 1:freezer:/
>> 0::/user.slice/user-1001.slice/session-85.scope
>> 
>> 
>> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
>> decided that the system has not mounted any v1 controllers that we care 
>> about, so we should just ignore all the v1 controllers (which has a 
>> non-empty name).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   fixed comments

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 149:

> 147: // other controllers (such as freezer) are ignored and
> 148: // are not considered in the checks below for
> 149: // anyCgroupsV1Controller/anyCgroupsV1Controller.

It still has the `anyCgroupsV1Controller/anyCgroupsV1Controller` typo. Not 
**V1** twice?

-

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

Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts

[Raffaello Giulietti]

When they return, these methods meet the property, even for -0.0 provided the 
underlying eq is floating-point ==, not *::compare:
-0.0 -> 0 -> 0.0
and there is a loss of information about the sign bit.

However, as currently stated, the property is not 100% correct. For example,
(long) (double) Long.MAX_VALUE == Long.MAX_VALUE
evaluates to true but you cannot conclude that Long.MAX_VALUE is converted 
exactly to a double: indeed, it is not.
The methods deal with these special cases and throw.
The tests for pattern matching should deal with special cases as well.




From: Brian Goetz 
Date: Wednesday, 25 May 2022 at 17:18
To: Raffaello Giulietti , 
core-libs-dev@openjdk.java.net 
Subject: Re: RFR: 8279986: methods Math::asXExact for safely checked primitive 
casts
Another way to evaluate answers here is: are we inventing new relations, or 
merely clarifying old ones?  The latter is often more desirable.

For example, we might say that x:X can be converted exactly to Y, for primitive 
X and Y, iff:

eq( (X) (Y) x, x )

where `eq` is `==` for non-floating primitive types, and derived from 
Float::compare and Double::compare for floating point.  This means we are not 
inventing any new conversions or comparisons, but merely combining ones we 
already have in the language/platform in a convenient way.

Do the toXExact methods you've defined have this characteristic?

(Though, while this is often the best starting place, it is not always a 
slam-dunk answer; sometimes there are good reasons to depart from existing 
relations, but we should be explicit about what those are.)


On 5/25/2022 9:46 AM, Raffaello Giulietti wrote:
The motivation behind this PR is not driven by pattern matching: John Rose (the 
reporter of the JBS issue) is concerned about having safer casts that, in 
addition, can be JITted to efficient code.

But I get your point that having non-throwing tests better serves pattern 
matching. However, I’m not sure that pattern matching alone would remove the 
more general need for the proposed methods.

As for the controversial question about -0.0, as you note any proposal will 
kind of suck. With “safer” cast methods we can have two (or even more) variants.

In the context of primitive pattern matching (including the relevant material 
in the JLS), however, it would be probably much simpler to allow for matches 
between integral types on one side and for matches between floating-point types 
on the other side, but not a mix. The nuisance with -0.0 and other special 
values would disappear altogether.

Thus, while examples like
if (anIntExpression instanceof byte b)
and
if (aDoubleExpression instanceof float f)
make perfect sense, would an example like
if (aDoubleExpression instanceof short s)
be pragmatically reasonable?

IIRC, the discussions about “Primitive type patterns” and “Evolving past 
reference type patterns” in the amber-spec-experts mailing list of April 2022 
don’t even mention the mixed integral/floating-point case.


Greetings
Raffaello


From: core-libs-dev 

 on behalf of Brian Goetz 

Date: Tuesday, 24 May 2022 at 00:09
To: Raffaello Giulietti , 
core-libs-dev@openjdk.java.net 

Subject: Re: RFR: 8279986: methods Math::asXExact for safely checked primitive 
casts
This work is quite timely as we are now paving the way for primitive
type patterns over in Project Amber, and also has a nontrivial
connection with Valhalla.  If you'll pardon a brief digression...

Instanceof and casting work together in a familiar way: before you cast,
you first ask instanceof -- but this is restricted currently to
reference types.  But the pattern is obvious: instanceof is the
precondition check for casting, which asks: "If I made this cast, would
I like the answer."  There are currently two reasons to not like the
answer: a CCE, or the operand is null (because, even though the cast
would succeed, if you tried to use it as a member of that type, you
wouldn't like the answer then.)

If we wanted to extend `instanceof` to primitive types, we are asking
the same question: if I were to cast to this type, would I like the
answer.  And casts involving primitives give us two more reasons to not
like the answer: an NPE (due to unboxing), or loss of precision.  Which
means that we have to specify in JLS 5.1 what cast with loss of
precision means.  At the very least, we will want to align this work
with that; the asXExact should be able to say "throws if the cast would
lose precision", where "lose precision" is precisely defined by the JLS.

Separately, Project Valhalla will let us define new primitive-like
types, such as HalfFloat and SuperLong. Conversions between primitives
are currently specified in a complex table in JLS 5.1.  But surely we
will want to support primitive widening conversions between 

Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance()

[Maxim Kartashev]

On Wed, 25 May 2022 14:47:06 GMT, Peter Levart  wrote:

>> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
>>  line 113:
>> 
>>> 111: CgroupInfo anyController = 
>>> infos.values().iterator().next();
>>> 112: CgroupSubsystem subsystem = 
>>> CgroupV2Subsystem.getInstance(anyController);
>>> 113: return subsystem != null ? new CgroupMetrics(subsystem) : 
>>> null;
>> 
>> Looking at implementation of CgroupV2Subsystem.getInstance(...), it seems 
>> that it always returns != null ...
>
> `CgroupV1Subsystem.getInstance(...)` also claims that it never returns 
> `null`, but has a code-path that actually returns `null` (when there is no 
> active controller). Is this a possible outcome?

@plevart Are you asking about the reason for the crash or about the changes?
If it's the former, then I believe that the crash comes not from 
`getInstance()` returning `null`, but from further down the stack because 
`null` is being passed to `getInstance()`. I could be wrong in interpreting the 
report, though.

If the question's about the changes, then those are restricted to CgroupV2, so 
I'm not sure how `CgroupV1Subsystem.getInstance(...)` returning null is 
related. FWIW, I also don't think we are going to get here if there are no 
active controllers. There's this code a few lines above:

if (!result.isAnyControllersEnabled()) {
return null;
}

-

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

[Ioi Lam]

> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  fixed comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8858/files
  - new: https://git.openjdk.java.net/jdk/pull/8858/files/1f17b6e8..9134182e

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

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

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

[Severin Gehwolf]

On Wed, 25 May 2022 08:40:48 GMT, Severin Gehwolf  wrote:

>> If you don't like the `default:` coding style, how about this:
>> 
>> 
>> switch (info.getName()) {
>> // Only the following controllers are important to Java. All
>> // other controllers (such as freezer) are ignored and
>> // are not considered in the checks below for
>> // anyCgroupsV1Controller/anyCgroupsV1Controller.
>> case CPU_CTRL:  infos.put(CPU_CTRL, info); break;
>> case CPUACCT_CTRL:  infos.put(CPUACCT_CTRL, info); break;
>> case CPUSET_CTRL:   infos.put(CPUSET_CTRL, info); break;
>> case MEMORY_CTRL:   infos.put(MEMORY_CTRL, info); break;
>> case BLKIO_CTRL:infos.put(BLKIO_CTRL, info); break;
>> case PIDS_CTRL: infos.put(PIDS_CTRL, info); break;
>> }
>
> It confused me, fwiw. Anyway up to you. It's not super important.

works for me. +1. Note the typo `anyCgroupsV1Controller/anyCgroupsV2Controller` 
not **V1** twice.

-

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

[Ioi Lam]

On Tue, 24 May 2022 19:49:35 GMT, Ioi Lam  wrote:

>> My bad. How about `Intentional incomplete switch. There are ...`? Anyway, 
>> why is the empty `default` case needed other than for the comment?
>
> To me, the `default:` switch is a clear indication that "everything else 
> comes here". So you won't be confused whether the comment is related to the 
> last line just above the comment.

If you don't like the `default:` coding style, how about this:


switch (info.getName()) {
// Only the following controllers are important to Java. All
// other controllers (such as freezer) are ignored and
// are not considered in the checks below for
// anyCgroupsV1Controller/anyCgroupsV1Controller.
case CPU_CTRL:  infos.put(CPU_CTRL, info); break;
case CPUACCT_CTRL:  infos.put(CPUACCT_CTRL, info); break;
case CPUSET_CTRL:   infos.put(CPUSET_CTRL, info); break;
case MEMORY_CTRL:   infos.put(MEMORY_CTRL, info); break;
case BLKIO_CTRL:infos.put(BLKIO_CTRL, info); break;
case PIDS_CTRL: infos.put(PIDS_CTRL, info); break;
}

-

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

Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts

[Brian Goetz]

Another way to evaluate answers here is: are we inventing new relations, 
or merely clarifying old ones?  The latter is often more desirable.


For example, we might say that x:X can be converted exactly to Y, for 
primitive X and Y, iff:


    eq( (X) (Y) x, x )

where `eq` is `==` for non-floating primitive types, and derived from 
Float::compare and Double::compare for floating point. This means we are 
not inventing any new conversions or comparisons, but merely combining 
ones we already have in the language/platform in a convenient way.


Do the toXExact methods you've defined have this characteristic?

(Though, while this is often the best starting place, it is not always a 
slam-dunk answer; sometimes there are good reasons to depart from 
existing relations, but we should be explicit about what those are.)




On 5/25/2022 9:46 AM, Raffaello Giulietti wrote:


Themotivation behind this PR isnot driven by pattern matching:John 
Rose (the reporter of the JBS issue) is concerned about having safer 
casts that, in addition, can be JITted to efficient code.


But I get your point that having non-throwing testsbetter serves 
pattern matching.However, I’m not sure that pattern matching alone 
would remove the more general need for the proposed methods.


As for the controversial question about -0.0, as you note any proposal 
will kind of suck. With “safer” cast methods we can have two (or even 
more) variants.


In the context of primitive pattern matching (including the relevant 
material in the JLS), however, it would be probably much simpler to 
allow for matches between integral types on one side and for matches 
between floating-point types on the other side, but not a mix. The 
nuisance with -0.0 and other special values would disappear altogether.


Thus, while examples like

    if (anIntExpression instanceof byte b)

and

    if (aDoubleExpression instanceof float f)

make perfect sense, would an example like
    if (aDoubleExpression instanceof short s)

be pragmatically reasonable?

IIRC, the discussions about “Primitive type patterns” and “Evolving 
past reference type patterns” in the amber-spec-experts mailing list 
of April 2022 don’t even mention the mixed integral/floating-point case.


Greetings
Raffaello

*From: *core-libs-dev  on behalf 
of Brian Goetz 

*Date: *Tuesday, 24 May 2022 at 00:09
*To: *Raffaello Giulietti , 
core-libs-dev@openjdk.java.net 
*Subject: *Re: RFR: 8279986: methods Math::asXExact for safely checked 
primitive casts


This work is quite timely as we are now paving the way for primitive
type patterns over in Project Amber, and also has a nontrivial
connection with Valhalla.  If you'll pardon a brief digression...

Instanceof and casting work together in a familiar way: before you cast,
you first ask instanceof -- but this is restricted currently to
reference types.  But the pattern is obvious: instanceof is the
precondition check for casting, which asks: "If I made this cast, would
I like the answer."  There are currently two reasons to not like the
answer: a CCE, or the operand is null (because, even though the cast
would succeed, if you tried to use it as a member of that type, you
wouldn't like the answer then.)

If we wanted to extend `instanceof` to primitive types, we are asking
the same question: if I were to cast to this type, would I like the
answer.  And casts involving primitives give us two more reasons to not
like the answer: an NPE (due to unboxing), or loss of precision.  Which
means that we have to specify in JLS 5.1 what cast with loss of
precision means.  At the very least, we will want to align this work
with that; the asXExact should be able to say "throws if the cast would
lose precision", where "lose precision" is precisely defined by the JLS.

Separately, Project Valhalla will let us define new primitive-like
types, such as HalfFloat and SuperLong. Conversions between primitives
are currently specified in a complex table in JLS 5.1. But surely we
will want to support primitive widening conversions between HalfFloat
and float (somehow; how we do this is a separate discussion.)  Which
brings us back to pattern matching; narrowing casts are inherently
partial, and declared patterns bring partiality into the "return type",
and are the natural way (when we have it) to express things like "cast,
but fail if you can't do so safely". This is preferable to throwing
(which currently is the our choice.)  So it might be a little
unfortunate to introduce throwing toXExactly now and then have to
introduce separate patterns which signal precision loss by match
failure.  (Though that's not the end of the world if there is some
duplication.)

What this says is that the current proposal of toXExact is not the
primitive, because we probably wouldn't want to implement a pattern in
terms of the throwing version.

Converting from float/double to integral types is particularly tricky
with -0.0.  Both answers kind of suck.  (This is a familiar situation,
and these 

Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts

[Brian Goetz]

Themotivation behind this PR isnot driven by pattern matching:John 
Rose (the reporter of the JBS issue) is concerned about having safer 
casts that, in addition, can be JITted to efficient code.




Yes, of course.  But also, as the platform evolves, it often happens 
that the same issues arises in separate places.  It would be terrible if 
asXExact had a different opinion of what could be converted exactly to 
an int, from the language's `instanceof int`. And similarly, it would be 
unfortunate if the semantics were driven by nothing more than "who got 
there first."  So we frequently discover things that we thought were 
independent increments of functionality, that turn out to want to be 
co-designed with other things, even those not known at the time we 
started.  This is just how this game works.


But I get your point that having non-throwing testsbetter serves 
pattern matching.However, I’m not sure that pattern matching alone 
would remove the more general need for the proposed methods.




Yes, not sure either.  The game here is "find the primitive"; our first 
move in this game is not always the right one.  So let's figure it out.


As for the controversial question about -0.0, as you note any proposal 
will kind of suck. With “safer” cast methods we can have two (or even 
more) variants.




Also, small changes in terminology can subtly bias our answer. Words 
like "exact", "information-preserving", "loss of precision", "safe", 
etc, all seem to circle the same concept, but the exact choice of 
terminology -- often made at random at the beginning of an investigation 
-- can bias us one way or the other.  (Concrete illustration: converting 
-0.0 to 0 seems questionable when your rule is "no loss of information", 
but seems more ambiguous when your rule is "safe".)


In the context of primitive pattern matching (including the relevant 
material in the JLS), however, it would be probably much simpler to 
allow for matches between integral types on one side and for matches 
between floating-point types on the other side, but not a mix. The 
nuisance with -0.0 and other special values would disappear altogether.




That was my first thought as well, but then I had second thoughts, as 
this seemed like wishful thinking.  If we are willing to say:


    long x = 7;
    if (x instanceof int) { /* yes, it is */ }

then it may seem quite odd to not also say

    double x = 7.0;
    if (x instanceof int) { /* this too? */ }

Because, the natural way to interpret the first is "is the value on the 
left representable in the type on the right."  Clearly 7 is 
representable as an int.  But so is 7.0; we lose nothing going from 
double 7.0 -> int 7 -> double 7.0.  And whoosh, now we're sucked into 
belly of the machine, staring down -0.0 and NaN other abominations, 
questioning our life choices.


That's not to say that your initial thought is wrong, just that it will 
be surprising if we go that way.  Maybe surprises are inevitable; maybe 
this is the least bad of possible surprises.  We should eliminate the 
"maybes" from this analysis first, though, before deciding.



Thus, while examples like

    if (anIntExpression instanceof byte b)

and

    if (aDoubleExpression instanceof float f)

make perfect sense, would an example like
    if (aDoubleExpression instanceof short s)

be pragmatically reasonable?

IIRC, the discussions about “Primitive type patterns” and “Evolving 
past reference type patterns” in the amber-spec-experts mailing list 
of April 2022 don’t even mention the mixed integral/floating-point case.




Correct, we hadn't gotten there yet, we were still trying to wrap our 
heads around how it should work in the easier cases.

Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance()

[Peter Levart]

On Wed, 25 May 2022 14:39:42 GMT, Peter Levart  wrote:

>> Following the logic from the comment directly above the changed line, since 
>> it doesn't matter which controller we pick, pick any available controller 
>> instead of the one called "memory" specifically. This way we are guarded 
>> against getting `null` as `anyController`, which is being immediately passed 
>> down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept 
>> `null` values. 
>> 
>> It is also worth noting that the previous checks (such as that at line 89) 
>> make sure that there exist at least one controller in the map.
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
> line 113:
> 
>> 111: CgroupInfo anyController = infos.values().iterator().next();
>> 112: CgroupSubsystem subsystem = 
>> CgroupV2Subsystem.getInstance(anyController);
>> 113: return subsystem != null ? new CgroupMetrics(subsystem) : 
>> null;
> 
> Looking at implementation of CgroupV2Subsystem.getInstance(...), it seems 
> that it always returns != null ...

`CgroupV1Subsystem.getInstance(...)` also claims that it never returns `null`, 
but has a code-path that actually returns `null` (when there is no active 
controller). Is this a possible outcome?

-

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

Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance()

[Peter Levart]

On Fri, 20 May 2022 08:34:58 GMT, Maxim Kartashev  
wrote:

> Following the logic from the comment directly above the changed line, since 
> it doesn't matter which controller we pick, pick any available controller 
> instead of the one called "memory" specifically. This way we are guarded 
> against getting `null` as `anyController`, which is being immediately passed 
> down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` 
> values. 
> 
> It is also worth noting that the previous checks (such as that at line 89) 
> make sure that there exist at least one controller in the map.

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 113:

> 111: CgroupInfo anyController = infos.values().iterator().next();
> 112: CgroupSubsystem subsystem = 
> CgroupV2Subsystem.getInstance(anyController);
> 113: return subsystem != null ? new CgroupMetrics(subsystem) : 
> null;

Looking at implementation of CgroupV2Subsystem.getInstance(...), it seems that 
it always returns != null ...

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently [v2]

[Jorn Vernee]

On Wed, 25 May 2022 14:03:41 GMT, Claes Redestad  wrote:

>> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` 
>> allows keys to be compacted when all byte values of the key fit in 4 bits, 
>> otherwise a byte array is allocated and used. This means that all transforms 
>> with a kind value above 15 will be forced to allocate and use array 
>> comparisons.
>> 
>> Removing unused and folding some transforms to ensure all existing kinds can 
>> fit snugly within the 0-15 value range realize a minor improvement to 
>> footprint, speed and allocation pressure of affected transforms, e.g. 
>> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
>> 
>> Baseline:
>> 
>> Benchmark  Mode  Cnt 
>> Score Error   Units
>> SCFB.makeConcatWithConstants   avgt   15  
>> 2048.475 ?  69.887   ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
>> 3487.311 ?  80.385B/op
>> 
>> 
>> Patched:
>> 
>> Benchmark  Mode  Cnt 
>> Score Error   Units
>> SCFB.makeConcatWithConstants   avgt   15  
>> 1961.985 ? 101.519   ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
>> 3156.478 ? 183.600B/op
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address review comments, fix unsafe shift, rework and remove ofBothArrays

Marked as reviewed by jvernee (Reviewer).

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently [v3]

[Claes Redestad]

> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` 
> allows keys to be compacted when all byte values of the key fit in 4 bits, 
> otherwise a byte array is allocated and used. This means that all transforms 
> with a kind value above 15 will be forced to allocate and use array 
> comparisons.
> 
> Removing unused and folding some transforms to ensure all existing kinds can 
> fit snugly within the 0-15 value range realize a minor improvement to 
> footprint, speed and allocation pressure of affected transforms, e.g. 
> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
> 
> Baseline:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 2048.475 ?  69.887   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3487.311 ?  80.385B/op
> 
> 
> Patched:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 1961.985 ? 101.519   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3156.478 ? 183.600B/op

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

  Missed correctly taking b1 into account in of(byte, int, int...) 
(java/lang/String/concat/ImplicitStringConcatShapes.java test failure)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8881/files
  - new: https://git.openjdk.java.net/jdk/pull/8881/files/2be3b25c..612b4ece

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

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

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently [v2]

[Claes Redestad]

> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` 
> allows keys to be compacted when all byte values of the key fit in 4 bits, 
> otherwise a byte array is allocated and used. This means that all transforms 
> with a kind value above 15 will be forced to allocate and use array 
> comparisons.
> 
> Removing unused and folding some transforms to ensure all existing kinds can 
> fit snugly within the 0-15 value range realize a minor improvement to 
> footprint, speed and allocation pressure of affected transforms, e.g. 
> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
> 
> Baseline:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 2048.475 ?  69.887   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3487.311 ?  80.385B/op
> 
> 
> Patched:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 1961.985 ? 101.519   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3156.478 ? 183.600B/op

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

  Address review comments, fix unsafe shift, rework and remove ofBothArrays

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8881/files
  - new: https://git.openjdk.java.net/jdk/pull/8881/files/001e9d16..2be3b25c

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

  Stats: 86 lines in 2 files changed: 50 ins; 12 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8881.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8881/head:pull/8881

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

Re: RFR: 8287244: Add bound check in indexed memory access var handle [v3]

[Jorn Vernee]

On Tue, 24 May 2022 18:15:45 GMT, Maurizio Cimadamore  
wrote:

>> Constructing indexed var handles using the `MemoryLayout` API produces 
>> `VarHandle` which do not check the input indices for out-of-bounds 
>> conditions.
>> While this can never result in a VM crash (after all the memory segment will 
>> protect against "true" OOB access), it is still possible for an access 
>> expression to refer to parts of a segment that are logically unrelated.
>> 
>> This patch adds a "logical" bound check to all indexed var handles generated 
>> using the layout API.
>> Benchmarks are not affected by the check. Users are still able to create 
>> custom "unchecked" var handles, using the combinator API in `MethodHandles`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comments

Very nice!

-

Marked as reviewed by jvernee (Reviewer).

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

Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts

[Raffaello Giulietti]

The motivation behind this PR is not driven by pattern matching: John Rose (the 
reporter of the JBS issue) is concerned about having safer casts that, in 
addition, can be JITted to efficient code.

But I get your point that having non-throwing tests better serves pattern 
matching. However, I’m not sure that pattern matching alone would remove the 
more general need for the proposed methods.

As for the controversial question about -0.0, as you note any proposal will 
kind of suck. With “safer” cast methods we can have two (or even more) variants.

In the context of primitive pattern matching (including the relevant material 
in the JLS), however, it would be probably much simpler to allow for matches 
between integral types on one side and for matches between floating-point types 
on the other side, but not a mix. The nuisance with -0.0 and other special 
values would disappear altogether.

Thus, while examples like
if (anIntExpression instanceof byte b)
and
if (aDoubleExpression instanceof float f)
make perfect sense, would an example like
if (aDoubleExpression instanceof short s)
be pragmatically reasonable?

IIRC, the discussions about “Primitive type patterns” and “Evolving past 
reference type patterns” in the amber-spec-experts mailing list of April 2022 
don’t even mention the mixed integral/floating-point case.


Greetings
Raffaello


From: core-libs-dev  on behalf of Brian 
Goetz 
Date: Tuesday, 24 May 2022 at 00:09
To: Raffaello Giulietti , core-libs-dev@openjdk.java.net 

Subject: Re: RFR: 8279986: methods Math::asXExact for safely checked primitive 
casts
This work is quite timely as we are now paving the way for primitive
type patterns over in Project Amber, and also has a nontrivial
connection with Valhalla.  If you'll pardon a brief digression...

Instanceof and casting work together in a familiar way: before you cast,
you first ask instanceof -- but this is restricted currently to
reference types.  But the pattern is obvious: instanceof is the
precondition check for casting, which asks: "If I made this cast, would
I like the answer."  There are currently two reasons to not like the
answer: a CCE, or the operand is null (because, even though the cast
would succeed, if you tried to use it as a member of that type, you
wouldn't like the answer then.)

If we wanted to extend `instanceof` to primitive types, we are asking
the same question: if I were to cast to this type, would I like the
answer.  And casts involving primitives give us two more reasons to not
like the answer: an NPE (due to unboxing), or loss of precision.  Which
means that we have to specify in JLS 5.1 what cast with loss of
precision means.  At the very least, we will want to align this work
with that; the asXExact should be able to say "throws if the cast would
lose precision", where "lose precision" is precisely defined by the JLS.

Separately, Project Valhalla will let us define new primitive-like
types, such as HalfFloat and SuperLong. Conversions between primitives
are currently specified in a complex table in JLS 5.1.  But surely we
will want to support primitive widening conversions between HalfFloat
and float (somehow; how we do this is a separate discussion.)  Which
brings us back to pattern matching; narrowing casts are inherently
partial, and declared patterns bring partiality into the "return type",
and are the natural way (when we have it) to express things like "cast,
but fail if you can't do so safely". This is preferable to throwing
(which currently is the our choice.)  So it might be a little
unfortunate to introduce throwing toXExactly now and then have to
introduce separate patterns which signal precision loss by match
failure.  (Though that's not the end of the world if there is some
duplication.)

What this says is that the current proposal of toXExact is not the
primitive, because we probably wouldn't want to implement a pattern in
terms of the throwing version.

Converting from float/double to integral types is particularly tricky
with -0.0.  Both answers kind of suck.  (This is a familiar situation,
and these can be very difficult to resolve, as for each position,
*someone* has decided the other position is untenable.)  I understand
the rationale behind Michael H's "but its not exact", but let's not
pretend one answer is good and the other sucks -- they both suck, and
therefore the decision can be made on other factors.

So I have a few new wrinkles to add to the story:

  - We should wait until we have candidate JLS text for "cast conversion
without loss of precision", and ensure the two are consistent, before
pushing;
  - I not quite comfortable with settling the -0.0 issue just yet, there
are some other explorations to complete first;
  - We should be prepared for the fact that we will, sometime soon, have
to implement this whole set again as patterns that do not throw.








On 5/5/2022 6:18 AM, Raffaello Giulietti wrote:
> Add a family of "safe" cast methods.
>
> -
>
> 

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Jorn Vernee]

On Wed, 25 May 2022 13:37:14 GMT, Claes Redestad  wrote:

>> Maybe an `assert b == b23456[i]` would be nice here.
>
> All usage implies the int is an argument position, which by spec is 
> constrained to be in the 0-255 range. But surely checking or asserting 
> shouldn't hurt.

Yes, please. IMHO it makes it clear that that is the assumption of this code.

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Claes Redestad]

On Wed, 25 May 2022 13:30:23 GMT, Jorn Vernee  wrote:

>> Maybe not... argument positions should fit in a byte as well. But, maybe 
>> there are other problematic cases? Or are the ints guaranteed to fit in a 
>> byte?
>
> Maybe an `assert b == b23456[i]` would be nice here.

All usage implies the int is an argument position, which by spec is constrained 
to be in the 0-255 range. But surely checking or asserting shouldn't hurt.

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Jorn Vernee]

On Wed, 25 May 2022 13:27:17 GMT, Jorn Vernee  wrote:

>> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` 
>> allows keys to be compacted when all byte values of the key fit in 4 bits, 
>> otherwise a byte array is allocated and used. This means that all transforms 
>> with a kind value above 15 will be forced to allocate and use array 
>> comparisons.
>> 
>> Removing unused and folding some transforms to ensure all existing kinds can 
>> fit snugly within the 0-15 value range realize a minor improvement to 
>> footprint, speed and allocation pressure of affected transforms, e.g. 
>> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
>> 
>> Baseline:
>> 
>> Benchmark  Mode  Cnt 
>> Score Error   Units
>> SCFB.makeConcatWithConstants   avgt   15  
>> 2048.475 ?  69.887   ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
>> 3487.311 ?  80.385B/op
>> 
>> 
>> Patched:
>> 
>> Benchmark  Mode  Cnt 
>> Score Error   Units
>> SCFB.makeConcatWithConstants   avgt   15  
>> 1961.985 ? 101.519   ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
>> 3156.478 ? 183.600B/op
>
> src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 239:
> 
>> 237: for (int i = 0; i < b23456.length; i++) {
>> 238: int b = b23456[i] & 0xFF;
>> 239: bitset |= b;
> 
> Looks like `b` is always truncated. I wonder what happens if the ints in this 
> array are larger than a byte (which seems to be possible in e.g. the case of 
> argument positions). Some higher order bits might be dropped, but the 
> resulting `b` might only have the least significant 4 bits set.
> 
> I think the untruncated value should be used to compute the bitset? `butset 
> |= b23456[i]`? Then the `inRange` check should reject that case.

Maybe not... argument positions should fit in a byte as well. But, maybe there 
are other problematic cases? Or are the ints guaranteed to fit in a byte?

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Jorn Vernee]

On Wed, 25 May 2022 13:28:52 GMT, Jorn Vernee  wrote:

>> src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 239:
>> 
>>> 237: for (int i = 0; i < b23456.length; i++) {
>>> 238: int b = b23456[i] & 0xFF;
>>> 239: bitset |= b;
>> 
>> Looks like `b` is always truncated. I wonder what happens if the ints in 
>> this array are larger than a byte (which seems to be possible in e.g. the 
>> case of argument positions). Some higher order bits might be dropped, but 
>> the resulting `b` might only have the least significant 4 bits set.
>> 
>> I think the untruncated value should be used to compute the bitset? `butset 
>> |= b23456[i]`? Then the `inRange` check should reject that case.
>
> Maybe not... argument positions should fit in a byte as well. But, maybe 
> there are other problematic cases? Or are the ints guaranteed to fit in a 
> byte?

Maybe an `assert b == b23456[i]` would be nice here.

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Jorn Vernee]

On Wed, 25 May 2022 09:38:08 GMT, Claes Redestad  wrote:

> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` 
> allows keys to be compacted when all byte values of the key fit in 4 bits, 
> otherwise a byte array is allocated and used. This means that all transforms 
> with a kind value above 15 will be forced to allocate and use array 
> comparisons.
> 
> Removing unused and folding some transforms to ensure all existing kinds can 
> fit snugly within the 0-15 value range realize a minor improvement to 
> footprint, speed and allocation pressure of affected transforms, e.g. 
> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
> 
> Baseline:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 2048.475 ?  69.887   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3487.311 ?  80.385B/op
> 
> 
> Patched:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 1961.985 ? 101.519   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3156.478 ? 183.600B/op

src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 239:

> 237: for (int i = 0; i < b23456.length; i++) {
> 238: int b = b23456[i] & 0xFF;
> 239: bitset |= b;

Looks like `b` is always truncated. I wonder what happens if the ints in this 
array are larger than a byte (which seems to be possible in e.g. the case of 
argument positions). Some higher order bits might be dropped, but the resulting 
`b` might only have the least significant 4 bits set.

I think the untruncated value should be used to compute the bitset? `butset |= 
b23456[i]`? Then the `inRange` check should reject that case.

-

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

Re: RFR: 8262889: Compiler implementation for Record Patterns [v7]

[Rémi Forax]

On Wed, 25 May 2022 04:20:35 GMT, Jan Lahoda  wrote:

>> 8262889: Compiler implementation for Record Patterns
>> 
>> A first version of a patch that introduces record patterns into javac as a 
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>> 
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to 
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very 
>> straightforward - effectivelly the record patterns are desugared into 
>> guards/conditions. This will likely be improved in some future 
>> version/preview
>> 
>> `MatchException` has been extended to cover additional cases related to 
>> record patterns.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 34 commits:
> 
>  - Merge branch 'master' into patterns-record-deconstruction3
>  - Post merge fix.
>  - Merge branch 'master' into patterns-record-deconstruction3
>  - Fixing (non-)exhaustiveness for enum.
>  - Merge branch 'type-pattern-third' into patterns-record-deconstruction3
>  - Merge branch 'master' into type-patterns-third
>  - Using Type.isRaw instead of checking the AST structure.
>  - Exhaustiveness should accept supertypes of the specified type.
>  - Renaming the features from deconstruction pattern to record pattern.
>  - Fixing guards after record patterns.
>  - ... and 24 more: 
> https://git.openjdk.java.net/jdk/compare/742644e2...f49d3f0a

Yai ! champagne (at least virtual).

-

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

Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Jim Laskey]

On Wed, 25 May 2022 09:38:08 GMT, Claes Redestad  wrote:

> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` 
> allows keys to be compacted when all byte values of the key fit in 4 bits, 
> otherwise a byte array is allocated and used. This means that all transforms 
> with a kind value above 15 will be forced to allocate and use array 
> comparisons.
> 
> Removing unused and folding some transforms to ensure all existing kinds can 
> fit snugly within the 0-15 value range realize a minor improvement to 
> footprint, speed and allocation pressure of affected transforms, e.g. 
> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
> 
> Baseline:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 2048.475 ?  69.887   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3487.311 ?  80.385B/op
> 
> 
> Patched:
> 
> Benchmark  Mode  Cnt 
> Score Error   Units
> SCFB.makeConcatWithConstants   avgt   15  
> 1961.985 ? 101.519   ns/op
> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
> 3156.478 ? 183.600B/op

Marked as reviewed by jlaskey (Reviewer).

-

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

Integrated: 8262889: Compiler implementation for Record Patterns

[Jan Lahoda]

On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda  wrote:

> 8262889: Compiler implementation for Record Patterns
> 
> A first version of a patch that introduces record patterns into javac as a 
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
> 
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to 
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very 
> straightforward - effectivelly the record patterns are desugared into 
> guards/conditions. This will likely be improved in some future version/preview
> 
> `MatchException` has been extended to cover additional cases related to 
> record patterns.

This pull request has now been integrated.

Changeset: e9bddc18
Author:Jan Lahoda 
URL:   
https://git.openjdk.java.net/jdk/commit/e9bddc18ab91c29d491b0e3bd145d641f6a62c5d
Stats: 2255 lines in 50 files changed: 2169 ins; 15 del; 71 mod

8262889: Compiler implementation for Record Patterns

Co-authored-by: Brian Goetz 
Co-authored-by: Jan Lahoda 
Co-authored-by: Aggelos Biboudis 
Reviewed-by: mcimadamore, vromero

-

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

Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v12]

[iaroslavski]

On Mon, 14 Mar 2022 19:12:29 GMT, iaroslavski  wrote:

>> Sorting:
>> 
>> - adopt radix sort for sequential and parallel sorts on 
>> int/long/float/double arrays (almost random and length > 6K)
>> - fix tryMergeRuns() to better handle case when the last run is a single 
>> element
>> - minor javadoc and comment changes
>> 
>> Testing:
>> - add new data inputs in tests for sorting
>> - add min/max/infinity values to float/double testing
>> - add tests for radix sort
>
> iaroslavski has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
>   
>   * Improved mixed insertion sort
>   * Optimized insertion sort
>   * Improved merging sort
>   * Optimized soring tests

This change will not lead to more crashes.

-

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

Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance()

[Vladimir Kempik]

On Fri, 20 May 2022 08:34:58 GMT, Maxim Kartashev  
wrote:

> Following the logic from the comment directly above the changed line, since 
> it doesn't matter which controller we pick, pick any available controller 
> instead of the one called "memory" specifically. This way we are guarded 
> against getting `null` as `anyController`, which is being immediately passed 
> down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` 
> values. 
> 
> It is also worth noting that the previous checks (such as that at line 89) 
> make sure that there exist at least one controller in the map.

Can some reviewer take a look at this please ?
This seems to be addressing rare corner issue.

-

Marked as reviewed by vkempik (Committer).

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

Re: Stream.fromForEach(Consumer>)

[Peter Levart]

Hi,

On 23/05/2022 14:53, Brian Goetz wrote:

...

Are there any useful Consumer> in the wild, that are 
actually typed like that?  I doubt it (there are many things 
convertible to it, though.)  Which suggests it might be fruitful to 
define a FI:


    interface PushSource {
    void accept(Consumer> pusher);
    }

    static Stream fromPush(PushSource source) { ... }

and Iterable::forEachRemaining and Optional::ifPresent will convert to 
it.



...the PushSource would probably be just the following (a replacement 
for Consumer>):


    interface PushSource {
    void accept(Consumer consumer);
    }

The outer Consumer is not that problematic, IMO, since it is subject of 
conversion from any similar signature via lambdas/method references as 
easily as PushSource. The inner Consumer type is more problematic. For 
example, suppose one would have the following API:


    interface Listener {
    void listen(String token);
    }

    record Parser(String text) {
    void parse(Listener listener) {
    for (var token : text.split(" ")) {
    listener.listen(token);
    }
    }
    }


...to convert a Parser instance `parser` to a Stream of tokens, one 
would have to do something like this:


var parser = new Parser("1 2 3 4");

var stream = Stream.fromForEach(consumer -> parser.parse(consumer::accept));

...and then the type of stream would be inferred as Stream, not 
Stream as one would like. Type hinting would be necessary:


var stream = Stream.fromForEach(consumer -> 
parser.parse(consumer::accept));



I don't believe Java type system allows doing such things more elegantly.

One point to highlight is also that such method is only suitable for 
"finite" generators. A generator that emits infinite number of elements 
would have no way to stop emitting them although the resulting Stream 
was later shortened with .limit(max) or .takeWhile(predicate), ... Such 
Stream would never stop. This could be solved by a warning in the 
javadoc though. Or by something that project Loom could provide in the 
form of continuation?


As to the name, `generate` is taken by the generate(Supplier 
supplier) form, but overload is technically possible, since it differs 
by parameters of unrelated type and number of parameters of the 
@FunctionalInterface methods (Supplier vs. Consumer). No conflicts 
should be expected.



Regards, Peter

Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v12]

[quincunx-45]

On Mon, 14 Mar 2022 19:12:29 GMT, iaroslavski  wrote:

>> Sorting:
>> 
>> - adopt radix sort for sequential and parallel sorts on 
>> int/long/float/double arrays (almost random and length > 6K)
>> - fix tryMergeRuns() to better handle case when the last run is a single 
>> element
>> - minor javadoc and comment changes
>> 
>> Testing:
>> - add new data inputs in tests for sorting
>> - add min/max/infinity values to float/double testing
>> - add tests for radix sort
>
> iaroslavski has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
>   
>   * Improved mixed insertion sort
>   * Optimized insertion sort
>   * Improved merging sort
>   * Optimized soring tests

Will the VM still exit if 
-XX:+ExitOnOutOfMemoryError/-XX:+CrashOnOutOfMemoryError/-XX:OnOutOfMemoryError/...
 was specified? I.e. can this change lead to more (avoidable) crashes?

-

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

Re: RFR: 8202449: overflow handling in Random.doubles [v3]

[Raffaello Giulietti]

On Tue, 24 May 2022 12:58:45 GMT, Raffaello Giulietti  
wrote:

>> Extend the range of Random.doubles(double, double) and similar methods.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8202449: overflow handling in Random.doubles

`Random.doubles(double, double)` and similar methods depend on 
`RandomGenerator.nextDouble(double, double)`.

Currently, this method is specified to throw when the range [origin, bound) 
given by the arguments is so large that its size (bound - origin) overflows. 
Since the proposed implementation doesn't suffer from this limitation, the 
specification needs to be modified, thus the need for a CSR.

-

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

Re: RFR: 8287206: Use WrongThreadException for confinement errors

[Maurizio Cimadamore]

On Wed, 25 May 2022 10:16:54 GMT, Maurizio Cimadamore  
wrote:

> > Thinking more about it: `IllegalStateException` is fine for a closed 
> > `MemorySession`. If used by wrong thread the exception was indeed 
> > confusing. Now that the abiguity is gone, I can change our (Lucene) code 
> > and just transform every ISE to an already closed in our code. I just 
> > wanted to bring up that issue here.
> > PR is here: [apache/lucene#912](https://github.com/apache/lucene/pull/912)
> 
> I've been thinking something similar. I'd suggest to keep the API as is, and 
> maybe revise it at a later point if lack of a specific exception for the 
> "already closed" case proves to be too cumbersome to workaround.

Basically, with this patch you only get ISE if you are accessing _in a moment 
in time_ when you are not supposed to. Similarly, when calling `close`, you get 
ISE if you are closing a segment that is in a bad state (e.g. already closed, 
or temporarily locked by some native call). This feels consistent. The 
confinement exception was the confounding factor, I think, and a lot of checks 
against the exception message came from there.

-

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

Re: RFR: 8287206: Use WrongThreadException for confinement errors

[Maurizio Cimadamore]

On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore  
wrote:

> This patch tweaks the foreign API to use the newly added 
> `WrongThreadException` instead of `IllegalStateException` to report 
> confinement errors.

> Thinking more about it: `IllegalStateException` is fine for a closed 
> `MemorySession`. If used by wrong thread the exception was indeed confusing. 
> Now that the abiguity is gone, I can change our (Lucene) code and just 
> transform every ISE to an already closed in our code. I just wanted to bring 
> up that issue here.
> 
> PR is here: [apache/lucene#912](https://github.com/apache/lucene/pull/912)

I've been thinking something similar. I'd suggest to keep the API as is, and 
maybe revise it at a later point if lack of a specific exception for the 
"already closed" case proves to be too cumbersome to workaround.

-

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

Re: RFR: 8287206: Use WrongThreadException for confinement errors

[Uwe Schindler]

On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore  
wrote:

> This patch tweaks the foreign API to use the newly added 
> `WrongThreadException` instead of `IllegalStateException` to report 
> confinement errors.

Thinking more about it: `IllegalStateException` is fine for a closed 
`MemorySession`. If used by wrong thread the exception was indeed confusing. 
Now that the abiguity is gone, I can change our (Lucene) code and just 
transform every ISE to an already closed in our code. I just wanted to bring up 
that issue here.

PR is here: https://github.com/apache/lucene/pull/912

-

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

Re: RFR: 8287206: Use WrongThreadException for confinement errors

[Uwe Schindler]

On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore  
wrote:

> This patch tweaks the foreign API to use the newly added 
> `WrongThreadException` instead of `IllegalStateException` to report 
> confinement errors.

I have seen this PR now: Would it be also a good idea to replace the 
`IllegalStateException` for already closed `MemorySession` by some special 
exception? In Lucene we catch `IllegalStateException` in our IO layer and check 
if message contains "closed", example:
- 
https://github.com/apache/lucene/blob/d182134bf88a44bf76ebd1c1d40b225ecdca1f4b/lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java#L137
 (catch)
- 
https://github.com/apache/lucene/blob/d182134bf88a44bf76ebd1c1d40b225ecdca1f4b/lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java#L93-L103
 (that's the hack)

The check using `ex.getMessage().contains("closed")` feels bad, especially if 
it may be translated to a locale.

-

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

Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v12]

[Laurent Bourgès]

On Mon, 14 Mar 2022 19:12:29 GMT, iaroslavski  wrote:

>> Sorting:
>> 
>> - adopt radix sort for sequential and parallel sorts on 
>> int/long/float/double arrays (almost random and length > 6K)
>> - fix tryMergeRuns() to better handle case when the last run is a single 
>> element
>> - minor javadoc and comment changes
>> 
>> Testing:
>> - add new data inputs in tests for sorting
>> - add min/max/infinity values to float/double testing
>> - add tests for radix sort
>
> iaroslavski has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
>   
>   * Improved mixed insertion sort
>   * Optimized insertion sort
>   * Improved merging sort
>   * Optimized soring tests

Well explained, vladimir.

I propose to adopt max_heap / 128 min to stay less than 1% footprint.

Then factors are 7 for ints, 8 for long, min.

For 1gb heap, it gives 8mb, so 2m ints or 1m long.

Laurent

-

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

Integrated: 8287206: Use WrongThreadException for confinement errors

[Maurizio Cimadamore]

On Tue, 24 May 2022 09:26:44 GMT, Maurizio Cimadamore  
wrote:

> This patch tweaks the foreign API to use the newly added 
> `WrongThreadException` instead of `IllegalStateException` to report 
> confinement errors.

This pull request has now been integrated.

Changeset: e1f140d2
Author:Maurizio Cimadamore 
URL:   
https://git.openjdk.java.net/jdk/commit/e1f140d270cc666d26b888a0a25ca7b02e1239af
Stats: 254 lines in 12 files changed: 150 ins; 1 del; 103 mod

8287206: Use WrongThreadException for confinement errors

Reviewed-by: alanb, darcy, mchung

-

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

RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently

[Claes Redestad]

The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey` allows 
keys to be compacted when all byte values of the key fit in 4 bits, otherwise a 
byte array is allocated and used. This means that all transforms with a kind 
value above 15 will be forced to allocate and use array comparisons.

Removing unused and folding some transforms to ensure all existing kinds can 
fit snugly within the 0-15 value range realize a minor improvement to 
footprint, speed and allocation pressure of affected transforms, e.g. 
~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:

Baseline:

Benchmark  Mode  Cnt 
Score Error   Units
SCFB.makeConcatWithConstants   avgt   15  
2048.475 ?  69.887   ns/op
SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
3487.311 ?  80.385B/op


Patched:

Benchmark  Mode  Cnt 
Score Error   Units
SCFB.makeConcatWithConstants   avgt   15  
1961.985 ? 101.519   ns/op
SCFB.makeConcatWithConstants:?gc.alloc.rate.norm   avgt   15  
3156.478 ? 183.600B/op

-

Commit messages:
 - Revert unrelated and unverified hashCode change
 - Improve TransformKey to pack more kinds of transforms efficiently

Changes: https://git.openjdk.java.net/jdk/pull/8881/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8881=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287292
  Stats: 44 lines in 1 file changed: 21 ins; 10 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8881.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8881/head:pull/8881

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

Re: RFR: 8279986: methods Math::asXExact for safely checked primitive casts [v6]

[Raffaello Giulietti]

> Add a family of "safe" cast methods.

Raffaello Giulietti has updated the pull request incrementally with one 
additional commit since the last revision:

  8279986: methods Math::asXExact for safely checked primitive casts

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8548/files
  - new: https://git.openjdk.java.net/jdk/pull/8548/files/5fb1fed4..4cf9a8cf

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

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

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

Re: RFR: 8273346: Expand library mappings to IEEE 754 operations

[Raffaello Giulietti]

On Wed, 25 May 2022 00:19:41 GMT, Joe Darcy  wrote:

> Generally add apiNote's to map from Java library methods to particular IEEE 
> 754 operations. For now, I only added such notes to java.lang.Math and not 
> java.lang.StrictMath.

src/java.base/share/classes/java/lang/Math.java line 771:

> 769:  * This method corresponds to the convertToIntegerTiesToEven
> 770:  * operation defined in IEEE 754.
> 771:  *

IEEE `convertToIntegerTiesToEven` rounds ties to the even integer.
The method's spec, however, requires ties to round toward positive infinity.
Unfortunately, IEEE 754 doesn't offer a `convertToIntegerTiesToPositive`

src/java.base/share/classes/java/lang/Math.java line 824:

> 822:  * This method corresponds to the convertToIntegerTiesToEven
> 823:  * operation defined in IEEE 754.
> 824:  *

see comment for `round(float)`

src/java.base/share/classes/java/math/RoundingMode.java line 49:

> 47:  * exponent range of {@code BigDecimal}, the mathematical result will
> 48:  * be exactly representable in the result precision or fall between
> 49:  * two representable values. In the case of falling between two

Perhaps better would be
`two adjacent representable values.`

-

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

Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]

[Maurizio Cimadamore]

On Tue, 24 May 2022 15:15:43 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add statement to close about thread termination

Looks good!

-

Marked as reviewed by mcimadamore (Reviewer).

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

Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v12]

[iaroslavski]

On Mon, 14 Mar 2022 19:12:29 GMT, iaroslavski  wrote:

>> Sorting:
>> 
>> - adopt radix sort for sequential and parallel sorts on 
>> int/long/float/double arrays (almost random and length > 6K)
>> - fix tryMergeRuns() to better handle case when the last run is a single 
>> element
>> - minor javadoc and comment changes
>> 
>> Testing:
>> - add new data inputs in tests for sorting
>> - add min/max/infinity values to float/double testing
>> - add tests for radix sort
>
> iaroslavski has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort)
>   
>   * Improved mixed insertion sort
>   * Optimized insertion sort
>   * Improved merging sort
>   * Optimized soring tests

Hi Piotr,

I agree that catching out-of-memory-error is very rarely in prod.
I can say that LSD Radix sort works 3-5 times faster than Quicksort
(array of int 2M elements), merging sort works 10-30 (!) times faster
than Quicksort (structured array of int 2M elements), therefore it
is worth to use algorithms with additional buffers.

If we allocate buffer like 'new int[size]' and there is no free memory,
sorting fails with OOME. If we catch memory error, we switch to
in-place algorithms and sorting will be completed successfully.
I checked such use case, it works fine.

It doesn't matter if we run several threads or not. Some threads
may use additional buffers, some threads - not, but finally
all threads will be completed without errors.

The known problem with OOME is handling inside catch block.
If we try to create new object, try to log message with details,
we can face with new OOME. In DualPivotQuicksort we return null only,
no any actions, no new objects etc.

-

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v3]

[Jaikiran Pai]

On Tue, 24 May 2022 13:08:53 GMT, liach  wrote:

> so on a mismatch, it would print parameter type initialized after 
> instantiation expected: true actual: false

That looks fine then. I hadn't looked closely at what testng prints and I had 
thought that on an assertion failure it would print only `AssertionError: 
parameter type initialized after instantiation` which wouldn't be accurate. The 
one you note above is more clear.

-

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

Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v3]

[Jaikiran Pai]

On Tue, 24 May 2022 13:07:08 GMT, liach  wrote:

>> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 605:
>> 
>>> 603: mv.visitLdcInsn(Type.getObjectType(dotToSlash(className)));
>>> 604: mv.visitMethodInsn(INVOKEVIRTUAL, JL_CLASS,
>>> 605: "getClassLoader", "()" + LJL_CLASSLOADER, false);
>> 
>> Hello @liach, should this instead be using the (application supplied) 
>> `loader` returned by the call to `ProxyGenerator.getClassLoader()` or maybe 
>> the `loader` member in the `ProxyGenerator` itself?
>
> This is equivalent to `jdk.proxy5.$Proxy5.class.getClassLoader()` in Java 
> source code, so this is exactly the application-supplied loader, which also 
> uses the same loader as the previous behavior of `forName` calls.
> 
> If you want to pass the loader from `ProxyGenerator` to the proxy, it 
> requires complex tricks. Hidden classes won't work due to serialization 
> incompatibility; accessor methods would be defined in `jdk.internal` and 
> exported specifically to the proxy modules, but writing a class so each proxy 
> gets its loader while what I wrote can already do is overkill.

Thank you @liach for that detail.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v8]

[Yasumasa Suenaga]

On Wed, 25 May 2022 01:50:57 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge remote-tracking branch 'upstream/master' into gcc12-warnings
>>  - Use getter function to access "_data"
>>  - Revert changes for bytecodeAssembler.cpp, classFileParser.cpp, 
>> symbolTable.cpp
>>  - revert changes for memnode.cpp and type.cpp
>>  - Add assert to check the range of BasicType
>>  - Merge remote-tracking branch 'upstream/master' into HEAD
>>  - Revert change for java.c , parse_manifest.c , LinuxPackage.c
>>  - Calculate char offset before realloc()
>>  - Use return value from JLI_Snprintf
>>  - Avoid pragma error in before GCC 12
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/c156bcc5...042c1c70
>
> src/hotspot/share/oops/array.hpp line 102:
> 
>> 100:   // standard operations
>> 101:   int  length() const { return _length; }
>> 102:   T* data() const { return 
>> reinterpret_cast(reinterpret_cast(this) + 
>> base_offset_in_bytes()); }
> 
> Adding the const-qualifier to the `data()` function and then implicitly
> casting it away (by casting through intptr_t) is wrong.  Either don't
> const-qualify (and leave it to some future use that needs such to address
> appropriately), or have two functions.  Also, the line length is excessive.
> So this:
> 
> 
> T* data() {
>   return reinterpret_cast(
> reinterpret_cast(this) + base_offset_in_bytes());
> }
> 
> and optionally add this:
> 
> const T* data() const {
>   return reinterpret_cast(
> reinterpret_cast(this) + base_offset_in_bytes());
> }

Thanks a lot @kimbarrett !

I updated around stringop-overflow warning in jfrTraceIdBits.inline.hpp , and 
added two `data()` in `Array` class. They works fine on my GCC 12 on Fedora 36. 
Could you review again?

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v9]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with two additional 
commits since the last revision:

 - Change Array::data() implementation
 - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/042c1c70..17cda224

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

  Stats: 39 lines in 4 files changed: 18 ins; 18 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

[Severin Gehwolf]

On Tue, 24 May 2022 19:49:35 GMT, Ioi Lam  wrote:

>> My bad. How about `Intentional incomplete switch. There are ...`? Anyway, 
>> why is the empty `default` case needed other than for the comment?
>
> To me, the `default:` switch is a clear indication that "everything else 
> comes here". So you won't be confused whether the comment is related to the 
> last line just above the comment.

It confused me, fwiw. Anyway up to you. It's not super important.

-

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

RFR: 8287285: Avoid redundant HashMap.containsKey call in java.util.zip.ZipFile.Source.get

[Andrey Turbanov]

The method `java.util.zip.ZipFile.Source#get` could be improved by usage of 
`Map.putIfAbsent` instead of separate `containsKey`/`get`/`put` calls. We known 
that HashMap `java.util.zip.ZipFile.Source#files` can contain only non-null 
values. And to check if putIfAbsent was successful or not we can just compare 
result with `null`.
It makes code a bit cleaner and faster.

-

Commit messages:
 - [PATCH] Avoid redundant Map.containsKey in ZipFile

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

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

Integrated: 8287181: Avoid redundant HashMap.containsKey calls in InternalLocaleBuilder.setExtension

[Andrey Turbanov]

On Sat, 30 Apr 2022 17:10:55 GMT, Andrey Turbanov  wrote:

> No need to separately perform `HashMap.containsKey` before `HashMap.remove` 
> call. If key is present - it will be removed anyway. If it's not present, 
> nothing will be deleted.

This pull request has now been integrated.

Changeset: 65850431
Author:Andrey Turbanov 
URL:   
https://git.openjdk.java.net/jdk/commit/65850431edd321c4cf49875f756ae28449c9f710
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8287181: Avoid redundant HashMap.containsKey calls in 
InternalLocaleBuilder.setExtension

Reviewed-by: naoto, rriggs

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v9]

[Jatin Bhateja]

On Wed, 25 May 2022 05:50:23 GMT, Jatin Bhateja  wrote:

>> Hi All,
>> 
>> Patch adds the planned support for new vector operations and APIs targeted 
>> for [JEP 426: Vector API (Fourth 
>> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173)
>> 
>> Following is the brief summary of changes:-
>> 
>> 1)  Extends the scope of existing lanewise API for following new vector 
>> operations.
>>-  VectorOperations.BIT_COUNT: counts the number of one-bits
>>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero 
>> bits
>>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing 
>> zero bits
>>- VectorOperations.REVERSE: reversing the order of bits
>>- VectorOperations.REVERSE_BYTES: reversing the order of bytes
>>- compress and expand bits: Semantics are based on Hacker's Delight 
>> section 7-4 Compress, or Generalized Extract.
>> 
>> 2)  Adds following new APIs to perform cross lane vector compress and 
>> expansion operations under the influence of a mask.
>>- Vector.compress
>>- Vector.expand 
>>- VectorMask.compress
>> 
>> 3) Adds predicated and non-predicated versions of following new APIs to load 
>> and store the contents of vector from foreign MemorySegments. 
>>   - Vector.fromMemorySegment
>>   - Vector.intoMemorySegment
>> 
>> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support 
>> for each newly added operation.
>> 
>> 
>>  Patch has been regressed over AARCH64 and X86 targets different AVX levels. 
>> 
>>  Kindly review and share your feedback.
>> 
>>  Best Regards,
>>  Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8284960: Review comments resolved.

Hi @vnkozlov , Your comments have been addressed.

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v8]

[Jatin Bhateja]

On Mon, 23 May 2022 22:17:40 GMT, Vladimir Kozlov  wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8284960: Integrating incremental patches.
>
> src/hotspot/cpu/x86/assembler_x86.cpp line 8173:
> 
>> 8171: 
>> 8172: void Assembler::vinsertf32x4(XMMRegister dst, XMMRegister nds, 
>> XMMRegister src, uint8_t imm8) {
>> 8173:   assert(VM_Version::supports_evex(), "");
> 
> Hmm, did we never trigger this wrong assert because the use was guarded by 
> correct check?

Yes.

-

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

Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v10]

[Jatin Bhateja]

> Hi All,
> 
> Patch adds the planned support for new vector operations and APIs targeted 
> for [JEP 426: Vector API (Fourth 
> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173)
> 
> Following is the brief summary of changes:-
> 
> 1)  Extends the scope of existing lanewise API for following new vector 
> operations.
>-  VectorOperations.BIT_COUNT: counts the number of one-bits
>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero 
> bits
>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing 
> zero bits
>- VectorOperations.REVERSE: reversing the order of bits
>- VectorOperations.REVERSE_BYTES: reversing the order of bytes
>- compress and expand bits: Semantics are based on Hacker's Delight 
> section 7-4 Compress, or Generalized Extract.
> 
> 2)  Adds following new APIs to perform cross lane vector compress and 
> expansion operations under the influence of a mask.
>- Vector.compress
>- Vector.expand 
>- VectorMask.compress
> 
> 3) Adds predicated and non-predicated versions of following new APIs to load 
> and store the contents of vector from foreign MemorySegments. 
>   - Vector.fromMemorySegment
>   - Vector.intoMemorySegment
> 
> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support 
> for each newly added operation.
> 
> 
>  Patch has been regressed over AARCH64 and X86 targets different AVX levels. 
> 
>  Kindly review and share your feedback.
> 
>  Best Regards,
>  Jatin

Jatin Bhateja has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 20 commits:

 - 8284960: Post merge cleanups.
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - 8284960: Review comments resolved.
 - 8284960: Integrating incremental patches.
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - 8284960: Changes to enable jdk.incubator.vector to be treated as preview 
participant. Code re-organization related to Reverse/ReverseByte IR transforms.
 - 8284960: Adding --enable-preview in vectorAPI benchmarks.
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - 8284960: Review comments resolution.
 - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960
 - ... and 10 more: https://git.openjdk.java.net/jdk/compare/742644e2...0f6e1584

-

Changes: https://git.openjdk.java.net/jdk/pull/8425/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8425=09
  Stats: 38021 lines in 228 files changed: 16652 ins; 16924 del; 4445 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8425.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8425/head:pull/8425

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