Re: RFR: 8287522: StringConcatFactory: Add in prependers and mixers in batches [v7]

[Remi Forax]

- Original Message -
> From: "Claes Redestad" 
> To: "core-libs-dev" 
> Sent: Friday, June 3, 2022 2:19:34 PM
> Subject: Re: RFR: 8287522: StringConcatFactory: Add in prependers and mixers 
> in batches [v7]

> On Fri, 3 Jun 2022 11:19:04 GMT, ExE Boss  wrote:
> 
>>> You would think that, but javac doesn't do anything fancy once you store to 
>>> a
>>> local
>>> 
>>> javap output for lines 108 through 111:
>>> 
>>>449: ldc   #148// float 0.1f
>>>451: fstore31
>>>453: fload 31
>>>455: invokedynamic #149,  0// InvokeDynamic
>>>#4:makeConcatWithConstants:(F)Ljava/lang/String;
>>>460: astore32
>>>462: fload 31
>>>464: aload 4
>>>466: invokedynamic #152,  0// InvokeDynamic
>>>#7:makeConcatWithConstants:(FLjava/lang/String;)Ljava/lang/String;
>>>471: astore33
>>>473: aload 4
>>>475: fload 31
>>>477: invokedynamic #155,  0// InvokeDynamic
>>>#10:makeConcatWithConstants:(Ljava/lang/String;F)Ljava/lang/String;
>>
>> I guess it only happens for `final` locals.
> 
> Yes, looks like javac does constant fold when the local is explicitly final:
> 
>   final float f = 0.1f;
>   System.out.println("const folding? " + f);
> 
> javap:
> 
> 0: getstatic #2  // Field
> java/lang/System.out:Ljava/io/PrintStream;
> 3: ldc   #3  // String const folding? 0.1
> 5: invokevirtual #4  // Method
> java/io/PrintStream.println:(Ljava/lang/String;)V
> 
> 
> Maybe this constant folding is something javac could be enhanced to do on
> effectively final locals, though I think we can defer adjusting
> `HelloClasslist` to break that optimization when that time comes.

The JLS defines final variables (and effectively final) here 
https://docs.oracle.com/javase/specs/jls/se18/html/jls-4.html#jls-4.12.4

Constant expressions does not list effectively final variables as constant, 
only final variables
https://docs.oracle.com/javase/specs/jls/se18/html/jls-15.html#jls-15.29

> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/8855

regards,
Rémi

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

[Rémi Forax]

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/74cd2687...05cf2d8b

src/java.base/share/classes/java/lang/reflect/Member.java line 96:

> 94:  */
> 95: public default Set accessFlags() {
> 96: return Set.of();

Is is not better to throw a NoSuchMethodError instead of Set.of() if there is 
no implementation.

-

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

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

[Rémi Forax]

On Tue, 15 Feb 2022 21:05:08 GMT, Joe Darcy  wrote:

>> Note that the presence or absence of `ACC_SUPER` has no effect since **Java 
>> 8**, which always treats it as set regardless of the actual contents of the 
>> binary class file.
>
> For completeness, I think including SUPER is reasonable, even though has been 
> a no-op for some time. (Some time in the future, there could be a class file 
> version aware additions to this enum class.) Well spotted omission.

`ACC_SUPER`may be reconed as `ACC_IDENTITY`by Valhalla, so i'm not sure it's a 
good idea to add ACC_SUPER.

-

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

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

[Rémi Forax]

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/1545e825...05cf2d8b

src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 130:

> 128: MANDATED(AccessFlag.MANDATED.mask());
> 129: 
> 130: private int mask;

it should be declared final ?

src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 134:

> 132: this.mask = mask;
> 133: }
> 134: private int mask() {return mask;}

seem useless, `mask` can be accessed directly

-

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

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: Stream.fromForEach(Consumer>)

[forax]

- Original Message -
> From: "Tagir Valeev" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" 
> Sent: Monday, May 23, 2022 8:01:44 AM
> Subject: Re:  Stream.fromForEach(Consumer>)

> Hello!
> 
> There's a Stream.builder for this purpose:
> 
> var builder = Stream.builder();
> new Parser("1 2 3").parse(builder);
> builder.build().forEach(System.out::println);
> 
> A little bit more verbose than your suggestion but this way it's more
> clear that the whole stream content will be buffered.

I should have mentioned the Stream.builder, as you said, the Stream.Builder 
stores the whole elements in memory before creating the stream,
it's equivalent to storing the elements in a List and calling List.stream.

Stream.fromForEach like stream.mapMulti() does not stores the elements.
(technically, stream.mapMulti() stores the elements if the default method 
implementation is used but this implementation is only used if you write your 
own Stream, the JDK implementation does not store the elements)

> 
> With best regards,
> Tagir Valeev

regards,
Rémi

> 
> On Sun, May 22, 2022 at 5:54 AM Remi Forax  wrote:
>>
>> Hi all,
>> a stream is kind of push iterator so it can be created from any object that 
>> has
>> a method like forEach(Consumer),
>> but sadly there is no static method to create a Stream from a Consumer of
>> Consumer so people usually miss that creating a Stream from events pushed to 
>> a
>> consumer is easy.
>>
>> By example, let say i've a very simple parser like this, it calls the
>> listener/callback for each tokens
>>
>>   record Parser(String text) {
>> void parse(Consumer listener) {
>>   for(var token: text.split(" ")) {
>>  listener.accept(token);
>>   }
>> }
>>   }
>>
>> Using the method Stream.fromForEach, we can create a Stream from the 
>> sequence of
>> tokens that are pushed through the listener
>>   var parser = new Parser("1 2");
>>   var stream = Stream.fromForEach(parser::parse);
>>
>> It can also works with an iterable, an optional or even a collection
>>   Iterable iterable = ...
>>   var stream = Stream.fromForEach(iterable::forEachRemaning);
>>
>>   Optional optional = ...
>>   var stream = Stream.fromForEach(optional::ifPresent);
>>
>>   List list = ...
>>   var stream = Stream.fromForEach(list::forEach);
>>
>> I known the last two examples already have their own method stream(), it's 
>> just
>> to explain how Stream.fromForEach is supposed to work.
>>
>> In term of implementation, Stream.fromForEach() is equivalent to creating a
>> stream using a mapMulti(), so it can be implemented like this
>>
>>   static  Stream fromForEach(Consumer> forEach) {
>> return Stream.of((T) null).mapMulti((__, consumer) -> 
>> forEach.accept(consumer));
>>   }
>>
>> but i think that Stream.fromForEach(iterable::forEachRemaning) is more 
>> readable
>> than Stream.of(iterable).mapMult(Iterable::forEachRemaining).
>>
>> The name fromForEach is not great and i'm sure someone will come with a 
>> better
>> one.
>>
>> regards,
> > Rémi

Stream.fromForEach(Consumer>)

[Remi Forax]

Hi all,
a stream is kind of push iterator so it can be created from any object that has 
a method like forEach(Consumer),
but sadly there is no static method to create a Stream from a Consumer of 
Consumer so people usually miss that creating a Stream from events pushed to a 
consumer is easy.

By example, let say i've a very simple parser like this, it calls the 
listener/callback for each tokens 

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

Using the method Stream.fromForEach, we can create a Stream from the sequence 
of tokens that are pushed through the listener
  var parser = new Parser("1 2");
  var stream = Stream.fromForEach(parser::parse);

It can also works with an iterable, an optional or even a collection
  Iterable iterable = ...
  var stream = Stream.fromForEach(iterable::forEachRemaning);

  Optional optional = ...
  var stream = Stream.fromForEach(optional::ifPresent);

  List list = ...
  var stream = Stream.fromForEach(list::forEach);

I known the last two examples already have their own method stream(), it's just 
to explain how Stream.fromForEach is supposed to work.

In term of implementation, Stream.fromForEach() is equivalent to creating a 
stream using a mapMulti(), so it can be implemented like this

  static  Stream fromForEach(Consumer> forEach) {
return Stream.of((T) null).mapMulti((__, consumer) -> 
forEach.accept(consumer));
  }

but i think that Stream.fromForEach(iterable::forEachRemaning) is more readable 
than Stream.of(iterable).mapMult(Iterable::forEachRemaining).

The name fromForEach is not great and i'm sure someone will come with a better 
one.

regards,
Rémi

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]

[Rémi Forax]

On Wed, 18 May 2022 11:27:24 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR brings over commits from the panama-foreign repo. These commits 
>> mostly pertain to the switch to ASM and away from MethodHandle combinators 
>> for the binding recipe specialization. But, there is one more commit which 
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>> 
>> Thanks,
>> Jorn
>> 
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on 
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 118 commits:
> 
>  - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into 
> JEP-19-ASM
>  - Merge branch 'pr/7959' into JEP-19-ASM
>  - Merge branch 'master' into JEP-19-VM-IMPL2
>  - ifdef NOT_PRODUCT -> ifndef PRODUCT
>  - Missing ASSERT -> NOT_PRODUCT
>  - Cleanup UL usage
>  - Fix failure with SPEC disabled (accidentally dropped change)
>  - indentation
>  - fix space
>  - Merge branch 'master' into JEP-19-ASM
>  - ... and 108 more: 
> https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a

LGTM !

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Rémi Forax]

On Tue, 17 May 2022 11:57:01 GMT, Jorn Vernee  wrote:

>> src/java.base/share/classes/jdk/internal/foreign/abi/SoftReferenceCache.java 
>> line 42:
>> 
>>> 40: 
>>> 41: private class Node {
>>> 42: private SoftReference ref;
>> 
>> this code looks like a double check locking so ref should be volatile
>
> Thanks. I thought the `moniterenter` & `monitorexit` were enough here for 
> visibility. I've read up on this and it looks like `volatile` is needed to 
> correctly order the constructor and apply call (and contents) before the 
> write to the `ref` field.

yes, otherwise another thread than the one inside the 
`monitorenter`/`monitorexit` that is initializing the object can see the object 
half initialized.

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Rémi Forax]

On Tue, 17 May 2022 08:16:32 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java 
>> line 98:
>> 
>>> 96: private static final String CLASS_DATA_DESC = 
>>> methodType(Object.class, MethodHandles.Lookup.class, String.class, 
>>> Class.class).descriptorString();
>>> 97: private static final String RELEASE0_DESC = 
>>> methodType(void.class).descriptorString();
>>> 98: private static final String ACQUIRE0_DESC = 
>>> methodType(void.class).descriptorString();
>> 
>> calling methodType() is quite slow because of the cache of MethodType so 
>> maybe those initialization should be done explicitly in a static block to 
>> avoid to recompute methodType(long).descriptorString() several times
>
> What about using MethodTypeDesc/ClassDesc as building block?

yes, it should be less expensive, the ClassDesc still need to be constructed 
from Class to allow refactoring.

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Rémi Forax]

On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR brings over commits from the panama-foreign repo. These commits 
>> mostly pertain to the switch to ASM and away from MethodHandle combinators 
>> for the binding recipe specialization. But, there is one more commit which 
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>> 
>> Thanks,
>> Jorn
>> 
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on 
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   BootstrapMethodError -> ExceptionInInitializerError

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java 
line 943:

> 941: Z, B, S, C, I, J, F, D, L;
> 942: 
> 943: static BasicType of(Class cls) {

This seems a duplication of ASM Type class for no good reason, 
Type.getOpcode(ILOAD) or Type.getOpcode(IRETURN) gives you the proper variant 
opcode

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Rémi Forax]

On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR brings over commits from the panama-foreign repo. These commits 
>> mostly pertain to the switch to ASM and away from MethodHandle combinators 
>> for the binding recipe specialization. But, there is one more commit which 
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>> 
>> Thanks,
>> Jorn
>> 
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on 
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   BootstrapMethodError -> ExceptionInInitializerError

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java 
line 967:

> 965: 
> 966: // unaligned constants
> 967: public final static ValueLayout.OfBoolean JAVA_BOOLEAN_UNALIGNED = 
> JAVA_BOOLEAN;

as far as i understand, those constants are accessed from the bytecode, they 
should be in their own class to cleanly separate the specializer part and the 
runtime part.

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Rémi Forax]

On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR brings over commits from the panama-foreign repo. These commits 
>> mostly pertain to the switch to ASM and away from MethodHandle combinators 
>> for the binding recipe specialization. But, there is one more commit which 
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>> 
>> Thanks,
>> Jorn
>> 
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on 
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   BootstrapMethodError -> ExceptionInInitializerError

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java 
line 157:

> 155: }
> 156: 
> 157: static MethodHandle doSpecialize(MethodHandle leafHandle, 
> CallingSequence callingSequence, ABIDescriptor abi) {

I think thise method should be split in to, one version for the upcall, one for 
the downcall, i do not see the point of merging the two codes.

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java 
line 816:

> 814: return;
> 815: }
> 816: if (con instanceof Integer) {

those can use the instanceof + type pattern

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Rémi Forax]

On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee  wrote:

>> Hi,
>> 
>> This PR brings over commits from the panama-foreign repo. These commits 
>> mostly pertain to the switch to ASM and away from MethodHandle combinators 
>> for the binding recipe specialization. But, there is one more commit which 
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>> 
>> Thanks,
>> Jorn
>> 
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on 
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   BootstrapMethodError -> ExceptionInInitializerError

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java 
line 98:

> 96: private static final String CLASS_DATA_DESC = 
> methodType(Object.class, MethodHandles.Lookup.class, String.class, 
> Class.class).descriptorString();
> 97: private static final String RELEASE0_DESC = 
> methodType(void.class).descriptorString();
> 98: private static final String ACQUIRE0_DESC = 
> methodType(void.class).descriptorString();

calling methodType() is quite slow because of the cache of MethodType so maybe 
those initialization should be done explicitly in a static block to avoid to 
recompute methodType(long).descriptorString() several times

src/java.base/share/classes/jdk/internal/foreign/abi/SoftReferenceCache.java 
line 42:

> 40: 
> 41: private class Node {
> 42: private SoftReference ref;

this code looks like a double check locking so ref should be volatile

-

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

Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline

[Remi Forax]

Ok,
let us know when it's out of draft,
i've several minor modifications about the code.

regards,
Rémi

- Original Message -
> From: "Jorn Vernee" 
> To: "core-libs-dev" , "hotspot-dev" 
> 
> Sent: Thursday, May 12, 2022 11:25:52 PM
> Subject: Re: RFR: 8286669: Replace MethodHandle specialization with ASM in 
> mainline

> On Thu, 12 May 2022 17:17:37 GMT, Jorn Vernee  wrote:
> 
>> Hi,
>> 
>> This PR brings over commits from the panama-foreign repo. These commits 
>> mostly
>> pertain to the switch to ASM and away from MethodHandle combinators for the
>> binding recipe specialization. But, there is one more commit which adds 
>> freeing
>> of downcall stubs, since those changes were mostly Java as well.
>> 
>> Thanks,
>> Jorn
> 
> Hmm, looks like there's an unexpected a tier1 failure on SysV in the GHA (I 
> only
> tested Windows locally, since these changes have been live in the panama repo
> for a while now...). Will convert back to draft and see what's up.
> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/8685

Re: HttpClient has no explicit way of releasing threads

[Remi Forax]

- Original Message -
> From: "Rafael Winterhalter" 
> To: "core-libs-dev" 
> Sent: Monday, May 9, 2022 11:43:49 PM
> Subject: HttpClient has no explicit way of releasing threads

> Hello,

Hello,

> 
> looking at thread dumps, I realized that the HttpClient implementation does
> not offer an explicit way to release its threads. Currently, the client:
> 
> 1. maintains a cached thread pool with a retention size of 60 seconds. If
> many such clients are created for short lived application, these threads
> pile up.
> 2. has a selector thread that only shuts down if the outer "facade"
> reference is collected via a weak reference. If an application is not
> running GC, this can take a while.
> 
> This is not a big problem but I have seen peaks with suddenly many, many
> threads (in test code) where many HttpClients were created for single use
> and I was wondering if it was ever considered to add a method for disposing
> the threads explicitly?

You can change the Executor (it's one parameter of the builder) to use whatever 
executors you want so you can shutdown that executor as you want.
This should fixed (1).

Also once you update to Java 19/21, it seems a good scenario to test the 
executor that spawn virtual threads instead of platform threads.

> 
> Alternatively, it might be an option to add a method like
> HttpClient.shared() which would return a singleton HttpClient (created on
> the first call, collected if no reference is kept anymore but reused in the
> meantime) to address such scenarios. I can of course add a singleton in my
> test project but I find this a bit awkward as it is something to remember
> and to repeat in all applications we maintain. Therefore, it might be
> convenient to add such methods for tests that usually aim to be decoupled.
> 
> Thanks for your consideration,
> best regards, Rafael

regards,
Rémi

Re: RFR: 8277090 : jsr166 refresh for jdk19 [v2]

[Rémi Forax]

On Mon, 2 May 2022 23:54:33 GMT, Doug Lea  wrote:

>> src/java.base/share/classes/java/util/concurrent/FutureTask.java line 222:
>> 
>>> 220: throw new IllegalStateException("Task has not 
>>> completed");
>>> 221: }
>>> 222: }
>> 
>> I think the code will be more readable if a switch expression and the arrow 
>> syntax are used
>> 
>> 
>> return switch(state()) {
>> case SUCCESS -> {
>> @SuppressWarnings("unchecked")
>> V result = (V) outcome;
>> yield result;
>> } 
>> case FAILED -> throw new IllegalStateException("Task completed with 
>> exception");
>> ...
>> };
>
> Sorry, I don't understand how it would be more readable. I like new switch 
> features because they extend the range of applicability of switch. But this 
> (and the two others) are just plain vanilla enum-based switches that were 
> handled well, and seem to have the same numbers of lines and structure either 
> way?

It's more readable because it's now clear that the aim is to return the value 
of the switch.
Also a switch expression has to be exhaustive, so there is no need for a 
'default'.

-

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

Re: RFR: 8277090 : jsr166 refresh for jdk19

[Rémi Forax]

On Mon, 2 May 2022 10:23:01 GMT, Doug Lea  wrote:

>> src/java.base/share/classes/java/util/concurrent/ExecutorService.java line 
>> 138:
>> 
>>> 136:  * @author Doug Lea
>>> 137:  */
>>> 138: public interface ExecutorService extends Executor, AutoCloseable {
>> 
>> The class documentation should be expanded to explain that an 
>> ExecutorService can be used with a try-with-resources
>
> Most AutoCloseables do not mention this in class-level javadocs. Is there a 
> reason you think this one should?

close() is now equivalent to the method shutdownAndAwaitTermination() shown in 
the javadoc so i believe , replacing it with a try-with-resources is better in 
term of documenting how an executor service can be used

-

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

Re: RFR: 8277090 : jsr166 refresh for jdk19

[Rémi Forax]

On Sun, 1 May 2022 10:53:55 GMT, Doug Lea  wrote:

> This is the jsr166 refresh for jdk19. See 
> https://bugs.openjdk.java.net/browse/JDK-8285450 and 
> https://bugs.openjdk.java.net/browse/JDK-8277090

src/java.base/share/classes/java/util/concurrent/ExecutorService.java line 138:

> 136:  * @author Doug Lea
> 137:  */
> 138: public interface ExecutorService extends Executor, AutoCloseable {

The class documentation should be expanded to explain that an ExecutorService 
can be used with a try-with-resources

src/java.base/share/classes/java/util/concurrent/FutureTask.java line 222:

> 220: throw new IllegalStateException("Task has not 
> completed");
> 221: }
> 222: }

I think the code will be more readable if a switch expression and the arrow 
syntax are used


return switch(state()) {
case SUCCESS -> {
@SuppressWarnings("unchecked")
V result = (V) outcome;
yield result;
} 
case FAILED -> throw new IllegalStateException("Task completed with 
exception");
...
};

src/java.base/share/classes/java/util/concurrent/FutureTask.java line 237:

> 235: throw new IllegalStateException("Task has not 
> completed");
> 236: }
> 237: }

Same here !

src/java.base/share/classes/java/util/concurrent/FutureTask.java line 247:

> 245: s = state;
> 246: }
> 247: switch (s) {

same here !

-

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

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[Remi Forax]

- Original Message -
> From: "liach" 
> To: "core-libs-dev" , "security-dev" 
> 
> Sent: Tuesday, April 19, 2022 3:31:24 AM
> Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

> On Mon, 18 Apr 2022 20:42:48 GMT, Remi Forax  wrote:
> 
>> The third parameter of defineProxy() is a lambda which is called for each 
>> method
>> that can be overriden, either toString/equals/hashCode but also any default
>> methods,
> if the lambda return true, the method is overriden, otherwise the default
> implementation is used.
> 
> Not quite, I mean calling default implementations in the super interface,
> consider:
> 
> interface Root { void cleanUp(); }
> interface FeatureOne extends Root { default void cleanUp() { /* do something 
> */
> } }
> interface FeatureTwo extends Root { default void cleanUp() { /* do something 
> */
> } }
> 
> My proxy implements both feature one and two, but in your API, there is no way
> for my `cleanUp` to call both `FeatureOne.super.cleanUp();` and
> `FeatureTwo.super.cleanUp();`. You should probably expose the lookup in your
> linker too, especially that you enabled nest access and you can just use that
> lookup to resolve, say, an implementation static method in the proxy user
> class.

yes, you are right, i should send the Lookup too.

> 
> Also the "delegate" in your API would significantly benefit from
> https://bugs.openjdk.java.net/browse/JDK-8282798 (#7744), too.

I don't think i need the carrier API, the idea is to have only one field in the 
proxy, this field can be a value type (exactly a primitive class) in the 
future, so being expanded into multiple fields by the VM at runtime.

> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/8278

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[Remi Forax]

- Original Message -
> From: "liach" 
> To: "core-libs-dev" , "security-dev" 
> 
> Sent: Monday, April 18, 2022 10:01:39 PM
> Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

> On Sun, 17 Apr 2022 16:17:30 GMT, liach  wrote:
> 
>> Convert dynamic proxies to hidden classes. Modifies the serialization of 
>> proxies
>> (requires change in "Java Object Serialization Specification"). Makes the
>> proxies hidden in stack traces. Removes duplicate logic in proxy building.
>> 
>> The main compatibility changes and their rationales are:
>> 1. Modification to the serialization specification: In the "An instance of 
>> the
>> class is allocated... The contents restored appropriately" section, I propose
>> explicitly state that handling of proxies are unspecified as to allow
>> implementation freedom, though I've seen deliberate attempts for proxies to
>> implement interfaces with `readResolve` in order to control their 
>> serialization
>> behavior.
>>- This is for the existing generated constructor accessor is 
>> bytecode-based,
>>which cannot refer to hidden classes.
>>- An alternative is to preserve the behavior, where the serialization
>>constructor calls `invokespecial` on the closest serializable superclass'
>>no-arg constructor, like in #1830 by @DasBrain.
>>- My rationale against preservation is such super calls are unsafe and 
>> should be
>>discouraged in the long term. Calling the existing constructor with a 
>> dummy
>>argument, as in my implementation, would be more safe.
>> 2. The disappearance of proxies in stack traces.
>>- Same behavior exists in lambda expressions: For instance, in 
>> `((Runnable) ()
>>-> { throw new Error(); }).run();`, the `run` method, implemented by the
>>lambda, will not appear in the stack trace, and isn't too problematic.
>> 
>> A summary of the rest of the changes:
>> 1. Merged the two passes of determining module and package of the proxy into
>> one. This reduced a lot of code and allowed anchor class (for hidden class
>> creation) selection be done together as well.
>> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
>> `java.base`. This API is intended for implementation of legacy (pre
>> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
>> complex tricks to obtain proper permissions as lookups.
>> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
>> passes methods computed by proxy generator as class data to the hidden proxy
>> class to reduce generated proxy class size and improve performance.
>> 
>> In addition, since this change is somewhat large, should we keep the old 
>> proxy
>> generator as well and have it toggled through a command-line flag (like the 
>> old
>> v49 proxy generator or the old reflection implementation)?
>> 
>> Please feel free to comment or review. This change definitely requires a CSR,
>> but I have yet to determine what specifications should be changed.
> 
> I strongly recommend a new API over revamping `Proxy` itself.
> https://github.com/forax/hidden_proxy would be a good prototype that serves
> most needs of the current Proxy API (except a few, like invoking default
> superinterface method). 

The third parameter of defineProxy() is a lambda which is called for each 
method that can be overriden, either toString/equals/hashCode but also any 
default methods,
if the lambda return true, the method is overriden, otherwise the default 
implementation is used.

> With hidden classes, I don't see much value in leaving
> the new API's produced instance in separate modules; what we have for hidden
> classes should be fine for the most part.
> 
> Imo the main obstacle is still the handling of serialization. The annotations
> are serializable, but currently hidden classes do not work with serialization
> at all and must use `writeReplace` and `readResolve`. And how to migrate
> annotations off proxies without breaking serialization is still a question as
> well. Maybe we can upgrade invocation handlers to allow them to declare custom
> `readResolve` logic for the proxy to facilitate migration away. How the new 
> API
> will treat serialization is still undetermined.

yes, i suppose that like with lambdas, we need a special overload of 
defineProxy that automatically implements writeReplace() and use a specific 
class SerializableProxy (mirroring how SerializableLambda works).

> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/8278

Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

[Remi Forax]

- Original Message -
> From: "Johannes Kuhn" 
> To: "Alan Bateman" , "core-libs-dev" 
> 
> Sent: Monday, April 18, 2022 12:53:45 PM
> Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

> On 18-Apr-22 9:36, Alan Bateman wrote:
>> On 17/04/2022 17:24, liach wrote:
>>> Convert dynamic proxies to hidden classes. Modifies the serialization
>>> of proxies (requires change in "Java Object Serialization
>>> Specification"). Makes the proxies hidden in stack traces. Removes
>>> duplicate logic in proxy building.
>>>
>>> The main compatibility changes and their rationales are:
>>> 1. Modification to the serialization specification: In the "An
>>> instance of the class is allocated... The contents restored
>>> appropriately" section, I propose explicitly state that handling of
>>> proxies are unspecified as to allow implementation freedom, though
>>> I've seen deliberate attempts for proxies to implement interfaces with
>>> `readResolve` in order to control their serialization behavior.
>>>     - This is for the existing generated constructor accessor is
>>> bytecode-based, which cannot refer to hidden classes.
>>>     - An alternative is to preserve the behavior, where the
>>> serialization constructor calls `invokespecial` on the closest
>>> serializable superclass' no-arg constructor, like in #1830 by @DasBrain.
>>>     - My rationale against preservation is such super calls are unsafe
>>> and should be discouraged in the long term. Calling the existing
>>> constructor with a dummy argument, as in my implementation, would be
>>> more safe.
>>> 2. The disappearance of proxies in stack traces.
>>>     - Same behavior exists in lambda expressions: For instance, in
>>> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
>>> implemented by the lambda, will not appear in the stack trace, and
>>> isn't too problematic.
>> 
>> It's great that you have time to experiment in this area but just to set
>> expectations that it will likely require significant discussion and
>> maybe prototyping of alternatives. It suspect the Reviewer effort will
>> end up being many times the effort required to do the actual work
>> because of the compatibility and security issues that will need to be
>> worked through.
>> 
>> I think you need to add non-discoverability to the list of compatibility
>> issues. Do we know for sure that there aren't frameworks and libraries
>> that use Class.forName on proxy classes? We've had issues in the past
>> with changes in this area.
>> 
>> It's too early to say but it might be that the compatibility risks may
>> nudge this one into creating a new API.
>> 
>> -Alan.
>> 
>> 
>> 
> 
> Proxies will have to be rethought at some future point - wrt Valhalla.
> 
> The current specification says:
> 
> > A proxy class implements exactly the interfaces specified at its
> creation, in the same order. Invoking getInterfaces on its Class object
> will return an array containing the same list of interfaces (in the
> order specified at its creation), [...]
> 
> In the current design Valhalla will add two interfaces - IdentityObject
> and ValueObject (afaik). One of them have to be implemented as well.
> Also, because the superclass is java.lang.reflect.Proxy, and that class
> has a protected final field, it can currently not implement ValueObject.

Recently, we (the Valhalla EG) have decided that IdentityObject/ValueObject 
were not the right way to represent identity and value class.
So no problem anymore on that side.

> 
> An other thing are annotations - currently they are implemented using
> Proxies. This implementation detail surfaces when serializing an
> annotation. Other implementation strategies are possible - for example
> spinning a record at runtime.
> But this leads to the question - how can one migrate away from
> serialized proxies in a compatible way?
> 
> In the end - a lot of stuff in the JDK depends on Proxies, and their
> limitations now begins to surface.
> 
> A new API may not be a bad idea. :)

Yes !
And we can leverage invokedynamic/method handles to avoid boxing/ bad perf.
The idea is that first time an implementation of an abstract method is 
required, an object (implementing an interface similar to InvocationHandler) 
acting as a bootstrap method is called to provide a method handle that will be 
used as implementation.

see https://github.com/forax/hidden_proxy for a prototype of that idea.

> 
> - Johannes

Rémi

Re: fast way to infer caller

[forax]

> From: "Kasper Nielsen" 
> To: "Remi Forax" 
> Cc: "Ceki Gülcü" , "core-libs-dev" 
> 
> Sent: Thursday, April 7, 2022 2:42:46 PM
> Subject: Re: fast way to infer caller

> On Thu, 7 Apr 2022 at 13:33, Remi Forax < [ mailto:fo...@univ-mlv.fr |
> fo...@univ-mlv.fr ] > wrote:

>> - Original Message -
>> > From: "Kasper Nielsen" < [ mailto:kaspe...@gmail.com | kaspe...@gmail.com 
>> > ] >
>> > To: "Ceki Gülcü" < [ mailto:c...@qos.ch | c...@qos.ch ] >
>>> Cc: "core-libs-dev" < [ mailto:core-libs-dev@openjdk.java.net |
>> > core-libs-dev@openjdk.java.net ] >
>> > Sent: Thursday, April 7, 2022 1:53:33 PM
>> > Subject: Re: fast way to infer caller


>> >> MethodHandles.lookup().lookupClass() looks very promising except that
>> >> there is no way to specify the depth.

>> >> I am looking for a method to obtain the Nth caller at a cost of around
>> >> 100 to 200 nanoseconds of CPU time. Do you think the JDK could cater
>> >> for this use case?


>> > Hi Ceki,

>> > I don't think you will find the numbers you are looking for with
>> > StackWalker.
>> > Compared to something like Reflection.getCallerClass() which
>> > MethodHandles.lookup() uses.
>> > There is still a very large (>100x) performance gap between the two.

>> > public class StackWalkerPerf {

>> > static final StackWalker sw =
>> > StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);

>> > @Benchmark
>> > public Class stackWalkerCallerClass() {
>> > return sw.getCallerClass();
>> > }

>> > @Benchmark
>> > public Class reflectionCallerClass() {
>> > return MethodHandles.lookup().lookupClass();
>> > }
>> > }

>> > Benchmark Mode Cnt Score Error Units
>> > StackWalkerPerf.reflectionCallerClass avgt 10 2,927 ± 0,012 ns/op
>> > StackWalkerPerf.stackWalkerCallerClass avgt 10 915,287 ± 9,565 ns/op

>>> The resulting class is not the same, so comparing the performance of two 
>>> codes
>> > that does not do the same thing is dubious, no ?
> I think it is a fair test. MethodHandles.lookup() is basically just wrapping
> Reflection.getCallerClass(). So if anything
> calling Reflection.getCallerClass() directly would be even faster than calling
> MethodHandles.lookup().lookupClass();

nope, see my previous mail to Ceki, the VM is cheating here if it can inline 
the call to MethodHandles.lookup(). 

> /Kasper

Rémi 

Re: fast way to infer caller

[Remi Forax]

- Original Message -
> From: "Kasper Nielsen" 
> To: "Ceki Gülcü" 
> Cc: "core-libs-dev" 
> Sent: Thursday, April 7, 2022 1:53:33 PM
> Subject: Re: fast way to infer caller

>>
>> MethodHandles.lookup().lookupClass() looks very promising except that
>> there is no way to specify the depth.
>>
>> I am looking for a method to obtain the Nth caller at a cost of around
>> 100 to 200 nanoseconds of CPU time.  Do you think the JDK could cater
>> for this use case?
>>
> 
> Hi Ceki,
> 
> I don't think you will find the numbers you are looking for with
> StackWalker.
> Compared to something like Reflection.getCallerClass() which
> MethodHandles.lookup() uses.
> There is still a very large (>100x) performance gap between the two.
> 
> public class StackWalkerPerf {
> 
>static final StackWalker sw =
> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
> 
>@Benchmark
>public Class stackWalkerCallerClass() {
>return sw.getCallerClass();
>}
> 
>@Benchmark
>public Class reflectionCallerClass() {
>return MethodHandles.lookup().lookupClass();
>}
> }
> 
> Benchmark   Mode  Cnt ScoreError  Units
> StackWalkerPerf.reflectionCallerClass   avgt   102,927 ± 0,012  ns/op
> StackWalkerPerf.stackWalkerCallerClass  avgt   10  915,287 ± 9,565  ns/op

The resulting class is not the same, so comparing the performance of two codes 
that does not do the same thing is dubious, no ?

> 
> /Kasper

Rémi

Re: fast way to infer caller

[Remi Forax]

- Original Message -
> From: "Ceki Gülcü" 
> To: "core-libs-dev" 
> Sent: Thursday, April 7, 2022 1:04:11 AM
> Subject: Re: fast way to infer caller

> Hi Jason,
> 
> Yes, the code was mentioned in SLF4J PR 271.
> 
> The benchmark can be found at
> 
> https://github.com/ceki/logback-perf/tree/master/src/main/java/ch/qos/logback/perf/caller
> 
> Here is the gist of the StackWalker variant:
> 
> public class CallerCompute {
> 
>static private StackWalker WALKER = StackWalker.getInstance();
> 
>static public String getCallerClass(int depth) {
> 
>List frames = WALKER.walk(s -> s.limit(4).toList());
> process the returned frames
>}
> }
> 
> The above executes at around 1.8 microseconds per call whereas the code
> in LogRecord.getSourceClassName() executes at 5 microseconds per call.
> The cost of LogRecord constructor is negligible at around 25 nanoseconds.
> 
> Anyway, thank you for the suggestion.

You can declare WALKER "final", it should help. This is also better to call 
getCallerClass ASAP and then pass the class to the other stack frames instead 
of having to skip those stack frames when calling getCallerClass. You may also 
try able to improve the speed by doing a == before calling equals(String) if 
you call String.intern() on THIS_CLASS_NAME.

> 
> --
> Ceki

Rémi

> 
> 
> On 4/7/2022 12:27 AM, Jason Mehrens wrote:
>> Ceki,
>> 
>> Looks like the benchmark code is from 
>> https://github.com/qos-ch/slf4j/pull/271
>> 
>> Looks like the benchmark code is doing a collection so perhaps that is some 
>> of
>> the performance hit?
>> Have you benchmarked java.util.LogRecord.getSourceClassName() to compare with
>> your StackWalker benchmark?
>> 
>> https://github.com/openjdk/jdk/blob/master/src/java.logging/share/classes/java/util/logging/LogRecord.java#L754
>> 
>> Jason
>> 
>> 
>> From: core-libs-dev  on behalf of Ceki
>> Gülcü 
>> Sent: Wednesday, April 6, 2022 4:26 PM
>> To: core-libs-dev
>> Subject: Re: fast way to infer caller
>> 
>> 
>> Hi Rémi,
>> 
>> Thank you for your answer.
>> 
>> According to some benchmarks on a i7-8565U Intel CPU (quite average
>> CPU), here are the costs of computing the caller class via different
>> methods:
>> 
>> using new Throwable().getStackTrace: 11 microseconds per call
>> using StackWalker API: 1.8 microseconds per call
>> using SecurityManager: 0.9 microseconds per call
>> 
>> While a six fold improvement (StackWalker compared to new Thorowable) is
>> nothing to sneeze at, the performance of StackWalker is not as good as
>> with a custom SecurityManager.
>> 
>> I have not said so explicitly but the aim here is to allow the user to
>> obtain a new logger by calling LoggerFactory.getLogger() with no
>> arguments with the returned logger named after the caller class.
>> 
>> Spending 1 or 2 microseconds for this call is OK if the logger is a
>> static field. However, if the logger is an instance field, then spending
>> 2 microseconds per host object instance just to obtain a logger might
>> have a noticeable impact on performance. It follows that the performance
>> of LoggerFactory.getLogger() must be exceptionally good, assuming we
>> wish to avoid having the user accidentally shooting herself on the foot,
>> ergo the 100 nanosecond performance per call requirement.
>> 
>> Noting that invoking MethodHandles.lookup().lookupClass() seems very
>> fast (about 2 nanoseconds), I would be very interested if  new
>> lookup(int index) method were added to java.lang.invoke.MethodHandles
>> class, with index designating the desired index on the call stack.
>> 
>> Does the above make sense? How difficult would it be to add such a method?
>> 
>> --
>> Ceki Gülcü
>> 
>> 
>> On 4/6/2022 5:52 PM, Remi Forax wrote:
>>> - Original Message -
>>>> From: "Ceki Gülcü" 
>>>> To: "core-libs-dev" 
>>>> Sent: Wednesday, April 6, 2022 5:30:51 PM
>>>> Subject: fast way to infer caller
>>>
>>>> Hello,
>>>
>>> Hello,
>>>
>>>>
>>>> As you are probably aware, one of the important primitives used in
>>>> logging libraries is inferring the caller of a given logging statement.
>>>> The current common practice is to create a throwable and process its
>>>> stack trace. This is rather wasteful and rather slow. As an alterna

Re: fast way to infer caller

[Remi Forax]

- Original Message -
> From: "Ceki Gülcü" 
> To: "core-libs-dev" 
> Sent: Wednesday, April 6, 2022 11:26:39 PM
> Subject: Re: fast way to infer caller

> Hi Rémi,
> 
> Thank you for your answer.
> 
> According to some benchmarks on a i7-8565U Intel CPU (quite average
> CPU), here are the costs of computing the caller class via different
> methods:
> 
> using new Throwable().getStackTrace: 11 microseconds per call
> using StackWalker API: 1.8 microseconds per call
> using SecurityManager: 0.9 microseconds per call
> 
> While a six fold improvement (StackWalker compared to new Throwable) is
> nothing to sneeze at, the performance of StackWalker is not as good as
> with a custom SecurityManager.
> 
> I have not said so explicitly but the aim here is to allow the user to
> obtain a new logger by calling LoggerFactory.getLogger() with no
> arguments with the returned logger named after the caller class.
> 
> Spending 1 or 2 microseconds for this call is OK if the logger is a
> static field. However, if the logger is an instance field, then spending
> 2 microseconds per host object instance just to obtain a logger might
> have a noticeable impact on performance. It follows that the performance
> of LoggerFactory.getLogger() must be exceptionally good, assuming we
> wish to avoid having the user accidentally shooting herself on the foot,
> ergo the 100 nanosecond performance per call requirement.
> 
> Noting that invoking MethodHandles.lookup().lookupClass() seems very
> fast (about 2 nanoseconds), I would be very interested if  new
> lookup(int index) method were added to java.lang.invoke.MethodHandles
> class, with index designating the desired index on the call stack.
> 
> Does the above make sense? How difficult would it be to add such a method ?

I think that for MethodHandles.lookup() and all other caller sensitive methods, 
the VM uses a trick, it inlines the call into the caller method, once this is 
done, the caller class is know statically and can be replaced by a constant. In 
order to use the same trick, you need a way to force the inlining through 
getLogger(), the is something the JDK can do but that is not available to user 
code.

And you do not want lookup(-1) because getLogger() can be called by reflection, 
java.lang.invoke or a lambda, in all these cases you have "hidden" stack frames 
in between the user code code and LoggerFactory.getLogger(), you need to skip 
those stack frames, this is what the StackWalker does.

Now, i don't think there is a real solution to you issue, worst i will try to 
convince you that this is not a real problem :)
You are offering convenience using magic to your user, this has a cost.

For me this is very similar to the trade off you have by offering to change the 
logger configuration at runtime.
This is convenient but it requires a volatile read (even when the logger is 
de-activated) which destroy performance in tight loop (you loose hoisting).
I believe that if your users are fine with that, they are also fine with a call 
to LoggerFactory.getLogger() being a little slow.

> 
> --
> Ceki Gülcü

Rémi

> 
> 
> On 4/6/2022 5:52 PM, Remi Forax wrote:
>> - Original Message -
>>> From: "Ceki Gülcü" 
>>> To: "core-libs-dev" 
>>> Sent: Wednesday, April 6, 2022 5:30:51 PM
>>> Subject: fast way to infer caller
>> 
>>> Hello,
>> 
>> Hello,
>> 
>>>
>>> As you are probably aware, one of the important primitives used in
>>> logging libraries is inferring the caller of a given logging statement.
>>> The current common practice is to create a throwable and process its
>>> stack trace. This is rather wasteful and rather slow. As an alternative,
>>> I have tried using the StackWalker API to infer the caller but was
>>> unsatisfied with the performance.
>>>
>>> MethodHandles.lookup().lookupClass() looks very promising except that
>>> there is no way to specify the depth.
>>>
>>> I am looking for a method to obtain the Nth caller at a cost of around
>>> 100 to 200 nanoseconds of CPU time.  Do you think the JDK could cater
>>> for this use case?
>> 
>> We have designed the StackWalker with that in mind
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html
>> 
>> see the discussion on StackWalker.getCallerClass()
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#getCallerClass()

Re: fast way to infer caller

[Remi Forax]

- Original Message -
> From: "Ceki Gülcü" 
> To: "core-libs-dev" 
> Sent: Wednesday, April 6, 2022 5:30:51 PM
> Subject: fast way to infer caller

> Hello,

Hello,

> 
> As you are probably aware, one of the important primitives used in
> logging libraries is inferring the caller of a given logging statement.
> The current common practice is to create a throwable and process its
> stack trace. This is rather wasteful and rather slow. As an alternative,
> I have tried using the StackWalker API to infer the caller but was
> unsatisfied with the performance.
> 
> MethodHandles.lookup().lookupClass() looks very promising except that
> there is no way to specify the depth.
> 
> I am looking for a method to obtain the Nth caller at a cost of around
> 100 to 200 nanoseconds of CPU time.  Do you think the JDK could cater
> for this use case?

We have designed the StackWalker with that in mind
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html

see the discussion on StackWalker.getCallerClass()
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#getCallerClass()

> 
> --
> Ceki Gülcü
> 
> Sponsoring SLF4J/logback/reload4j at https://github.com/sponsors/qos-ch

regards,
Rémi

Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19

[Rémi Forax]

On Mon, 28 Mar 2022 16:49:58 GMT, Vicente Romero  wrote:

> Please review this PR which is updating the ASM included in the JDK to ASM 
> 9.2. This version should be included in JDK19. There are basically two 
> commits here, one that was automatically generated and that mostly changes 
> file headers etc and another one, smaller, that make changes to the code to 
> adapt it to our needs, JDK version etc. In this second commit one can see 
> that two classes that were removed by the automatically generated change have 
> been copied back:
> 
> jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter
> jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter
> 
> This is because package: `jdk.jfr.internal.instrument` needs them.
> 
> TIA

I suppose that you are raising commons.RemappingMethodAdapter and 
commons.RemappingAnnotationAdapter from the dead because you want to fix the 
code in jdk.jfr.internal.instrument later ?

Otherwise it's a little dangerous because that code as started to drift, the 
new remapper has new code not supported by the old remapper. BTW, @deprecated 
on ASM source is equivalent to. @Deprecated + forRemoval in the JDK.

The support of Java 19 is fine, the same code will be included in ASM soon.

-

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

Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes

[Rémi Forax]

On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss  wrote:

> javac should be changed to allow fully qualified access to private inner 
> classes in the permits clause of an enclosing class.

This is not how private works, private means accessible between the '{' and the 
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class, 
thus a private member class is not visible from the permits clause.

-

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

Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v10]

[Rémi Forax]

On Mon, 21 Mar 2022 05:17:31 GMT, ExE Boss  wrote:

>> Jim Laskey has updated the pull request incrementally with three additional 
>> commits since the last revision:
>> 
>>  - Typos
>>  - Update Carrier.java
>>  - Redo API to use list, bring Carrier.component back
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 309:
> 
>> 307: static {
>> 308: LOOKUP = MethodHandles.lookup();
>> 309: UNSAFE = Unsafe.getUnsafe();
> 
> It might be better to use `java.lang.invoke.MethodHandleStatics.UNSAFE`, and 
> probably also `java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP`.

The package is java.lang.runtime not java.lang.invoke so both fields are not 
accessible.

-

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

Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?

[Remi Forax]

- Original Message -
> From: "Cheng Jin" 
> To: "core-libs-dev" 
> Sent: Thursday, March 17, 2022 5:42:57 PM
> Subject: When to initialize the method's class for 
> MethodHandles.Lookup.findStatic()?

> Hi there,
> 
> The document of
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType)
> in the Java API is ambiguous in terms of when to initialize the method's class
> as follows (the same description as in other OpenJDK versions)
> 
> If the returned method handle is invoked, the method's class will be
> initialized, if it has not already been initialized.
> 
> 
> It occurs to me that the method's class should be initialized when invoking 
> the
> method handle but OpenJDK actually chooses to do the initialization in
> lookup.findStatic() rather than in mh.invoke()
> e.g.
> import java.lang.invoke.*;
> 
> public class Test_1 {
>static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class,
>"testMethod", MethodType.methodType(int.class, int.class)); <---
>Test_2.class gets initialized and verified.
> 
>public static void main(String[] args) throws Throwable {
>Test_1.mh.invoke();
>}
> }
> 
> public class Test_2 {
>static int testMethod(int value) { return (value + 1); }
> }
> 
> So there should be more clear explanation what is the correct or expected
> behaviour at this point and why OpenJDK doesn't comply with the document to
> delay the initialization of the method's class to mh.invoke().

Hi,
if by initialization you mean running the static block, then it's a bug.

As far as i remember, the JVMS spec cleanly separate the Linking exceptions 
from the Runtime exceptions.
https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-6.html#jvms-6.5.invokestatic

The linking exceptions occurs when calling findStatic() while the runtime 
exceptions will appear at runtime when calling invokeExact()/invoke().

> 
> Best Regards
> Cheng Jin

regards,
Rémi

Re: Making enum hashcodes consistent across JVM's

[Remi Forax]

- Original Message -
> From: "dfranken jdk" 
> To: "core-libs-dev" 
> Sent: Thursday, March 17, 2022 1:49:08 PM
> Subject: Making enum hashcodes consistent across JVM's

> Dear all,
> 
> Currently enums do not have a well-defined hashCode() implementation so
> they defer to Object.hashCode() which just uses an internal mechanism
> to determine the hashcode, likely based on the object's place in the
> heap.

No, and no.

Enum.hashCode() is well defined, it is equivalent to super.hashCode() and it 
can not be overridden by a specific enum (see the javadoc).

> 
> This may confuse a lot of developers as other classes such as String do
> have a well-defined hashcode which is consistent across multiple JVM's.

One of the reasons why String.hashCode() is defined is because the compiler 
(javac) compiles a switch on String using String.hashCode() so the hashCode at 
compile time and at runtime has to be the same.

Having a well defined hashCode was a mistake, first it means that we can not 
change the implementation later (the current hashCode of String has too many 
collisions) and it also make DDOS attacks easier because you can pre-compute a 
list of strings with the same hashCode.

> 
> Could it be a good idea to make enums' hashCode() method more reliably
> and predictable? For instance by using the hashcode of the name of its
> value?

Enum already have a kind of perfect hash, ordinal() which is reliable and 
predictable but it's not a great value as hashCode.

> 
> For example:
> 
> class MyEnum { A, B }
> -> A.hashCode() == A.name().hashCode()

This is not a good hashCode,

  enum Foo { Aa, BB }
  Foo.Aa.hashCode() == Foo.BB.hashCode()


> 
> Kind regards,
> 
> Dave Franken

regards,
Rémi

Re: RFR: 8283237: CallSite should be a sealed class

[Rémi Forax]

On Thu, 17 Mar 2022 07:32:40 GMT, liach  wrote:

>> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>> 
>>> 86:  */
>>> 87: public
>>> 88: abstract sealed class CallSite permits ConstantCallSite, 
>>> VolatileCallSite, MutableCallSite {
>> 
>> Nitpicking with my JSR 292 hat,
>> given that the permits clause is reflected in the javadoc,
>> the order should be ConstantCS, MutableCS and VolatileCS,
>> it's both in the lexical order and in the "memory access" of setTarget() 
>> order , from stronger access to weaker access.
>
> I agree that Constant, Mutable, Volatile order is better, ranked by the 
> respective cost for `setTarget()` and (possibly) invocation, and earlier ones 
> in the list are more preferable if conditions allow.
> 
> However, in the current API documentation, the order is Constant, Mutable, 
> and Volatile. Should I update that or leave it?
> 
> /*
>  * 
>  * If a mutable target is not required, an {@code invokedynamic} 
> instruction
>  * may be permanently bound by means of a {@linkplain ConstantCallSite 
> constant call site}.
>  * If a mutable target is required which has volatile variable semantics,
>  * because updates to the target must be immediately and reliably witnessed 
> by other threads,
>  * a {@linkplain VolatileCallSite volatile call site} may be used.
>  * Otherwise, if a mutable target is required,
>  * a {@linkplain MutableCallSite mutable call site} may be used.
>  * 
>  */

For me, this is unrelated, for this paragraph it's easier to explain the 
semantics of MutableCallSite with an otherwise, i.e. it's mutable but you do 
not want the cost of a volatile acces.

-

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

Re: RFR: 8283237: CallSite should be a sealed class

[Rémi Forax]

On Wed, 16 Mar 2022 13:09:30 GMT, liach  wrote:

> Change `CallSite` to a sealed class, as `CallSite` is an abstract class which 
> does not allow direct subclassing by users per its documentation. Since I 
> don't have a JBS account, I posted the content for the CSR in a GitHub Gist 
> at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone 
> can submit a CSR for me.

src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:

> 86:  */
> 87: public
> 88: abstract sealed class CallSite permits ConstantCallSite, 
> VolatileCallSite, MutableCallSite {

Nitpicking with my JSR 292 hat,
given that the permits clause is reflected in the javadoc,
the order should be ConstantCS, MutableCS and VolatileCS,
it's both in the lexical order and in the "memory access" of setTarget() order 
, from stronger access to weaker access.

-

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

Re: Behavior change in the jar tool JDK11 vs JDK8.

[Remi Forax]

- Original Message -
> From: "Pasam Soujanya1" 
> To: "core-libs-dev" 
> Sent: Monday, March 14, 2022 12:48:42 PM
> Subject: Behavior change in the jar tool JDK11 vs JDK8.

> There is a significant difference in the way the JAR tool (starting JDK11) 
> seems
> to be responding to target files that are not present , when compared to JDK8.
> With JDK 8, the jar tool just reports about target files that are absent, and
> creates the jar file with whatever targets are available.
> 
> With JDK8, If I try to create a jar file with one existing file and another
> invalid file name I can see achieve getting created with the file that exists.
> Only the warning seems to be printed for the target that doesn't exists:
> 
> $ jar -cvf sample.jar exists.txt does_not_exist.txt
> does_not_exist.txt : no such file or directory
> added manifest
> adding: exists.txt(in = 0) (out= 0)(stored 0%)
> 
> From JDK11 onward upto the latest, though I can see the same verbose output 
> the
> jar file(sample.jar) is not created.
> 
> Looking at the code, the targets that do exist are written to a temporary JAR
> file, but the following "validation" code, which runs once the tool has
> finished writing the temporary JAR file, only moves/renames the temporary file
> to the specified JAR location if "ok" is true. If "ok" is false the temporary
> file is simply deleted:
> https://github.com/openjdk/jdk11u/blob/22186cb1fe22b4b30fc72c67ce9946cd4f03199d/src/jdk.jartool/share/classes/sun/tools/jar/Main.java#L451
> 
> Is this behavior change intentional? I couldn't find anything documented in
> release notes of JDK 9,10,11. If not intentional, can someone help me create
> bug report on OpenJDK jira? Thank you.

I believe it's a bugfix, since the creation of the command "jar", jar follows 
the semantics of "tar", so the behavior you are seeing now is the correct one.
But i was not able to pinpoint the bug corresponding to that issue.

> 
> Regards,
> Pasam Soujanya.

regards,
Rémi

Re: Questions about enhancement and Correction to Java OpenJDK Floating Point?

[Remi Forax]

- Original Message -
> From: "A Z" 
> To: "core-libs-dev" 
> Sent: Monday, March 14, 2022 7:49:04 AM
> Subject: Questions about enhancement and Correction to Java OpenJDK Floating 
> Point?

Hi Terry,
if you want to have the same output as C, instead of println() use printf().

In your example, using
  out.printf("%f\n", c);

prints
  0.01

  0.01

regards,
Rémi

> To whom it may concern,
> 
> Having noticed
> 
> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8190947
> https://bugs.openjdk.java.net/browse/JDK-8190991
> 
> and similar, at
> https://community.oracle.com/tech/developers/discussion/4126262/big-issue-with-float-double-java-floating-point
> 
> I have been referred on to the core-libs-dev area.
> 
> The software development company I represent wishes to keep its name
> confidential,
> and no-mentioned, at this time.
> 
> A number of us at our end have found that floating point and StrictMath
> arithmetic
> on both float and double does not result in range accuracy, but produces
> denormal
> and pronormal values.
> 
> We are aware of the Java Language Specification, and IEEE 754 specifications,
> to these issues, but are still finding that they are not the most relevant or
> great issue.
> 
> While we are aware of the BigDecimal and BigInteger workarounds, and
> furthermore, the calculator class including big-math
> https://github.com/eobermuhlner,
> we are finding in the development, debugging, and editing of our Java 
> programs,
> that using other classes to operate and exchange for the lack of range 
> accuracy
> from float,
> double and java.lang.StrictMath, that we are getting bogged down with what 
> turns
> into
> more inferior software.  The known and available workaround approaches are
> becoming
> stop-gap measures, perforcedly put in place, while introducing other problems
> into OpenJDK or Java software that don't have any particular, immediate,
> solutions.
> 
> Substituting float and double data in and out of BigDecimal and BigInteger
> produces
> source code which is much messier, complicated, error prone, difficult to
> understand
> and to change, is definitely slower, and is an inferior substitute when float
> and double are more than enough in the overwhelming majority of corresponding
> cases.
> This is particularly showing up in 2D and 3D Graphics software, by the default
> OpenJDK Libraries, but also through JMonkeyEngine 3.5.
> 
> Possessing the option to immediately deal with the precondition, postcondition
> and field types of float and double is far superior and more than ideal.
> All this is before the massive advantage of being able to use operators,
> but the change case becomes overwhelming when along a range accurate,
> double (or maybe float, also) supporting Scientific Calculator class.
> 
> If I want to discuss (at least OpenJDK) change in this area, I have
> been pointed to the core-libs area, by one of the respondents
> of the article:
> 
> https://community.oracle.com/tech/developers/discussion/4126262/big-issue-with-float-double-java-floating-point.
> 
> Is there anyone here, at core-libs-dev, who can point
> me in a better Oracle or OpenJDK direction, to discuss further
> and see about Java float and double and StrictMath floating point
> arithmetic denormal and pronormal values being repaired away
> and being made range accurate for all evaluation operations
> with them?
> 
> Certainly since other languages already have, that are open source
> and open resource file ones.  It is a mathematical fact that, for
> consistent, necessary and even fast term, 10% of 10% must
> always precisely be 1%, and by no means anything else.
> 
> Consider these three different language API evaluations,
> using their equivalents of float and double to perform
> the floating point equivalent of that precise evaluation:
> 
> //--
> //The C Language.
> #include 
> 
> int main()
> {
>printf("Program has started...");
>printf("\n");
>printf("\n");
>double a = 0.1D;
>double b = 0.1D;
>double c = a*b;
>printf("%lf",c);
>printf("\n");
>printf("\n");
>float d = 0.1F;
>float e = 0.1F;
>float f = d*e;
>printf("%lf",f);
>printf("\n");
>printf("\n");
>printf("Program has Finished.");
>return 0;
> }
> 
> /*
> Program has started...
> 
> 0.01
> 
> 0.01
> 
> Program has Finished.
> */
> //--
> //The C++ Language.
> 
> #include 
> 
> using namespace std;
> 
> int main()
> {
>cout << "Program has started..." << endl;
>double a = 0.1D;
>double b = 0.1D;
>double c = a*b;
>cout << endl << c << endl << endl;
>float d = 0.1F;
>float e = 0.1F;
>float f = d*e;
>cout << f << endl << endl;
>cout << "Program has Finished.";
>return 0;
> }
> 
> /*
> Program has started...
> 
> 0.01
> 
> 0.01
> 
> Program has Finished.
> */
> 
> 

Re: RFR: JDK-8282798 java.lang.runtime.Carrier

[forax]

- Original Message -
> From: "Brian Goetz" 
> To: "Remi Forax" 
> Cc: "Jim Laskey" , "core-libs-dev" 
> 
> Sent: Tuesday, March 8, 2022 4:37:41 PM
> Subject: Re: RFR: JDK-8282798 java.lang.runtime.Carrier

> You keep saying “what we need”.  But really, what I think you mean is “what 
> the
> scheme I have in mind needs.”  Can we try and separate these out?  I don’t
> think any of these are *needed*, but maybe you want to make the argument that
> you have a better scheme in mind, and we should choose that scheme, and
> therefore then we might need them.

I'm open to any other schemes, it would be a kind of sad if the carrier API is 
added to java.lang.runtime to not use it for pattern matching.

So what other scheme you have in mind ?

Rémi

> 
>> On Mar 8, 2022, at 10:04 AM, Remi Forax  wrote:
>> 
>> Hi Jim,
>> I believe that there is a mismatch about what is needed for the pattern 
>> matching
>> and the API you propose.
>> The Carrier API allows to map one tuple of types to one storage 
>> representation
>> based on ints, longs and references.
>> 
>> But what we need is to have several shapes for the same storage, mapping is 
>> not
>> one to one but many to one, more like how unions are mapped in C.
>> For a switch, if we have several cases, each one need it's own shape to store
>> the bindings but at the same time we also need one field to be the same for 
>> all
>> shapes (the field of type int indicating which case match) so one storage but
>> many shapes.
>> 
>> Rémi
>> 
>> - Original Message -
>>> From: "Jim Laskey" 
>>> To: "core-libs-dev" 
>>> Sent: Tuesday, March 8, 2022 3:11:39 PM
>>> Subject: RFR: JDK-8282798 java.lang.runtime.Carrier
>> 
>>> We propose to provide a runtime anonymous carrier class object generator;
>>> java.lang.runtime.Carrier. This generator class is designed to share 
>>> anonymous
>>> classes when shapes are similar. For example, if several clients require
>>> objects containing two integer fields, then Carrier will ensure that each
>>> client generates carrier objects using the same underlying anonymous class.
>>> 
>>> See JBS for details.
>>> 
>>> -
>>> 
>>> Commit messages:
>>> - Introduce java.lang.runtime.Carrier
>>> 
>>> Changes: https://git.openjdk.java.net/jdk/pull/7744/files
>>> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7744=00
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8282798
>>> Stats: 1085 lines in 2 files changed: 1085 ins; 0 del; 0 mod
>>> Patch: https://git.openjdk.java.net/jdk/pull/7744.diff
>>> Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744
>>> 
> >> PR: https://git.openjdk.java.net/jdk/pull/7744

Re: RFR: JDK-8282798 java.lang.runtime.Carrier

[Remi Forax]

- Original Message -
> From: "John R Rose" 
> To: "core-libs-dev" 
> Sent: Tuesday, March 8, 2022 5:55:00 PM
> Subject: Re: RFR: JDK-8282798 java.lang.runtime.Carrier

> On Tue, 8 Mar 2022 15:59:59 GMT, Maurizio Cimadamore 
> wrote:
> 
>>> We propose to provide a runtime anonymous carrier class object generator;
>>> java.lang.runtime.Carrier. This generator class is designed to share 
>>> anonymous
>>> classes when shapes are similar. For example, if several clients require
>>> objects containing two integer fields, then Carrier will ensure that each
>>> client generates carrier objects using the same underlying anonymous class.
>>> 
>>> See JBS for details.
>>
>> src/java.base/share/classes/java/lang/runtime/Carrier.java line 48:
>> 
>>> 46:
>>> 47: /**
>>> 48:  * This  class is used to create objects that have number and types of
>> 
>> The class javadoc seems a bit on the thin side. I would say something on the
>> fact that the shape of the carrier is determined by a MethodType, etc, and 
>> add
>> an example to illustrate basic usage.
> 
> Agreed.  Also, this class is unusual in that it is not instantiated; like
> `Arrays` or `Collections` it is a (small) bundle of static factories (or are
> they algorithms?).  I think as such it should be called `Carriers`.
> 
> I'm slightly surprised the MH factories are not factored through a metaobject 
> of
> the form
> 
> record CarrierBinding(
>MethodType methodType,
>MethodHandle constructor,
>List> componentTypes,
>List components)
> { … }

Yes, i've done something very similar
https://github.com/forax/switch-carrier/blob/master/src/main/java/com/github/forax/carrier/java/lang/runtime/Patterns.java#L172

Note that constructor.type() == methodType and constructor.parameterList() == 
componenetTypes,
so the record can be reduced to a tuple (constructor / components).

> 
> 
> The presupposition here, I suppose, is that carriers will only be used by 
> condy
> or similar quasi-statically configured clients, so having the multiple lookups
> through a hidden table is no burden, and the clients can always keep the
> associations correct (between constructors and various component accessors).
> 
> **But** if I were to use carriers to manage intermediate structures in (say) a
> serialization package (for instance) I would need to make my own records like
> the above for my own bookkeeping, and I would be slightly miffed that the
> Carrier API insisted on doing a de-novo lookup twice on each MT key (to say
> nothing of me having to create the MT key first).

It depends if you want to use the same carrier for several cases or not.

> 
> **And** if I were to use carriers in that way (away from condy), **then** I
> would want to skip the step of building the MethodType, and wish for a factory
> method for CarrierBinding instances that took a plain List, as well as 
> a
> factory method that took the MethodType (which is convenient for condy).

As i said above, constructor.type() == methodType so anyway the MethodType has 
to be created.

> 
> BTW, it would be normal to give the name `Carrier` (which is a good name BTW) 
> to
> the record type that embodies the group of method handles, so `record
> Carrier(…constructor…components…) {…factories…}`.
> 
> I suppose stuff like this could be added later.  But it's worth considering 
> now,
> simply because there is an early decision point between a class named 
> `Carrier`
> and a static-only class named `Carriers`.

Rémi

Re: RFR: JDK-8282798 java.lang.runtime.Carrier

[Remi Forax]

Hi Jim,
I believe that there is a mismatch about what is needed for the pattern 
matching and the API you propose.
The Carrier API allows to map one tuple of types to one storage representation 
based on ints, longs and references.

But what we need is to have several shapes for the same storage, mapping is not 
one to one but many to one, more like how unions are mapped in C.
For a switch, if we have several cases, each one need it's own shape to store 
the bindings but at the same time we also need one field to be the same for all 
shapes (the field of type int indicating which case match) so one storage but 
many shapes.

Rémi

- Original Message -
> From: "Jim Laskey" 
> To: "core-libs-dev" 
> Sent: Tuesday, March 8, 2022 3:11:39 PM
> Subject: RFR: JDK-8282798 java.lang.runtime.Carrier

> We propose to provide a runtime anonymous carrier class object generator;
> java.lang.runtime.Carrier. This generator class is designed to share anonymous
> classes when shapes are similar. For example, if several clients require
> objects containing two integer fields, then Carrier will ensure that each
> client generates carrier objects using the same underlying anonymous class.
> 
> See JBS for details.
> 
> -
> 
> Commit messages:
> - Introduce java.lang.runtime.Carrier
> 
> Changes: https://git.openjdk.java.net/jdk/pull/7744/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7744=00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8282798
>  Stats: 1085 lines in 2 files changed: 1085 ins; 0 del; 0 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/7744.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7744/head:pull/7744
> 
> PR: https://git.openjdk.java.net/jdk/pull/7744

Re: Replace simple iterations of Map.entrySet with Map.forEach calls

[Remi Forax]

- Original Message -
> From: "Stuart Marks" 
> To: "-" , "core-libs-dev" 
> 
> Sent: Saturday, March 5, 2022 1:58:30 AM
> Subject: Re: Replace simple iterations of Map.entrySet with Map.forEach calls

> Hi, I have to say I'm pretty skeptical of this change overall.
> 
> It sounds like the main issue is iterating over the entrySet of a map that 
> might
> be
> modified concurrently. The bug report says
> 
> > for concurrent maps, regular iteration of entrySet may fail spontaneously
> 
> This isn't true for ConcurrentHashMap or ConcurrentSkipListMap, which have
> weakly
> consistent iterators that won't error out. It *is* true for synchronizedMap,
> which
> can throw ConcurrentModificationException if the map is modified during
> iteration. I
> guess that's mostly what you're referring to.
> 
> The problem with synchronizedMap (and the other synchronized wrappers) is that
> they
> require external synchronization around iteration, and it's pretty much
> impossible
> to cover all the cases where iteration occurs. It's used everywhere outside 
> the
> JDK.
> Even within the JDK, it appears quite frequently and is hard to avoid. For
> example,
> consider
> 
> collection.addAll(synchronizedMap.keySet())
> 
> AbstractCollection::addAll iterates its argument, and this is inherited in a
> bunch
> of places.
> 
> The root of the problem is that the synchronized wrappers support concurrent
> updating only in a very narrow range of use cases. If people have trouble with
> concurrent map updating, they need to use a real concurrent data structure, or
> they
> need to put everything inside their own class and have it do locking around
> higher-level operations. Replacing a few entrySet iteration cases with
> Map::forEach
> doesn't help anything.

Hi DrDeprecator,
i wonder if we should not deprecate the method synchronized*, usually they 
cause more harm than good for the reason you explain.

> 
> s'marks

Rémi

> 
> 
> 
> On 2/22/22 3:09 PM, - wrote:
>> Hello,
>> In the patch for 8281631 ,
>> xenoamess intentionally avoided
>>  repeatedly
>> calling Map::size, for it may not be constant-timed due to being
>> concurrent. This alerted me that the for loops on Map::entrySet may be
>> similarly not thread-safe.
>> 
>> I believe that we should migrate for loop on Map::entrySet to Map::forEach
>> calls; concurrent map callees usually have dedicated logic
>> 
>> to mitigate such thread-safety issues. Synchronized map
>> 
>> callees are benefitted too.
>> 
>> Another lesser benefit is reduced object allocation for iteration. While
>> the most popular implementations (HashMap, LinkedHashMap, WeakHashMap,
>> TreeMap) don't need to allocate entries for iteration, many other (EnumMap,
>> IdentityHashMap, concurrent maps) do, and iterating those maps with forEach
>> is less costly. (Note that 8170826
>>  takes care of
>> implementing proper forEach in EnumMap)
>> 
>> A JBS issue has already been submitted at
>> https://bugs.openjdk.java.net/browse/JDK-8282178, and I have prepared a
>> patch. This patch modifies the putAll implementations of a few JDK maps to
>> let the callee feed entries through Map::forEach to be put into the maps.
>> Two places of Map::entrySet calls in Collectors has also been updated.
>> 
>> For most other existing usages in the JDK, I don't think they will benefit
>> from such a migration: They mostly iterate on the entryset of the popular
>> implementations that already host entries and are single-threaded, and I
>> don't think it's in our best interest to touch those use cases.
>> 
>> So, here are my questions:
>> 1. Is such a concept of replacing Map::entrySet calls with Map::forEach
>> calls reasonable, or is there any fundamental flaw?
>> If the concept sounds good, I will submit a patch, so we can answer
>> 2. Is the changes implemented correct for this concept, i.e. targeting code
>> where users supply callee maps?
>> 
>> Best regards

Re: RFR: 8282143: Objects.requireNonNull should be ForceInline

[Remi Forax]

- Original Message -
> From: "Paul Sandoz" 
> To: "core-libs-dev" 
> Sent: Tuesday, March 1, 2022 1:48:02 AM
> Subject: Re: RFR: 8282143: Objects.requireNonNull should be ForceInline

> On Sat, 19 Feb 2022 05:51:52 GMT, Quan Anh Mai  wrote:
> 
>> Hi,
>> 
>> `Objects.requireNonNull` may fail to be inlined. The call is expensive and 
>> may
>> lead to objects escaping to the heap while the null check is cheap and is 
>> often
>> elided. I have observed this when using the vector API when a call to
>> `Objects.requireNonNull` leads to vectors being materialised in a hot loop.
>> 
>> Should the other `requireNonNull` be `ForceInline` as well?
>> 
>> Thank you very much.
> 
> `Objects.requireNonNull` is also used on the critical path of VarHandles, and
> other places in `j.l.invoke` so I think this is generally beneficial beyond 
> use
> by the Vector API.

It is also use by javac when the JLS requires to emit a nullcheck, like for 
example when creating a method reference.

> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/7543

Re: Should System.exit be controlled by a Scope Local?

[Remi Forax]

Hi Ethan,
there is a far simpler solution, call org.apache.ivy.run(args, true) instead of 
org.apache.ivy.main(args) in your tool provider.

regards,
Rémi

- Original Message -
> From: "Ethan McCue" 
> To: "core-libs-dev" 
> Sent: Saturday, February 26, 2022 11:14:19 PM
> Subject: Should System.exit be controlled by a Scope Local?

> I have a feeling this has been considered and I might just be articulating
> the obvious - but:
> 
> As called out in JEP 411, one of the remaining legitimate uses of the
> Security Manager is to intercept calls to System.exit. This seems like a
> decent use case for the Scope Local mechanism.
> 
> 
>public class Runtime {
>...
>private final ScopeLocal EXIT =
> ScopeLocal.newInstance();
> 
>...
> 
>public void overridingExitBehavior(IntConsumer exit, Runnable run) {
>ScopeLocal.with(EXIT, exit).run(run);
>}
> 
>...
> 
>public void exit(int status) {
>if (EXIT.isBound()) {
>EXIT.get().accept(status);
>}
>else {
>Shutdown.exit(status);
>}
>}
>}
> 
> 
> One of the likely minor benefits in the scope of things, but related to the
> parts of the ecosystem I am doodling with so I'll mention it, is that it
> would become possible to wrap "naive" cli programs with the ToolProvider
> SPI without rewriting their code if this System.out, and System.err all
> became reliably configurable.
> 
> For instance, Apache Ivy's CLI has a main class that looks like this
> 
> https://github.com/apache/ant-ivy/blob/424fa89419147f50a41b4bdc665d8ea92b5da516/src/java/org/apache/ivy/Main.java
> 
>package org.apache.ivy;
> 
>public final class Main {
>...
> 
>public static void main(String[] args) throws Exception {
>try {
>run(args, true);
>System.exit(0);
>} catch (ParseException ex) {
>System.err.println(ex.getMessage());
>System.exit(1);
>}
>}
> }
> 
> Making these otherwise static parts of the system configurable would enable
> a third party library to write
> 
>public final class IvyToolProvider implements ToolProvider {
>@Override
>public String name() {
>return "ivy";
>}
> 
>@Override
>public int run(PrintWriter out, PrintWriter err, String... args) {
>var exit = new AtomicInteger(0);
>Runtime.getRuntime().overridingExitBehavior(exit::set, () -> {
>System.overridingOut(out, () -> {
> System.overridingErr(err, Main::main);
>}
>};
>return exit.get();
>}
>}
> 
> Whether that would be enough to make it so that people other than Christian
> Stein use the mechanism is anyone's guess, but might be worth a shot.
> 
> https://grep.app/search?q=java.util.spi.ToolProvider

Re: [External] : Sequenced Collections

[forax]

- Original Message -
> From: "Stuart Marks" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" , "Tagir Valeev" 
> 
> Sent: Tuesday, February 15, 2022 6:06:54 AM
> Subject: Re: [External] : Sequenced Collections

>> Here is the first sentence of the javadoc for java.util.List "An ordered
>> collection (also known as a sequence)."
>> And the first paragraph of java.util.RandomAccess "Marker interface used by 
>> List
>> implementations to indicate that they support fast (generally constant time)
>> random access. The primary purpose of this interface is to allow generic
>> algorithms to alter their behavior to provide good performance when applied 
>> to
>> either random or sequential access lists"
>> 
>> You can find that the actual design, mixing ordered collection and indexed
>> collection into one interface named List not great, but you can not say that
>> this is not the actual design.
> 
> Hi Rémi,
> 
> You have a talent for omitting pieces of the specification that are 
> inconvenient
> to
> your argument. The complete first paragraph of the List specification is
> 
> An ordered collection (also known as a sequence). The user of this 
> interface
> has precise control over where in the list each element is inserted. The 
> user
> can access elements by their integer index (position in the list), and 
> search
> for elements in the list.
> 
> Clearly, a List is not *merely* an ordered collection or sequence. Positioning
> (which I refer to as external ordering) and access by index are also inherent 
> to
> List.

I don't disagree, my point is that java.util.List is both an ordered collection 
or sequence and an indexed collection.
So from the POV of an implementer it's both, but from the POV of a user, if you 
want only an ordered collection or sequence, you will use List.

Introducing SequencedCollection may be ok in term of implementation, but it 
will cause trouble in term of usage,
an alarmist view is to think that introducing SequencedCollection will cause 
chaos, but i think it will simple than that, people will quickly learn that as 
OrderedSet, NavigableSet or Queue, SequencedCollection is a second class 
citizen interface and should rarely used in the signature of public methods.

> 
> The original design does support "random access" and "sequential" (linked)
> lists.
> However, 20 years of experience with LinkedList, and with alternative 
> algorithms
> that check the RandomAccess marker interface, have shown that this doesn't 
> work
> very
> well. It would be a bad idea to extend that design by making LinkedHashSet
> implement
> List or for it to provide a List view.

Yes, you right about that, but it does not mean that SequencedCollection will 
solve anything.

> 
> SortedSet is internally ordered and is therefore semantically incompatible 
> with
> the
> external ordering of List. This incompatibility exists regardless of whether
> SortedSet implements List directly or merely provides a List view.

If it's a view, you now that a view keeps the semantics of the original 
collection, this is how a view works, so i agree that a SortedSet can not 
implement List directly, but it do not think it's an issue if SortedSet 
provides a view.

> 
> In object design you can always take a sledgehammer to something and pound on 
> it
> until it kind of looks like what you want. Indeed, you could do that with List
> in
> the way that you suggest, but the result will be confusing to work with and
> burdensome to implement. So, no, I'm not going to do that.

I don't think there is a good solution here,
I'm wary about the fact that you want to fix a 20 year old design bug by trying 
to shoehorn SequencedCollection with the hope that it will fix that design 
error,
my experience is that this kind of refactoring does more harm than good.
And we can not use List because we dpo not want to introduce new sequential 
implementations of List.  

And you did not answer about providing a bidirectional iterator of the fact 
that LinkedHashMap does not implement SequencedMap correctly, as an example
the following code may throw an AssertionError with a SequencedSet created from 
a LinkedHashMap constructed with true as last argument.

  void brokenInvariant(SequencedSet set) {
var first = set.getFirst();
set.contains("foo");
assertEquals(first, set.getFirst());
  }

> 
> s'marks

Rémi

> 
> 
> 
> 
> On 2/13/22 9:40 AM, fo...@univ-mlv.fr wrote:
>> 
>> 
>> 
>> 
>> *From: *"Tagir Valeev" 
>> *To: *"Stuart Marks&qu

Re: RFR: JDK-8281766: Update java.lang.reflect.Parameter to implement Member

[Rémi Forax]

On Tue, 15 Feb 2022 01:13:43 GMT, Joe Darcy  wrote:

> Retrofitting of Parameter to implement Member, an interface already 
> implemented by similar reflective modeling classes.
> 
> Since Parameter is final, there is little compatibility impact from adding a 
> new method.
> 
> Please also review the corresponding CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8281767
> 
> I'll update the copyright year before pushing; I judged this as trivial 
> enough to not require a regression test.

I agree with Mandy and David, a Member is the thingy inside a class, so a 
parameter is no a member, it has no declaring class.

-

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

Re: [External] : Sequenced Collections

[forax]

> From: "Tagir Valeev" 
> To: "Stuart Marks" 
> Cc: "Remi Forax" , "core-libs-dev"
> 
> Sent: Saturday, February 12, 2022 4:24:24 AM
> Subject: Re: [External] : Sequenced Collections

> Wow, I missed that the Sequenced Collections JEP draft was posted!
> Of course, I strongly support this initiative and am happy that my proposal 
> got
> some love and is moving forward. In general, I like the JEP in the way it is. 
> I
> have only two slight concerns:
> 1. I'm not sure that having addition methods (addFirst, addLast, putFirst,
> putLast) is a good idea, as not every mutable implementation may support them.
> While this adds some API unification, it's quite a rare case when this could 
> be
> necessary. I think most real world applications of Sequenced* types would be
> around querying, or maybe draining (so removal operations are ok). Probably it
> would be enough to add addFirst, addLast, putFirst, putLast directly to the
> compatible implementations/subinterfaces like List, LinkedHashSet, and
> LinkedHashMap removing them from the Sequenced* interfaces. In this case,
> SortedSet interface will not be polluted with operations that can never be
> implemented. Well my opinion is not very strong here.

> 2. SequencedCollection name is a little bit too long. I think every extra 
> letter
> adds a hesitation for users to use the type, especially in APIs where it could
> be the most useful. I see the Naming section and must admit that I don't have
> better ideas. Well, maybe just Sequenced would work? Or too vague?

> Speaking of Remi's suggestion, I don't think it's a good idea. Maybe it could 
> be
> if we designed the Collection API from scratch.

?? 
Here is the first sentence of the javadoc for java.util.List "An ordered 
collection (also known as a sequence )." 
And the first paragraph of java.util.RandomAccess "Marker interface used by 
List implementations to indicate that they support fast (generally constant 
time) random access. The primary purpose of this interface is to allow generic 
algorithms to alter their behavior to provide good performance when applied to 
either random or sequential access lists" 

You can find that the actual design, mixing ordered collection and indexed 
collection into one interface named List not great, but you can not say that 
this is not the actual design. 

> But given the current state of Java collections, it's better to add new
> interfaces than to put the new semantics to the java.util.List and greatly
> increase the amount of non-random-accessed lists in the wild.
> There are tons of code that implicitly assume fast random access of every
> incoming list (using indexed iteration inside). The suggested approach could
> become a performance disaster.

If you take several Java developers, some will stick to the javadoc definition, 
a List is either sequential or random access and some will think that a List is 
only random access. Because of that, adding more sequential implementations 
under the List interface is an issue. 

Introducing SequencesCollection (more on the name later), a super interface of 
List solves that issue, the new implementations will only implement the 
sequential part of interface List. 
But it does not solve the other problems, mainly adding 4 interfaces when one 
is enough, not being backward compatible because of inference and the weird 
semantics of LinkedHashMap. 

We still need SortedSet or LinkedHashSet to not directly implement 
SequencesCollection but to use delegation and a have a method returning a view. 
The same reasoning applied to SortedMap, LinkedHashMap. 
By using views, there is no need to the two other proposed interfaces 
SequenceSet and SequenceMap. 

Another question is ListIterator, a list can be iterated forward and backward, 
a SequenceCollection can do almost the same thing, with iterator() and 
reversed().iterator(). It's not exactly the same semantics but i don't think it 
exist an implementation of SequenceCollection that can be implemented without 
the property that given one element, it's predecessor and successor can be 
found in O(1). 
Do we need a new SequenceCollectionIterator that provides the method 
next/hasNext/previous/hasPrevious but not add/set/nextIndex/previousIndex ? 

For the name, Java uses single simple name of one syllable for the important 
interface List, Set, Map or Deque (the javadoc of Deque insist that Deque 
should be pronounced using one syllable). 
So the name should be Seq. 
The main issue with the name "Seq" is that it is perhaps too close to the name 
"Set". 
Also, it can not be "Sequence" because of CharSequence. 

interface Seq extends Collection { 
void addFirst(); 
void addLast(); 
E getFirst(); 
E getLast(); 
E removeFirst(); 
E removeLast(); 
Seq reversed(); 
} 

interf

Re: [External] : Sequenced Collections

[forax]

- Original Message -
> From: "Stuart Marks" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" 
> Sent: Friday, February 11, 2022 8:25:19 PM
> Subject: Re: [External] : Sequenced Collections

> Hi Rémi,
> 
> I see that you're trying to reduce the number of interfaces introduced by
> unifying
> things around an existing interface, List. Yes, it's true that List is an
> ordered
> collection. However, your analysis conveniently omits other facts about List
> that
> make it unsuitable as a general "ordered collection" interface. Specifically:
> 
> 1) List supports element access by int index; and
> 
> 2) List is externally ordered. That is, its ordering is determined by a
> succession
> of API calls, irrespective of element values. This is in contrast to SortedSet
> et al
> which are internally ordered, in that the ordering is determined by the 
> element
> values.
> 
> The problem with indexed element access is that it creates a bunch of hidden
> performance pitfalls for any data structure where element access is other than
> O(1).
> So get(i) degrades to O(n), binarySearch degrades from O(log n) to O(n). (This
> is in
> the sequential implementation; the random access implementation degrades to 
> O(n
> log
> n)). Apparently innocuous indexed for-loops degrade to quadratic. This is one 
> of
> the
> reasons why LinkedList is a bad List implementation.
> 
> If we refactor LinkedHashSet to implement List, we basically have created
> another
> situation just like LinkedList. That's a step in the wrong direction.


Everybody is focused on LinkedList but that's not the problem, the problem is 
that, unlike every other languages, in Java, doing an indexed loop on a List is 
a bad idea.
There are plenty of other implementations of AbstractSequentialList, other than 
LinkedList, that exhibits the same bad performance when used in an indexed loop.
A simple search on github find a lot of classes that implements 
AbstractSequentialList
  
https://github.com/search?l=Java=6=%22extends+AbstractSequentialList%22+-luni+-LinkedList=Code

In fact, the problem of performance is not something inherent to the List API, 
it's true for any interface of the collection API,
By example, AbstractSet.contains() is linear, 
CopyOnWriteArrayList/CopyOnWriteArraySet.add() is linear etc.

The whole idea of using interfaces in the collection API is at the same time
- great because you have more interropt
- awful because you have usually no idea of the complexity of the method you 
call.

BTW, if we want to be serious and solve the issue of indexed Loop on List,
we should not have stop half way with the enhanced for loop, and also provides 
a way to get an index for an element when looping
And obviously, there is also a need for a compiler warning to explain that 
doing an indexed loop on a List is bad.

> 
> Turning to internal ordering (SortedSet): it's fundamentally incompatible with
> List's external ordering. List has a lot of positional mutation operations 
> such
> as
> add(i, obj); after this call, you expect obj to appear at position i. That 
> can't
> work with a SortedSet.

I don't propose that SortedSet implements List, i propose to add a new method 
asList() to SortedSet that return a view of the sorted set as a List.
Obviously, like the Set returned by Map.keySet() does not implement all the 
methods of Set, calling add() throws an UnsupportedOperationException,
the method List.add(int,E) will throw an UnsupportedOperationException when 
called on the result of SortedSet.asList()

> 
> There is implicit positioning semantics in other methods that don't have index
> arguments. For example, replaceAll replaces each element of a List with the
> result
> of calling a function on that element. Crucially, the function result goes 
> into
> the
> same location as the original element. That to cannot work with SortedSet.

Not with SortedSet, like above, replace() is an optional method, it will not be 
implemented by the List returned by SortedSet.asList(),
but LinkedHashSet.asList() may return a List that implements replace(). 

> 
> Well, we can try to deal with these issues somehow, like making certain 
> methods
> throw UnsupportedOperationException, or by relaxing the semantics of the 
> methods
> so
> that they no longer have the same element positioning semantics. Either of 
> these
> approaches contorts the List interface to such an extent that it's no longer a
> List.

As said above, it's a list view, like by example the List returned by 
Arrays.asList() does not implement add(E)/add(int,E) or remove(int)/remove(E),
the view of the List does not have to implement the methods that do a mutation 
using an index.

> 
> So, no, it's not useful or effective to try to make List be the com

Sequenced Collections

[Remi Forax]

I've read the draft of the JEP on sequenced collection, and i think the 
proposed design can be improved.
  https://bugs.openjdk.java.net/browse/JDK-8280836

I agree with the motivation, there is a need for an API to consider the element 
of a list, a sorted set and a linked hash set as an ordered sequence of 
elements with a simple way to access/add/remove the first/last element and also 
reverse the elements as view.

I disagree about the conclusion that we need to introduce 4 new interfaces for 
that matter.

Here are the reasons
1/ Usually an ordered collection is called a list. Introducing an interface 
SequencedCollection for something which is usually called a list will cause 
more harm than good. Or maybe we should rename LISP to SEQP :)

2/ There is already an interface List in Java, that represents an ordered 
sequence of elements, with LinkedList being the name of the the double linked 
list implementation. You can argue that there is a slight difference between 
the semantics of java.util.List and the proposed syntax of 
java.util.SequencedCollection, but given that people already have difficulties 
to understand basic data structure concepts, as a teacher i dread to have a 
discussion on those slight differences that are only true in Java.

If the collection API was not already existing, we may discuss about having the 
same interface java.util.List to both indexed collection and ordered 
collection, but that boat has sailed a long time ago.

So in first approach, we should refactor sorted set and linked hash set to 
directly implement java.util.List and all the proposed methods into 
java.util.List. But as you hint in the Risks and Assumptions section, this will 
cause regression due to inference and also we will have trouble with 
LinkedHashMap (see below).

3/ LinkedHashMap mixes 3 implementations in one class, some of these 
implementations does not conform to the semantics of SequencedMap.
- You can opt-out having the key sequentially ordered as defined by 
SequencedMap by using the constructor LinkedHashMap(int initialCapacity, float 
loadFactor, boolean accessOrder) and passing true as last parameter. 
- You can opt-out having the key sequentially ordered as defined by 
SequencedMap by overriding removeEldestEntry(), removing the first entry at the 
same time you add a new one.

Because all these reasons, i think we should move to another design, using 
delegation instead of inheritance, which for the collection framework means 
exposing new way to access/modify sorted set and linked hash set through 
java.util.List views.

The concept of views is not a new concept, it's used in Arrays.asList(), 
List.subList() or Map.keySet()/values()/entrySet() (and more). The idea is not 
that a sorted set is a list but that it provides a method to see it as a list. 
It solves our problem of compatibility by not adding super types to existing 
type and also the problem of the semantics of LinkedHashMap because a view 
keeps the semantics of the data structure it originated.

Here is the proposed new methods in List, SortedSet and SortedMap.

interface List extends Collection {
  // new methods
  void addFirst();
  void addLast();
  E getFirst();
  E getLast();
  E removeFirst();
  E removeLast();
  List reversedList(); // or descendingList() ??
}

interface SortedSet implements Set {
  // new methods
  List asList(); 
}

interface SortedMap implements Map {
  // new methods
  List keyList();  // do not use covariant return type
  List> entryList();  // same
}

I believe this design is objectively better than the one proposed because as a 
user being able to use new interfaces is a slow process, the 
libraries/dependencies must be updated to take the new interfaces as parameter 
before the new types can be used. By contrast, the proposed design only enhance 
existing interfaces so people will enjoy the new methods directly when 
introduced.

Rémi

Re: RFR: JDK-8280168: Add Objects.toIdentityString [v7]

[Rémi Forax]

On Mon, 24 Jan 2022 21:31:37 GMT, Joe Darcy  wrote:

>> While it is strongly recommend to not use the default toString for a class, 
>> at times it is the least-bad alternative. When that alternative needs to be 
>> used, it would be helpful to have the implementation already available, such 
>> as in Objects.toDefaultString(). This method is analagous to 
>> System.identityHashCode.
>> 
>> Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8280184
>
> 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 eight additional commits since 
> the last revision:
> 
>  - Respond to review feedback.
>  - Merge branch 'master' into JDK-8280168
>  - Merge branch 'master' into JDK-8280168
>  - Appease jcheck.
>  - Add toIdentityString
>  - Respond to review feedback to augment test.
>  - Respond to review feedback.
>  - JDK-8280168 Add Objects.toDefaultString

toIdentityString is a better name than toDefaultString.

It's fine for me but given that "identity" has a slightly different meaning in 
the context of Valhalla that in System.identityHashCode(), it may be good to 
ask Brian about that name.

-

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

Re: Thread.dispatchUncaughtException possible NPE?

[Remi Forax]

- Original Message -
> From: "Andrey Turbanov" 
> To: "core-libs-dev" 
> Sent: Monday, January 17, 2022 10:37:04 AM
> Subject: Thread.dispatchUncaughtException possible NPE?

> Hello.

Hello Andrey,

> I see that Thread.dispatchUncaughtException calls
> getUncaughtExceptionHandler() which reads volatile field twice:
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Thread.java#L1978
> 
> private volatile UncaughtExceptionHandler uncaughtExceptionHandler;
> 
> public UncaughtExceptionHandler getUncaughtExceptionHandler() {
>return uncaughtExceptionHandler != null ?
>uncaughtExceptionHandler : group;
> }
> 
> private void dispatchUncaughtException(Throwable e) {
>getUncaughtExceptionHandler().uncaughtException(this, e);
> }
> 
> 
> I wonder if it's possible to get a NPE here? Another thread could
> change uncaughtExceptionHandler between 2 volatile reads.
> Or JVM somehow forbid this?

yes, it's a bug, the field should be read only once.

> 
> 
> 
> 
> Andrey Turbanov

Rémi

Re: LambdaMetafactory requires full privilege access, but doesn't seem to actually restrict functionality

[Remi Forax]

- Original Message -
> From: "Steven Schlansker" 
> To: "core-libs-dev" 
> Sent: Wednesday, January 12, 2022 9:56:30 PM
> Subject: LambdaMetafactory requires full privilege access, but doesn't seem 
> to actually restrict functionality

> Hi core-libs-dev,
> 
> I am maintaining a module for the popular Jackson JSON library that attempts 
> to
> simplify code-generation code without losing performance.
> Long ago, it was a huge win to code-generate custom getter / setter / field
> accessors rather than use core reflection. Now, the gap is closing a lot with
> MethodHandles, but there still seems to be some benefit.
> 
> The previous approach used for code generation relied on the CGLib + ASM
> libraries, which as I am sure you know leads to horrible-to-maintain code 
> since
> you essentially write bytecode directly.

I don't see the issue here, writing bytecodes in not that hard :)

> Feature development basically stopped because writing out long chains of
> `visitVarInsn(ASTORE, 3)` and the like scares off most contributors, myself
> included.

yes, sadly known issue, generating assembly code even a highlevel one like the 
Java byetcode by hands require a niche knowledge which sadly is rarely teached 
at university anymore (and let's not talked about bootcamp).  

> 
> As an experiment, I started to port the custom class generation logic to use
> LambdaMetafactory. The idea is to use the factory to generate `Function T>` getter and `BiConsumer` setter implementations.

If you want to make the serailization/deserialization to JSON fast you will 
mostly battle two things,
 - polymorphic call site, a call that can call some many different 
implementations that the VM will not try to inline,
   usually the trick is to just have one of such call to take care of the 
different kind of objects.
 - boxing, that kind of code tends to abstract over any values, but boxing 
means allocation if there is no inlining.  

For me, it means that you want two specialized codes, the decoder that switches 
over the keys and call the correct setter and the encoder that calls the 
getters in sequence.

Trying to abstract over getters and setters is a step too far because you loose 
the benefit to write a code specific to a peculiar class.

I suppose you try to use the method handles directly with invokeExact() if you 
are able to remove the boxing
or with invoke() if not ?

Because it's not clear to me why you want to use the LambdaMetafactory ?

[...]

> 
> I'm also curious for any feedback on the overall approach of using the
> Metafactory, perhaps I am way off in the weeds, and should just trust
> MethodHandles to perform well if you use invokeExact :) JMH does seem to show
> some benefit though especially with Graal compiler.
> 
> Thanks a bunch for any thoughts,
> Steven Schlansker

Rémi

Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger [v3]

[Remi Forax]

- Original Message -
> From: "Bernd Eckenfels" 
> To: "core-libs-dev" 
> Sent: Jeudi 18 Novembre 2021 12:07:19
> Subject: Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger 
> [v3]

> What about a new API multiply method which takes an forkjoinpool, and only if
> that is used/specified it will use the parallel mode (and only if Notsitzes
> threshold applies?). Most of the pool tuning can then done with this argument.
> It also avoids surprise threads.

You don't need it, here is the usual trick if you want to specify a specific 
fork join pool
https://stackoverflow.com/questions/21163108/custom-thread-pool-in-java-8-parallel-stream

> 
> Gruss
> Bernd

regards,
Rémi

> --
> http://bernd.eckenfels.net
> 
> Von: core-libs-dev  im Auftrag von kabutz
> 
> Gesendet: Thursday, November 18, 2021 11:41:40 AM
> An: core-libs-dev@openjdk.java.net 
> Betreff: Re: RFR: JDK-8277175 : Add a parallel multiply method to BigInteger
> [v3]
> 
> On Thu, 18 Nov 2021 07:26:45 GMT, David Holmes  wrote:
> 
>> To add my 2c IMO a parallel version of this type absolutely **must** be 
>> opt-in.
>> There are simply far too many side-effects of using the FJP and multiple
>> threads to perform the calculation in parallel as if it is just a minor
>> implementation detail. A clear API is 1000x better than a "kill switch".
>>
>> And yes you may still need to expose some kind of tuning knob.
>>
>> David
> 
> Yes, it **must** be opt-in. However I'm not sure that a tuning knob will be
> necessary. BigInteger has thresholds for using different multiply algorithms
> and these are also not configurable.
> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/6409

Re: RFR: 8261847: performace of java.lang.Record::toString should be improved

[Rémi Forax]

On Tue, 16 Nov 2021 13:03:35 GMT, Jim Laskey  wrote:

>> (I'm not reviewer.)
>> 
>> I think `.toArray(Class[]::new)` should be better here. `.toList` seems 
>> unnecessary.
>
> Class[] types = Stream.of(getters)
> .map(g -> g.type().returnType())
> .toArray(Class[]::new);

or do not use a stream but a simple loop, codes in bootstrap methods should be 
low-level

-

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

Re: RFR: 8274412: Add a method to Stream API to consume and close the stream without using try-with-resources

[Remi Forax]

- Original Message -
> From: "Glavo" 
> To: "Tagir F.Valeev" 
> Cc: "core-libs-dev" 
> Sent: Samedi 16 Octobre 2021 06:25:40
> Subject: Re: RFR: 8274412: Add a method to Stream API to consume and close 
> the stream without using try-with-resources

> I don't think it is a perfect solution to combine the close operation
> with the terminator operation, because there are similar issues
> in the intermediate operation.
> 
> Consider this use case (for example only):
> 
> try (var stream = Files.list(path)
>.flatMap(dir -> {
>try {
>return Files.list(dir);
>} catch (IOException e) {
>return Stream.empty();
>}
>})) {
>// ...
> }
> 
> It looks closed, but it doesn't. Closing the Stream generated by flatMap
> will not close all member Stream. 

The stream is closed, from the javadoc
"Each mapped stream is closed after its contents have been placed into this 
stream."


> Further consideration is needed to
> deal with the problem of closing the Stream. Perhaps it is a withCleaner
> method that registers the Stream with the cleaner, or other solutions.
> 
> 

regards,
Rémi

> 
> 
> Tagir F.Valeev  于2021年10月4日周一 下午2:52写道：
> 
>> Currently, when the stream holds a resource, it's necessary to wrap it
>> with try-with-resources. This undermines the compact and fluent style of
>> stream API calls. For example, if we want to get the `List` of files inside
>> the directory and timely close the underlying filehandle, we should use
>> something like this:
>>
>>
>> List paths;
>> try (Stream stream = Files.list(Path.of("/etc"))) {
>> paths = stream.toList();
>> }
>> // use paths
>>
>>
>> I suggest to add a new default method to Stream interface named
>> `consumeAndClose`, which allows performing terminal stream operation and
>> closing the stream at the same time. It may look like this:
>>
>>
>> default  R consumeAndClose(Function, ? extends R>
>> function) {
>> Objects.requireNonNull(function);
>> try(this) {
>> return function.apply(this);
>> }
>> }
>>
>>
>> Now, it will be possible to get the list of the files in the fluent manner:
>>
>>
>> List list =
>> Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
>>
>> -
>>
>> Commit messages:
>>  - Fix tests
>>  - 8274412: Add a method to Stream API to consume and close the stream
>> without using try-with-resources
>>
>> Changes: https://git.openjdk.java.net/jdk/pull/5796/files
>>  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5796=00
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8274412
>>   Stats: 140 lines in 5 files changed: 135 ins; 0 del; 5 mod
>>   Patch: https://git.openjdk.java.net/jdk/pull/5796.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jdk
>> pull/5796/head:pull/5796
>>
>> PR: https://git.openjdk.java.net/jdk/pull/5796

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

[Rémi Forax]

On Thu, 14 Oct 2021 00:54:57 GMT, Mandy Chung  wrote:

>> src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
>>  line 151:
>> 
>>> 149: var setter = isReadOnly ? null : 
>>> JLIA.unreflectField(field, true);
>>> 150: Class type = field.getType();
>>> 151: if (type == Boolean.TYPE) {
>> 
>> dumb question: any reason why `boolean.class` (which is compiled to a LDC) 
>> is not used?
>
> I only see `boolean.class` compiled to `getstatic Boolean.TYPE`.   Is there a 
> javac flag to compile it to a LDC?

The LDC bytecode instruction for a class takes a class name not a descriptor as 
parameter, so there is no way to encode LDC Z. Valhalla may change that.

-

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

Re: RFR: 8274412: Add a method to Stream API to consume and close the stream without using try-with-resources

[Remi Forax]

I agree with the idea of a try() syntax, but i don't think we need more 
interfaces.

Yes, John is right about the fact that the TWR Aucloseable does not work well 
with checked exceptions,
but the issue is more that there is nothing that works well with checked 
exceptions because Java has no way to compose them correctly
(remainder: we should remove the idea of checked exceptions from the language 
given that it's a backward compatible change and Kotlin, C# are safer than Java 
because users of those languages are not used to write a 
try/catch/printStackTrace in those languages unlike in Java).

So for me, AutoCloseable is enough.

The problem is more than a fluent API are expression oriented and a 
try-with-resources is statement oriented, hence the mismatch.

Like we have introduced the switch expression, here we need a 
try-with-resources expression.

There are good reasons to have a try-with-resources expression
1) it can be used to track a fluent chain that should close() the resources
2) the compiler can guarantee that the stream/reader/etc around the resources 
does not leak outside.

In term of syntax, i don't think that the arrow syntax (the one used for a case 
of a switch case or a lambda) should be used here, because using a block 
expression seems to be always a mistake.

I like the proposal from John, a try(expression) or a try expression (without 
the parenthesis).

regards,
Rémi


- Original Message -
> From: "John Rose" 
> To: "Paul Sandoz" , "Brian Goetz" 
> , "Tagir F.Valeev"
> 
> Cc: "core-libs-dev" 
> Sent: Lundi 11 Octobre 2021 20:42:20
> Subject: Re: RFR: 8274412: Add a method to Stream API to consume and close 
> the stream without using try-with-resources

> So the purpose of TWR is to hold an object with a “close-debt”
> (debt of a future call to close) and pay it at the end of a block,
> sort of like C++ RAII (but also sort of not).
> 
> But fluent syntaxes (which I like very much and hope to see
> more of in the future!) don’t play well with blocks, so if a
> fluent chain (any part of that chain:  It’s multiple objects)
> incurs a “close-debt”, it’s hard to jam a TWR block into it.
> 
> Hence the current proposal.  I agree with Brian and Paul
> that we haven’t examined all the corners of this problem
> yet.  And I’d like to poke at the concept of “close-debt” to
> help with the examination.
> 
> Just for brain storming, I think we could model “close-debt”
> outside either fluent API structure or TWR block structure.
> Java O-O APIs are the pre-eminent way to model things in
> Java, and they work exceedingly well, when used with skill.
> 
> AutoCloseable models close-debt of course.  But it has two
> weaknesses:  It doesn’t model anything *other* than the
> debt, and its (sole) method skates awkwardly around the
> issue of checked exceptions.  (It requires an override with
> exception type narrowing to be used in polite company.)
> AC is more of an integration hook with TWR, rather than
> a general-purpose model for close-debt.  Therefore it doesn’t
> teach us much about close-debt in a fluent setting.
> 
> Surely we can model close-debt better.  Let’s say that an
> operation (expression) with close-debt *also* has a return
> value and (for grins) *also* has an exception it might throw.
> This gets us to an API closer to Optional.  (If you hear the
> noise of a monad snuffling around in the dark outside
> your tent, you are not wrong.)
> 
> interface MustClose_1 {
>   T get() throws X;  //or could throw some Y or nothing at all
>   void close() throws X;
> }
> 
> (I wish we had an easier way to associate such an X
> with such a T, so that Stream could be more
> interoperable with simple Stream.  It’s a pain to
> carry around those X arguments.  So I’ll drop X now.)
> 
> interface MustClose_2 {
>   T get();
>   void close() throws Exception;
> }
> 
> An expression of this type requires (in general) two
> operations to finish up:  It must be closed, and the result
> (type T) must be gotten.  There’s an issue of coupling between
> the two methods; I would say, decouple their order, so that
> the “get” call, as with Optional, can be done any time,
> and the “close” call can be done in any order relative
> to “get”.  Both calls should be idempotent, I think.
> But that’s all second-order detail.
> 
> A first-order detail is the apparent but incorrect 1-1 relation
> between T values and close-debts.  That’s very wrong;
> a closable stream on 1,000 values has one close-debt,
> not 1,000.  So maybe we need:
> 
> interface MustClose_3 {
>   S map(Function value);
>   void close() throws Exception;
> }
> 
> That “map” method looks a little like Remi’s apply
> method.  Did I mention this design requires skill
> (as well as flexibility, with one hand already tied
> by checked exceptions)?  I’m at the edge of my own
> skill here, but I think there’s good ground to explore
> here.
> 
> In a fluent setting, a Stream that incurs a close-debt
> might be typed (after incurring the 

Re: RFR: 8274412: Add a method to Stream API to consume and close the stream without using try-with-resources

[Remi Forax]

- Original Message -
> From: "Tagir F.Valeev" 
> To: "core-libs-dev" 
> Sent: Lundi 4 Octobre 2021 08:51:55
> Subject: RFR: 8274412: Add a method to Stream API to consume and close the 
> stream without using try-with-resources

> Currently, when the stream holds a resource, it's necessary to wrap it with
> try-with-resources. This undermines the compact and fluent style of stream API
> calls. For example, if we want to get the `List` of files inside the directory
> and timely close the underlying filehandle, we should use something like this:
> 
> 
> List paths;
> try (Stream stream = Files.list(Path.of("/etc"))) {
>paths = stream.toList();
> }
> // use paths
> 
> 
> I suggest to add a new default method to Stream interface named
> `consumeAndClose`, which allows performing terminal stream operation and
> closing the stream at the same time. It may look like this:
> 
> 
>default  R consumeAndClose(Function, ? extends R> 
> function)
>{
>Objects.requireNonNull(function);
>try(this) {
>return function.apply(this);
>}
>}
> 
> 
> Now, it will be possible to get the list of the files in the fluent manner:
> 
> 
> List list = Files.list(Path.of("/etc")).applyAndClose(Stream::toList);


I would prefer the method to be called applyAndClose() because it is what the 
method does, it applies the function and closes the stream.

There are two missing information in the javadoc
- the function taken as parameter should not return a stream, because the 
stream will be closed
  This is not okay
List list = Files.list(Path.of("/etc")).applyAndClose(s -> 
s).toList();


- if there are intermediary operations, they have to be done in the function 
taken as parameter and not before calling applyAndClose()
  This is okay
List list = Files.list(Path.of("/etc")).applyAndClose(s -> 
s.map(path -> Integer.parseInt(path.toString())).toList());

  This is not okay
List list = Files.list(Path.of("/etc")).map(path -> 
Integer.parseInt(path.toString())).applyAndClose(Stream::toList);


In both case, IDEs can help, but i think it should be written explicitly in the 
javadoc.


> 
> -
> 
> Commit messages:
> - Fix tests
> - 8274412: Add a method to Stream API to consume and close the stream without
> using try-with-resources
> 
> Changes: https://git.openjdk.java.net/jdk/pull/5796/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5796=00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8274412
>  Stats: 140 lines in 5 files changed: 135 ins; 0 del; 5 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/5796.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5796/head:pull/5796
> 
> PR: https://git.openjdk.java.net/jdk/pull/5796

Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable [v4]

[Rémi Forax]

On Mon, 4 Oct 2021 15:49:43 GMT, Peter Levart  wrote:

>> This patch improves reflective access speed as shown by the included 
>> benchmarks:
>> 
>> https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a
>> 
>> ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection 
>> with Method Handle) perform better in some circumstances.
>
> Peter Levart has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   More @stable fields in Constructor/Field to align them with Method fields

I miss that, that very cool.

-

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

Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable [v4]

[Rémi Forax]

On Mon, 4 Oct 2021 15:49:43 GMT, Peter Levart  wrote:

>> This patch improves reflective access speed as shown by the included 
>> benchmarks:
>> 
>> https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a
>> 
>> ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection 
>> with Method Handle) perform better in some circumstances.
>
> Peter Levart has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   More @stable fields in Constructor/Field to align them with Method fields

Can you surreptitiously add "java.lang" too (asking for a friend)

-

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

Re: Discussion: easier Stream closing

[Remi Forax]

At that point, the real question is why not call close() at the end of all 
terminal operations by wrapping each implementation in a try-with-resources ?

Rémi

- Original Message -
> From: "Brian Goetz" 
> To: "Tagir Valeev" , "core-libs-dev" 
> 
> Sent: Lundi 27 Septembre 2021 22:41:00
> Subject: Re: Discussion: easier Stream closing

> In Java 8, I think we were reluctant to lean on the idiom of "pass me a
> lambda and I'll pass it the confined data"), because Java developers
> were already struggling to understand lambdas.  But now that we're
> mostly over that hurdle, API points that accept Consumer are
> a powerful way to gain confinement (as we've seen in StackWalker, and
> also in the explorations of ScopeLocal in Loom.)  So I have no objection
> to this idiom.
> 
> I have some concern that putting these side-by-side with existing
> Files.walk(...) methods will be a source of confusion, creating two
> different ways to do the same thing.
> 
> I would rather not add anything new to BaseStream; it was a mistake to
> expose at all, and I'd rather see it deprecated than add more to it.
> However, adding it individually to the Stream implementations is
> equivalent, and doesn't have this problem.
> 
> The consumeAndClose approach is clever, in that it adds one API point
> that works for all streams, rather than having to add a new API point
> for every factory of a closeable stream; on the other hand, it is
> dramatically less discoverable, and so requires more education to get
> people to use it (and, as you say, has the exception problem.)
> 
> On 9/26/2021 5:27 AM, Tagir Valeev wrote:
>> Hello!
>>
>> With current NIO file API, a very simple problem to get the list of
>> all files in the directory requires some ceremony:
>>
>> List paths;
>> try (Stream stream = Files.list(Path.of("/etc"))) {
>>  paths = stream.toList();
>> }
>>
>> If we skip try-with-resources, we may experience OS file handles leak,
>> so it's desired to keep it. Yet, it requires doing boring stuff. In
>> this sense, classic new File("/etc").list() was somewhat more
>> convenient (despite its awful error handling).
>>
>> I like how this problem is solved in StackWalker API [1]: it limits
>> the lifetime of the Stream by requiring a user-specified function to
>> consume it. After the function is applied, the stream is closed
>> automatically. We could add a similar overload to the Files API. As an
>> additional feature, we could also translate all UncheckedIOException's
>> back to IOException for more uniform exception processing:
>>
>> /**
>>   * @param dir  The path to the directory
>>   * @param function  function to apply to the stream of directory files
>>   * @param  type of the result
>>   * @return result of the function
>>   * @throws IOException if an I/O error occurs when opening the directory, or
>>   * UncheckedIOException is thrown by the supplied function.
>>   */
>> public static  T list(Path dir, Function, ?
>> extends T> function) throws IOException {
>>  try (Stream stream = Files.list(dir)) {
>>  return function.apply(stream);
>>  }
>>  catch (UncheckedIOException exception) {
>>  throw exception.getCause();
>>  }
>> }
>>
>> In this case, getting the List of all files in the directory will be
>> as simple as
>>
>> List paths = Files.list(Path.of("/etc"), Stream::toList);
>> This doesn't limit the flexibility. For example, if we need only file
>> names instead of full paths, we can do this:
>> List paths = Files.list(Path.of("/etc"), s ->
>> s.map(Path::getFileName).toList());
>>
>> Alternatively, we could enhance the BaseStream interface in a similar
>> way. It won't allow us to translate exceptions, but could be useful
>> for every stream that must be closed after consumption:
>>
>> // in BaseStream:
>>
>> /**
>>   * Apply a given function to this stream, then close the stream.
>>   * No further operation on the stream will be possible after that.
>>   *
>>   * @param function function to apply
>>   * @param  type of the function result
>>   * @return result of the function
>>   * @see #close()
>>   */
>> default  R consumeAndClose(Function function) {
>>  try(@SuppressWarnings("unchecked") S s = (S) this) {
>>  return function.apply(s);
>>  }
>> }
>>
>> The usage would be a little bit longer but still more pleasant than
>> explicit try-with-resources:
>>
>> List list = 
>> Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
>>
>> On the other hand, in this case, we are free to put intermediate
>> operations outside of consumeAndClose, so we won't need to nest
>> everything inside the function. Only terminal operation should be
>> placed inside the consumeAndClose. E.g., if we need file names only,
>> like above, we can do this:
>>
>> List list =
>> Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
>>
>> What do you think?
>>
>>
>> [1]
> > 

Re: Discussion: easier Stream closing

[Remi Forax]

- Original Message -
> From: "Tagir Valeev" 
> To: "core-libs-dev" 
> Sent: Dimanche 26 Septembre 2021 11:27:58
> Subject: Discussion: easier Stream closing

> Hello!
> 
> With current NIO file API, a very simple problem to get the list of
> all files in the directory requires some ceremony:
> 
> List paths;
> try (Stream stream = Files.list(Path.of("/etc"))) {
>paths = stream.toList();
> }
> 
> If we skip try-with-resources, we may experience OS file handles leak,
> so it's desired to keep it. Yet, it requires doing boring stuff. In
> this sense, classic new File("/etc").list() was somewhat more
> convenient (despite its awful error handling).
> 
> I like how this problem is solved in StackWalker API [1]: it limits
> the lifetime of the Stream by requiring a user-specified function to
> consume it. After the function is applied, the stream is closed
> automatically. We could add a similar overload to the Files API. As an
> additional feature, we could also translate all UncheckedIOException's
> back to IOException for more uniform exception processing:
> 
> /**
> * @param dir  The path to the directory
> * @param function  function to apply to the stream of directory files
> * @param  type of the result
> * @return result of the function
> * @throws IOException if an I/O error occurs when opening the directory, or
> * UncheckedIOException is thrown by the supplied function.
> */
> public static  T list(Path dir, Function, ?
> extends T> function) throws IOException {
>try (Stream stream = Files.list(dir)) {
>return function.apply(stream);
>}
>catch (UncheckedIOException exception) {
>throw exception.getCause();
>}
> }
> 
> In this case, getting the List of all files in the directory will be
> as simple as
> 
> List paths = Files.list(Path.of("/etc"), Stream::toList);
> This doesn't limit the flexibility. For example, if we need only file
> names instead of full paths, we can do this:
> List paths = Files.list(Path.of("/etc"), s ->
> s.map(Path::getFileName).toList());


It seems a nice addition, forgetting the try-with-resources is a very common 
source of bugs in my students code.
Part of the issue is that there is no way to easily test if there are still 
open descriptors during tests.
I wonder if the VM can not offer such test using a special flag 
(-XX:+CheckPendingFileDescriptor) or something like that.

> 
> Alternatively, we could enhance the BaseStream interface in a similar
> way. It won't allow us to translate exceptions, but could be useful
> for every stream that must be closed after consumption:
> 
> // in BaseStream:
> 
> /**
> * Apply a given function to this stream, then close the stream.
> * No further operation on the stream will be possible after that.
> *
> * @param function function to apply
> * @param  type of the function result
> * @return result of the function
> * @see #close()
> */
> default  R consumeAndClose(Function function) {
>try(@SuppressWarnings("unchecked") S s = (S) this) {
>return function.apply(s);
>}
> }
> 
> The usage would be a little bit longer but still more pleasant than
> explicit try-with-resources:
> 
> List list = Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
> 
> On the other hand, in this case, we are free to put intermediate
> operations outside of consumeAndClose, so we won't need to nest
> everything inside the function. Only terminal operation should be
> placed inside the consumeAndClose. E.g., if we need file names only,
> like above, we can do this:
> 
> List list =
> Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
> 
> What do you think?

This one does not work because if Path::getFileName fails with an exception, 
close() will not be called.
What you are proposing here is equivalent to
  try(var stream = Files.list(Path.of("/etc")).map(Path::getFileName)) {
stream.toList()
  }

instead of
  try(var stream = Files.list(Path.of("/etc"))) {
stream..map(Path::getFileName).toList()
  }

regards,
Rémi

> 
> 
> [1]
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#walk(java.util.function.Function)

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

[Remi Forax]

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

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

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

> 
> Thanks,
> David

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

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

Re: Implementing MethodHandleProxies#asInterfaceInstance with hidden classes

[Remi Forax]

- Original Message -
> From: "-" 
> To: "Brian Goetz" , "core-libs-dev" 
> 
> Sent: Lundi 23 Août 2021 08:34:17
> Subject: Re: Implementing MethodHandleProxies#asInterfaceInstance with hidden 
> classes

> Thanks for the quick reply!
> 
> The main drawback, API wise, with LMF is that it does not accept
> non-direct method handles [1], while the MHP is able accept any method
> handle, such as native ones from JEP 389 [2]. LMF also lacks the
> ability to restore a method handle from a wrapper instance. Using LMF
> also requires user discretion on which methods to implement, and the
> lookup's required privileges may change too. In general, MHP would be
> more runtime-oriented and transient compared to LMF.
> 
> I am inclined toward a separate bytecode generation implementation for
> the improved MHP asInterfaceInstance, somewhat similar to what's in
> JEP 416's implementation [3]. The extra bridges for all compatible
> methods, accessors to backing method handle/interface, potential to
> expand to abstract classes, different hidden class modes would reduce
> the commonalities between the two implementations and make
> refactorings on LMF to support these MHP functionalities costly.

In my opinion, what is missing is a java.lang.invoke.Proxy, the equivalent of 
java.lang.reflect.Proxy but using defineHiddenClass + a bootstrap method + 
method handles instead of the InvocationHandler + j.l.r.Method. With that, 
implementing java.lang.invoke.MethodHandleProxies on top of of 
java.lang.invoke.Proxy is just a matter of wiring.

As you said, there is a need for a more dynamic version of the 
LambdaMetafactory.
I believe it should work with any interfaces not only functional interfaces.
This would avoid statically typed langages like Kotlin or Scala, or injection 
frameworks like Weld, Spring or Guice to generate a lot of proxy classes and it 
should be straightforward enough so static compilers like graal native image or 
Android d8 can generate the proxy classes at compile time to support "native" 
compilation (the same way lambdas are currently supported).
So we get best of both worlds, efficient dynamic proxies at runtime AND 
supports of "native" compilation.

Rémi

> 
> [1]
> https://github.com/openjdk/jdk/blob/b690f29699180149d33b6a83de10697790587a87/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java#L141
> [2] https://openjdk.java.net/jeps/389#The-C-linker
> [3]
> https://github.com/openjdk/jdk/blob/cff856f9df89293cbc8f2f1e977148cd6ece4f85/src/java.base/share/classes/jdk/internal/reflect/ClassByteBuilder.java
> 
> 
> On Sun, Aug 22, 2021 at 9:26 PM Brian Goetz  wrote:
>>
>> This was an early attempt at the functionality provided by LambdaMetafactory.
>> It could probably be reimplemented on top of that, but probably could be
>> deprecated in favor of LMF as well.
>>
>> Sent from my iPad
>>
>> > On Aug 22, 2021, at 10:08 PM, liangchenb...@gmail.com wrote:
>> >
>> > Currently, java.lang.invoke.MethodHandleProxies#asInterfaceInstance [1] is
>> > implemented with java.lang.reflect.Proxy. After looking at its public API,
>> > including Javadoc, it seems that MethodHandleProxies would benefit from a
>> > hidden class implementation without changing its public API definition
>> > (including Javadocs).
>> >
>> > Recently, there is JEP 416 [2] for reimplementing reflection based on
>> > method handles. This implementation utilizes hidden classes with method
>> > handles passed in classdata and retrieved to condy, which allows generic
>> > method handles (beyond regular constable ones) to be optimized in method
>> > calls. Similarly, for MethodHandleProxies, hidden classes allow the
>> > implementations to detach from classloaders and be freely recyclable; they
>> > can use class data to store the adapted method handles (which can
>> > significantly speed up invocations); and it can allow it to support
>> > implementing single-abstract-method abstract classes in the future (as
>> > discussed in its Javadoc) as well, with only minor tweaks.
>> >
>> > Does this sound feasible? I want to ensure it is a good idea before any
>> > implementation is attempted. If there is any issue with this vision, please
>> > don't hesitate to point it out. Feel free to comment, too! If this looks
>> > good, I hope an issue can be created for it.
>> >
>> > Best
>> >
>> > [1]
>> > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/lang/invoke/MethodHandleProxies.html
> > > [2] https://openjdk.java.net/jeps/416

Re: RFR: 8272137: Make Iterable classes streamable

[Rémi Forax]

On Mon, 9 Aug 2021 12:28:23 GMT, CC007  
wrote:

> create Streamable and ParallelStreamable interface and use them in Collection 
> and Optional

Hi Rick,
I do not think that such interfaces are a good addition to the JDK,
we do not want to promote the fact that you can create a Stream from an 
Iterable because the resulting Stream is not efficient (or is not as efficient 
as it could be) and we can already uses a Supplier if we want to to 
represent a factory of streams

See my comment on the bug for more details

-

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

Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310

[Rémi Forax]

On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev 
 wrote:

> Dear colleagues,
> 
> Please review the patch that replaces the lambdas with anonymous classes 
> which solves the startup time regression as shown below.
> 
> I attached the Bytestacks flamegraphs for both original (regression) and 
> fixed versions. The flamegraphs clearly show the lambdas were causing the 
> performance issue.
> 
> [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip)
> 
> Although the proposed JDK-8270321 patch fixes the startup time (it might 
> appear even better than it was before the regression was introduced, i.e. 
> before JDK-8266310) and generally fixes the footprint regression, it may 
> increase MaxRSS slightly compared to the version before JDK-8266310, which is 
> shown in the below graphs. (updated)
> 
> ![StartupTime2](https://user-images.githubusercontent.com/6394632/126898224-a05fda62-f723-4f2c-9af9-e02cbfe1c9c8.png)
> 
> ![MaxRSS](https://user-images.githubusercontent.com/6394632/126822404-899ab904-efc1-4377-9e0d-d8cdb5c0e5d0.png)
> 
> (update: added ZGC graphs)
> 
> ![StartupTime_ZGC](https://user-images.githubusercontent.com/6394632/126898242-52c09582-c2a4-4623-aad2-f47055277193.png)
> 
> ![MaxRSS_ZGC](https://user-images.githubusercontent.com/6394632/126898244-31c3eeb5-a768-4a52-8960-960cc4a72d7b.png)
> 
> I additionally include the heap objects histograms to show the change does 
> not increase the total live objects size significantly with only 1000 bytes 
> the total difference, namely 1116128 bytes in 25002 live objects after the 
> proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the 
> version with the original patch reverted (i.e. before JDK-8266310).
> 
> [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip)
> 
> The patch was tested w/hotspot/tier1/tier2 test groups.

Hi Sergey,
thanks for the explanation, i don't think there is a need for more data with 
-Xint.

Using anonymous classes instead of lambdas is good for me.

-

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

Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310

[Rémi Forax]

On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev 
 wrote:

> Dear colleagues,
> 
> Please review the patch that replaces the lambdas with anonymous classes 
> which solves the startup time regression as shown below.
> 
> I attached the Bytestacks flamegraphs for both original (regression) and 
> fixed versions. The flamegraphs clearly show the lambdas were causing the 
> performance issue.
> 
> [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip)
> 
> Although the proposed JDK-8270321 patch fixes the startup time (it might 
> appear even better than it was before the regression was introduced, i.e. 
> before JDK-8266310), it increases MaxRSS slightly compared to the version 
> before JDK-8266310, which is shown in the below graphs.
> 
> ![StartupTime](https://user-images.githubusercontent.com/6394632/126822380-5b01ce90-b249-4294-9a62-8269440f942d.png)
> 
> ![MaxRSS](https://user-images.githubusercontent.com/6394632/126822404-899ab904-efc1-4377-9e0d-d8cdb5c0e5d0.png)
> 
> I additionally include the heap objects histograms to show the change does 
> not increase the total live objects size significantly with only 1000 bytes 
> the total difference, namely 1116128 bytes in 25002 live objects after the 
> proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the 
> version with the original patch reverted (i.e. before JDK-8266310).
> 
> [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip)
> 
> The patch was tested w/hotspot/tier1/tier2 test groups.

I don't understand your analysis, you are testing the startup time with -Xint 
which disable JITs, but there is no mention of -Xint in the bug report.
It's obvious to me that there is a regression with -Xint given that the lambda 
creation is using invokedynamic which is only optimizable by JITs. 

I think you should first try to reproduce the issue with the correct flags and 
then follow the advice from Mandy on how to implement the fix. Using an 
anonymous class may introduce more allocation than using a lambda once the code 
is JITed.

-

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

Re: RFR: 6506405: Math.abs(float) is slow

[Rémi Forax]

On Wed, 7 Jul 2021 22:22:45 GMT, Joe Darcy  wrote:

> 
> > Your patch change the semantics, actually
> > `Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE`
> > with your patch
> > `Math.abs(Integer.MIN_VALUE) == Integer.MAX_VALUE`
> 
> Does it? The overriding of int arguments shouldn't be affected.

You are right, it does not change abs(int) so it should be Ok.

> 
> -Joe

-

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

Re: RFR: 6506405: Math.abs(float) is slow

[Rémi Forax]

On Wed, 7 Jul 2021 20:28:37 GMT, Brian Burkhalter  wrote:

> Please consider this change to make the `float` and `double` versions of 
> `java.lang.Math.abs()` branch-free.

Your patch change the semantics, actually
  `Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE`
with your patch
  `Math.abs(Integer.MIN_VALUE) == Integer.MAX_VALUE`

-

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

Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method [v5]

[Remi Forax]

- Original Message -
> From: "Roger Riggs" 
> To: "core-libs-dev" 
> Sent: Mardi 29 Juin 2021 18:44:04
> Subject: Re: [jdk17] RFR: 8269096: Add java.util.Objects.newIdentity method 
> [v5]

> On Tue, 29 Jun 2021 05:47:03 GMT, Tagir F. Valeev  wrote:
> 
>>> Roger Riggs has updated the pull request incrementally with one additional
>>> commit since the last revision:
>>> 
>>>   Add test synchronizing on return value of Objecst.newIdentity()
>>
>> Probably it would be better to have an inner class named like `Identity` 
>> instead
>> of anonymous class? When debugging or analyzing memory dumps, it would be 
>> more
>> user-friendly to see `Objects$Identity` than `Objects$1`.
>> 
>> Probably, not the part of this feature request, but it would be nice to add
>> another method with string parameter, like `Objects.newIdentity("MY
>> SENTINEL")`. The string should be stored in the field and returned from
>> toString(). Again, this would make it easier to find where the object comes
>> from during debugging or memory dump analysis. For the record, here's what we
>> have in IntelliJ IDEA sources (Apache 2.0 licensed):
>> 
>> 
>> public final class ObjectUtils {
>>   private ObjectUtils() { }
>> 
>>   ...
>> 
>>   /**
>>* Creates a new object which could be used as sentinel value (special 
>> value to
>>distinguish from any other object). It does not equal
>>* to any other object. Usually should be assigned to the static final 
>> field.
>>*
>>* @param name an object name, returned from {@link #toString()} to 
>> simplify the
>>debugging or heap dump analysis
>>* (guaranteed to be stored as sentinel object field). If 
>> sentinel is
>>assigned to the static final field,
>>* it's recommended to supply that field name (possibly 
>> qualified
>>with the class name).
>>* @return a new sentinel object
>>*/
>>   public static @NotNull Object sentinel(@NotNull @NonNls String name) {
>> return new Sentinel(name);
>>   }
>> 
>>   private static final class Sentinel {
>> private final String myName;
>> 
>> Sentinel(@NotNull String name) {
>>   myName = name;
>> }
>> 
>> @Override
>> public String toString() {
>>   return myName;
>> }
>>   }
> 
> @amaembo Good suggestion for the Valhalla API and implementation.  Today, only
> the hashCode() identifies an Object.
> For JDK17, it will continue to be a raw Object() instance.

We should try to have a very limited API surface for now.
We want to let the door open to having Object being both an identity class 
(when used with "new") and an abstract class (when inherited) by using two 
different generic specialization of the same class or whatever other scheme we 
come before Valhalla is complete.

> 
> -
> 
> PR: https://git.openjdk.java.net/jdk17/pull/112

Rémi

Release of ASM 9.2

[Remi Forax]

Hi everybody,
we are happy to announce the release of ASM 9.2 which support Java 18 
(available via Maven Central).

I still hope to find the time this summer to work of the support of Valhalla 
new bytecodes.

Rémi

Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]

[Rémi Forax]

On Fri, 18 Jun 2021 09:08:04 GMT, Jan Lahoda  wrote:

>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 222:
>> 
>>> 220:   String invocationName,
>>> 221:   MethodType invocationType,
>>> 222:   Object... labels) throws 
>>> Throwable {
>> 
>> Is it not better to take a Class and a String... as separated parameters 
>> instead of taking Object... and doing the conversion to a Class and an array 
>> of String later in Java
>
> This is to represent cases like:
> 
>  E sel = null;
>  switch (sel) {
>  case A -> {}
>  case E e && "B".equals(e.name()) -> {}
>  case C -> {}
>  case E e -> {}
>  }
> 
> 
> The method needs to know which cases represent constants and which represent 
> patterns (even though the primary type of all the patterns will be the enum 
> type), so we cannot easily put the `Class` first (or elide it), and then a 
> `String...`, unless we represent the patterns in the `String...` array 
> somehow.

Ok got it,
At some point, we will have to represent patterns either as String and parse 
them or as Condy of condy if we want a more structured recursive way.

-

PR: https://git.openjdk.java.net/jdk17/pull/81

Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]

[Rémi Forax]

On Thu, 17 Jun 2021 18:33:56 GMT, Jan Lahoda  wrote:

>> Currently, an enum switch with patterns is desugared in a very non-standard, 
>> and potentially slow, way. It would be better to use the standard 
>> `typeSwitch` bootstrap to classify the enum constants. The bootstrap needs 
>> to accept enum constants as labels in order to allow this. A complication is 
>> that if an enum constant is missing, that is not an incompatible change for 
>> the switch, and the switch should simply work as if the case for the missing 
>> constant didn't exist. So, the proposed solution is to have a new bootstrap 
>> `enumConstant` that converts the enum constant name to the enum constant, 
>> returning `null`, if the constant does not exist. It delegates to 
>> `ConstantBootstraps.enumConstant` to do the actual conversion. And 
>> `typeSwitch` accepts `null`s as padding.
>> 
>> How does this look?
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Creating a new bootstrap method for (pattern matching) enum switches, as 
> suggested.

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 222:

> 220:   String invocationName,
> 221:   MethodType invocationType,
> 222:   Object... labels) throws Throwable 
> {

Is it not better to take a Class and a String... as separated parameters 
instead of taking Object... and doing the conversion to a Class and an array of 
String later in Java

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 238:

> 236: MethodHandle target =
> 237: MethodHandles.insertArguments(DO_ENUM_SWITCH, 2, 
> (Object) labels);
> 238: target = MethodHandles.explicitCastArguments(target, 
> invocationType);

why explicitCast is used here instead of the classical cast asType() ?

-

PR: https://git.openjdk.java.net/jdk17/pull/81

Re: JEP 411: Deprecation with removal would break most existing Java libraries

[Remi Forax]

- Mail original -
> De: "Rafael Winterhalter" 
> À: "core-libs-dev" 
> Envoyé: Dimanche 13 Juin 2021 22:28:33
> Objet: JEP 411: Deprecation with removal would break most existing Java 
> libraries

> I am currently looking into how I should address JEP 411 in my library Byte
> Buddy and I find it rather challenging. The problem I am facing is that I
> know of several users who rely on the security manager in their Java 8/11
> applications. I would like to continue to support those users' use cases as
> long as I support Java versions that contain the security manager, which
> will be for many years to come. At the same time, I would like to address
> the announced removal of the API and make sure that Byte Buddy can work
> without it prior to the deadline when the library in its current state
> would no longer link.

Can you tell us about more about the scenarii where people are relying on the 
uses of doPrivileged() by ByteBuddy ?

> 
> From my understanding of the intention of JEP 411, the API was supposed to
> be stubbed – similar to Android’s stubbing of the API - rather than being
> removed. However, with the announced deprecation for removal of
> AccessController and SecurityManager, I understand that I would need to
> fully remove the dispatching to work with future Java versions.
> 
> Furthermore, it is difficult to create a working facade for dispatching to
> the security manager only if it is available. Methods like
> AccessController.doPrivileged are caller sensitive and by adding a utility
> to a library, this utility would leak to any potential user. It would
> therefore require package-private dispatchers for any relevant package,
> which would lead to a lot of copy-paste to retain backwards compatibility
> (given that a library cannot assume to be run as a module).
> 
> Finally, removing the API would mean that Byte Buddy versions of the last
> ten years would no longer link in future JDKs. For Byte Buddy where new
> Java versions often require an update, that might not be a big issue but
> many other libraries do support the API, I don’t feel it would be a rather
> severe restriction and cause unnecessary breakage if API is removed, rather
> than stubbed. I am thinking of libraries like Netty here which are rather
> omnipresent and would suddenly no longer link, a concept that is unlikely
> intuitive to a lot of developers.

In my opinion, the best way to deal with that is to have two codes one which is 
compatible with an AccessController.doPrivileged and one without it, using a 
multi release jar.
I can see a Maven/Gradle plugin that takes a code that uses an AccessController 
and generate a new code with a higher Java version that remove the calls to 
AccessController,
as a kind of forward port.

But we are not yet there, AccessController has to be removed first.

> 
> Therefore, my question is: should SecurityManager, AccessController and the
> Policy APIs really be deprecated for removal? Rather, I think that the APIs
> should be deprecated, but be retained with stubbed implementations.
> System.getSecurityMananger would then always return null.
> System.setSecurityManager on the other hand could be deprecated for
> removal. This way, existing code could continue to work as if the security
> manager is not active, which already is the common scenario and would not
> cause any disruption at the small price of keeping a handful of some
> stubbed classes.

I guess the real question is when those classes will be removed.
If it's after several LTS releases, nobody will care. If it's tomorrow, that's 
another story.

> 
> Thanks for advice on how this is intended to be handled by library
> developers like me.
> Best regards, Rafael

regards,
Rémi

Re: case null vs case dominance

[forax]

- Mail original -
> De: "Brian Goetz" 
> À: "Remi Forax" , "core-libs-dev" 
> 
> Cc: "amber-spec-experts" 
> Envoyé: Lundi 7 Juin 2021 17:06:20
> Objet: Re: case null vs case dominance

> On 6/7/2021 5:51 AM, Remi Forax wrote:
>> Hi all,
>> the first part of the message is about javac error message that could be
>> improved,
>> the second part is about the current spec being not very logical.
>>
>> With this code
>>
>>  Object o = null;
>>  var value = switch(o) {
>>//case null -> 0;
>>case Object __ -> 0;
>>case null -> 0;
>>  };
>>  System.out.println(value);
>>
>> The error message is
>>PatternMatching101.java:70: error: this case label is dominated by a 
>> preceding
>>case label
>>case null -> 0;
>>^
>>
>> The error message is wrong here, because it's 'case null' and you can put a 
>> case
>> null where you want but below a total pattern, so the error mesage should
>> reflect that.
> 
> But the case null *is* dominated by the total pattern, and therefore dead.

it depends on the order you applies the rules
you can also says that because there is a case null, Object does not accept 
null so it's not a total pattern so it doesn't not dominate null.

I think it's an error but i think there should be a special rule for it.

> 
>> Here is an example that compiles showing that case null can be below a case
>> String, which it dominates
>>
>>  Object o = null;
>>  var value = switch(o) {
>>case String s -> 0;
>>case null -> 0;
>>default -> 0;
>>  };
> 
> In this case, the String pattern is not total, so it does not dominate null.

That's my point, cf above.

Rémi

javac throws an AssertionError while compiling a switch on types

[Remi Forax]

Hi all,
javac does like this code, there is a check missing because javac should not 
reach Gen with a code like this.

Object o = null;
var value = switch(o) {
  default -> 0;
  case String s -> 0;
};
System.out.println(value);

An exception has occurred in the compiler (17-internal). Please file a bug 
against the Java compiler via the Java bug reporting page 
(http://bugreport.java.com) after checking the Bug Database 
(http://bugs.java.com) for duplicates. Include your program, the following 
diagnostic, and the parameters passed to the Java compiler in your report. 
Thank you.
java.lang.AssertionError
at jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
at jdk.compiler/com.sun.tools.javac.util.Assert.check(Assert.java:46)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.handleSwitch(Gen.java:1310)
at 
jdk.compiler/com.sun.tools.javac.jvm.Gen.doHandleSwitchExpression(Gen.java:1238)
at 
jdk.compiler/com.sun.tools.javac.jvm.Gen.visitSwitchExpression(Gen.java:1202)
at 
jdk.compiler/com.sun.tools.javac.tree.JCTree$JCSwitchExpression.accept(JCTree.java:1381)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genExpr(Gen.java:877)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitLetExpr(Gen.java:2376)
at 
jdk.compiler/com.sun.tools.javac.tree.JCTree$LetExpr.accept(JCTree.java:3288)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genExpr(Gen.java:877)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitVarDef(Gen.java:1081)
at 
jdk.compiler/com.sun.tools.javac.tree.JCTree$JCVariableDecl.accept(JCTree.java:1028)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genDef(Gen.java:610)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStat(Gen.java:645)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStat(Gen.java:631)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStats(Gen.java:682)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitBlock(Gen.java:1097)
at 
jdk.compiler/com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1092)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genDef(Gen.java:610)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStat(Gen.java:645)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genMethod(Gen.java:967)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitMethodDef(Gen.java:930)
at 
jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:922)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genDef(Gen.java:610)
at jdk.compiler/com.sun.tools.javac.jvm.Gen.genClass(Gen.java:2415)
at 
jdk.compiler/com.sun.tools.javac.main.JavaCompiler.genCode(JavaCompiler.java:737)
at 
jdk.compiler/com.sun.tools.javac.main.JavaCompiler.generate(JavaCompiler.java:1617)
at 
jdk.compiler/com.sun.tools.javac.main.JavaCompiler.generate(JavaCompiler.java:1585)
at 
jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:946)
at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:317)
at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:64)
at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:50)

regards,
Rémi

case null vs case dominance

[Remi Forax]

Hi all,
the first part of the message is about javac error message that could be 
improved,
the second part is about the current spec being not very logical.

With this code

Object o = null;
var value = switch(o) {
  //case null -> 0;
  case Object __ -> 0;
  case null -> 0;
};
System.out.println(value);

The error message is
  PatternMatching101.java:70: error: this case label is dominated by a 
preceding case label
  case null -> 0;
  ^

The error message is wrong here, because it's 'case null' and you can put a 
case null where you want but below a total pattern, so the error mesage should 
reflect that.

Here is an example that compiles showing that case null can be below a case 
String, which it dominates

Object o = null;
var value = switch(o) {
  case String s -> 0;
  case null -> 0;
  default -> 0;
};

Also with default, the spec says that the code below should compile (and it 
works with javac),
because default and case null are disjoint in term of type, but it feels wrong 
to me.

Object o = null;
var value = switch(o) {
  default -> 0;
  case null -> 0;
};
System.out.println(value);

The problem is that sometimes 'default' acts as a total pattern sometimes it 
does not, if there is a case null. I wonder if it's not better to ask users to 
put the case null on top.

Rémi

Pattern matching, not informative error message when the binding name is missing

[Remi Forax]

Hi all,
with this code

  sealed interface Vehicle {}
  record Car(String owner, String color) implements Vehicle {}
  record Bus(String owner) implements Vehicle {}

  public static void example3() {
var vehicles = List.of(
new Car("Bob", "red"),
new Bus("Ana")
);
var tax = vehicles.stream()
.mapToInt(v -> switch(v) {
case Car -> 100;
case Bus -> 200;
default -> throw new AssertionError();
}).sum();
System.out.println("tax " + tax);
  }

If there is no binding name in the case,
  case Car
instead of
  case Car car
the error message is not very explicit

PatternMatching101.java:40: error: cannot find symbol
 case Car -> 100;
  ^
  symbol:   variable Car
  location: class PatternMatching101

I believe that in this case, it's possible to know that this is a switch on 
types, so provide a better error message.

regards,
Rémi

Pattern matching - exhaustiveness is botched

[Remi Forax]

I don't know if you know but using the latest version of the source,
this code does not compile.

  sealed interface Vehicle {}
  record Car(String owner, String color) implements Vehicle {}
  record Bus(String owner) implements Vehicle {}

  public static void example2() {
var vehicles = List.of(
new Car("Bob", "red"),
new Bus("Ana")
);
for(var vehicle: vehicles) {
  switch(vehicle) {
case Car car -> System.out.println("car !");
case Bus bus -> System.out.println("bus !");
//default -> throw new AssertionError();
  }
}
  }


PatternMatching101.java:25: error: the switch statement does not cover all 
possible input values
  switch(vehicle) {
  ^

same issue with a switch expression.

regards,
Rémi

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v13]

[Rémi Forax]

On Fri, 4 Jun 2021 20:20:26 GMT, Jan Lahoda  wrote:

>> This is a preview of a patch implementing JEP 406: Pattern Matching for 
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>> 
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>> 
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common 
>> superinterface for expressions and patterns, `CaseLabelTree`, which 
>> expressions and patterns implement, and a list of case labels is returned 
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree` 
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent 
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be 
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary 
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed 
>> primitives, as there is no value that is not part of the input domain so 
>> that could be used to represent `case null`. Requires a bit shuffling with 
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It 
>> produces code that delegates to a new bootstrap method, that will classify 
>> the input value to the switch and return the case number, to which the 
>> switch then jumps. To support guards, the switches (and the bootstrap 
>> method) are restartable. The bootstrap method as such is written very simply 
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String 
>> s`/`case null: case String s:`/`case String s: case null:`, handling of 
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>> 
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Applying review feedback.
>  - Tweaking javadoc.

Dynamic constants are needed when de-structuring classes that are not record at 
top-level, to make the type that will carry the bindings, from invokedynamic to 
where they are accessed, opaque. So dynamic constants are not needed yet !

-

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

Re: [External] : Re: RFR: 8199318: add idempotent copy operation for Map.Entry

[forax]

> De: "John Rose" 
> À: "Remi Forax" 
> Cc: "Peter Levart" , "Rémi Forax"
> , "core-libs-dev" 
> Envoyé: Vendredi 4 Juin 2021 02:05:41
> Objet: Re: [External] : Re: RFR: 8199318: add idempotent copy operation for
> Map.Entry

> On Jun 3, 2021, at 3:17 PM, [ mailto:fo...@univ-mlv.fr | fo...@univ-mlv.fr ]
> wrote:

>>> De: "John Rose" mailto:r.r...@oracle.com | r.r...@oracle.com ] >
>>> À: "Remi Forax" < [ mailto:fo...@univ-mlv.fr | fo...@univ-mlv.fr ] >
>>> Cc: "Peter Levart" < [ mailto:peter.lev...@gmail.com | 
>>> peter.lev...@gmail.com ]
>>> >, "Rémi Forax" < [ mailto:fo...@openjdk.java.net | fo...@openjdk.java.net 
>>> >] >,
>>> "core-libs-dev" < [ mailto:core-libs-dev@openjdk.java.net |
>>> core-libs-dev@openjdk.java.net ] >
>>> Envoyé: Jeudi 3 Juin 2021 22:51:28
>>> Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

>>> ...

>>> interface ComparableRecord>
>>> extends Comparable { … }

>>> record Foo(int x, String y) implements ComparableRecord { … }

>>> [ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
>>> http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

>> [repost with a link]

>> The main issue with this kind of code is that the JIT does not see through 
>> the
>> ClassValue.

> Wow, it’s almost as if we’ve discussed this already! ;-)
> So, [ https://bugs.openjdk.java.net/browse/JDK-8238260 |
> https://bugs.openjdk.java.net/browse/JDK-8238260 ]
> Part of my agenda here is finding more reasons why
> JDK-8238260 deserves some love.

> Currently, the translation strategy for Records
> requires a lot of boilerplate generated into each
> subclass for toString, equals, and hashCode.
> It is partially declarative, because it uses indy.
> So, yay for that. But it is also a bad smell (IMO)
> that the trick needs to be played in each subclass.

> If ClassValue were performant, we could have
> just one method in Record for each of toString,
> equals, and hashCode, and have them DTRT.
> The user code would then be able to call
> super.toString() to get the standard behavior
> in a refining wrapper method in a subclass.

> Looking further, it would be nice to have methods
> which (a) inherit from supers as reusable code
> ought to, but (b) re-specialize themselves once
> per subclass indy-style. This is a VM trick I hope
> to look into after we do specialization. For now,
> ClassValue is the way to simulate that.

yes, it's a specialization problem. 
I wonder if it an be resolved using a combination of a species-static variable 
and magic shibboleth to ask the type parameter to be always reified 

interface ComparableRecord  > extends Comparable< T > { 
species-static Comparator COMPARATOR; // a parameteric condy ? 
species-static { 
COMPARATOR = ... 
} 

default int compareTo(T other) { 
return COMPARATOR.compare(this, other); 
} 
} 

>> Tweaking a little bit your code, I get
>> [
>> https://urldefense.com/v3/__https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8__;!!GqivPVa7Brio!MheW9rHDHXlXYNKUju7v5G0vXlpj1YOoDWFjG9AcpnXnVz2TxlMYDM7i86yFtT_B$
>> | https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8 ]

>> (It's a PITA that we have to use a raw type to workaround circularly defined
>> parameter type)

> I tried to DTRT and got into Inference Hell, surrounded
> by a swarms of unfriendly wildcards with pitchforks.
> Glad it wasn’t just me.

> Inspired by your more whole-hearted use of streams
> to build the code, I updated my example. Thanks.

> — John

Rémi 

Re: RFR: 8199318: add idempotent copy operation for Map.Entry

[forax]

> De: "John Rose" 
> À: "Remi Forax" < fo...@univ-mlv.fr >
> Cc: "Peter Levart" < peter.lev...@gmail.com >, "Rémi Forax" <
> fo...@openjdk.java.net >, "core-libs-dev" < core-libs-dev@openjdk.java.net >
> Envoyé: Jeudi 3 Juin 2021 22:51:28
> Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

> On Jun 3, 2021, at 12:46 PM, Remi Forax < [ mailto:fo...@univ-mlv.fr |
> fo...@univ-mlv.fr ] > wrote:

>> I kind of regret that the compiler does not provide automatically an
>> implementation of compareTo if the record implements Comparable.
>> People sucks at writing compareTo and the resulting bugs are hard to
>> find/reproduce.

> That’s a slippery slope. IIRC we consciously stopped
> before that step.

> That said, there are other ways to fix this. We should
> have utilities (maybe in the JDK but not the JLS) which
> build such methods and make it easy for users to grab onto
> them. Maybe something like this:

> interface ComparableRecord>
> extends Comparable { … }

> record Foo(int x, String y) implements ComparableRecord { … }

> [ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
> http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

[repost with a link] 

The main issue with this kind of code is that the JIT does not see through the 
ClassValue. 

Tweaking a little bit your code, I get 
https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8 

(It's a PITA that we have to use a raw type to workaround circularly defined 
parameter type) 

> — John

Rémi 

Re: RFR: 8199318: add idempotent copy operation for Map.Entry

[forax]

> De: "John Rose" 
> À: "Remi Forax" 
> Cc: "Peter Levart" , "Rémi Forax"
> , "core-libs-dev" 
> Envoyé: Jeudi 3 Juin 2021 22:51:28
> Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

> On Jun 3, 2021, at 12:46 PM, Remi Forax < [ mailto:fo...@univ-mlv.fr |
> fo...@univ-mlv.fr ] > wrote:

>> I kind of regret that the compiler does not provide automatically an
>> implementation of compareTo if the record implements Comparable.
>> People sucks at writing compareTo and the resulting bugs are hard to
>> find/reproduce.

> That’s a slippery slope. IIRC we consciously stopped
> before that step.

> That said, there are other ways to fix this. We should
> have utilities (maybe in the JDK but not the JLS) which
> build such methods and make it easy for users to grab onto
> them. Maybe something like this:

> interface ComparableRecord>
> extends Comparable { … }

> record Foo(int x, String y) implements ComparableRecord { … }

> [ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
> http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

The main issue with this kind of code is that the JIT does not see through the 
ClassValue. 

Tweaking a little bit your code, I get 
(It's a PITA that we have to use a raw type to workaround circularly defined 
parameter type) 

import java.util.ArrayList; 
import java.util.Comparator; 
import java.util.List; 
import java.util.stream.Stream; 

@SuppressWarnings({"rawtypes","unchecked"}) 
interface ComparableRecord> extends 
Comparable { 
@Override default int compareTo(T that) { 
if (this.getClass() != that.getClass()) { 
throw new IllegalArgumentException("not same class"); 
} 
return COMPARE_TO_METHODS.get(this.getClass()).compare(this, that); 
} 
static final ClassValue> COMPARE_TO_METHODS = new 
ClassValue<>() { 
@Override 
protected Comparator computeValue(Class type) { 
return Stream.of(type.getRecordComponents()) 
.map(component -> { 
var accessor = component.getAccessor(); 
return Comparator.comparing(r -> { 
try { 
return (Comparable) accessor.invoke(r); 
} catch (ReflectiveOperationException ex) { 
throw new IllegalArgumentException(ex); 
} 
}); 
}) 
.reduce((r1, r2) -> 0, Comparator::thenComparing, (_1, _2) -> { throw null; }); 
} 
}; 

static void main(String[] args) { 
record Foo(int x, String y) implements ComparableRecord { } 

var list = Stream.of(new Foo(2, "foo"), new Foo(2, "bar")) .sorted().toList(); 
System.out.println(list); 
} 
} 

> — John

Re: RFR: 8199318: add idempotent copy operation for Map.Entry

[Remi Forax]

- Mail original -
> De: "Peter Levart" 
> À: "Rémi Forax" , "core-libs-dev" 
> 
> Envoyé: Jeudi 3 Juin 2021 20:49:05
> Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

> On 02/06/2021 19:24, Rémi Forax wrote:
>> I foolishly hoped that nobody would subclass a class with `Immutable` in its
>> name,
>> oh boy i was wrong:)
> 
> 
> There's nothing wrong in subclassing an immutable class. As long as the
> subclass keeps being immutable. Was it subclassed and made mutable by that?

It has to be immutable all the way up, you have the same issue if the subclass 
is not final.

If you filter out guava an google cache, github get you 12 pages of result and 
only one stupid code
https://github.com/klayders/interlude/blob/95fd59a911d2baa8ac36ae6b877955aa4fbd095e/src/main/java/l2/gameserver/skills/SkillEntry.java#L12

A lot of code uses SimpleImmutableEntry as a pair, my hope is that the 
introduction of records will clean that practice.

That said, there is also several broken codes, mostly two issues,
either a.equals(n) is not equivalent to b.equals(a) or a.equals(b) is not 
equivalent to a.compareTo(b) == 0.

I kind of regret that the compiler does not provide automatically an 
implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to 
find/reproduce.

> 
> 
> Peter

Rémi

Re: RFR: 8268124: Update java.lang to use switch expressions [v3]

[Rémi Forax]

On Thu, 3 Jun 2021 11:01:02 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.lang` 
>> packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8268124: small refactoring; fixed misplaced comment and added missing 
> lambda operator

Looks good to me

-

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

Re: RFR: 8268124: Update java.lang to use switch expressions [v3]

[Rémi Forax]

On Thu, 3 Jun 2021 10:57:16 GMT, Patrick Concannon  
wrote:

> My mistake. I've replaced the colon now with the lambda operator.

Drive by comment, in term of name, `->` is the arrow operator not the lambda 
operator.
- lambda = parameters + arrow + code
- arrow case =  case + arrow + code

The difference is important because a lambda is a function while an arrow case 
is a block.

-

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

Re: RFR: 8199318: add idempotent copy operation for Map.Entry

[Rémi Forax]

On Wed, 2 Jun 2021 17:12:21 GMT, Stuart Marks  wrote:

> A quick search reveals that Guava has a public subclass of 
> SimpleImmutableEntry:
> https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/cache/RemovalNotification.html

>There are possibly others. It doesn't seem worth the incompatibility to me, as 
>it would break stuff in order to have only a "cleaner" meaning for 
>"immutable." Also, there are other classes in the JDK that claim they are 
>immutable but which can be subclassed, e.g., BigInteger. I don't see a good 
>way of fixing this.

Thanks, i've done some digging too,
I foolishly hoped that nobody would subclass a class with `Immutable` in its 
name,
oh boy i was wrong :)

So documenting the existing behavior seems the only possible way.

-

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

Re: RFR: 8268124: Update java.lang to use switch expressions

[Rémi Forax]

On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.lang` 
> packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 366:

> 364: }
> 365: default -> throw new IllegalArgumentException(methodName);
> 366: };

I thinki it's simpler to have something like that

  var handle = switch(methodName) {
...
  };
  return methodType != null ? new ConstantCallSite(handle) : handle;

-

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

Re: RFR: 8268124: Update java.lang to use switch expressions

[Rémi Forax]

On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.lang` 
> packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 278:

> 276: private static boolean isObjectMethod(Method m) {
> 277: return switch (m.getName()) {
> 278: case "toString" -> (m.getReturnType() == String.class

the extra parenthesis are not needed

-

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

Re: RFR: 8268124: Update java.lang to use switch expressions

[Rémi Forax]

On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.lang` 
> packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/lang/invoke/MemberName.java line 331:

> 329: assert(false) : this+" != 
> "+MethodHandleNatives.refKindName((byte)originalRefKind);
> 330: yield true;
> 331: }

this code always yield true, better to check if the assert are enabled, do the 
switch in that case and always return true

-

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

Re: RFR: 8268124: Update java.lang to use switch expressions

[Rémi Forax]

On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.lang` 
> packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line 
1663:

> 1661: case D_TYPE -> mv.visitInsn(Opcodes.DCONST_0);
> 1662: case L_TYPE -> mv.visitInsn(Opcodes.ACONST_NULL);
> 1663: default -> throw new InternalError("unknown type: " + type);

perhaps

  mv.visitInsn(switch(type) { ...

-

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

Re: RFR: 8199318: add idempotent copy operation for Map.Entry

[Rémi Forax]

On Wed, 2 Jun 2021 00:39:25 GMT, Stuart Marks  wrote:

> I'm fixing this along with a couple intertwined issues.
> 
> 1. Add Map.Entry::copyOf as an idempotent copy operation.
> 
> 2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not 
> really immutable) but that subclasses can be modifiable.
> 
> 3. Clarify some confusing, historical wording about Map.Entry instances being 
> obtainable only via iteration of a Map's entry-set view. This was never 
> really true, since anyone could implement the Map.Entry interface, and it 
> certainly was not true since JDK 1.6 when SimpleEntry and 
> SimpleImmutableEntry were added.

LGTM,
i wonder if we should not declare SimpleImmutableEntry final, while it's not a 
backward compatible change,
it's may be better on the long term because SimpleImmutableEntry is already 
used as an immutable type,
so instead of documenting the fact that SimpleImmutableEntry is not declared 
final thus SimpleImmutableEntry as a type does not guarantte shallow 
immutability, it may be better to fix the root cause.

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]

[Rémi Forax]

On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda  wrote:

>> This is a preview of a patch implementing JEP 406: Pattern Matching for 
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>> 
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>> 
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common 
>> superinterface for expressions and patterns, `CaseLabelTree`, which 
>> expressions and patterns implement, and a list of case labels is returned 
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree` 
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent 
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be 
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary 
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed 
>> primitives, as there is no value that is not part of the input domain so 
>> that could be used to represent `case null`. Requires a bit shuffling with 
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It 
>> produces code that delegates to a new bootstrap method, that will classify 
>> the input value to the switch and return the case number, to which the 
>> switch then jumps. To support guards, the switches (and the bootstrap 
>> method) are restartable. The bootstrap method as such is written very simply 
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String 
>> s`/`case null: case String s:`/`case String s: case null:`, handling of 
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>> 
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 12 commits:
> 
>  - Post-merge fix - need to include jdk.internal.javac in the list of 
> packages used by jdk.compiler again, as we now (again) have a preview feature 
> in javac.
>  - Correcting LineNumberTable for rule switches.
>  - Merging master into JDK-8262891
>  - Fixing various error-related bugs.
>  - Avoiding fall-through from the total case to a synthetic default but 
> changing total patterns to default.
>  - Reflecting recent spec changes.
>  - Reflecting review comments.
>  - Reflecting review comments on SwitchBootstraps.
>  - Trailing whitespaces.
>  - Cleanup, reflecting review comments.
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk/compare/083416d3...fd748501

> > I've updated the code to produce better/more useful LineNumberTable for 
> > rule switches.
> 
> @lahodaj I re-tested previous example and tested few others. Now everything 
> seems to be good with `LineNumberTable` entries +1
> 
[...]
> Don't know about importance of this, and whether this was expected, but 
> decided to mention.

Look likes a mistake for me, you only need a StackFrame in front of the call to 
invokedynamic if it's the target of a goto and in your example, there is no 
guard so no backward goto.

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Rémi Forax]

On Tue, 25 May 2021 16:14:56 GMT, Jan Lahoda  wrote:

> I'd like to note this is a preview feature - we can change the desugaring. At 
> the same time, I don't think this does not work with sub-patterns (those can 
> be easily desugared to guards, I think).

Yes, but in that case the classcheck du to a sub-pattern matching will be 
either an instanceof or some other invokedynamic.
Having several indy will bloat the code and if we use instanceof, why not using 
instanceof all along the way.

> Regarding efficiency, it may be a balance between classfile overhead (which 
> will be higher if we need to desugar every guard to a separate method), and 
> the possibility to tweak the matching at runtime.

I fear more the 2 bytes length bytecode limit of a method than the constant 
pool length limit mostly because people are already using a bunch of lambdas to 
simulate pattern matching.

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Rémi Forax]

On Tue, 25 May 2021 14:22:57 GMT, Jan Lahoda  wrote:

> The reason for this integer (which is not a constant in the case of this 
> switch) is to restart the matching in case guards fail to "match". 
> Considering the example here:
> 
> ```
> class Example {
> void example(Object o) {
> switch (o) {
> case String s && s.length() == 0 ->
> System.out.println("1st case");
> case String s && s.length() == 1 ->  // line 6
> System.out.println("2nd case");  // line 7
> case String s -> // line 8
> System.out.println("3rd case");  // line 9
> default ->   // line 10
> System.out.println("default case");  // line 11
> }
> }
> }
> ```
> 
> If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`, 
> returning `0`. Then the guard will be evaluated `s.length() == 0`. If the 
> length is not zero, the local variable 3 will be reassigned to `1`(bytecode 
> index 58, 59) and the whole switch is restarted - just this time, the 
> matching in the indy will start at index `1`, not `0`, i.e. `indy[...](o, 
> 1)`. And so on. I believe there is a text explaining the meaning of the 
> parameter in the javadoc of the bootstrap, and in TransPatterns in javac.

The problem with this design is that calling example("foo") forces the VM will 
do 6 checkcast String on "foo", and it doesn't work with sub-patterns. 
Desugaring the guards as static method like with lambdas seems more promising, 
indy can be called with the pairs [String, MethodHandle(s -> s.length() == 0)], 
[String, MethodHandle(s -> s.length() == 0)] and [_,_]  (with the guards passed 
as a constant method handles again like lambdas are desugared).
It means that the indy needs to capture all local variables that are used in 
the guards, instead of passing an int.

I believe that a translation using constant method handles for guards is far 
more efficient than using an int and a backward branch but it has a higher cost 
in term of runtime data structures.

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Rémi Forax]

On Tue, 25 May 2021 12:20:02 GMT, Jan Lahoda  wrote:

> Thanks Evgeny, I'll take a look.
> 
> @forax, do you mean why there is "0" in:
> 11: invokedynamic #13, 0
> ?

Not this one, the one on the stack.

7: iconst_0   < this zero
8: istore_3
9: aload_2
10: iload_3
11: invokedynamic #13, 0 // InvokeDynamic
#0:typeSwitch:(Ljava/lang/Object;I)I

Why the descriptor is (Ljava/lang/Object;I)I instead of (Ljava/lang/Object;)I,
what is the semantics associated to that integer ?

> The "0" is an artifact of how invokedynamic is represented in the classfile 
> (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap 
> shows the value of the first zero byte. That is probably not needed anymore, 
> but there is nothing special in this patch, I think - all invokedynamic calls 
> look like this, AFAIK.

I know that a little to well, i'm one of the guys behind the design of indy :)

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]

[Remi Forax]

- Mail original -
> De: "Evgeny Mandrikov" 
> À: "build-dev" , "compiler-dev" 
> , "core-libs-dev"
> , "javadoc-dev" 
> Envoyé: Mardi 25 Mai 2021 11:32:03
> Objet: Re: RFR: 8262891: Compiler implementation for Pattern Matching for 
> switch (Preview) [v4]

> On Mon, 17 May 2021 19:01:01 GMT, Jan Lahoda  wrote:
> 
>>> Good work. There's a lot to take in here. I think overall, it holds up well 
>>> - I
>>> like how case labels have been extended to accommodate for patterns. In the
>>> front-end, I think there are some questions over the split between Attr and
>>> Flow - maybe it is unavoidable, but I'm not sure why some control-flow 
>>> analysis
>>> happens in Attr instead of Flow and I left some questions to make sure I
>>> understand what's happening.
>>> 
>>> In the backend it's mostly good work - but overall the structure of the 
>>> code,
>>> both in Lower and in TransPattern is getting very difficult to follow, given
>>> there are many different kind of switches each with its own little twist
>>> attached to it. It would be nice, I think (maybe in a separate cleanup?) if 
>>> the
>>> code paths serving the different switch kinds could be made more separate, 
>>> so
>>> that, when reading the code we can worry only about one possible code shape.
>>> That would make the code easier to document as well.
>>
>> @mcimadamore, @forax, thanks a lot for comments! I tried to apply the 
>> requested
>> changes in recent commits
>> (https://github.com/openjdk/jdk/pull/3863/commits/3fc2502a33cec20f39fe571eb25538ee3b459a9b
>> https://github.com/openjdk/jdk/pull/3863/commits/aeddb85e65bf77ed62dc7fa1ad00c29646d02c99
>> ).
>> 
>> I've also tried to update the implementation for the latest spec changes 
>> here:
>> https://github.com/openjdk/jdk/pull/3863/commits/54ba974e1aac00bbde1c3bd2627f01caaaeda09b
>> 
>> The spec changes are: total patterns are nullable; pattern matching ("new")
>> statement switches must be complete (as switch expressions).
>> 
>> Any further feedback is welcome!
> 
> Hi @lahodaj   ,
> 
> I tried `javac` built from this PR and for the following `Example.java`
> 
> 
> class Example {
>void example(Object o) {
>switch (o) {
>case String s && s.length() == 0 ->
>System.out.println("1st case");
>case String s && s.length() == 1 ->  // line 6
>System.out.println("2nd case");  // line 7
>case String s -> // line 8
>System.out.println("3rd case");  // line 9
>default ->   // line 10
>System.out.println("default case");  // line 11
>}
>}
> }
> 
> 
> execution of
> 
> 
> javac --enable-preview --release 17 Example.java
> javap -v -p Example.class
> 
> 
> produces
> 
> 
>  void example(java.lang.Object);
>descriptor: (Ljava/lang/Object;)V
>flags: (0x)
>Code:
>  stack=2, locals=7, args_size=2
> 0: aload_1
> 1: dup
> 2: invokestatic  #7  // Method
> 
> java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object;
> 5: pop
> 6: astore_2
> 7: iconst_0
> 8: istore_3
> 9: aload_2
>10: iload_3
>11: invokedynamic #13,  0 // InvokeDynamic
>#0:typeSwitch:(Ljava/lang/Object;I)I
>16: tableswitch   { // 0 to 2
>   0: 44
>   1: 74
>   2: 105
> default: 122
>}
>44: aload_2
>45: checkcast #17 // class java/lang/String
>48: astore4
>50: aload 4
>52: invokevirtual #19 // Method 
> java/lang/String.length:()I
>55: ifeq  63
>58: iconst_1
>59: istore_3
>60: goto  9
>63: getstatic #23 // Field
>java/lang/System.out:Ljava/io/PrintStream;
>66: ldc   #29 // String 1st case
>68: invokevirtual #31 // Method
>java/io/PrintStream.println:(Ljava/lang/String;)V
>71: goto  133
>74: aload_2
>75: checkca

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Rémi Forax]

On Thu, 20 May 2021 20:37:57 GMT, Roger Riggs  wrote:

> OutputStreamWriter would be a better choice with that in mind. It does not 
> have the convenience methods for converting various types to strings but 
> would not hide the exceptions. Developers could wrap it in a PrintWriter to 
> get the convenience and take on the responsibility of dealing with exceptions 
> by polling.

yes, instead of OutputStreamWriter, i wonder if BufferedWriter is not better 
given that reader part uses BufferedReader and that is mirror 
java.nio.file.Files which also uses BufferedReader/BufferedWriter as types for 
the pair reader/writer.

-

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

Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams

[Rémi Forax]

On Thu, 20 May 2021 19:57:22 GMT, Roger Riggs  wrote:

> Methods are added to java.lang.Process to read and write characters and lines 
> from and to a spawned Process.
> The Charset used to encode and decode characters to bytes can be specified or 
> use the
> operating system native encoding as is available from the "native.encoding" 
> system property.

I've added the same comment on the bug itself.
I don't think that using PrintWriter is a good idea here given that a 
PrintWriter shallow the IOException

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v3]

[Rémi Forax]

On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne  
wrote:

>> 8266846: Add java.time.InstantSource
>
> Stephen Colebourne has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8266846: Add java.time.InstantSource

my bad

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v3]

[Rémi Forax]

On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne  
wrote:

>> 8266846: Add java.time.InstantSource
>
> Stephen Colebourne has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8266846: Add java.time.InstantSource

It's a side effect of JEP 403, i believe

-

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

Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v2]

[Rémi Forax]

On Mon, 17 May 2021 19:04:11 GMT, Jan Lahoda  wrote:

>> This is a preview of a patch implementing JEP 406: Pattern Matching for 
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>> 
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>> 
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common 
>> superinterface for expressions and patterns, `CaseLabelTree`, which 
>> expressions and patterns implement, and a list of case labels is returned 
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree` 
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent 
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be 
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary 
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed 
>> primitives, as there is no value that is not part of the input domain so 
>> that could be used to represent `case null`. Requires a bit shuffling with 
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It 
>> produces code that delegates to a new bootstrap method, that will classify 
>> the input value to the switch and return the case number, to which the 
>> switch then jumps. To support guards, the switches (and the bootstrap 
>> method) are restartable. The bootstrap method as such is written very simply 
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String 
>> s`/`case null: case String s:`/`case String s: case null:`, handling of 
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>> 
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - Reflecting recent spec changes.
>  - Reflecting review comments.
>  - Reflecting review comments on SwitchBootstraps.

It's not clear to me if it's the first change and several will follow or not.

The code looks good but the metaprotocol is not the right one IMO,
i would prefer the one we discuss in April
https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-April/002992.html

But this can be tackle in another PR

-

Marked as reviewed by fo...@github.com (no known OpenJDK username).

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