Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v3]

[Rémi Forax]

On Mon, 15 Jul 2024 11:33:30 GMT, Jorn Vernee  wrote:

>> This PR limits the number of cases in which we deoptimize frames when 
>> closing a shared Arena. The initial intent of this was to improve the 
>> performance of shared arena closure in cases where a lot of threads are 
>> accessing and closing shared arenas at the same time (see attached 
>> benchmark), but unfortunately even disabling deoptimization altogether does 
>> not have a great effect on that benchmark.
>> 
>> Nevertheless, I think the extra logging/testing/benchmark code, and comments 
>> I've written, together with reducing the number of cases where we deoptimize 
>> (which makes it clearer exactly why we need to deoptimize in the first 
>> place), will be useful going forward. So, I've a create this PR out of them.
>> 
>> In this PR:
>> - Deoptimizing is now only done in cases where it's needed, instead of 
>> always. Which is in cases where we are not inside an `@Scoped` method, but 
>> are inside a compiled frame that has a scoped access somewhere inside of it.
>> - I've separated the stack walking code (`for_scope_method`) from the code 
>> that checks for a reference to the arena being closed 
>> (`is_accessing_session`), and added logging code to the former. That also 
>> required changing vframe code to accept an `ouputStream*` rather than always 
>> printing to `tty`.
>> - Added a new test (`TestConcurrentClose`), that tries to close many shared 
>> arenas at the same time, in order to stress that use case.
>> - Added a new benchmark (`ConcurrentClose`), that stresses the cases where 
>> many threads are accessing and closing shared arenas.
>> 
>> I've done several benchmark runs with different amounts of threads. The 
>> confined case stays much more consistent, while the shared cases balloons up 
>> in time spent quickly when there are more than 4 threads:
>> 
>> 
>> Benchmark Threads   Mode  Cnt Score Error  Units
>> ConcurrentClose.sharedAccess   32   avgt   10  9017.397 ± 202.870  us/op
>> ConcurrentClose.sharedAccess   24   avgt   10  5178.214 ± 164.922  us/op
>> ConcurrentClose.sharedAccess   16   avgt   10  2224.420 ± 165.754  us/op
>> ConcurrentClose.sharedAccess8   avgt   10   593.828 ±   8.321  us/op
>> ConcurrentClose.sharedAccess7   avgt   10   470.700 ±  22.511  us/op
>> ConcurrentClose.sharedAccess6   avgt   10   386.697 ±  59.170  us/op
>> ConcurrentClose.sharedAccess5   avgt   10   291.157 ±   7.023  us/op
>> ConcurrentClose.sharedAccess4   avgt   10   209.178 ±   5.802  us/op
>> ConcurrentClose.sharedAccess1   avgt   10  ...
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   improve benchmark

Even if the int vs long issue is fixed for this case, i think we should 
recommand to call `withInvokeExactBehavior()` after creating any VarHandle so 
all the auto-conversions are treated as runtime errors.

This is what i do with my students (when using compareAndSet) and it makes this 
kind of perf issue easy to find and easy to fix.

-

PR Comment: https://git.openjdk.org/jdk/pull/20158#issuecomment-2228979913

Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v2]

[Rémi Forax]

On Mon, 15 Jul 2024 10:50:34 GMT, Jorn Vernee  wrote:

> This is what I was thinking of as well. close() on a shared arena can be 
> called by any thread, so it would be possible to have an executor service 
> with 1-n threads that is dedicated to closing memory.

This delays both the closing of the Arena and the freeing of the segments, so 
bugs may be not discovered if the arena is accessed in between the time the 
thread pool is notified and the time the close() is effectively called.

And you loose the structured part of the API, you can not use a 
try-with-resources anymore. I think that part can be fixed using a wrapper on 
top of Arena.ofShared().

-

PR Comment: https://git.openjdk.org/jdk/pull/20158#issuecomment-2228280373

Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v2]

[Rémi Forax]

On Fri, 12 Jul 2024 20:59:26 GMT, Jorn Vernee  wrote:

>> This PR limits the number of cases in which we deoptimize frames when 
>> closing a shared Arena. The initial intent of this was to improve the 
>> performance of shared arena closure in cases where a lot of threads are 
>> accessing and closing shared arenas at the same time (see attached 
>> benchmark), but unfortunately even disabling deoptimization altogether does 
>> not have a great effect on that benchmark.
>> 
>> Nevertheless, I think the extra logging/testing/benchmark code, and comments 
>> I've written, together with reducing the number of cases where we deoptimize 
>> (which makes it clearer exactly why we need to deoptimize in the first 
>> place), will be useful going forward. So, I've a create this PR out of them.
>> 
>> In this PR:
>> - Deoptimizing is now only done in cases where it's needed, instead of 
>> always. Which is in cases where we are not inside an `@Scoped` method, but 
>> are inside a compiled frame that has a scoped access somewhere inside of it.
>> - I've separated the stack walking code (`for_scope_method`) from the code 
>> that checks for a reference to the arena being closed 
>> (`is_accessing_session`), and added logging code to the former. That also 
>> required changing vframe code to accept an `ouputStream*` rather than always 
>> printing to `tty`.
>> - Added a new test (`TestConcurrentClose`), that tries to close many shared 
>> arenas at the same time, in order to stress that use case.
>> - Added a new benchmark (`ConcurrentClose`), that stresses the cases where 
>> many threads are accessing and closing shared arenas.
>> 
>> I've done several benchmark runs with different amounts of threads. The 
>> confined case stays much more consistent, while the shared cases balloons up 
>> in time spent quickly when there are more than 4 threads:
>> 
>> 
>> Benchmark Threads   Mode  Cnt Score Error  Units
>> ConcurrentClose.sharedAccess   32   avgt   10  9017.397 ± 202.870  us/op
>> ConcurrentClose.sharedAccess   24   avgt   10  5178.214 ± 164.922  us/op
>> ConcurrentClose.sharedAccess   16   avgt   10  2224.420 ± 165.754  us/op
>> ConcurrentClose.sharedAccess8   avgt   10   593.828 ±   8.321  us/op
>> ConcurrentClose.sharedAccess7   avgt   10   470.700 ±  22.511  us/op
>> ConcurrentClose.sharedAccess6   avgt   10   386.697 ±  59.170  us/op
>> ConcurrentClose.sharedAccess5   avgt   10   291.157 ±   7.023  us/op
>> ConcurrentClose.sharedAccess4   avgt   10   209.178 ±   5.802  us/op
>> ConcurrentClose.sharedAccess1   avgt   10  ...
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   track has_scoped_access for compiled methods

Knowing that all the segments are freed during close() is something you may 
want.
But having the execution time of close() be linear with the number of threads 
is also problematic. Maybe, it means that we need another kind of Arena that 
works like shared() but allow the freed to be done asynchronously 
(ofSharedAsyncFree ?).

Note that the semantics of ofSharedAsyncFree() is different from ofAuto(), 
ofAuto() relies on the GC to free a segment so the delay before a segment is 
freed is not time bounded if the application has enough memory, the memory of 
the segment may never be reclaimed. With ofSharedAsyncFree(), the segments are 
freed by the last thread, so while this mechanism is not deterministic, it is 
time bounded.

-

PR Comment: https://git.openjdk.org/jdk/pull/20158#issuecomment-2226992713

Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v2]

[Rémi Forax]

On Fri, 12 Jul 2024 20:59:26 GMT, Jorn Vernee  wrote:

>> This PR limits the number of cases in which we deoptimize frames when 
>> closing a shared Arena. The initial intent of this was to improve the 
>> performance of shared arena closure in cases where a lot of threads are 
>> accessing and closing shared arenas at the same time (see attached 
>> benchmark), but unfortunately even disabling deoptimization altogether does 
>> not have a great effect on that benchmark.
>> 
>> Nevertheless, I think the extra logging/testing/benchmark code, and comments 
>> I've written, together with reducing the number of cases where we deoptimize 
>> (which makes it clearer exactly why we need to deoptimize in the first 
>> place), will be useful going forward. So, I've a create this PR out of them.
>> 
>> In this PR:
>> - Deoptimizing is now only done in cases where it's needed, instead of 
>> always. Which is in cases where we are not inside an `@Scoped` method, but 
>> are inside compiled code.
>> - I've separated the stack walking code (`for_scope_method`) from the code 
>> that checks for a reference to the arena being closed 
>> (`is_accessing_session`), and added logging code to the former. That also 
>> required changing vframe code to accept an `ouputStream*` rather than always 
>> printing to `tty`.
>> - Added a new test (`TestConcurrentClose`), that tries to close many shared 
>> arenas at the same time, in order to stress that use case.
>> - Added a new benchmark (`ConcurrentClose`), that stresses the cases where 
>> many threads are accessing and closing shared arenas.
>> 
>> I've done several benchmark runs with different amounts of threads. The 
>> confined case stays much more consistent, while the shared cases balloons up 
>> in time spent quickly when there are more than 4 threads:
>> 
>> 
>> Benchmark Threads   Mode  Cnt Score Error  Units
>> ConcurrentClose.sharedAccess   32   avgt   10  9017.397 ± 202.870  us/op
>> ConcurrentClose.sharedAccess   24   avgt   10  5178.214 ± 164.922  us/op
>> ConcurrentClose.sharedAccess   16   avgt   10  2224.420 ± 165.754  us/op
>> ConcurrentClose.sharedAccess8   avgt   10   593.828 ±   8.321  us/op
>> ConcurrentClose.sharedAccess7   avgt   10   470.700 ±  22.511  us/op
>> ConcurrentClose.sharedAccess6   avgt   10   386.697 ±  59.170  us/op
>> ConcurrentClose.sharedAccess5   avgt   10   291.157 ±   7.023  us/op
>> ConcurrentClose.sharedAccess4   avgt   10   209.178 ±   5.802  us/op
>> ConcurrentClose.sharedAccess1   avgt   1052.042 ±   0.630  us/op
>> ConcurrentClose.confine...
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   track has_scoped_access for compiled methods

Nice work !

Thinking a bit about how to improve the benchmark and given the semantics of 
Arena.close(), there is a trick that you can use. There are two kinds of memory 
segments, the one that only visible from Java and the one that are visible not 
only from Java. By example, a memory segment created from a mmap or a memory 
segment with an address sent to a C code are visible from outside Java, for 
those, you have no choice but wait in Arena.close() until all threads have 
answered to the handshakes. For all the other memory segments, because they are 
only visible from Java, their memory can be reclaimed asynchronously, i.e. the 
last thread of the handshakes can free the corresponding memory segments, so 
the thread that call Arena.close() is free to run even if the memory is not yet 
reclaimed.

>From my armchair, that seems a awful lot of engeneering so it may not worth 
>it, but now you know :)

-

PR Comment: https://git.openjdk.org/jdk/pull/20158#issuecomment-2226858328

Re: RFR: 8333265: De-duplicate method references in java.util.stream.FindOps

[Rémi Forax]

On Thu, 30 May 2024 12:50:36 GMT, Claes Redestad  wrote:

> Extracting duplicate method references to static field reduce proxy class 
> spinning and loading. In this case 2 less classes loaded when using 
> `findAny()` on each type of stream.

For constant method reference, the solution is to use a constant dynamic 
instead of an invokedynamic.

-

PR Comment: https://git.openjdk.org/jdk/pull/19477#issuecomment-2139645936

Re: RFR: 8330465: Stable Values and Collections (Internal) [v4]

[Rémi Forax]

On Wed, 15 May 2024 11:27:04 GMT, ExE Boss  wrote:

>>> Maybe export this interface to `jdk.unsupported`?
>> 
>> I don't we should do that. In general, we need jdk.unsupported to go away in 
>> the long term. Also integrity of the platform depends on java.base being 
>> very stingy and not exporting internal packages to other modules.
>
> Given that `TrustedFieldType` is more generic than stable values, it could be 
> moved to `jdk.internal.misc` or `jdk.internal.reflect`, then 
> `jdk.unsupported` could use it without exporting new packages to 
> `jdk.unsupported`.

At some point in the future, 'jdk.unsupported' will be removed

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601548492

Re: RFR: 8305457: Implement java.io.IO [v3]

[Rémi Forax]

On Tue, 7 May 2024 16:09:08 GMT, Pavel Rappo  wrote:

>> I do not think the step to "standardise" a preview feature exists ? When a 
>> preview feature becomes a released feature, the code is very lightly edited, 
>> at least it this is my experience. 
>> 
>> You can change both readln and readLine and if `java.io.IO` is removed, at 
>> least the code of readLine() will be
>
>> I do not think the step to "standardise" a preview feature exists ? When a 
>> preview feature becomes a released feature, the code is very lightly edited, 
>> at least it this is my experience.
> 
> We may call it differently, but I think both you and I are referring to this 
> part of [JEP 12](https://openjdk.org/jeps/12) (emphasis mine):
> 
>> Eventually, the JEP owner must decide the preview feature's fate. If the 
>> decision is to remove the preview feature, then the owner must file an issue 
>> in JBS to remove the feature in the next JDK feature release; no new JEP is 
>> needed. **On the other hand, if the decision is to finalize, then the owner 
>> must file a new JEP, noting refinements informed by developer feedback. The 
>> title of this JEP should be the feature's name, omitting the earlier suffix 
>> of (Preview) / (Second Preview), and without adding any new suffix such as 
>> (Standard) or (Final). This JEP will ultimately reach Targeted status for 
>> the next JDK feature release.**
> 
>> You can change both readln and readLine and if `java.io.IO` is removed, at 
>> least the code of readLine() will be
> 
> Sorry, Rémi, but no. As long as this feature is in preview, I'd optimise for 
> easier removal (back out) of the feature rather than clean combined code.

Okay

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593013670

Re: RFR: 8305457: Implement java.io.IO

[Rémi Forax]

On Tue, 7 May 2024 11:00:52 GMT, Pavel Rappo  wrote:

>> src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 74:
>> 
>>> 72: 
>>> 73: @Override
>>> 74: public String readln(String prompt) {
>> 
>> this code can be simplified using an early return (and the body of the 
>> try/catch can be reduced so it is more clear which statement can cause the 
>> IOException)
>> 
>> synchronized (writeLock) {
>> synchronized(readLock) {
>> if (!prompt.isEmpty()) {
>> pw.print(prompt);
>> pw.flush(); // automatic flushing does not cover print
>> }
>>char[] array;
>> try {
>> array = readline(false);
>> } catch (IOException x) {
>> throw new IOError(x);
>> }
>> if (array != null) {
>> return new String(array);
>> }
>> }
>> }
>> return null;
>
> This method started as a copy of `readLine`. In its current form, it's very 
> evident how `readln` differs from `readLine`, and I would leave it this way 
> for now. If the feature is standardised, we could refactor both methods 
> together, as you suggested.
> 
> If the `java.io.IO` part of the feature or the feature itself is withdrawn, 
> then we could consider refactoring the existing `readLine`. Does it make 
> sense?

I do not think the step to "standardise" a preview feature exists ? When a 
preview feature becomes a released feature, the code is very lightly edited, at 
least it this is my experience. 

You can change both readln and readLine and if `java.io.IO` is removed, at 
least the code of readLine() will be

>> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
>>  line 88:
>> 
>>> 86: @Override
>>> 87: public JdkConsole println(Object obj) {
>>> 88: writer().println(obj);
>> 
>> the result of 'writer()' can be stored in a local variable (printing code 
>> are not JITed as often as the rest of the codes)
>
> I assume it's about performance. If so, I would defer any performance-related 
> tweaks until they are necessary. Interactive reading from console does not 
> sound like something requiring that level of performance tweaking.

yes, let see what @cl4es will say when he will test the time to print "hello 
world"

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592385449
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592387034

Re: RFR: 8305457: Implement java.io.IO

[Rémi Forax]

On Mon, 6 May 2024 21:45:12 GMT, Pavel Rappo  wrote:

> Please review this PR which introduces the `java.io.IO` top-level class and 
> three methods to `java.io.Console` for [Implicitly Declared Classes and 
> Instance Main Methods (Third Preview)].
> 
> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
> PR].
> 
> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
> https://bugs.openjdk.org/browse/JDK-8323335
> [draft PR]: https://github.com/openjdk/jdk/pull/18921

Otherwise looks good.

src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 74:

> 72: 
> 73: @Override
> 74: public String readln(String prompt) {

this code can be simplified using an early return (and the body of the 
try/catch can be reduced so it is more clear which statement can cause the 
IOException)

synchronized (writeLock) {
synchronized(readLock) {
if (!prompt.isEmpty()) {
pw.print(prompt);
pw.flush(); // automatic flushing does not cover print
}
   char[] array;
try {
array = readline(false);
} catch (IOException x) {
throw new IOError(x);
}
if (array != null) {
return new String(array);
}
}
}
return null;

src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
 line 88:

> 86: @Override
> 87: public JdkConsole println(Object obj) {
> 88: writer().println(obj);

the result of 'writer()' can be stored in a local variable (printing code are 
not JITed as often as the rest of the codes)

-

PR Comment: https://git.openjdk.org/jdk/pull/19112#issuecomment-2097502361
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1591846059
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1591847579

Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v7]

[Rémi Forax]

On Fri, 12 Apr 2024 14:53:01 GMT, Viktor Klang  wrote:

>> This PR implements Gatherer-inspired encoding of `flatMap` that shows that 
>> it is both competitive performance-wise as well as improve correctness.
>> 
>> Below is the performance of `Stream::flatMap` (for reference types):
>> 
>> Before this PR (on my MacBook, aarch64):
>> 
>> Non-short-circuiting:
>> 
>> Benchmark (size)   Mode  CntScore   Error  Units
>> FlatMap.par_array 10  thrpt   12   257582,480 ? 31360,242  ops/s
>> FlatMap.par_array100  thrpt   1255202,015 ? 14011,668  ops/s
>> FlatMap.par_array   1000  thrpt   12 6546,252 ?  3675,764  ops/s
>> FlatMap.par_doublestream  10  thrpt   12   267423,410 ? 37338,089  ops/s
>> FlatMap.par_doublestream 100  thrpt   1227140,703 ?  4979,878  ops/s
>> FlatMap.par_doublestream1000  thrpt   12 2978,178 ?  1890,250  ops/s
>> FlatMap.par_intstream 10  thrpt   12   268194,530 ? 37295,092  ops/s
>> FlatMap.par_intstream100  thrpt   1230897,034 ?  5388,245  ops/s
>> FlatMap.par_intstream   1000  thrpt   12 3969,043 ?  2494,641  ops/s
>> FlatMap.par_longstream10  thrpt   12   279756,861 ? 19519,497  ops/s
>> FlatMap.par_longstream   100  thrpt   1245733,955 ? 18385,144  ops/s
>> FlatMap.par_longstream  1000  thrpt   12 5115,615 ?  4147,237  ops/s
>> FlatMap.seq_array 10  thrpt   12  2937192,934 ? 58605,583  ops/s
>> FlatMap.seq_array100  thrpt   1241859,462 ?   139,021  ops/s
>> FlatMap.seq_array   1000  thrpt   12  442,677 ? 1,041  ops/s
>> FlatMap.seq_doublestream  10  thrpt   12  4972651,093 ? 35997,267  ops/s
>> FlatMap.seq_doublestream 100  thrpt   1299265,005 ?   193,497  ops/s
>> FlatMap.seq_doublestream1000  thrpt   12 1037,030 ? 3,254  ops/s
>> FlatMap.seq_intstream 10  thrpt   12  5133751,888 ? 23516,294  ops/s
>> FlatMap.seq_intstream100  thrpt   12   145166,206 ?   399,263  ops/s
>> FlatMap.seq_intstream   1000  thrpt   12 1565,004 ? 3,207  ops/s
>> FlatMap.seq_longstream10  thrpt   12  5047029,363 ? 24742,300  ops/s
>> FlatMap.seq_longstream   100  thrpt   12   233225,307 ?  7162,604  ops/s
>> FlatMap.seq_longstream  1000  thrpt   12 2999,926 ? 9,945  ops/s
>> 
>> // Short-circuiting:
>> 
>> Benchmark   (size)   Mode  Cnt   Score   Error  Units
>> FlatMap.par_iterate_double  10  thrpt   12   46336,834 ?  6803,751  ops/s
>> FlatMap.par_iterate_double 100 ...
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding additional, short-circuit-specific, cases to the FlatMap benchmark

Hello, i will not pretend I fully understand the code, but i think it's 
important to keep a constistent code style.
(and no I do not want to start an holy war about the code style, I just think 
we should keep it consistent)

-

PR Comment: https://git.openjdk.org/jdk/pull/18625#issuecomment-2053586457

Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v7]

[Rémi Forax]

On Fri, 12 Apr 2024 14:53:01 GMT, Viktor Klang  wrote:

>> This PR implements Gatherer-inspired encoding of `flatMap` that shows that 
>> it is both competitive performance-wise as well as improve correctness.
>> 
>> Below is the performance of `Stream::flatMap` (for reference types):
>> 
>> Before this PR (on my MacBook, aarch64):
>> 
>> Non-short-circuiting:
>> 
>> Benchmark (size)   Mode  CntScore   Error  Units
>> FlatMap.par_array 10  thrpt   12   257582,480 ? 31360,242  ops/s
>> FlatMap.par_array100  thrpt   1255202,015 ? 14011,668  ops/s
>> FlatMap.par_array   1000  thrpt   12 6546,252 ?  3675,764  ops/s
>> FlatMap.par_doublestream  10  thrpt   12   267423,410 ? 37338,089  ops/s
>> FlatMap.par_doublestream 100  thrpt   1227140,703 ?  4979,878  ops/s
>> FlatMap.par_doublestream1000  thrpt   12 2978,178 ?  1890,250  ops/s
>> FlatMap.par_intstream 10  thrpt   12   268194,530 ? 37295,092  ops/s
>> FlatMap.par_intstream100  thrpt   1230897,034 ?  5388,245  ops/s
>> FlatMap.par_intstream   1000  thrpt   12 3969,043 ?  2494,641  ops/s
>> FlatMap.par_longstream10  thrpt   12   279756,861 ? 19519,497  ops/s
>> FlatMap.par_longstream   100  thrpt   1245733,955 ? 18385,144  ops/s
>> FlatMap.par_longstream  1000  thrpt   12 5115,615 ?  4147,237  ops/s
>> FlatMap.seq_array 10  thrpt   12  2937192,934 ? 58605,583  ops/s
>> FlatMap.seq_array100  thrpt   1241859,462 ?   139,021  ops/s
>> FlatMap.seq_array   1000  thrpt   12  442,677 ? 1,041  ops/s
>> FlatMap.seq_doublestream  10  thrpt   12  4972651,093 ? 35997,267  ops/s
>> FlatMap.seq_doublestream 100  thrpt   1299265,005 ?   193,497  ops/s
>> FlatMap.seq_doublestream1000  thrpt   12 1037,030 ? 3,254  ops/s
>> FlatMap.seq_intstream 10  thrpt   12  5133751,888 ? 23516,294  ops/s
>> FlatMap.seq_intstream100  thrpt   12   145166,206 ?   399,263  ops/s
>> FlatMap.seq_intstream   1000  thrpt   12 1565,004 ? 3,207  ops/s
>> FlatMap.seq_longstream10  thrpt   12  5047029,363 ? 24742,300  ops/s
>> FlatMap.seq_longstream   100  thrpt   12   233225,307 ?  7162,604  ops/s
>> FlatMap.seq_longstream  1000  thrpt   12 2999,926 ? 9,945  ops/s
>> 
>> // Short-circuiting:
>> 
>> Benchmark   (size)   Mode  Cnt   Score   Error  Units
>> FlatMap.par_iterate_double  10  thrpt   12   46336,834 ?  6803,751  ops/s
>> FlatMap.par_iterate_double 100 ...
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding additional, short-circuit-specific, cases to the FlatMap benchmark

src/java.base/share/classes/java/util/stream/ReferencePipeline.java line 280:

> 278: @Override
> 279: Sink opWrapSink(int flags, Sink sink) {
> 280: final boolean shorts = isShortCircuitingPipeline();

Same as above (here having the two final aligned but having different meanings 
require some mental gymnastic)

src/java.base/share/classes/java/util/stream/ReferencePipeline.java line 326:

> 324: @Override
> 325: Sink opWrapSink(int flags, Sink sink) {
> 326: final IntConsumer fastPath =

same as above

src/java.base/share/classes/java/util/stream/ReferencePipeline.java line 377:

> 375: @Override
> 376: Sink opWrapSink(int flags, Sink sink) {
> 377: final DoubleConsumer fastPath =

Same as above

src/java.base/share/classes/java/util/stream/ReferencePipeline.java line 429:

> 427: @Override
> 428: Sink opWrapSink(int flags, Sink sink) {
> 429: final LongConsumer fastPath =

Same as above

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1563891484
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1563891603
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1563891736
PR Review Comment: https://git.openjdk.org/jdk/pull/18625#discussion_r1563891826

Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v7]

[Rémi Forax]

On Fri, 12 Apr 2024 14:53:01 GMT, Viktor Klang  wrote:

>> This PR implements Gatherer-inspired encoding of `flatMap` that shows that 
>> it is both competitive performance-wise as well as improve correctness.
>> 
>> Below is the performance of `Stream::flatMap` (for reference types):
>> 
>> Before this PR (on my MacBook, aarch64):
>> 
>> Non-short-circuiting:
>> 
>> Benchmark (size)   Mode  CntScore   Error  Units
>> FlatMap.par_array 10  thrpt   12   257582,480 ? 31360,242  ops/s
>> FlatMap.par_array100  thrpt   1255202,015 ? 14011,668  ops/s
>> FlatMap.par_array   1000  thrpt   12 6546,252 ?  3675,764  ops/s
>> FlatMap.par_doublestream  10  thrpt   12   267423,410 ? 37338,089  ops/s
>> FlatMap.par_doublestream 100  thrpt   1227140,703 ?  4979,878  ops/s
>> FlatMap.par_doublestream1000  thrpt   12 2978,178 ?  1890,250  ops/s
>> FlatMap.par_intstream 10  thrpt   12   268194,530 ? 37295,092  ops/s
>> FlatMap.par_intstream100  thrpt   1230897,034 ?  5388,245  ops/s
>> FlatMap.par_intstream   1000  thrpt   12 3969,043 ?  2494,641  ops/s
>> FlatMap.par_longstream10  thrpt   12   279756,861 ? 19519,497  ops/s
>> FlatMap.par_longstream   100  thrpt   1245733,955 ? 18385,144  ops/s
>> FlatMap.par_longstream  1000  thrpt   12 5115,615 ?  4147,237  ops/s
>> FlatMap.seq_array 10  thrpt   12  2937192,934 ? 58605,583  ops/s
>> FlatMap.seq_array100  thrpt   1241859,462 ?   139,021  ops/s
>> FlatMap.seq_array   1000  thrpt   12  442,677 ? 1,041  ops/s
>> FlatMap.seq_doublestream  10  thrpt   12  4972651,093 ? 35997,267  ops/s
>> FlatMap.seq_doublestream 100  thrpt   1299265,005 ?   193,497  ops/s
>> FlatMap.seq_doublestream1000  thrpt   12 1037,030 ? 3,254  ops/s
>> FlatMap.seq_intstream 10  thrpt   12  5133751,888 ? 23516,294  ops/s
>> FlatMap.seq_intstream100  thrpt   12   145166,206 ?   399,263  ops/s
>> FlatMap.seq_intstream   1000  thrpt   12 1565,004 ? 3,207  ops/s
>> FlatMap.seq_longstream10  thrpt   12  5047029,363 ? 24742,300  ops/s
>> FlatMap.seq_longstream   100  thrpt   12   233225,307 ?  7162,604  ops/s
>> FlatMap.seq_longstream  1000  thrpt   12 2999,926 ? 9,945  ops/s
>> 
>> // Short-circuiting:
>> 
>> Benchmark   (size)   Mode  Cnt   Score   Error  Units
>> FlatMap.par_iterate_double  10  thrpt   12   46336,834 ?  6803,751  ops/s
>> FlatMap.par_iterate_double 100 ...
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding additional, short-circuit-specific, cases to the FlatMap benchmark

src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 439:

> 437:  * Returns whether any of the stages in the (entire) pipeline is 
> short-circuiting
> 438:  * or not.
> 439:  * @return {@code true} if any stage in this pipeline is stateful,

stateful -> short-circuiting

src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 442:

> 440:  * {@code false} if not.
> 441:  */
> 442: protected final boolean isShortCircuitingPipeline() {

protected can be replaced by package-private

src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 448:

> 446:  u = u.nextStage) {
> 447: }
> 448: return result;

can be written in a simpler way

for(var stage = sourceStage.nextStage; stage != null; stage = stage.nextStage) {
  if (StreamOpFlag.SHORT_CIRCUIT.isKnown(u.combinedFlags)) {
return true;
  }
  return false;
}

no local variable and no SSA phi

src/java.base/share/classes/java/util/stream/DoublePipeline.java line 267:

> 265: @Override
> 266: Sink opWrapSink(int flags, Sink sink) {
> 267: final DoubleConsumer fastPath =

this `final` is unecessary, inconsistent with the style of all other fiels of 
this package.

src/java.base/share/classes/java/util/stream/DoublePipeline.java line 281:

> 279: @Override
> 280: public void accept(double e) {
> 281: try (final var result = mapper.apply(e)) {

`final` is unecessary and i will keep the type instead of `var` because this 
code quite complex and eliding the type does not help (and also mixing var and 
not var in the same method is not recommanded cf Stuart guide on where using 
'var')

src/java.base/share/classes/java/util/stream/IntPipeline.java line 300:

> 298: @Override
> 299: Sink opWrapSink(int flags, Sink sink) {
> 300: final IntConsumer fastPath =

same as above

src/java.base/share/classes/java/util/stream/LongPipeline.java line 283:

> 281: @Override
> 282: Sink opWrapSink(int flags, Sink sink) {
> 283: final 

Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases

[Rémi Forax]

On Tue, 9 Apr 2024 12:01:49 GMT, Claes Redestad  wrote:

> This patch suggests a workaround to an issue with huge SCF MH expression 
> trees taking excessive JIT compilation resources by reviving (part of) the 
> simple bytecode-generating strategy that was originally available as an 
> all-or-nothing strategy choice. 
> 
> Instead of reintroducing a binary strategy choice I propose a threshold 
> parameter, controlled by 
> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions 
> below or at this threshold there's no change, for expressions with an arity 
> above it we use the `StringBuilder`-chain bytecode generator. 
> 
> There are a few trade-offs at play here which influence the choice of 
> threshold. The simple high arity strategy will for example not see any reuse 
> of LambdaForms but strictly always generate a class per indy callsite, which 
> means we might end up with a higher total number of classes generated and 
> loaded in applications if we set this value too low. It may also produce 
> worse performance on average. On the other hand there is the observed 
> increase in C2 resource usage as expressions grow unwieldy. On the other 
> other hand high arity expressions are likely rare to begin with, with less 
> opportunities for sharing than the more common low-arity expressions. 
> 
> I turned the submitted test case into a few JMH benchmarks and did some 
> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`:
> 
> Baseline strategy:
> 13 args: 6.3M
> 23 args: 18M
> 123 args: 868M
> 
> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`:
> 13 args: 2.11M
> 23 args: 3.67M
> 123 args: 4.75M
> 
> For 123 args the memory overhead of the baseline strategy is 180x, but for 23 
> args we're down to a 5x memory overhead, and down to a 3x overhead for 13 
> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) 
> I've conservatively chosen a threshold at arity 20. This keeps C2 resource 
> pressure at a reasonable level (< 18M) while avoiding perturbing performance 
> at the vast majority of call sites.
> 
> I was asked to use the new class file API for mainline. There's a version of 
> this patch implemented using ASM in 7c52a9f which might be a reasonable basis 
> for a backport.

One class per arity + value classes can be a good combo ?

-

PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2051792260

Re: RFR: 8322292: Rearrange comparison of fields in Record.equals()

[Rémi Forax]

On Tue, 19 Dec 2023 11:01:12 GMT, Hannes Greule  wrote:

> Isn't Arrays.equals() used under the hood?
No, for arrays == is used

-

PR Comment: https://git.openjdk.org/jdk/pull/17143#issuecomment-1863374656

Re: RFR: 8319123: Implementation of JEP-461: Stream Gatherers (Preview) [v7]

[Rémi Forax]

On Tue, 14 Nov 2023 07:51:42 GMT, Viktor Klang  wrote:

>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)
>
> Viktor Klang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressing last review feedback

Hello,
the relation between a stateless gatherer (default initializer) and the default 
combiner are not obvious to me.

A default initializer means stateless and a default combiner means sequential, 
so if i want the integrator of my stateless gatherer to be called in parallel, 
i need to not use the default combiner but at the same time the combiner I will 
pass as parameter will not be called because the gatherer is stateless ?

Having to create a combiner that will be not called seems weird. I'm sure i'm 
missing something ?

-

PR Comment: https://git.openjdk.org/jdk/pull/16420#issuecomment-1809873419

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/Gatherers.java line 64:

> 62:  *needlessly
> 63:  * 3. allows for more efficient composition and evaluation
> 64:  */

The downside is that if this object escape there will be spurious 
ClassCastException where the compiler insert cast for erasure that are hard to 
diagnose.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387015480

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/GathererOp.java line 232:

> 230:  */
> 231: @SuppressWarnings("unchecked")
> 232: private GathererOp(Gatherer gatherer, GathererOp 
> upstream) {

Is it not better to use a static method named by example `fuse`, i don't 
understand why it has to be a constructor.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386996068

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/GathererOp.java line 448:

> 446: private final long targetSize;
> 447: private final Hybrid leftPredecessor;
> 448: private final AtomicBoolean cancelled;

It could be a volatile boolean and a comment is needed saying this field is 
only be accessed if greedy is true. And inside the constructors this should be 
the last field of the constructor to be initialized

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1387006826

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/GathererOp.java line 180:

> 178: finisher.accept(state, this);
> 179: sink.end();
> 180: state = null; // GC assistance

Not sure it really helps the GC, given that it may go through a GC barrier

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386990459

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/GathererOp.java line 95:

> 93: public void accept(X x) {
> 94: final var b = rightMost;
> 95: (b == null ? (rightMost = new NodeBuilder.Builder<>()) : 
> b).accept(x);

I think this code is unecessarily hard to read.

var rightMost = this.rightMost;
if (rightMost == null) {
  rightMost = this.rightMost = new NodeBuilder.Builder<>();
}
rightMost.accept(x);

src/java.base/share/classes/java/util/stream/GathererOp.java line 99:

> 97: 
> 98: public NodeBuilder join(NodeBuilder that) {
> 99: if (isEmpty())

Please add curly braces around if and if/else

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386982341
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386984238

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/GathererOp.java line 50:

> 48:  */
> 49: final class GathererOp extends ReferencePipeline {
> 50: @SuppressWarnings("unchecked")

Could you explain why you need a SuppressWarnings here, especially the cast to 
(ReferencePipeline) in the else case

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386975669

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Wed, 8 Nov 2023 17:22:05 GMT, Rémi Forax  wrote:

>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)
>
> src/java.base/share/classes/java/util/stream/Gatherer.java line 490:
> 
>> 488:  * more elements sent to it, {@code false} if otherwise
>> 489:  */
>> 490: default boolean isRejecting() { return false; }
> 
> Is it really a good default ?

It is not better to have 2 Downstream (Downstream and a subtype) like you hae 
two integrators (Integrator and Greedy)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386972223

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/Gatherer.java line 444:

> 442:  */
> 443: static  Gatherer of(
> 444: Supplier initializer,

wildcards are missing here too, Supplier, Integrator and BiConsumer>.

src/java.base/share/classes/java/util/stream/Gatherer.java line 490:

> 488:  * more elements sent to it, {@code false} if otherwise
> 489:  */
> 490: default boolean isRejecting() { return false; }

Is it really a good default ?

src/java.base/share/classes/java/util/stream/Gatherer.java line 530:

> 528:  * @param  the type of results this integrator can produce
> 529:  */
> 530: @ForceInline

If we add this kind of the methods, we should add them on all function 
interfaces of java.util.function and java.util.stream.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386965623
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386966965
PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386970112

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Wed, 8 Nov 2023 15:17:57 GMT, Viktor Klang  wrote:

>> It's still possible to have a situation where PECS signature could be 
>> useful, and I don't see any downsides. A user may want to reuse the same 
>> static integrator instead of creating several identical lambdas just because 
>> the target type is more specific. Sometimes, people complain later, and 
>> you'll have to fix this. For example, see that `Stream.generate()` [was 
>> fixed separately](https://bugs.openjdk.org/browse/JDK-8132097). So why not 
>> doing this from the very beginning?
>
> @amaembo In this case, given that it is intended as Preview, I think it makes 
> sense to receive some user feedback on this. Note that Collector has been 
> without PECS for Collector.of since 1.8: 
> [of](https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collector.html#of-java.util.function.Supplier-java.util.function.BiConsumer-java.util.function.BinaryOperator-java.util.function.Function-java.util.stream.Collector.Characteristics...-)
> 
> Right now I don't want to deviate from the Collector-heritage until there's 
> more usage feedback, and I'm definitely not ruling out that PECS:ing the 
> factories might be worth it.

I don't see the point of waiting here. Usually requests for PECSing signatures 
always comes two or three years after the API is realing when people starts to 
use the API in anger so the preview time is likely not enough.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386958540

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Sun, 5 Nov 2023 16:18:52 GMT, Tagir F. Valeev  wrote:

>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)
>
> src/java.base/share/classes/java/util/stream/Gatherer.java line 330:
> 
>> 328: static  Gatherer ofSequential(
>> 329: Integrator integrator,
>> 330: BiConsumer> finisher) {
> 
> Probably, accepting `Consumer>` and adapting it would 
> be more user-friendly?

In that case the integrator should be a BiConsumer too, no ?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386961034

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Wed, 8 Nov 2023 17:10:05 GMT, Tagir F. Valeev  wrote:

>> Has this proven to be a problem for things like 
>> [Collectors.mapping(…)](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Collectors.html#mapping(java.util.function.Function,java.util.stream.Collector))
>>  ? 樂 
>> 
>> There's one implication on turning it into a wildcard—it may actually cause 
>> issues implementing the composition directly in the override, as you won't 
>> have a typename to refer to.
>> 
>> Do you have a concrete example of where the current "encoding" causes a 
>> caller-problem?
>
> The API should be client-friendly, not implementor-friendly, given that it's 
> expected to have much more clients than implementors. An implementor can 
> easily delegate to a private method to add missing type parameters. I did 
> exactly this in the 
> [teeing](https://github.com/openjdk/jdk/blob/7d25f1c6cb770e21cfad8096c1637a24e65fab8c/src/java.base/share/classes/java/util/stream/Collectors.java#L1913)
>  Collector before. There should not be a reason to expose a parameter, which 
> is merely an implementation detail. This also complicates the public 
> documentation of the already complex feature, specifying the parameter which 
> is not necessary to specify. 
> 
>> Has this proven to be a problem for things like 
>> [Collectors.mapping(…)](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Collectors.html#mapping(java.util.function.Function,java.util.stream.Collector))
>>  ?
> 
> I'm not sure how I can prove this, given that you don't usually know the 
> downstream accumulator, so if somebody hits this problem, they will be forced 
> to solve it in another way (e.g., extracting parts of complex collector to 
> separate variables), so you won't find the code in the wild which uses 
> `mapping` type arguments explicitly. Yes, `Collectors.mapping` and 
> `groupingBy` did this mistake in the past, and there's no reason to repeat it 
> again.
> 
>> Do you have a concrete example of where the current "encoding" causes a 
>> caller-problem?
> 
> For example, I want to create a reusable gatherer that performs scan-concat 
> and wraps every resulting string with '[...]'. First try:
> 
> 
> var gatherer = Gatherers.scan(() -> "", String::concat)
>   .andThen(Gatherer.ofSequential((_, t, pusher) -> pusher.push(STR."[{t}]")));
> 
> 
> Now, the type of `gatherer` is `Gatherer`, but I want 
> `Gatherer`. I have the following alternatives:
> 1. Specify the variable type explicitly:
> 
> Gatherer gatherer = Gatherers.scan(() -> "", 
> String::concat)
> .andThen(Gatherer.ofSequential((_, t, pusher) -> 
> pusher.push(STR."[{t}]")));
> 
> 2. Specify `ofSequential` type arguments:
> 
> var gatherer = Gatherers.scan(() -> "", String::concat)
> .andThen(Gatherer.ofSequential((_, t, pusher) -> 
> pusher.push(STR."[{t}]")));
> 
> 3. Specify the types of lambda arguments explicitly:
> 
> var gatherer = Gatherers.scan(() -> "", String::concat)
> .andThen(Gatherer.ofSequential((Void _, String t, Downstream String> pusher) -> pusher.push(STR."[{t}]")));
> 
> 4. Specify `.andThen` type a...

I agree with Tagir here, it should be an ? and if you want to capture it in a 
type variable you can use the standard trick of having the private method 
declaring the type variable and the public method with the wildcard calling the 
private method.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386954327

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Tue, 31 Oct 2023 13:19:30 GMT, Viktor Klang  wrote:

>> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)
>
> src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 242:
> 
>> 240: }
>> 241: 
>> 242: // Terminal evaluation methods
> 
> This is needed in order to let subclasses override terminal operations such 
> as, but not limited to, `collect`. (since a terminal operation needs to mark 
> the entire pipeline as consumed).

It's not clear in which case the exception is thrown from the POV of a user of 
the API.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386945056

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Fri, 3 Nov 2023 16:51:34 GMT, Viktor Klang  wrote:

>> Since stream facilities are package-private, we can just use no access 
>> modifier and remove all new `protected` access modifier (on methods, fields, 
>> constructors) in this PR.
>
> @liach We could do that, yet I'm not sure that is strictly an improvement. 
> What would be the benefit in doing so specifically for this PR?

It is an improvement, it reduces the visibility and `protected` is rarely used 
in modern code because inheriting from a class from a package you do not 
control means that the API of the subclass is not stable.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386943330

Re: RFR: JDK-8319123 : Implementation of JEP-461: Stream Gatherers (Preview)

[Rémi Forax]

On Mon, 30 Oct 2023 15:38:35 GMT, Viktor Klang  wrote:

> This Pull-Request implements [JEP-461](https://openjdk.org/jeps/461)

src/java.base/share/classes/java/util/stream/AbstractPipeline.java line 223:

> 221:  * consumed
> 222:  */
> 223: protected AbstractPipeline(AbstractPipeline 
> previousPreviousStage, AbstractPipeline previousStage, int 
> opFlags) {

Same here

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16420#discussion_r1386943713

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow [v3]

[Rémi Forax]

On Fri, 3 Nov 2023 16:43:30 GMT, Jan Lahoda  wrote:

>> Consider code like:
>> 
>> void test(Object o) {
>> switch (o) {
>> case X1 -> {}
>> case X2 -> {}
>> ...(about 100 cases)
>> ``` 
>> 
>> javac will compile the switch into a switch whose selector is an indy 
>> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
>> types in the cases.
>> 
>> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
>> performing `instanceof` checks between the switch's selector and the given 
>> case type. The problem is that when the number of cases is high enough, 
>> (more than ~40-50), the chain gets too long, and the tests won't inline 
>> anymore. This then leads to a very bad performance, when compared to 
>> manually written if-instanceof-else-if-instanceof- chain.
>> 
>> The proposal herein is to use bytecode (written using the ClassFile 
>> API/library) instead of the `MethodHandle`s chain. The overall performance 
>> of this seems to be similar to the manually written 
>> if-instanceof-else-if-instanceof- chain.
>> 
>> Using the benchmark from the bug, and this patch, I am getting:
>> 
>> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
>> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
>> 
>> 
>> The most tricky part of this new way to generate the tests is handling of 
>> non-type case labels, and in particular cases with enum constant labels. The 
>> resolution of enum constants is deferred as much as possible, by using an 
>> indirection through the `ResolvedEnumLabels`.
>> 
>> Further improvements may be possible, esp. for some specific cases (like all 
>> cases having a type, and the type being a final class).
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reflecting review feedback.

Looks good to me :)

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

> 72: private static final MethodHandle MAPPED_ENUM_LOOKUP;
> 73: 
> 74: private static final MethodTypeDesc typesSwitchDescriptor =

Given that it's a static final, the name should be in uppercase, 
`TYPES_SWITCH_DESCRIPTOR`

-

PR Comment: https://git.openjdk.org/jdk/pull/16489#issuecomment-1792807480
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381980130

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow [v2]

[Rémi Forax]

On Fri, 3 Nov 2023 15:32:34 GMT, Jan Lahoda  wrote:

>> Consider code like:
>> 
>> void test(Object o) {
>> switch (o) {
>> case X1 -> {}
>> case X2 -> {}
>> ...(about 100 cases)
>> ``` 
>> 
>> javac will compile the switch into a switch whose selector is an indy 
>> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
>> types in the cases.
>> 
>> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
>> performing `instanceof` checks between the switch's selector and the given 
>> case type. The problem is that when the number of cases is high enough, 
>> (more than ~40-50), the chain gets too long, and the tests won't inline 
>> anymore. This then leads to a very bad performance, when compared to 
>> manually written if-instanceof-else-if-instanceof- chain.
>> 
>> The proposal herein is to use bytecode (written using the ClassFile 
>> API/library) instead of the `MethodHandle`s chain. The overall performance 
>> of this seems to be similar to the manually written 
>> if-instanceof-else-if-instanceof- chain.
>> 
>> Using the benchmark from the bug, and this patch, I am getting:
>> 
>> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
>> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
>> 
>> 
>> The most tricky part of this new way to generate the tests is handling of 
>> non-type case labels, and in particular cases with enum constant labels. The 
>> resolution of enum constants is deferred as much as possible, by using an 
>> indirection through the `ResolvedEnumLabels`.
>> 
>> Further improvements may be possible, esp. for some specific cases (like all 
>> cases having a type, and the type being a final class).
>
> Jan Lahoda has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Some more get->orElseThrow
>  - Reflecting review feedback.

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

> 435: cb.aload(3);
> 436: 
> cb.constantInstruction(extraClassLabels.size());
> 437: cb.aaload();

Arrays are mutable in Java, so the VM can not know if the array of non 
denotable classes (`extraClassLabels`) will be changed or not so the result of 
aaload is not a constant so the call to isInstance can not be optimized. Using 
a immutable list (`List.of()`) instead of an array should work, because all the 
implementation of List.of() are using @Stable. In that case aaload becomes 
invokevirtual List.get().

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381915611

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow

[Rémi Forax]

On Fri, 3 Nov 2023 15:29:05 GMT, Jan Lahoda  wrote:

> Thanks for all the comments so far - I think I've either reflected them, or 
> wrote a comment to each of them. Please let me know if there's something 
> else, or if I've forgotten something.

You idea to use an extra array is clever. Using an immutable List instead of an 
array should allow the VM to constant fold the Class.isInstance (see above).

-

PR Comment: https://git.openjdk.org/jdk/pull/16489#issuecomment-1792698583

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow [v2]

[Rémi Forax]

On Fri, 3 Nov 2023 15:32:34 GMT, Jan Lahoda  wrote:

>> Consider code like:
>> 
>> void test(Object o) {
>> switch (o) {
>> case X1 -> {}
>> case X2 -> {}
>> ...(about 100 cases)
>> ``` 
>> 
>> javac will compile the switch into a switch whose selector is an indy 
>> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
>> types in the cases.
>> 
>> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
>> performing `instanceof` checks between the switch's selector and the given 
>> case type. The problem is that when the number of cases is high enough, 
>> (more than ~40-50), the chain gets too long, and the tests won't inline 
>> anymore. This then leads to a very bad performance, when compared to 
>> manually written if-instanceof-else-if-instanceof- chain.
>> 
>> The proposal herein is to use bytecode (written using the ClassFile 
>> API/library) instead of the `MethodHandle`s chain. The overall performance 
>> of this seems to be similar to the manually written 
>> if-instanceof-else-if-instanceof- chain.
>> 
>> Using the benchmark from the bug, and this patch, I am getting:
>> 
>> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
>> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
>> 
>> 
>> The most tricky part of this new way to generate the tests is handling of 
>> non-type case labels, and in particular cases with enum constant labels. The 
>> resolution of enum constants is deferred as much as possible, by using an 
>> indirection through the `ResolvedEnumLabels`.
>> 
>> Further improvements may be possible, esp. for some specific cases (like all 
>> cases having a type, and the type being a final class).
>
> Jan Lahoda has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Some more get->orElseThrow
>  - Reflecting review feedback.

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

> 427: Label next = element.next();
> 428: cb.labelBinding(element.target());
> 429: if (element.caseLabel() instanceof Class 
> classLabel &&

I think you can do a if else of isPresent inside instanceof Class to avoid 
to reapeat the instanceof and store `classLabel.describeConstable()` into a 
local variable.

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

> 515:  
>  BiPredicate.class,
> 516:  
>  Class[].class));
> 517: return MethodHandles.insertArguments(typeSwitch, 2, new 
> ResolvedEnumLabels(caller, enumDescs.toArray(s -> new EnumDesc[s])),

you can use the method reference `EnumDesc[]::new`instead of `s -> new 
EnumDesc[s]` and same below Class[]::new  (the wirldcard should not be 
necessary)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381910073
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381908291

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow [v2]

[Rémi Forax]

On Fri, 3 Nov 2023 15:32:34 GMT, Jan Lahoda  wrote:

>> Consider code like:
>> 
>> void test(Object o) {
>> switch (o) {
>> case X1 -> {}
>> case X2 -> {}
>> ...(about 100 cases)
>> ``` 
>> 
>> javac will compile the switch into a switch whose selector is an indy 
>> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
>> types in the cases.
>> 
>> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
>> performing `instanceof` checks between the switch's selector and the given 
>> case type. The problem is that when the number of cases is high enough, 
>> (more than ~40-50), the chain gets too long, and the tests won't inline 
>> anymore. This then leads to a very bad performance, when compared to 
>> manually written if-instanceof-else-if-instanceof- chain.
>> 
>> The proposal herein is to use bytecode (written using the ClassFile 
>> API/library) instead of the `MethodHandle`s chain. The overall performance 
>> of this seems to be similar to the manually written 
>> if-instanceof-else-if-instanceof- chain.
>> 
>> Using the benchmark from the bug, and this patch, I am getting:
>> 
>> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
>> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
>> 
>> 
>> The most tricky part of this new way to generate the tests is handling of 
>> non-type case labels, and in particular cases with enum constant labels. The 
>> resolution of enum constants is deferred as much as possible, by using an 
>> indirection through the `ResolvedEnumLabels`.
>> 
>> Further improvements may be possible, esp. for some specific cases (like all 
>> cases having a type, and the type being a final class).
>
> Jan Lahoda has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Some more get->orElseThrow
>  - Reflecting review feedback.

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

> 383: 
> 384: byte[] classBytes = 
> Classfile.of().build(ClassDesc.of(typeSwitchClassName(caller.lookupClass())), 
> clb -> {
> 385: clb.withFlags(AccessFlag.FINAL, AccessFlag.SYNTHETIC)

`AccessFlag.SUPER` is missing, this will make this class a value class in the 
Valhalla repo

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381896567

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow

[Rémi Forax]

On Fri, 3 Nov 2023 08:41:12 GMT, Jan Lahoda  wrote:

> Consider code like:
> 
> void test(Object o) {
> switch (o) {
> case X1 -> {}
> case X2 -> {}
> ...(about 100 cases)
> ``` 
> 
> javac will compile the switch into a switch whose selector is an indy 
> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
> types in the cases.
> 
> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
> performing `instanceof` checks between the switch's selector and the given 
> case type. The problem is that when the number of cases is high enough, (more 
> than ~40-50), the chain gets too long, and the tests won't inline anymore. 
> This then leads to a very bad performance, when compared to manually written 
> if-instanceof-else-if-instanceof- chain.
> 
> The proposal herein is to use bytecode (written using the ClassFile 
> API/library) instead of the `MethodHandle`s chain. The overall performance of 
> this seems to be similar to the manually written 
> if-instanceof-else-if-instanceof- chain.
> 
> Using the benchmark from the bug, and this patch, I am getting:
> 
> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
> 
> 
> The most tricky part of this new way to generate the tests is handling of 
> non-type case labels, and in particular cases with enum constant labels. The 
> resolution of enum constants is deferred as much as possible, by using an 
> indirection through the `ResolvedEnumLabels`.
> 
> Further improvements may be possible, esp. for some specific cases (like all 
> cases having a type, and the type being a final class).

Here is a test that uses a hidden class that works with the current 
implementation.
If i'm not mistaken, the proposed implementation fails that test. 


public class SwitchBootstrapTest {
  public static void main(String[] args) throws Throwable {
var className = SwitchBootstrapTest.class.getName();
byte[] data;
try(var input = SwitchBootstrapTest.class.getResourceAsStream('/' + 
className.replace('.', '/') + ".class")) {
  data = input.readAllBytes();
}

var lookup = MethodHandles.lookup().defineHiddenClass(data, true, 
ClassOption.NESTMATE, ClassOption.STRONG);
var hiddenClass = lookup.lookupClass();
var constructor = lookup.findConstructor(hiddenClass, 
methodType(void.class));
var instance = constructor.invoke();

var methodType = methodType(int.class, Object.class, int.class);
var callSite = SwitchBootstraps.typeSwitch(lookup, "", methodType, 
hiddenClass, Object.class);
var index = (int) callSite.getTarget().invokeExact(instance, 0);
System.out.println("index " + index);
  }
}

-

PR Comment: https://git.openjdk.org/jdk/pull/16489#issuecomment-1792629864

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow

[Rémi Forax]

On Fri, 3 Nov 2023 08:41:12 GMT, Jan Lahoda  wrote:

> Consider code like:
> 
> void test(Object o) {
> switch (o) {
> case X1 -> {}
> case X2 -> {}
> ...(about 100 cases)
> ``` 
> 
> javac will compile the switch into a switch whose selector is an indy 
> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
> types in the cases.
> 
> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
> performing `instanceof` checks between the switch's selector and the given 
> case type. The problem is that when the number of cases is high enough, (more 
> than ~40-50), the chain gets too long, and the tests won't inline anymore. 
> This then leads to a very bad performance, when compared to manually written 
> if-instanceof-else-if-instanceof- chain.
> 
> The proposal herein is to use bytecode (written using the ClassFile 
> API/library) instead of the `MethodHandle`s chain. The overall performance of 
> this seems to be similar to the manually written 
> if-instanceof-else-if-instanceof- chain.
> 
> Using the benchmark from the bug, and this patch, I am getting:
> 
> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
> 
> 
> The most tricky part of this new way to generate the tests is handling of 
> non-type case labels, and in particular cases with enum constant labels. The 
> resolution of enum constants is deferred as much as possible, by using an 
> indirection through the `ResolvedEnumLabels`.
> 
> Further improvements may be possible, esp. for some specific cases (like all 
> cases having a type, and the type being a final class).

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

> 423: if (element.label() instanceof Class 
> classLabel) {
> 424: cb.aload(0);
> 425: 
> cb.instanceof_(classLabel.describeConstable().get());

You should use orElseThrow() (see the javadoc) instead of get(). This code does 
not work because hidden class Class are not denotable. My first idea was that 
it was not a real problem because no code generated by javac will use an hidden 
class but this is not true. A type pattern can use the current class and this 
class can be loaded by a defineHiddenClass.

A way to solve that issue is to store the non denotable classes inside a list 
and send that list as a class data when calling defineHiddenClass.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381775426

Re: RFR: 8319220: Pattern matching switch with a lot of cases is unduly slow

[Rémi Forax]

On Fri, 3 Nov 2023 08:41:12 GMT, Jan Lahoda  wrote:

> Consider code like:
> 
> void test(Object o) {
> switch (o) {
> case X1 -> {}
> case X2 -> {}
> ...(about 100 cases)
> ``` 
> 
> javac will compile the switch into a switch whose selector is an indy 
> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the 
> types in the cases.
> 
> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s 
> performing `instanceof` checks between the switch's selector and the given 
> case type. The problem is that when the number of cases is high enough, (more 
> than ~40-50), the chain gets too long, and the tests won't inline anymore. 
> This then leads to a very bad performance, when compared to manually written 
> if-instanceof-else-if-instanceof- chain.
> 
> The proposal herein is to use bytecode (written using the ClassFile 
> API/library) instead of the `MethodHandle`s chain. The overall performance of 
> this seems to be similar to the manually written 
> if-instanceof-else-if-instanceof- chain.
> 
> Using the benchmark from the bug, and this patch, I am getting:
> 
> MyBenchmark.testIfElse100  thrpt5  521826.326 ± 7510.042  ops/s
> MyBenchmark.testSwitch100  thrpt5  505440.170 ± 3757.178  ops/s
> 
> 
> The most tricky part of this new way to generate the tests is handling of 
> non-type case labels, and in particular cases with enum constant labels. The 
> resolution of enum constants is deferred as much as possible, by using an 
> indirection through the `ResolvedEnumLabels`.
> 
> Further improvements may be possible, esp. for some specific cases (like all 
> cases having a type, and the type being a final class).

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

> 395: cb.iload(1);
> 396: Label dflt = cb.newLabel();
> 397: record Element(Label target, Label next, Object 
> label) {}

'label' is not a Label, is there a better name to make the difference between 
the switch label and the bytecode label

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

> 421: Label next = element.next();
> 422: cb.labelBinding(element.target());
> 423: if (element.label() instanceof Class 
> classLabel) {

It's too bad we can not use a switch on the label here instead of a bunch of 
instanceof :)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381754699
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381756247

Re: RFR: JDK-8315458 Implementation of Implicitly Declared Classes and Instance Main Method (Second Preview) [v2]

[Rémi Forax]

On Thu, 2 Nov 2023 12:24:31 GMT, Jim Laskey  wrote:

>> Uhmm. In the spec I see implicit classes named here:
>> 
>> 
>>  For reference, the following constructs are declared implicitly in source 
>> code, but are not marked as mandated because only formal parameters and 
>> modules can be so marked in a class file (JVMS §4.7.24, JVMS §4.7.25):
>>  ```
>> 
>> Note the `but are not marked as mandated`.
>> 
>> I don't think javac sets the mandated flag for any of these. Note that 
>> ACC_MANDATED has the same value as the ACC_MODULE flag, so setting that flag 
>> on a class w/o at least filtering out when writing the classfile will be 
>> problematic.
>
> Hmmm. I'll check with Gavin, but I thought in discussions Alex said that it 
> should be mandated. At any rate, seems the ACC_MANDATED should be removed for 
> stated reasons.

It should be MANDATED in theory given it's not an artifact of the compilation 
like SYNTHETIC is. Sadly, as Maurizio said ACC_MANDATED has the same value as 
ACC_MODULE.
I think there is a value to still make it as SYNTHETIC, apart if we add a new 
specific attribute for implicit classes.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16461#discussion_r1380604311

Re: RFR: 8315850: Improve AbstractMap anonymous Iterator classes [v3]

[Rémi Forax]

On Fri, 8 Sep 2023 18:55:29 GMT, altrisi  wrote:

>> Per Minborg has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Fix additional formating issue
>>  - Don't use polymorphism and reformat code
>
> With the recent changes this ends up just moving the iterator classes around 
> (from anonymous to inner) and making a field final. I don't understand how 
> fully splitting them (vs the previous `AbstractIterator` revision) helps 
> against polymorphism, given they both would store the same `Iterator` types 
> in the field (whatever the result of `entrySet` is), and the `next` method is 
> two different implementations anyway, that I'd assume would be treated 
> differently. Though I don't know enough about the JIT (or benchmarked this) 
> to know if it does make a difference.

@altrisi,
the profile stored by the VM is attached to a specific bytecode, if you share 
that bytecode, you share the profile.

see https://wiki.openjdk.org/display/HotSpot/MethodData

-

PR Comment: https://git.openjdk.org/jdk/pull/15615#issuecomment-1712119725

Re: RFR: 8315850: Improve AbstractMap anonymous Iterator classes

[Rémi Forax]

On Thu, 7 Sep 2023 18:05:51 GMT, Stuart Marks  wrote:

>> Hello,
>> In Java, sharing code may have a runtime cost (or not) depending on the 
>> shape of the code, because the VM may have to speculate on the class of some 
>> value at runtime.
>> Here, in hasNext() and remove(), the VM has to find the class of the field 
>> 'i' at runtime.
>> 
>> Iterators are performance sentive (they are used in for loop) and for that 
>> they need the escape analysis to work.
>> If the iterator code is polymorphic, so part of the code may be unknown so 
>> the escape analysis will consider that the iterator escape. So usually, it's 
>> a good practice to not share the iterator code.
>> 
>> Also the field should be package private (or even maybe private) but not 
>> protected. Protected is very rare in Java because usually classes that share 
>> implementation details are in the same package (or in the same nestmate 
>> nest).
>
> @forax Note that HashMap uses a similar subclassing idiom for the iterators 
> of the keySet, values, and entrySet collections in order to share the 
> iterator logic:
> 
> https://github.com/openjdk/jdk/blob/jdk-21%2B35/src/java.base/share/classes/java/util/HashMap.java#L1626

Hi @stuart-marks ,
for HashMap, all the fields of HashIterator have only one implementation, in 
this patch, the field of AbstractIterator is typed Iterator (so multiple 
implementations at runtime). This is not the same code shape.

I think the best is to write a simple to test that iterate over both the 
keySet() and the values() of an implementation of AbstractMap that only defines 
size() and get() and use -XX:+PrintInlining to see if the VM reports a profile 
pollution for the code before and after this change.

-

PR Comment: https://git.openjdk.org/jdk/pull/15615#issuecomment-1710632697

Re: RFR: 8315850: Improve AbstractMap anonymous Iterator classes

[Rémi Forax]

On Thu, 7 Sep 2023 11:48:51 GMT, Per Minborg  wrote:

> This PR proposes to slightly improve some iterators of `AbstractMap`:
> 
> * Code reuse
> * A field declared `final`
> * Add missing `@Override` annotations

Hello,
In Java, sharing code may have a runtime cost (or not) depending on the shape 
of the code, because the VM may have to speculate on the class of some value at 
runtime.
Here, in hasNext() and remove(), the VM has to find the class of the field 'i' 
at runtime.

Iterators are performance sentive (they are used in for loop) and for that they 
need the escape analysis to work.
If the iterator code is polymorphic, so part of the code may be unknown so the 
escape analysis will consider that the iterator escape. So usually, it's a good 
practice to not share the iterator code.

Also the field should be package private (or even maybe private) but not 
protected. Protected is very rare in Java because usually classes that share 
implementation details are in the same package (or in the same nestmate nest).

-

PR Comment: https://git.openjdk.org/jdk/pull/15615#issuecomment-1710058719

Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v6]

[Rémi Forax]

On Fri, 25 Aug 2023 22:22:43 GMT, Mandy Chung  wrote:

>> 8268829: Provide an optimized way to walk the stack with Class object only
>> 
>> `StackWalker::walk` creates one `StackFrame` per frame and the current 
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some 
>> frameworks
>> like logging may only interest in the Class object but not the method name 
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller 
>> class.  It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter 
>> out the element.
>> 
>> This PR proposes to add `StackWalker.Kind` enum to specify the information 
>> that a stack walker
>> collects.  If no method information is needed, a `StackWalker` of 
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the 
>> method information
>> and (2) the memory used for the stack walking.   In addition, this can also 
>> fix
>> 
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): 
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>> 
>> New factory methods to take a parameter to specify the kind of stack walker 
>> to be created are defined.
>> This provides a simple way for existing code, for example logging 
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function 
>> for traversing
>> `StackFrame`s.
>> 
>> For example: to find the first caller filtering a known list of 
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create 
>> a stack walker instance:
>> 
>> 
>>  StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, 
>> Option.RETAIN_CLASS_REFERENCE);
>>  Optional> callerClass = walker.walk(s ->
>>  s.map(StackFrame::getDeclaringClass)
>>   .filter(interestingClasses::contains)
>>   .findFirst());
>> 
>> 
>> If method information is accessed on the `StackFrame`s produced by this 
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be 
>> thrown.
>> 
>>  Javadoc & specdiff
>> 
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>> 
>>  Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>> 
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`.  Existing code would hav...
>
> Mandy Chung has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - fixup javadoc
>  - Review feedback: move JLIA to ClassFrameInfo

src/java.base/share/classes/java/lang/StackWalker.java line 81:

> 79:  * Optional> callerClass = walker.walk(s ->
> 80:  * s.map(StackFrame::getDeclaringClass)
> 81:  *  .filter(interestingClasses::contains)

Usually it's more `.filter(Predicate.not(implClasses::contains))`

src/java.base/share/classes/java/lang/StackWalker.java line 88:

> 86:  * {@snippet lang="java" :
> 87:  * List stack = StackWalker.getInstance().walk(s ->
> 88:  * s.limit(10).collect(Collectors.toList()));

`s.limit(10).toList()`

src/java.base/share/classes/java/lang/StackWalker.java line 505:

> 503:  */
> 504: public static StackWalker getInstance(Kind kind, Option... options) {
> 505: return getInstance(Objects.requireNonNull(kind), 
> Set.of(Objects.requireNonNull(options)));

Set.of() already does a NPE check

src/java.base/share/classes/java/lang/StackWalker.java line 649:

> 647:  * s.dropWhile(f -> f.getClassName().startsWith("com.foo."))
> 648:  *  .limit(10)
> 649:  *  .collect(Collectors.toList()));

`.toList())`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306390180
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306390266
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306390553
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306390713

Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v6]

[Rémi Forax]

On Fri, 25 Aug 2023 22:22:43 GMT, Mandy Chung  wrote:

>> 8268829: Provide an optimized way to walk the stack with Class object only
>> 
>> `StackWalker::walk` creates one `StackFrame` per frame and the current 
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some 
>> frameworks
>> like logging may only interest in the Class object but not the method name 
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller 
>> class.  It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter 
>> out the element.
>> 
>> This PR proposes to add `StackWalker.Kind` enum to specify the information 
>> that a stack walker
>> collects.  If no method information is needed, a `StackWalker` of 
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the 
>> method information
>> and (2) the memory used for the stack walking.   In addition, this can also 
>> fix
>> 
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): 
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>> 
>> New factory methods to take a parameter to specify the kind of stack walker 
>> to be created are defined.
>> This provides a simple way for existing code, for example logging 
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function 
>> for traversing
>> `StackFrame`s.
>> 
>> For example: to find the first caller filtering a known list of 
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create 
>> a stack walker instance:
>> 
>> 
>>  StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, 
>> Option.RETAIN_CLASS_REFERENCE);
>>  Optional> callerClass = walker.walk(s ->
>>  s.map(StackFrame::getDeclaringClass)
>>   .filter(interestingClasses::contains)
>>   .findFirst());
>> 
>> 
>> If method information is accessed on the `StackFrame`s produced by this 
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be 
>> thrown.
>> 
>>  Javadoc & specdiff
>> 
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>> 
>>  Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>> 
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`.  Existing code would hav...
>
> Mandy Chung has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - fixup javadoc
>  - Review feedback: move JLIA to ClassFrameInfo

src/java.base/share/classes/java/lang/StackFrameInfo.java line 82:

> 80: 
> 81: {
> 82: // Get a snapshot of type which doesn't get changed by racing 
> threads.

Also the constructed MethodType is non mutable

src/java.base/share/classes/java/lang/StackFrameInfo.java line 94:

> 92: type = JLIA.getMethodType(sig, 
> declaringClass().getClassLoader());
> 93: }
> 94: assert type instanceof MethodType : "bad method type " + type;

Not sure this assert is useful given the next instruction is a cast to 
MethodType

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306389071
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306389160

Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v6]

[Rémi Forax]

On Fri, 25 Aug 2023 22:22:43 GMT, Mandy Chung  wrote:

>> 8268829: Provide an optimized way to walk the stack with Class object only
>> 
>> `StackWalker::walk` creates one `StackFrame` per frame and the current 
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some 
>> frameworks
>> like logging may only interest in the Class object but not the method name 
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller 
>> class.  It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter 
>> out the element.
>> 
>> This PR proposes to add `StackWalker.Kind` enum to specify the information 
>> that a stack walker
>> collects.  If no method information is needed, a `StackWalker` of 
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the 
>> method information
>> and (2) the memory used for the stack walking.   In addition, this can also 
>> fix
>> 
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): 
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>> 
>> New factory methods to take a parameter to specify the kind of stack walker 
>> to be created are defined.
>> This provides a simple way for existing code, for example logging 
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function 
>> for traversing
>> `StackFrame`s.
>> 
>> For example: to find the first caller filtering a known list of 
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create 
>> a stack walker instance:
>> 
>> 
>>  StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, 
>> Option.RETAIN_CLASS_REFERENCE);
>>  Optional> callerClass = walker.walk(s ->
>>  s.map(StackFrame::getDeclaringClass)
>>   .filter(interestingClasses::contains)
>>   .findFirst());
>> 
>> 
>> If method information is accessed on the `StackFrame`s produced by this 
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be 
>> thrown.
>> 
>>  Javadoc & specdiff
>> 
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>> 
>>  Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>> 
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`.  Existing code would hav...
>
> Mandy Chung has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - fixup javadoc
>  - Review feedback: move JLIA to ClassFrameInfo

src/java.base/share/classes/java/lang/ClassFrameInfo.java line 34:

> 32: static final JavaLangInvokeAccess JLIA = 
> SharedSecrets.getJavaLangInvokeAccess();
> 33: 
> 34: protected Object classOrMemberName;// Class or ResolvedMemberName 
> initialized by VM

I believe that the package-private access is enough here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306387977

Re: RFR: 8266571: Sequenced Collections [v12]

[Rémi Forax]

On Fri, 21 Jul 2023 16:19:35 GMT, Hollis Waite  wrote:

>  a SequencedCollection is backed by a doubly linked list. It's up to the user 
> to understand performance implications.

Beware of this kind of thinking, implementations of collections in the JDK 
change.
Many hashtable implementations in other langages are not using chaining anymore 
 because open adressing allows collisions to stay in the same cache line and/or 
the code to be vectorized, so HashMap/linkedHashMap implementations may not be 
the same in a future version of the JDK.

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1645982619

Re: RFR: 8266571: Sequenced Collections [v12]

[Rémi Forax]

On Fri, 21 Jul 2023 11:16:11 GMT, Hollis Waite  wrote:

>> Stuart Marks has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 96 commits:
>> 
>>  - Merge branch 'master' into JDK-8266571-SequencedCollections
>>  - Optimizations for ReverseOrderListView; check indexes in reversed domain.
>>  - Wording tweaks to SequencedMap / NavigableMap.
>>  - Change "The implementation in this class" to "... interface."
>>  - Delegate more methods in the views of ReverseOrderSortedMapView.
>>  - Add missing @throws and @since tags.
>>  - Convert code samples to snippets.
>>  - Various editorial changes.
>>  - Fix up toArray(T[]) on reverse-ordered views.
>>  - Remove unnecessary 'final' from a couple places.
>>  - ... and 86 more: https://git.openjdk.org/jdk/compare/2ea62c13...2827aa69
>
> It'd be convenient if SequencedCollection overrode Iterable.iterator() to 
> return a ListIterator. That would make it simpler to derive a List from 
> LinkedHashMap.values().

@hwaite, the problem is that the derived List will not be a random access List, 
so it will perform badly with any code that loop over List.get() (yes, you are 
not supposed to do that but sadly a lot of people do).

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1645649005

Re: RFR: 8306647: Implementation of Structured Concurrency (Preview) [v2]

[Rémi Forax]

On Thu, 25 May 2023 08:21:39 GMT, Alan Bateman  wrote:

>> This is the implementation of:
>> 
>> - JEP 453: Structured Concurrency (Preview)
>> - JEP 446: Scoped Values (Preview)
>> 
>> For the most part, this is just moving code and tests.  StructuredTaskScope 
>> moves to j.u.concurrent as a preview API, ScopedValue moves to j.lang as a 
>> preview API, and module jdk.incubator.concurrent has been removed. The 
>> significant API changes since incubator are:
>> 
>> - StructuredTaskScope.fork returns Subtask instead of Future (JEP 453 has a 
>> section on this)
>> - ScopedValue.where methods are replaced with runWhere, callWhere and 
>> getWhere
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 12 commits:
> 
>  - Sync up from loom repo
>  - Remove csm.Threads
>  - Merge
>  - Test should not be in update for main line
>  - Sync with loom repo
>  - Sync up tests frmo loom repo
>  - Sync up with loom repo
>  - Sync update API/impl/tests
>  - Merge
>  - Sync up with loom repo
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/89f2d458...e92ba524

I've rewritten all my tests to use the new API, it works !
https://github.com/forax/loom-fiber/tree/java21

One surprising thing is that Subtask.get() give less leeway to the owner thread 
than to the other virtual threads so in onComplete() storing a Subtask to use 
it later by the owner thread does not work well if join() is not called yet.
This is perhaps a bit too much.

Anyway, good for me !

-

PR Comment: https://git.openjdk.org/jdk/pull/13932#issuecomment-1568813363

Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v20]

[Rémi Forax]

On Fri, 26 May 2023 06:07:05 GMT, Joe Darcy  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Improving error recovery in presence of less important syntactic errors in 
>> top-level methods and fields.
>>   
>>   Author: Jan Lahoda 
>
> test/jdk/tools/launcher/InstanceMainTest.java line 31:
> 
>> 29:  * @run main InstanceMainTest
>> 30:  */
>> 31: public class InstanceMainTest extends TestHelper {
> 
> By my reading of the spec, "main" methods can be defined in record classes 
> and enum classes as well as normal classes.
> Are there tests for record and enum main method operation?

You can also have a `main` method inside an interface. For enum classes and 
interfaces, the main has to be static.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1206285923

Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]

[Rémi Forax]

On Mon, 8 May 2023 10:46:34 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 
>> 342:
>> 
>>> 340: 
>>> 341: // individual handle fields
>>> 342: clb.withField(ORIGINAL_TARGET_NAME, CD_MethodHandle, 
>>> ACC_PRIVATE | ACC_FINAL);
>> 
>> Would a @Stable field help here? E.g if the returned functional interface 
>> instance is stored in a `static final` field, it should enable better 
>> performance?
>
> (actually, not sure - as the class is saved in a class value cache, so 
> probably adding stable won't make any difference).

The class is loaded as a hidden class, so all final fields are truly final 
(unlike classes but like records), so @Stable is not needed.

The class value cache only change performance if the cache was used in a hot 
path. And currently a class value cache is not considered as a cache by the 
JIT, i.e. the JIT does not know that if the key is a constant, the value is a 
constant.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1187495168

Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v9]

[Rémi Forax]

On Fri, 5 May 2023 17:35:33 GMT, Vicente Romero  wrote:

>> `Flags.MANDATED` on a class is currently enough to know if it's a unnamed 
>> class or not, but it's not enough for classes not produced by javac.
>
> good point

Just to be clear, here, Flags.UNNAMED_CLASS is needed because an an unnamed 
class must have a main(), which is tested later.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186357338

Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v9]

[Rémi Forax]

On Fri, 5 May 2023 17:08:35 GMT, Vicente Romero  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Recommended changes #2
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java 
> line 3999:
> 
>> 3997: Name name = names.fromString(simplename);
>> 3998: JCModifiers anonMods = F.at(primaryPos)
>> 3999: 
>> .Modifiers(Flags.FINAL|Flags.MANDATED|Flags.SYNTHETIC|Flags.UNNAMED_CLASS, 
>> List.nil());
> 
> I wonder if testing if a class has flags: 
> `Flags.FINAL|Flags.MANDATED|Flags.SYNTHETIC` should be enough to know if it 
> is unnamed or not and we don't need to use the new `UNNAMED_CLASS` flag

`Flags.MANDATED` on a class is currently enough to know if it's a unnamed class 
or not, but it's not enough for classes not produced by javac.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186335538

Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v12]

[Rémi Forax]

On Tue, 2 May 2023 21:22:16 GMT, Chen Liang  wrote:

>> As John Rose has pointed out in this issue, the current j.l.r.Proxy based 
>> implementation of MethodHandleProxies.asInterface has a few issues:
>> 1. Exposes too much information via Proxy supertype (and WrapperInstance 
>> interface)
>> 2. Does not allow future expansion to support SAM[^1] abstract classes
>> 3. Slow (in fact, very slow)
>> 
>> This patch addresses all 3 problems:
>> 1. It implements proxies with one hidden class for each requested interface 
>> and replaced WrapperInstance inheritance with a check to the class data. 
>> This can avoid unexpected passing of `instanceof`, and avoids the nasty 
>> problem of exporting a JDK interface to a dynamic module to ensure access.
>> 2. This patch obtains already generated classes from a ClassValue by the 
>> requested interface type; the ClassValue can later be updated to compute 
>> implementation generation for abstract classes as well.
>> 3. This patch's generated hidden classes has call performance on par with 
>> those of lambda expressions; the creation performance is slightly less than 
>> that of LambdaMetafactory: 
>> https://jmh.morethan.io/?gist=fcb946d83ee4ac7386901795ca49b224
>> 
>> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's 
>> no longer applicable. Tests in `jdk/java/lang/invoke` and 
>> `jdk/java/lang/reflect` pass.
>> 
>> [^1]: single abstract method
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Minor cleanup, attempt to migrate lookup validation blocked by security 
> manager

I disagree for two reasons
- the first is inlining, 2ns/op vs 6ns/op is not very relevant, what is 
relevant is that in case you have full inlining of the callee inside the caller 
code and not in the other case. As Cliff Click said, inlining is the mother of 
all optimizations, without inlining there is a bunch of other optimizations 
that can not be applied. Moreover, in a lot of cases where proxies are used 
(Spring, CDI, Logger, etc), proxies are composed, if each call through a proxy 
is not inlined the performance difference is massive.
- second is control, as a user i've the control on the profile pollution that 
occurs because of the use of the proxy interface while i've no control on the 
profile pollution dues to the proxy implementation which depend on how others 
parts / libraries of the application are also using proxies.

And I don't buy the Leyden argument, the condensers of Leyden act at jlink 
time. A condenser is free to use a different strategy or not that the one used 
at runtime. Currently, GraalVM AOT, the closest existing thing to Leyden is 
using two entrypoints, one for the classical JDK and one for Graal AOT. Why not 
using the two entrypoints strategy for MHProxies.asInterfaceInstance() when 
Leyden will be implemented ?

-

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1532173983

Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]

[Rémi Forax]

On Tue, 17 Jan 2023 15:55:40 GMT, Jan Lahoda  wrote:

>> The pattern matching switches are using a bootstrap method 
>> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch. 
>> Basically, for a switch like:
>> 
>> switch (obj) {
>> case String s when s.isEmpty() -> {}
>> case String s -> {}
>> case CharSequence cs -> {}
>> ...
>> }
>> 
>> 
>> this method will produce a MethodHandle that will be analyze the provided 
>> selector value (`obj` in the example), and will return the case index to 
>> which the switch should jump. This method also accepts a (re)start index for 
>> the search, which is used to handle guards. For example, if the 
>> `s.isEmpty()` guard in the above sample returns false, the matching is 
>> restarted on the next case.
>> 
>> The current implementation is fairly slow, it basically goes through the 
>> labels in a loop. The proposal here is to replace that with a MethodHandle 
>> structure like this:
>> 
>> obj == null ? -1
>>   : switch (restartIndex) {
>> case 0 -> obj instanceof String ? 0 : obj instanceof 
>> CharSequence ? 2 : ... ;
>> case 1 -> obj instanceof String ? 1 : obj instanceof 
>> CharSequence ? 2 : ... ;
>> case 2 -> obj instanceof CharSequence ? 2 : ... ;
>> ...
>> default -> ;
>> }
>> 
>> 
>> This appear to run faster than the current implementation, using testcase 
>> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these 
>> are the results
>> 
>> PatternsOptimizationTest.testLegacyIndyLongSwitch   thrpt   25   1515989.562 
>> ± 32047.918  ops/s
>> PatternsOptimizationTest.testHandleIndyLongSwitch   thrpt   25   2630707.585 
>> ± 37202.210  ops/s
>> 
>> PatternsOptimizationTest.testLegacyIndyShortSwitch  thrpt   25   6789310.900 
>> ± 61921.636  ops/s
>> PatternsOptimizationTest.testHandleIndyShortSwitch  thrpt   25  10771729.464 
>> ± 69607.467  ops/s
>> 
>> 
>> The "LegacyIndy" is the current implementation, "HandleIndy" is the one 
>> proposed here. The translation in javac used is the one from #9746 in all 
>> cases.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains four additional commits since 
> the last revision:
> 
>  - Adding comments
>  - Improving performance
>  - Merge branch 'master' into JDK-8291966
>  - 8291966: SwitchBootstrap.typeSwitch could be faster

Also there is a lot of use cases where the type switch is called only with 
instances of the same class, for those scenarii, the VM should be able to fully 
remove the type switch and inline the body of the corresponding case like this 
is done with a virtual call.

I don't think there is a way currently to explain to the VM that the a hash map 
really acts as a cache so if the key is a constant then the value is a constant 
too (this optimization is also missing when JITing ClassValue.get()).

-

PR Comment: https://git.openjdk.org/jdk/pull/9779#issuecomment-1531526385

Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v11]

[Rémi Forax]

On Mon, 1 May 2023 14:56:27 GMT, Chen Liang  wrote:

>> As John Rose has pointed out in this issue, the current j.l.r.Proxy based 
>> implementation of MethodHandleProxies.asInterface has a few issues:
>> 1. Exposes too much information via Proxy supertype (and WrapperInstance 
>> interface)
>> 2. Does not allow future expansion to support SAM[^1] abstract classes
>> 3. Slow (in fact, very slow)
>> 
>> This patch addresses all 3 problems:
>> 1. It implements proxies with one hidden class for each requested interface 
>> and replaced WrapperInstance inheritance with a check to the class data. 
>> This can avoid unexpected passing of `instanceof`, and avoids the nasty 
>> problem of exporting a JDK interface to a dynamic module to ensure access.
>> 2. This patch obtains already generated classes from a ClassValue by the 
>> requested interface type; the ClassValue can later be updated to compute 
>> implementation generation for abstract classes as well.
>> 3. This patch's generated hidden classes has call performance on par with 
>> those of lambda expressions; the creation performance is slightly less than 
>> that of LambdaMetafactory: 
>> https://jmh.morethan.io/?gist=fcb946d83ee4ac7386901795ca49b224
>> 
>> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's 
>> no longer applicable. Tests in `jdk/java/lang/invoke` and 
>> `jdk/java/lang/reflect` pass.
>> 
>> [^1]: single abstract method
>
> Chen Liang 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 24 additional commits since 
> the last revision:
> 
>  - Renames
>  - Merge branch 'master' into explore/mhp-iface
>  - Consolidate iteration over public methods
>  - Define MH proxy class in a dynamic module
>  - Fix test that depend on WrapperInstance
>  - Require an original lookup in constructor arguments to prevent unintended 
> instantiation
>  - pass interface info via condy,
>drop explicit ea flags
>initialize benchmark work variable randomly
>benchmarks just need 1 fork
>  - Nuke WrapperInstance
>  - Merge branch 'master' into explore/mhp-iface
>  - Whitespace, cleanup, rename benchmarks to be informative
>  - ... and 14 more: https://git.openjdk.org/jdk/compare/c212c27d...f5d0ef0a

This is the method/class specialization problem all other again.
As Jorn said, we should use the same strategy as with the lambda metafactory, 
one hidden class per method handle, given that otherwise it's too easy to have 
profile pollution. This implementation already cache the bytecode so 
specializing different method handles with the same interfaces + classloader 
reuse the same bytecode.

I really hope that with Valhalla specialized generics, we will be able share 
more by specializing the same proxy class with different method handles, each 
one acting as a constant. So one class for all, but one species per method 
handle.

-

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1531164433

Re: RFR: 8291966: SwitchBootstrap.typeSwitch could be faster [v2]

[Rémi Forax]

On Tue, 17 Jan 2023 15:55:40 GMT, Jan Lahoda  wrote:

>> The pattern matching switches are using a bootstrap method 
>> `SwitchBootstrap.typeSwitch` to implement the jumps in the switch. 
>> Basically, for a switch like:
>> 
>> switch (obj) {
>> case String s when s.isEmpty() -> {}
>> case String s -> {}
>> case CharSequence cs -> {}
>> ...
>> }
>> 
>> 
>> this method will produce a MethodHandle that will be analyze the provided 
>> selector value (`obj` in the example), and will return the case index to 
>> which the switch should jump. This method also accepts a (re)start index for 
>> the search, which is used to handle guards. For example, if the 
>> `s.isEmpty()` guard in the above sample returns false, the matching is 
>> restarted on the next case.
>> 
>> The current implementation is fairly slow, it basically goes through the 
>> labels in a loop. The proposal here is to replace that with a MethodHandle 
>> structure like this:
>> 
>> obj == null ? -1
>>   : switch (restartIndex) {
>> case 0 -> obj instanceof String ? 0 : obj instanceof 
>> CharSequence ? 2 : ... ;
>> case 1 -> obj instanceof String ? 1 : obj instanceof 
>> CharSequence ? 2 : ... ;
>> case 2 -> obj instanceof CharSequence ? 2 : ... ;
>> ...
>> default -> ;
>> }
>> 
>> 
>> This appear to run faster than the current implementation, using testcase 
>> similar to the one used for https://github.com/openjdk/jdk/pull/9746 , these 
>> are the results
>> 
>> PatternsOptimizationTest.testLegacyIndyLongSwitch   thrpt   25   1515989.562 
>> ± 32047.918  ops/s
>> PatternsOptimizationTest.testHandleIndyLongSwitch   thrpt   25   2630707.585 
>> ± 37202.210  ops/s
>> 
>> PatternsOptimizationTest.testLegacyIndyShortSwitch  thrpt   25   6789310.900 
>> ± 61921.636  ops/s
>> PatternsOptimizationTest.testHandleIndyShortSwitch  thrpt   25  10771729.464 
>> ± 69607.467  ops/s
>> 
>> 
>> The "LegacyIndy" is the current implementation, "HandleIndy" is the one 
>> proposed here. The translation in javac used is the one from #9746 in all 
>> cases.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains four additional commits since 
> the last revision:
> 
>  - Adding comments
>  - Improving performance
>  - Merge branch 'master' into JDK-8291966
>  - 8291966: SwitchBootstrap.typeSwitch could be faster

Sequential search can be faster, the array length is known by the JIT so the 
loop can be unrolled and subtype checks are usually fast.

A hash table is more complex to build because it needs to store the actual 
classes of the objects switched upon (those classes may not have been loaded 
yet), so it's a data structure that need to be dynamically updated by several 
threads. While it's possible to write this kind of data structures, it will 
require quite a lot of engineering to get it right.
So yes, it's planned.

-

PR Comment: https://git.openjdk.org/jdk/pull/9779#issuecomment-1529136366

Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v5]

[Rémi Forax]

On Fri, 21 Apr 2023 16:25:04 GMT, Jan Lahoda  wrote:

>> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
>> 
>>  - the pattern matching for switch and record patterns features are made 
>> final, together with updates to tests.
>>  - parenthesized patterns are removed.
>>  - qualified enum constants are supported for case labels.
>> 
>> This change herein also includes removal record patterns in for each loop, 
>> which may be split into a separate PR in the future.
>
> Jan Lahoda has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Adding test.
>  - Removing redundant continue, as noted on the review.

> I believe the exhaustiveness algorithm needs rules for union types, both in 
> javac and in the JLS:
> 
> ...
> I would expect the above switch to be exhaustive, since it covers all 
> possible components of the union type. In other words, the union type should 
> act as a sealed hierarchy which permits Foo and Bar - and the two cases 
> should be merged together (e.g. `case Foo` + `case Bar` should cover `Foo | 
> Bar`). For the records, I get same error on JDK 20.

Precise exception type, here, `Foo | Bar` inside a catch is not an union, it's 
a way to merge catches when the body of catches are the same. In term of 
typing, `Foo | Bar` behave as the lub of Foo and Bar, apart in a throw.

So the switch is not exhaustive here because `lub(Foo, Bar) = Exception`.

We may add a special case in JLS for supporting precise exception type in 
switch given that this issue have been reported several times, but this is not 
something that should be fixed by this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/13074#issuecomment-1519675744

Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v4]

[Rémi Forax]

On Fri, 21 Apr 2023 15:30:24 GMT, Maurizio Cimadamore  
wrote:

>> Also, surprisingly, if I make C and D classes (instead of interfaces):
>> 
>> class Test {
>> sealed interface I permits C, D { }
>> final class C implements I { }
>> final class D implements I { }
>> 
>> interface F { }
>> 
>>  int test(Z o) {
>> return switch (o) {
>> case C c -> 1;
>> case D d -> 2;
>> };
>> }
>> }
>> 
>> I get errors like:
>> 
>> 
>> Foo.java:10: error: incompatible types: Z cannot be converted to Test.C
>> case C c -> 1;
>>  ^
>>   where Z is a type-variable:
>> Z extends I,F declared in method test(Z)
>> 
>> 
>> Which seems odd?
>
> Nevermind - these issues are due to a misunderstanding of the rules with 
> intersection types - e.g. given A & B, if a pattern covers _any_ of A, B, 
> then it covers A & B.
> 
> Similarly, the second issue I reported is in reality caused by the fact that 
> in the snippet with `class`, I also added `final` which then makes I & F a 
> type with no concrete witnesses (so cast will always fail from there to 
> either C/D).

> Which compiles correctly, but it doesn't look exhaustive to me (because of F) 
> ?

Can you expand a little more, because for me a Z either implement C or D or 
both C and D.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1173916081

Re: RFR: 6983726: remove Proxy from MethodHandleProxies.asInterfaceInstance SAM conversion [v8]

[Rémi Forax]

On Thu, 6 Apr 2023 18:33:29 GMT, Chen Liang  wrote:

>> As John Rose has pointed out in this issue, the current j.l.r.Proxy based 
>> implementation of MethodHandleProxies.asInterface has a few issues:
>> 1. Exposes too much information via Proxy supertype (and WrapperInstance 
>> interface)
>> 2. Does not allow future expansion to support SAM[^1] abstract classes
>> 3. Slow (in fact, very slow)
>> 
>> This patch addresses all 3 problems:
>> 1. It implements proxies with one hidden class for each requested interface 
>> and replaced WrapperInstance inheritance with an annotation. This can avoid 
>> unexpected passing of `instanceof`, and avoids the nasty problem of 
>> exporting a JDK interface to a dynamic module to ensure access.
>> 2. This patch obtains already generated classes from a ClassValue by the 
>> requested interface type; the ClassValue can later be updated to compute 
>> implementation generation for abstract classes as well.
>> 3. This patch's generated hidden classes has call performance on par with 
>> those of lambda expressions; the creation performance is slightly less than 
>> that of LambdaMetafactory: 
>> https://jmh.morethan.io/?gist=fcb946d83ee4ac7386901795ca49b224
>> 
>> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's 
>> no longer applicable. Tests in `jdk/java/lang/invoke` and 
>> `jdk/java/lang/reflect` pass.
>> 
>> In addition, I have a question: in 
>> [8161245](https://bugs.openjdk.org/browse/JDK-8161245) it seems some fields 
>> can be optimized as seen in 
>> [ciField.cpp](https://github.com/openjdk/jdk/blob/6aec6f3a842ead30b26cd31dc57a2ab268f67875/src/hotspot/share/ci/ciField.cpp#L219).
>>  Does it affect the execution performance of MethodHandle in hidden classes' 
>> Condy vs. MethodHandle in regular final field in hidden classes?
>> 
>> [^1]: single abstract method
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   pass interface info via condy,
>   drop explicit ea flags
>   initialize benchmark work variable randomly
>   benchmarks just need 1 fork

It's the Classfile API that is not able to generate stack map information.
The class should be "java/lang/RuntimeException" not "RuntimeException" (apart 
if the class name is truncated/simplified).

-

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1499494601

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v53]

[Rémi Forax]

On Tue, 4 Apr 2023 15:16:19 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   RuntimeException is the only exception type that can is deduced from a 
> lambda.

src/java.base/share/classes/java/lang/StringTemplate.java line 577:

> 575:  */
> 576: static  Processor 
> of(Function process) {
> 577: return process::apply;

The wildcards are missing :)

static  Processor of(Function process) {

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1157431904

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v53]

[Rémi Forax]

On Tue, 4 Apr 2023 15:16:19 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   RuntimeException is the only exception type that can is deduced from a 
> lambda.

src/java.base/share/classes/java/lang/runtime/ReferenceKey.java line 70:

> 68:  * null.
> 69:  */
> 70: int hashcode;

All the fields of the keys should be final, otherwise a publication problem can 
occur (the key is visible from another thread but its fields are not yet 
initialized)

src/java.base/share/classes/java/lang/runtime/StringTemplateImpl.java line 100:

> 98: try {
> 99: return (List)valuesMH.invokeExact(this);
> 100: } catch (Throwable ex) {

Errors likes OutOfMemoryError and runtime exception should be rethrown instead 
of being wrapped

src/java.base/share/classes/java/lang/runtime/StringTemplateImpl.java line 109:

> 107: try {
> 108: return (String)interpolateMH.invokeExact(this);
> 109: } catch (Throwable ex) {

see above

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1157415311
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1157417666
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1157417849

Re: RFR: 8304846: Provide a shared utility to dump generated classes defined via Lookup API [v6]

[Rémi Forax]

On Mon, 3 Apr 2023 20:24:56 GMT, Mandy Chung  wrote:

>> This implements a shared utility to dump generated classes defined as 
>> normal/hidden classes via `Lookup` API.   This replaces the implementation 
>> in `LambdaMetaFactory` and method handle implementation that dumps the 
>> hidden class bytes on disk for debugging.   
>> 
>> For classes defined via `Lookup::defineClass`, `Lookup::defineHiddenClass` 
>> and `Lookup::defineHiddenClassWithClassData`, by default they will be dumped 
>> to the path specified in `-Djava.lang.invoke.Lookup.dumpClasses=` 
>> 
>> The hidden classes generated for lambdas, `LambdaForms` and method handle 
>> implementation use non-default dumper so that they can be controlled via a 
>> separate system property and path as in the current implementation.
>> 
>> To dump lambda proxy classes, set this system property:
>>-Djdk.internal.lambda.dumpProxyClasses=
>> 
>> To dump LambdaForms and method handle implementation, set this system 
>> property:
>>-Djava.lang.invoke.MethodHandle.DUMP_CLASS_FILES=true
>>
>> P.S. `ProxyClassesDumper` is renamed to `ClassFileDumper` but for some 
>> reason, it's not shown as rename.
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   comments from Jorn Vernee

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java line 199:

> 197: @Override
> 198: public Path run() {
> 199: if (!Files.exists(path)) {

I do not think this is necessary, Files.createDirectories() already only create 
directories if they do not exist.
Moreover, someone can change the state of the files in between the two calls 
(Files.exists and Files.createDirectories) given that those two calls are not 
atomic from the filesystem POV.

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java line 216:

> 214: }
> 215: 
> 216: private static HexFormat HEX = HexFormat.of().withUpperCase();

'final' ?

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java line 226:

> 224: // control characters
> 225: if (c <= 31 || BAD_CHARS.contains(c)) {
> 226: sb.append("%");

sb.append('%')  // using a character instead of a String

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156456578
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156456936
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156457481

Re: RFR: 8266571: Sequenced Collections [v4]

[Rémi Forax]

On Wed, 29 Mar 2023 20:54:15 GMT, Chen Liang  wrote:

>> Yes,
>> The spec says :"Changes to the underlying collection might or might not be 
>> visible in this reversed view, depending upon the implementation." so i 
>> believe the default implementation i proposed is a valid implementation
>
> In the JEP, it says:
>> Any modifications to the original collection are visible in the view.
> 
> If we don't have an efficient reversed view, I don't see a point of declaring 
> a collection sequenced; same reason for declaring a sequenced/deque vs. a 
> full-on list with inefficient list random access operations.

The quote is from the javadoc of reversed (see above), it seems the JEP and the 
javadoc disagree :(

> If we don't have an efficient reversed view, I don't see a point of declaring 
> a collection sequenced

Collections in the JDK provides more efficient implementations, this is what 
the code this PR does.
Providing a default implementation matters more for external libraries.

And that's why i think we do not need the interface SequencedCollection, 
because all these methods can be declared on Collection instead. Adding them on 
Collection has also the added benefit that as a user, all Clojure collections 
or any other implementations not in the JDK also get a good enough 
implementation of the method reversed().

My fear is that if we introduce all these methods on SequencedCollection 
instead of Collection, library implementations will never be updated to use 
SequencedCollection instead of Collection (because implementing 
SequencedCollection requires the library to work only on JDK 21+) while if 
reversed is declared on Collection, library maintainers will have more pressure 
to write an efficient implementation of reversed for their implementations (and 
will be able to do that without requiring the JDK 21).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1152494872

Re: RFR: 8266571: Sequenced Collections [v4]

[Rémi Forax]

On Wed, 29 Mar 2023 19:54:48 GMT, Tagir F. Valeev  wrote:

>> In the same spirit, `reversed()` should also have a default implementation 
>> equivalent to
>> 
>>   
>> Collections.unmodifiableSequenceCollection(Arrays.asList(this.toArray())).reversed()
>
> @forax but this would not be a view: changes in the underlying collection 
> won't be reflected

Yes,
The spec says :"Changes to the underlying collection might or might not be 
visible in this reversed view, depending upon the implementation." so i believe 
the default implementation i proposed is a valid implementation

-

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1152426988

Re: RFR: 8266571: Sequenced Collections [v4]

[Rémi Forax]

On Wed, 29 Mar 2023 19:06:20 GMT, Chen Liang  wrote:

>> Stuart Marks has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Simplify handling of cached keySet, values, and entrySet views.
>
> src/java.base/share/classes/java/util/SequencedCollection.java line 107:
> 
>> 105:  */
>> 106: default void addFirst(E e) {
>> 107: throw new UnsupportedOperationException();
> 
> Can this be defaulted to `this.reversed().addLast()` instead? If this throws 
> uoe, the reversed should throw uoe as well; and the new default can simplify 
> implementations by much as well.

In the same spirit, `reversed()` should also have a default implementation 
equivalent to

  
Collections.unmodifiableSequenceCollection(Arrays.asList(this.toArray())).reversed()

-

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1152384938

Re: RFR: 8266571: Sequenced Collections [v4]

[Rémi Forax]

On Tue, 28 Mar 2023 02:37:22 GMT, Stuart Marks  wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplify handling of cached keySet, values, and entrySet views.

This is taking a personal turn and I do not see the point of discussing that 
matter here, so please send me an email at fo...@univ-mlv.fr

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1488580845

Re: RFR: 8266571: Sequenced Collections [v4]

[Rémi Forax]

On Tue, 28 Mar 2023 02:37:22 GMT, Stuart Marks  wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplify handling of cached keySet, values, and entrySet views.

Hi Erik,
I think you misunderstood me, currently Oracle supports most of the development 
of the OpenJDK financially, that's a fact and i'm glad that Oracle has taken 
that mantle because I'm remembering very well the sad state of Java at the time 
SUN was dying. 

I believe that introducing the interface SequencedCollection instead of adding 
the methods to Collection (i get it's not the exact same semantics) have an 
impact that is too big and will require more work than it should for both the 
maintainers of the JDK and the maintainers of the libraries having their own 
implementations of the Java collections.

This has nothing to do with the paycheck of some of us.

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1488423778

Re: RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Wed, 29 Mar 2023 06:52:35 GMT, ExE Boss  wrote:

> There is SortedMap, 
yes, but no sorted "collection". An interface with a special semantic for set, 
map or list do not have the same impact in term of number of lines to maintain 
than a collection with a special semantics.

And java.util.Collection is a kind like java.lang.Object, it does not define a 
real semantics, equals/hashCode are defined in set and list not collection, so 
it's more that Collection is defined by its lack of semantics. So the same way, 
it's weird to have two java.lang.Object, it's weird to have two Collections.

> and non‑null collection interfaces are unnecessary with JEP Null‑Restricted 
> and Nullable Types
i think we are agreeing here, there is no need for a special interface for 
non-null collections, so why do we need one for ordered collections.

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1488287644

Re: RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Tue, 28 Mar 2023 00:01:27 GMT, Stuart Marks  wrote:

>> This change is absolutely massive, implementing reversed() basically doubles 
>> the number of implementations which means multiple years of debugging / spec 
>> fixing.
>> 
>> Reversing a List makes sense, reversing a LinkedHashSet/LinkedHashMap is a 
>> nice to have. Having the concept of first and last 
>> (getFirst()/getLast()/etc) on Collection is something long awaited. 
>> 
>> I understand that wanting to separate the concept of Collection and 
>> SequencedCollection can be conceptually nice, but 
>> multiplying the number of interfaces also multiplies the number of 
>> implementations. Pragmatically, this patch is too big compared to how useful 
>> it is.
>> 
>> I get that Oracle is rich, but this JEP is not only a burden for Oracle but 
>> for the whole community.
>
> @forax 
> 
> Funnily, I was thinking the other day that this change is quite small given 
> that I've been working on it for over two years. :-)
> 
> Anyway, thanks for looking through the implementation.

@stuart-marks, i read your message as not too not far from the sunken cost 
fallacy.

I still believe that introducing an interface for the concept of sequenced 
collection makes this change far bigger that it could be. After all, there is 
no interface for non-null collections, read-only collections, non structurally 
modifiable collections or sorted collections, so why an interface for ordered 
collection ?

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1487621122

Re: RFR: 6983726: remove Proxy from MethodHandleProxies.asInterfaceInstance SAM conversion

[Rémi Forax]

On Mon, 27 Mar 2023 23:34:52 GMT, Chen Liang  wrote:

> As John Rose has pointed out in this issue, the current j.l.r.Proxy based 
> implementation of MethodHandleProxies.asInterface has a few issues:
> 1. Exposes too much information via Proxy supertype (and WrapperInstance 
> interface)
> 2. Does not allow future expansion to support SAM[^1] abstract classes
> 3. Slow (in fact, very slow)
> 
> This patch addresses all 3 problems:
> 1. It implements proxies with one hidden class for each requested interface 
> and replaced WrapperInstance inheritance with an annotation. This can avoid 
> unexpected passing of `instanceof`, and avoids the nasty problem of exporting 
> a JDK interface to a dynamic module to ensure access.
> 2. This patch obtains already generated classes from a ClassValue by the 
> requested interface type; the ClassValue can later be updated to compute 
> implementation generation for abstract classes as well.
> 3. This patch's generated hidden classes has acceptable call and creation 
> performance compared to the baseline; though the methods to access wrapper 
> information see huge performance drops, they are not anticipated to be used 
> in a very frequent basis, while the old implementation's wrapper access 
> methods are more optimized (2ns/op) than interface implementation methods 
> (6ns/op). [Oracle JDK 20 vs 
> this](https://jmh.morethan.io/?gists=bf98de7b2128e7e5d14e697fd9921eb9,e5115a2a8fa0a45159e15fab0d95b5d8)
> 
> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's 
> no longer applicable. Tests in `jdk/java/lang/invoke` and 
> `jdk/java/lang/reflect` pass.
> 
> Alternative implementation:
> [An alternative 
> implementation](https://github.com/openjdk/jdk/commit/72dbf9d4e01c455854d9b865cb2a47c38f37a8e0)
>  was to generate a proxy class for each methodhandle than sharing across 
> methodhandles. That implementation was abandoned for its bad proxy creation 
> performance, despite it having excellent call performance. [Alternative 
> implementation vs 
> this](https://jmh.morethan.io/?gists=08abb39f224574550925beb8be1b2f59,e5115a2a8fa0a45159e15fab0d95b5d8)
> 
> In addition, I have a question: in 
> [8161245](https://bugs.openjdk.org/browse/JDK-8161245) it seems some fields 
> can be optimized as seen in 
> [ciField.cpp](https://github.com/openjdk/jdk/blob/6aec6f3a842ead30b26cd31dc57a2ab268f67875/src/hotspot/share/ci/ciField.cpp#L219).
>  Does it affect the execution performance of MethodHandle in hidden classes' 
> Condy vs. MethodHandle in regular final field in hidden classes?
> 
> [^1]: single abstract method

Let suppose you have an interface like:

interface StringConsumer extends Consumer {}


The implementation needs to override both accept(Object) and accept(String).

-

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1486908491

Re: RFR: 6983726: remove Proxy from MethodHandleProxies.asInterfaceInstance SAM conversion

[Rémi Forax]

On Mon, 27 Mar 2023 23:34:52 GMT, Chen Liang  wrote:

> As John Rose has pointed out in this issue, the current j.l.r.Proxy based 
> implementation of MethodHandleProxies.asInterface has a few issues:
> 1. Exposes too much information via Proxy supertype (and WrapperInstance 
> interface)
> 2. Does not allow future expansion to support SAM[^1] abstract classes
> 3. Slow (in fact, very slow)
> 
> This patch addresses all 3 problems:
> 1. It implements proxies with one hidden class for each requested interface 
> and replaced WrapperInstance inheritance with an annotation. This can avoid 
> unexpected passing of `instanceof`, and avoids the nasty problem of exporting 
> a JDK interface to a dynamic module to ensure access.
> 2. This patch obtains already generated classes from a ClassValue by the 
> requested interface type; the ClassValue can later be updated to compute 
> implementation generation for abstract classes as well.
> 3. This patch's generated hidden classes has acceptable call and creation 
> performance compared to the baseline; though the methods to access wrapper 
> information see huge performance drops, they are not anticipated to be used 
> in a very frequent basis, while the old implementation's wrapper access 
> methods are more optimized (2ns/op) than interface implementation methods 
> (6ns/op). [Oracle JDK 20 vs 
> this](https://jmh.morethan.io/?gists=bf98de7b2128e7e5d14e697fd9921eb9,e5115a2a8fa0a45159e15fab0d95b5d8)
> 
> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's 
> no longer applicable. Tests in `jdk/java/lang/invoke` and 
> `jdk/java/lang/reflect` pass.
> 
> Alternative implementation:
> [An alternative 
> implementation](https://github.com/openjdk/jdk/commit/72dbf9d4e01c455854d9b865cb2a47c38f37a8e0)
>  was to generate a proxy class for each methodhandle than sharing across 
> methodhandles. That implementation was abandoned for its bad proxy creation 
> performance, despite it having excellent call performance. [Alternative 
> implementation vs 
> this](https://jmh.morethan.io/?gists=08abb39f224574550925beb8be1b2f59,e5115a2a8fa0a45159e15fab0d95b5d8)
> 
> In addition, I have a question: in 
> [8161245](https://bugs.openjdk.org/browse/JDK-8161245) it seems some fields 
> can be optimized as seen in 
> [ciField.cpp](https://github.com/openjdk/jdk/blob/6aec6f3a842ead30b26cd31dc57a2ab268f67875/src/hotspot/share/ci/ciField.cpp#L219).
>  Does it affect the execution performance of MethodHandle in hidden classes' 
> Condy vs. MethodHandle in regular final field in hidden classes?
> 
> [^1]: single abstract method

I believe you can have better performance if you pass the method handle as the 
class data of the hidden class and you load it with a constant dynamic
  
https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClassWithClassData(byte[],java.lang.Object,boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)
and
  
https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/invoke/MethodHandles.html#classData(java.lang.invoke.MethodHandles.Lookup,java.lang.String,java.lang.Class)

-

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1486668297

Re: RFR: 6983726: remove Proxy from MethodHandleProxies.asInterfaceInstance SAM conversion

[Rémi Forax]

On Mon, 27 Mar 2023 23:34:52 GMT, Chen Liang  wrote:

> As John Rose has pointed out in this issue, the current j.l.r.Proxy based 
> implementation of MethodHandleProxies.asInterface has a few issues:
> 1. Exposes too much information via Proxy supertype (and WrapperInstance 
> interface)
> 2. Does not allow future expansion to support SAM[^1] abstract classes
> 3. Slow (in fact, very slow)
> 
> This patch addresses all 3 problems:
> 1. It implements proxies with one hidden class for each requested interface 
> and replaced WrapperInstance inheritance with an annotation. This can avoid 
> unexpected passing of `instanceof`, and avoids the nasty problem of exporting 
> a JDK interface to a dynamic module to ensure access.
> 2. This patch obtains already generated classes from a ClassValue by the 
> requested interface type; the ClassValue can later be updated to compute 
> implementation generation for abstract classes as well.
> 3. This patch's generated hidden classes has acceptable call and creation 
> performance compared to the baseline; though the methods to access wrapper 
> information see huge performance drops, they are not anticipated to be used 
> in a very frequent basis, while the old implementation's wrapper access 
> methods are more optimized (2ns/op) than interface implementation methods 
> (6ns/op). [Oracle JDK 20 vs 
> this](https://jmh.morethan.io/?gists=bf98de7b2128e7e5d14e697fd9921eb9,e5115a2a8fa0a45159e15fab0d95b5d8)
> 
> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's 
> no longer applicable. Tests in `jdk/java/lang/invoke` and 
> `jdk/java/lang/reflect` pass.
> 
> Alternative implementation:
> [An alternative 
> implementation](https://github.com/openjdk/jdk/commit/72dbf9d4e01c455854d9b865cb2a47c38f37a8e0)
>  was to generate a proxy class for each methodhandle than sharing across 
> methodhandles. That implementation was abandoned for its bad proxy creation 
> performance, despite it having excellent call performance. [Alternative 
> implementation vs 
> this](https://jmh.morethan.io/?gists=08abb39f224574550925beb8be1b2f59,e5115a2a8fa0a45159e15fab0d95b5d8)
> 
> In addition, I have a question: in 
> [8161245](https://bugs.openjdk.org/browse/JDK-8161245) it seems some fields 
> can be optimized as seen in 
> [ciField.cpp](https://github.com/openjdk/jdk/blob/6aec6f3a842ead30b26cd31dc57a2ab268f67875/src/hotspot/share/ci/ciField.cpp#L219).
>  Does it affect the execution performance of MethodHandle in hidden classes' 
> Condy vs. MethodHandle in regular final field in hidden classes?
> 
> [^1]: single abstract method

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

> 204: try {
> 205: proxy = (Object) info.ctor.invokeExact(mhs); // non-varargs
> 206: } catch (Throwable e) {

At least Error should be directly propagated (especially OutOfMemoryError)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1150405757

Re: RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Sat, 25 Mar 2023 03:54:23 GMT, Stuart Marks  wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - More specification tweaks.
>  - Add simple overrides to ArrayList.

This change is absolutely massive, implementing reversed() basically doubles 
the number of implementations which means multiple years of debugging / spec 
fixing.

Reversing a List makes sense, reversing a LinkedHashSet/LinkedHashMap is a nice 
to have. Having the concept of first and last (getFirst()/getLast()/etc) on 
Collection is something long awaited. 

I understand that wanting to separate the concept of Collection and 
SequencedCollection can be conceptually nice, but 
multiplying the number of interfaces also multiplies the number of 
implementations. Pragmatically, this patch is too big compared to how useful it 
is.

I get that Oracle is rich, but this JEP is not only a burden for Oracle but for 
the whole community.

-

PR Comment: https://git.openjdk.org/jdk/pull/7387#issuecomment-1483761783

Re: RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Sat, 25 Mar 2023 03:54:23 GMT, Stuart Marks  wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - More specification tweaks.
>  - Add simple overrides to ArrayList.

src/java.base/share/classes/java/util/LinkedHashSet.java line 297:

> 295:  */
> 296: public SequencedSet reversed() {
> 297: class ReverseLinkedHashSetView extends AbstractSet implements 
> SequencedSet {

This class should be declared `static` (and private) which means it should not 
be declared inside reversed.

src/java.base/share/classes/java/util/LinkedHashSet.java line 313:

> 311: boolean present = LinkedHashSet.this.contains(e);
> 312: LinkedHashSet.this.addFirst(e);
> 313: return ! present;

why there is a space between '!' and 'present' given that you have removed this 
space in another change above ?

src/java.base/share/classes/java/util/LinkedList.java line 1293:

> 1291: @SuppressWarnings("serial")
> 1292: static class ReverseOrderLinkedListView extends LinkedList 
> implements java.io.Externalizable {
> 1293: final LinkedList list;

Using 3 different fields feel ugly given it seems you only need one.
Why do you not use the casting strategy you are using for the other views ?

src/java.base/share/classes/java/util/List.java line 802:

> 800:  */
> 801: default E getFirst() {
> 802: if (this.isEmpty())

weirdly, sometimes you use braces around the ''if and sometimes you don't ?

src/java.base/share/classes/java/util/ReverseOrderListView.java line 60:

> 58: }
> 59: 
> 60: ReverseOrderListView(List list, boolean modifiable) {

should be 'private' to force users to use 'of'

src/java.base/share/classes/java/util/ReverseOrderListView.java line 191:

> 189: if (o == this)
> 190: return true;
> 191: if (!(o instanceof List))

should be `if (!(o instanceof List list))`

src/java.base/share/classes/java/util/ReverseOrderListView.java line 209:

> 207: int hashCode = 1;
> 208: for (E e : this)
> 209: hashCode = 31*hashCode + (e==null ? 0 : e.hashCode());

`31 * hashCode` instead of `31*hashCode`

src/java.base/share/classes/java/util/ReverseOrderSortedMapView.java line 44:

> 42: public static  SortedMap of(SortedMap map) {
> 43: if (map instanceof ReverseOrderSortedMapView) {
> 44: return ((ReverseOrderSortedMapView)map).base;

use `map instanceof ReverseOrderSortedMapView view` instead to remove the 
ugly cast below

src/java.base/share/classes/java/util/ReverseOrderSortedMapView.java line 215:

> 213: static  Iterator descendingValueIterator(SortedMap 
> map) {
> 214: return new Iterator<>() {
> 215: Iterator keyIterator = descendingKeyIterator(map);

`private final` or better declare it above as a local variable and capture it 
inside the anonymous class

src/java.base/share/classes/java/util/ReverseOrderSortedMapView.java line 329:

> 327: K prevKey = null;
> 328: boolean dead = false;
> 329: Iterator> it = descendingEntryIterator(base);

see above about declare it as a local variable, all other fields should be 
`private`

src/java.base/share/classes/java/util/ReverseOrderSortedSetView.java line 61:

> 59: return true;
> 60: 
> 61: if (!(o instanceof Set))

use `if (!(o instanceof Set set))` to avoid the cast below

-

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148321554
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148321635
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148321852
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322049
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322292
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322490
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322511
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322728
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148322895
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148323316
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148323602

Re: RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Sat, 25 Mar 2023 03:54:23 GMT, Stuart Marks  wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - More specification tweaks.
>  - Add simple overrides to ArrayList.

src/java.base/share/classes/java/util/Collections.java line 1184:

> 1182: 
> 1183: @SuppressWarnings("unchecked")
> 1184: private SequencedCollection rc() {

I suggest to use 'delegate' as name instead of 'rc' (no idea what 'rc' means)

src/java.base/share/classes/java/util/Collections.java line 6014:

> 6012:  */
> 6013: public static  SequencedSet 
> newSequencedSetFromMap(SequencedMap map) {
> 6014: if (! map.isEmpty())

This line does an implicit NPE check, either make it explicit using 
requireNonNull or at least add a comment

src/java.base/share/classes/java/util/Collections.java line 6023:

> 6021:  */
> 6022: private static class SequencedSetFromMap extends SetFromMap 
> implements SequencedSet {
> 6023: private final E nsee(Map.Entry e) {

`static` instead of `final`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148320373
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148320720
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148320942

Re: RFR: 8266571: Sequenced Collections [v2]

[Rémi Forax]

On Sat, 25 Mar 2023 03:54:23 GMT, Stuart Marks  wrote:

>> PR for Sequenced Collections implementation.
>
> Stuart Marks has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - More specification tweaks.
>  - Add simple overrides to ArrayList.

src/java.base/share/classes/java/util/ArrayList.java line 573:

> 571: } else {
> 572: Object[] es = elementData;
> 573: sz--;

This line and the following two lines can be understood as the field size being 
updated which is not the case.
I think it's more readable to use

@SuppressWarnings("unchecked") E oldValue = (E) es[sz - 1];
fastRemove(es, sz - 1);

The JIT will common the subexpression 'sz - 1' at runtime.

src/java.base/share/classes/java/util/Collections.java line 1173:

> 1171:  * @serial include
> 1172:  */
> 1173: static class UnmodifiableSequencedCollection extends 
> UnmodifiableCollection

`private` is missing

-

PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148319735
PR Review Comment: https://git.openjdk.org/jdk/pull/7387#discussion_r1148319899

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]

[Rémi Forax]

On Thu, 23 Mar 2023 12:17:55 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 204:
>> 
>>> 202: Object[] values
>>> 203: ) throws Throwable {
>>> 204: List asList = Collections.unmodifiableList(new 
>>> ArrayList<>(Arrays.asList(values)));
>> 
>> Suggestion:
>> 
>> List asList = List.of(values);
>> 
>> For defensive copy.
>> Don't think processors are harmed by the null-hostile behavior of these 
>> list, for many template implementations already use null-hostile lists.
>
> The values list can't be null-hostile for the same reason that string 
> concatenation can't be null-hostile. Please point to examples of null-hostile 
> lists (other that fragments) being used in the template code. Apologies if I 
> misinterpreted your meaning.

There is a trick to do a defensive copy in case the list can contains a null, 
you can use `stream.toList()`,
so

  List asList = Arrays.stream(values).toList();

is what you are looking for.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1146455491

Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]

[Rémi Forax]

On Thu, 23 Mar 2023 15:49:37 GMT, Jim Laskey  wrote:

>> On the flip side, for those that don't use `var`;
>> 
>> 
>> Processor simple = Processor.of(st-> new 
>> JSONObject(st.interpolate()));
>> Processor string = 
>> Processor.of(StringTemplate::interpolate);
>> 
>> 
>> vs.
>> 
>> 
>> SimpleProcessor simple = st-> new JSONObject(st.interpolate());
>> StringProcessor string = StringTemplate::interpolate;
>
> It's coming back to me why we didn't do this in the first place (these 
> projects tend to go on for months and years). `SimpleProcessor` exists 
> because of that ugly second parameter, `E`, in `Processor`, when a 
> majority of the time `E` will be the unchecked `RuntimeException`. 
> `StringProcessor` exists because 90+% of template processors will produce 
> strings. From there, we originally had `Process.of`, `SimpleProcessor.of` and 
> `StringProcessor.of`. We realized that the `@FunctionalInterface` route was 
> cleaner and there was no need for `of` -- keep interfaces simple. I would 
> argue that if you are creating a template processor, it is better to expose 
> the result type and not bury in a `var`.

Not a lot of people will write a processor and among those few, most of them 
will create a class that implement `Processor` (to have a proper name and a 
place to put documentation) so providing several reified names 
(`SimpleProcessor` and `StringProcessor`) is not that useful for implementers. 
For users, it's not something necessary to understand how processors work or 
how a specific processor should be used.

It looks like a loose loose situation for me, implementers do not need them and 
users will find them confusing (especially the difference between a processor 
and a simple processor).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1146450918

Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]

[Rémi Forax]

On Wed, 22 Feb 2023 17:05:00 GMT, Kasper Nielsen  wrote:

>> Volker Simonis has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Remove assertions which insist on Lambda proxy classes being strongly 
>> linked to their class loader
>>  - Removed unused import of STRONG und updated copyright year
>
> In all the places I've seen LambdaMetaFactory used, it is because of 
> performance over reflection/non-static method handles. See, for example, 
> https://www.optaplanner.org/blog/2018/01/09/JavaReflectionButMuchFaster.html. 
> I believe Optaplanner is still using it.
> 
> There is also quite a number of posts on StackOverflow on people trying to 
> use LambdaMetafactory:
> https://stackoverflow.com/search?q=LambdaMetafactory

@kaspernielsen, i believe that now that hidden class + class data are part of 
the public API,
you do not need to use lambda metafactory directly. But it requires Java 17 not 
8 or 11.

-

PR: https://git.openjdk.org/jdk/pull/12493

Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]

[Rémi Forax]

On Thu, 9 Feb 2023 18:11:18 GMT, Volker Simonis  wrote:

>> Prior to 
>> [JDK-8239384](https://bugs.openjdk.org/browse/JDK-8239384)/[JDK-8238358](https://bugs.openjdk.org/browse/JDK-8238358)
>>  LambdaMetaFactory has created VM-anonymous classes which could easily be 
>> unloaded once they were not referenced any more. Starting with JDK 15 and 
>> the new "hidden class" based implementation, this is not the case any more, 
>> because the hidden classes will be strongly tied to their defining class 
>> loader. If this is the default application class loader, these hidden 
>> classes can never be unloaded which can easily lead to Metaspace exhaustion 
>> (see the [test case in the JBS 
>> issue](https://bugs.openjdk.org/secure/attachment/102601/LambdaClassLeak.java)).
>>  This is a regression compared to previous JDK versions which some of our 
>> applications have been affected from when migrating to JDK 17.
>> 
>> The reason why the newly created hidden classes are strongly linked to their 
>> defining class loader is not clear to me. JDK-8239384 mentions it as an 
>> "implementation detail":
>> 
>>> *4. the lambda proxy class has the strong relationship with the class 
>>> loader (that will share the VM metaspace for its defining loader - 
>>> implementation details)*
>> 
>> From my current understanding the strong link between a hidden class created 
>> by `LambdaMetaFactory` and its defining class loader is not strictly 
>> required. In order to prevent potential OOMs and fix the regression compared 
>> the JDK 14 and earlier I propose to create these hidden classes without the 
>> `STRONG` option.
>> 
>> I'll be happy to add the test case as JTreg test to this PR if you think 
>> that would be useful.
>
> Volker Simonis has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Remove assertions which insist on Lambda proxy classes being strongly 
> linked to their class loader
>  - Removed unused import of STRONG und updated copyright year

Volker, i think you are mixing lambda metafactory and hidden classes.

Hidden Classes have been envision has a way to generate adapters at runtime for 
languages that run on the JVM.
They replace VM anonymous classes that were both unsafe and always not bound to 
a classloader.

Lambda metafactory is the entry point to create lambda proxy for Java (the 
language).
The current implementation is using hidden classes to represent lambda proxy. 
In the past, it was using VM anonymous classes that why lambdas were not tie to 
a classloader, the fact that the implementation was using VM anonymous class 
was leaking.

To unload a Java class, its clasloader has to be unreachable. I see no reason 
this behavior should not be the same for Java lambdas classes. I believe it's 
what David is saying too.

Now, for https://github.com/aws/aws-sdk-java-v2/issues/3701, you can fix it by 
using a ClassValue to cache the class and/or using a hidden class. For me, this 
has nothing to do with Java lambdas, i.e. nothing to do with the lambda 
metafactory.

-

PR: https://git.openjdk.org/jdk/pull/12493

Re: RFR: 8302154: Hidden classes created by LambdaMetaFactory can't be unloaded [v2]

[Rémi Forax]

On Thu, 9 Feb 2023 18:11:18 GMT, Volker Simonis  wrote:

>> Prior to 
>> [JDK-8239384](https://bugs.openjdk.org/browse/JDK-8239384)/[JDK-8238358](https://bugs.openjdk.org/browse/JDK-8238358)
>>  LambdaMetaFactory has created VM-anonymous classes which could easily be 
>> unloaded once they were not referenced any more. Starting with JDK 15 and 
>> the new "hidden class" based implementation, this is not the case any more, 
>> because the hidden classes will be strongly tied to their defining class 
>> loader. If this is the default application class loader, these hidden 
>> classes can never be unloaded which can easily lead to Metaspace exhaustion 
>> (see the [test case in the JBS 
>> issue](https://bugs.openjdk.org/secure/attachment/102601/LambdaClassLeak.java)).
>>  This is a regression compared to previous JDK versions which some of our 
>> applications have been affected from when migrating to JDK 17.
>> 
>> The reason why the newly created hidden classes are strongly linked to their 
>> defining class loader is not clear to me. JDK-8239384 mentions it as an 
>> "implementation detail":
>> 
>>> *4. the lambda proxy class has the strong relationship with the class 
>>> loader (that will share the VM metaspace for its defining loader - 
>>> implementation details)*
>> 
>> From my current understanding the strong link between a hidden class created 
>> by `LambdaMetaFactory` and its defining class loader is not strictly 
>> required. In order to prevent potential OOMs and fix the regression compared 
>> the JDK 14 and earlier I propose to create these hidden classes without the 
>> `STRONG` option.
>> 
>> I'll be happy to add the test case as JTreg test to this PR if you think 
>> that would be useful.
>
> Volker Simonis has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Remove assertions which insist on Lambda proxy classes being strongly 
> linked to their class loader
>  - Removed unused import of STRONG und updated copyright year

Hi Volker, in the future, we may change the implementation of lambdas again, by 
example Valhalla parametric generics, Isolated methods (JDK-8158765), 
JDK-8087219 or JDK-8001537,  may allow to re-use the same class file/method for 
all lambdas implementing the same interface. I fear that forcing unloading may 
block future refactoring.

-

PR: https://git.openjdk.org/jdk/pull/12493

Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]

[Rémi Forax]

On Fri, 27 Jan 2023 13:53:45 GMT, Glavo  wrote:

>> I checked the `java.base` module, and all the `Collection#toArray()` method 
>> of collections be implemented correctly.
>> 
>> Their return values can be trusted, so many unnecessary array duplication 
>> can be eliminated.
>
> Glavo has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Collections.isTrustedCollection

This may change the startup time a lot because initializing the annotation 
parser (and the factory around) and creating the annotations is quite slow.

-

PR: https://git.openjdk.org/jdk/pull/12212

Re: RFR: 8301220: Return value of toArray() of Сollection types from java.base should be trusted [v2]

[Rémi Forax]

On Fri, 27 Jan 2023 13:53:45 GMT, Glavo  wrote:

>> I checked the `java.base` module, and all the `Collection#toArray()` method 
>> of collections be implemented correctly.
>> 
>> Their return values can be trusted, so many unnecessary array duplication 
>> can be eliminated.
>
> Glavo has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Collections.isTrustedCollection

I think the simplest solution is to have a non public interface declared inside 
java.util. Like java.util.RandomAccess, but non public.
The main advantage to use an interface is that you can document it and it's 
easy to find all the implementations.

-

PR: https://git.openjdk.org/jdk/pull/12212

Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]

[Rémi Forax]

On Tue, 24 Jan 2023 13:40:37 GMT, Viktor Klang  wrote:

>> `coll instanceof ListN list` should work.
>
> @forax @stuart-marks Yeah, that works. It's unfortunate that it's not 
> possible to match on the actual (generic) type, as then both sides of the || 
> could use type unification to avoid having to do the cast on the return value 
> as well. Submitted a commit to switch to the wildcard version.

Avoiding the cast to `List` requires two things:
- we need a way to say at declaration that a parameter type E is covariant, 
this is not true for List but this is true for ImmutableList (so true for List2 
and ListN).
- we need a way to do an union between two types when doing pattern matching .

This former is still in discussion as a feature of Valhalla (it's a part of Dan 
Smith Phd), the later is still in discussion as a feature of Amber, so maybe in 
the future this will be possible is the star aligned.

-

PR: https://git.openjdk.org/jdk/pull/11847

Re: RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]

[Rémi Forax]

On Tue, 24 Jan 2023 10:23:14 GMT, Viktor Klang  wrote:

>> src/java.base/share/classes/java/util/ImmutableCollections.java line 169:
>> 
>>> 167: @SuppressWarnings("unchecked")
>>> 168: static  List listCopy(Collection coll) {
>>> 169: if (coll instanceof List12 || (coll instanceof ListN && ! 
>>> ((ListN)coll).allowNulls)) {
>> 
>> Maybe replace the cast with an instanceof pattern here?
>
> @stuart-marks Sorry, missed this notification. I initially had the same idea, 
> but decided against it because it forces me to suppress "rawtypes" since 
> `coll instanceof ListN` is not considered to be a rawtype, but `coll 
> instanceof ListN c` is. And currently it won't allow for `coll instanceof 
> ListN c`...

`coll instanceof ListN list` should work.

-

PR: https://git.openjdk.org/jdk/pull/11847

Re: RFR: 8300237: Minor improvements in MethodHandles [v5]

[Rémi Forax]

On Wed, 18 Jan 2023 07:32:58 GMT, Sergey Tsypanov  wrote:

>> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` 
>> when we don't need a copy
>> - comparison of two lists can be done without `Stream.reduce()`
>
> Sergey Tsypanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use comparingInt()

Looks good to me,
i'm not an official reviewer but claes already review this PR so this is fine.

-

PR: https://git.openjdk.org/jdk/pull/12025

Re: RFR: 8300237: Minor improvements in MethodHandles [v3]

[Rémi Forax]

On Tue, 17 Jan 2023 18:07:37 GMT, Sergey Tsypanov  wrote:

>> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` 
>> when we don't need a copy
>> - comparison of two lists can be done without `Stream.reduce()`
>
> Sergey Tsypanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Polishing

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 59:

> 57: import java.nio.ByteOrder;
> 58: import java.security.ProtectionDomain;
> 59: import java.util.*;

oops

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6747:

> 6745: .max(Comparator.comparing(MethodType::parameterCount))
> 6746: .map(MethodType::ptypes)
> 6747: .map(longest -> List.of(Arrays.copyOfRange(longest, 
> skipSize, longest.length)))

i think you can fuse these to map() calls

-

PR: https://git.openjdk.org/jdk/pull/12025

Re: RFR: 8300237: Minor improvements in MethodHandles [v2]

[Rémi Forax]

On Tue, 17 Jan 2023 10:18:42 GMT, Claes Redestad  wrote:

>> Using lambdas inside MethodHandles is quite dangerous given that lambdas are 
>> initialized using method handles. It may work now because 
>> longuestParameterList() is not called when initializing a lambda but it may 
>> make any changes in the implementation of lambdas painfull in the future.
>
> Precious little method handle use in lambda bootstrap since JDK 11. Though I 
> agree with the sentiment - having fixed a number of bootstrap issues in the 
> past - `MethodHandles` is a small step up the abstraction ladder and the code 
> in particular already uses a number of method refs and lambdas.

ok, two small changes,
-  formatting: usually the method call in a stream are aligned with the '.' at 
the beginning
```
 stream
   .filter(...)
  .map(...)
   ```
   instead of at the end.

- the reduce is a max(),
  `max(Comparator.comparingInt(List::size))`

-

PR: https://git.openjdk.org/jdk/pull/12025

Re: RFR: 8300237: Minor improvements in MethodHandles

[Rémi Forax]

On Tue, 17 Jan 2023 09:51:31 GMT, Claes Redestad  wrote:

>> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` 
>> when we don't need a copy
>> - comparison of two lists can be done without `Stream.reduce()`
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6754:
> 
>> 6752: filter(t -> t.parameterCount() > skipSize).
>> 6753: map(MethodType::ptypes).
>> 6754: reduce((p, q) -> p.length >= q.length ? p : 
>> q).orElse(EMPTY);
> 
> Could avoid the need to introduce `EMPTY` if you make the stream expression 
> return the longest list directly:
> 
> reduce((p, q) -> {
> var longest = (p.size() >= q.size()) ? p : q;
> return List.of(Arrays.copyOfRange(longest, skipSize, 
> longest.size()); // checking isEmpty() is redundant here since we filter on 
> `t.parameterCount() > skipSize`
> }).orElse(List.of());

Using lambdas inside MethodHandles is quite dangerous given that lambdas are 
initialized using method handles. It may work now because 
longuestParameterList() is not called when initializing a lambda but it may 
make any changes in the implementation of lambdas painfull in the future.

-

PR: https://git.openjdk.org/jdk/pull/12025

Re: RFR: 8299576: Reimplement java.io.Bits using VarHandle access [v7]

[Rémi Forax]

On Mon, 9 Jan 2023 09:22:25 GMT, Per Minborg  wrote:

>> Currently, `java.io.Bits` is using explicit logic to read/write various 
>> primitive types to/from byte arrays. Switching to the use of `VarHandle` 
>> access would provide better performance and less code. 
>> 
>> Also, using a standard API for these conversions means future `VarHandle` 
>> improvements will benefit `Bits` too. 
>> 
>> Improvements in `Bits` will propagate to `ObjectInputStream`, 
>> `ObjectOutputStream` and `RandomAccessFile`.
>> 
>> Initial benchmarks and performance discussions can be found here: 
>> https://github.com/openjdk/panama-foreign/pull/762
>
> Per Minborg has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove faulty test tag, improve other test tag, fix comments

yes,
see 
https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/MethodHandle.html#sigpoly
and 
https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html

-

PR: https://git.openjdk.org/jdk/pull/11840

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v16]

[Rémi Forax]

On Thu, 10 Nov 2022 15:09:14 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Clean up new StringTemplate creation
>   
>   Centralized StringTemplate creation in StringTemplateImplFactory. Added 
> consistent defensive copying of lists. Changed List to List on 
> StringTemplate.interpolate and StringTemplate.of.

src/java.base/share/classes/java/lang/runtime/TemplateRuntime.java line 36:

> 34: import java.lang.template.StringTemplate;
> 35: import java.lang.template.ValidatingProcessor;
> 36: import java.util.*;

They come back :)

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v13]

[Rémi Forax]

On Sat, 5 Nov 2022 22:23:23 GMT, Rémi Forax  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Internalize FormatConcatItem
>
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 276:
> 
>> 274:  * @implNote Contents of both lists are copied to construct 
>> immutable lists.
>> 275:  */
>> 276: public static StringTemplate of(List fragments, 
>> List values) {
> 
> Should be `StringTemplate of(List fragments, List values) {`
> 
> The call to List.copyOf() will change the List to List.

> raw List ?

oops, formatting issue.

The idea is that `values` can be typed `List` because the call to 
List.copyOf() will take the `List` and return a `List`.

And as a type of a parameter `List` is better than `List` because 
`List` is a subtype of `List` but not a subtype of `List`.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v3]

[Rémi Forax]

On Wed, 9 Nov 2022 21:12:46 GMT, Jim Laskey  wrote:

>> Changing
>
> Raw list?

I think this comment is not on the right part of the code.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v13]

[Rémi Forax]

On Thu, 3 Nov 2022 17:23:53 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Internalize FormatConcatItem

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 158:

> 156: values[j++] = value;
> 157: }
> 158: }

The sides effects `fragments[i++] += fragmentIter.next();` and `i--` are ugly 
and can easily be overlook.
Also there is no need to use an iterator here, we know that the fragments have 
a List.get() in O(1).
The idea is to concat the last fragment of a template with the first one of the 
next template, so i propose

...
String[] fragments = new String[size + 1];
String last = "";
int i = 0, j = 0;
for (StringTemplate st : sts) {
List templateFragments = st.fragments();
fragments[i++] = last.concat(templateFragments.get(0));  // concat 
last fragment with first new fragment
int k = 0;
for(; k < templateFragments.size() - 1; k++) {
  fragments[i++] = templateFragments.get(k);
}
last = templateFragments.get(k);
for (Object value : st.values()) {
values[j++] = value;
}
}
fragments[i] = last;

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v13]

[Rémi Forax]

On Thu, 3 Nov 2022 17:23:53 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Internalize FormatConcatItem

src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 
100:

> 98:  * // check or manipulate the fragments and/or values
> 99:  * ...
> 100:  * return StringTemplate.interpolate(fragments, values);;

There is two semicolons instead of one at the end of this line

src/java.base/share/classes/java/lang/template/ValidatingProcessor.java line 
126:

> 124:  * }
> 125:  * The {@link StringTemplate#interpolate()} method is available for 
> those processors
> 126:  * that just need to work with the interpolatation;

type `interpolatation`-> `interpolation`

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v13]

[Rémi Forax]

On Thu, 3 Nov 2022 17:23:53 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Internalize FormatConcatItem

src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 159:

> 157: }
> 158: }
> 159: return new 
> SimpleStringTemplate(TemplateRuntime.toList(fragments), 
> TemplateRuntime.toList(values));

Should be
  `return new SimpleStringTemplate(List.copyOf(fragments), 
TemplateRuntime.toList(values));`
because only a value can be null, a fragment should not be null

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v13]

[Rémi Forax]

On Thu, 3 Nov 2022 17:23:53 GMT, Jim Laskey  wrote:

>> Enhance the Java programming language with string templates, which are 
>> similar to string literals but contain embedded expressions. A string 
>> template is interpreted at run time by replacing each expression with the 
>> result of evaluating that expression, possibly after further validation and 
>> transformation. This is a [preview language feature and 
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Internalize FormatConcatItem

src/java.base/share/classes/java/lang/template/StringTemplate.java line 276:

> 274:  * @implNote Contents of both lists are copied to construct 
> immutable lists.
> 275:  */
> 276: public static StringTemplate of(List fragments, List 
> values) {

Should be `StringTemplate of(List fragments, List values) {`

The call to List.copyOf() will change the List to List.

src/java.base/share/classes/java/lang/template/StringTemplate.java line 299:

> 297:  * @throws NullPointerException fragments or values is null or if 
> any of the fragments is null
> 298:  */
> 299: public static String interpolate(List fragments, 
> List values) {

Should be `String interpolate(List fragments, List values) {` as 
above

src/java.base/share/classes/java/lang/template/StringTemplate.java line 305:

> 303: int valuesSize = values.size();
> 304: if (fragmentsSize != valuesSize + 1) {
> 305: throw new RuntimeException("fragments must have one more 
> element than values");

Should be IllegalArgumentException (see method of() above)

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v7]

[Rémi Forax]

On Thu, 3 Nov 2022 14:37:55 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/java/lang/template/ProcessorLinkage.java line 60:
>> 
>>> 58:  * @throws NullPointerException if any of the arguments are null
>>> 59:  */
>>> 60: MethodHandle linkage(List fragments, MethodType type);
>> 
>> I suggest changing the protocol here to be able to take all bootstrap 
>> arguments into account, and return a `CallSite` instead. That will allow a 
>> `ProcessorLinkage` to take the lookup and name into account as well, and 
>> allows returning e.g. a `MutableCallSite` as well.
>> 
>> Maybe this can still be changed later as well though, since the interface is 
>> sealed.
>
> Yes - this will all go away during preview.

I disagree with Jorn,
- CallSite.dynamicInvoker() can be used to see a `MutableCallSite` as a 
`MethodHandle` so returning a `MethodHandle` is as powerful as returning a 
`CallSite`.
- Having a `ProcessorLinkage` that takes a Lookup as parameter is a security 
risk because it means that a processor have full access to the user code that 
calls the processor at runtime.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v11]

[Rémi Forax]

On Wed, 2 Nov 2022 21:53:00 GMT, Jorn Vernee  wrote:

>> src/java.base/share/classes/jdk/internal/template/TemplateSupport.java line 
>> 147:
>> 
>>> 145: 
>>> 146: return support.processWithProcessor();
>>> 147: }
>> 
>> Thinking about this protocol some more: this seems to depend on the 
>> processor being a constant, which precludes specialized linking of call 
>> sites where the processor is a dynamic argument.
>> 
>> The returned method handle/call site could maybe be changed to take the 
>> processor instance as a leading argument instead. The processor can then be 
>> used to pass additional arguments to the processing code (like the DB 
>> example in the JEP). (and, AFAICS, that will also allow using calls to this 
>> BSM as the sole translation strategy for every type of processor, which 
>> seems more robust if types are changed later on to (un-)implement 
>> `ProcessorLinkage`)
>> 
>> The `linkage` method could instead be a `static` method, which is somehow 
>> tied to the type of the processor. Since it's currently a sealed interface 
>> you could have a mapping from each implementer to the `linkage` method for 
>> the types you care about (only `FormatProcessor` atm). If that is to be 
>> opened up to public extension in the future, something like type classes 
>> would be needed I think, so that the runtime can reliably map from the 
>> processor type to the static `linkage` method.
>> 
>> WDYT?
>
> (FWIW, I don't see this as prohibitive for this PR to go ahead, but maybe 
> something to consider before finalizing the feature)

Yes, the current code only supports processors with no fields, which is Okay 
given that the interface  `ProcessorLinkage` is sealed.

If/When the processor linkage API is open for everyone as you said the design 
will have to be changed.
But going the other way, creates a general mechanism is painful because either 
you have an adhoc way to associate  the processor to a BSM-like method or you 
have to delay until runtime the linkage using a monomorphic/polymorphic cache 
(I know Jim has tested that before back-pedaling to a simpler more constrained 
good enough solution).

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v7]

[Rémi Forax]

On Wed, 2 Nov 2022 18:27:30 GMT, Jim Laskey  wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java 
>> line 429:
>> 
>>> 427: }
>>> 428: 
>>> 429: private JCClassDecl newStringTemplateClass() {
>> 
>> I find it weird to have the compiler emit an implementation of 
>> StringTemplate just to capture what is there in the code. If there are many 
>> usages of string templates, this could lead to a proliferation of synthetic 
>> classes. Perhaps we should consider using a metafactory here, like we do for 
>> lambdas, so that we can return some private JDK StringTemplate 
>> implementation type.
>
> The main consideration is performance. I spent quite a bit of time playing 
> around with different implementations including metafactories (hence the 
> carrier class work.) Since a majority of use cases will be STR and FMT, the 
> number of classes will likely be just a few per application. Because of the 
> change to force processor always, I will be revisiting this during the 
> preview period to work on other solutions (I mentioned 
> ProcessorFactories/ProcessorBuilders earlier).

@JimLaskey, someone will implement a LOG at some point in the future and you 
will get many template classes per application class.
You mention the carrier but i believe you can implement something similar to a 
carrier erasing the type of the values but without using method handles to try 
to make it type safe and instead pass the real types as a class data of a 
hidden class and later as a type argument once the reified generics will be 
released.

-

PR: https://git.openjdk.org/jdk/pull/10889

Re: RFR: JDK-8285932 Implementation of JEP-430 String Templates (Preview) [v7]

[Rémi Forax]

On Tue, 1 Nov 2022 18:22:07 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/java/lang/template/TemplateRuntime.java line 119:
>> 
>>> 117: Class tsClass = st.getClass();
>>> 118: if (tsClass.isSynthetic()) {
>>> 119: try {
>> 
>> I do not know if this code is worth of optimizing but the way to avoid to 
>> recompute the List> each time is to use a java.lang.ClassValue and 
>> store the classes inside an unmodifiable List. (Field[] -> Class[] -> 
>> List>) The last leg can be done just by calling List.of(), there is 
>> no need for an ArrayList here
>
> Will use List.of. I think use case is raw and caching should be left to the 
> user.

i agree

-

PR: https://git.openjdk.org/jdk/pull/10889