Re: RFR: 8292914: Drop the counter from lambda class names [v4]

[Ioi Lam]

On Thu, 16 Feb 2023 01:42:16 GMT, David M. Lloyd  wrote:

>> The class generated for lambda proxies is now defined as a hidden class. 
>> This means that the counter, which was used to ensure a unique class name 
>> and avoid clashes, is now redundant. In addition to performing redundant 
>> work, this also impacts build reproducibility for native image generators 
>> which might already have a strategy to cope with hidden classes but cannot 
>> cope with indeterminate definition order for lambda proxy classes.
>> 
>> This solves JDK-8292914 by making lambda proxy names always be stable 
>> without any configuration needed. This would also replace #10024.
>
> David M. Lloyd has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Fix LambdaAsm test
>  - Fix stack walker test

For debugging, the `-Djava.lang.invoke.MethodHandle.DUMP_CLASS_FILES=true`  
property addes a unique suffix to the generated `LambdaForm$MH` classes.

https://github.com/openjdk/jdk/blob/1480d418e3b7d1f36ace24a043a273fca446eefa/src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java#L128-L130

So for Lambda proxy classes, instead of inventing a new way of unique class 
names, perhaps we can use the existing AtomicInteger counter only when 
`-Djdk.internal.lambda.dumpProxyClasses=` is specified?

-

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

Re: RFR: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false [v2]

[Martin Doerr]

On Wed, 15 Feb 2023 20:30:00 GMT, Richard Reingruber  wrote:

>> This fixes the linked issue by trimming the caller of a frame to be 
>> deoptimized back to its `unextended_sp` iff it is compiled. The creation of 
>> the section `dead after deoptimization` shown in the attachment 
>> [yield_after_deopt_failure.log](https://bugs.openjdk.org/secure/attachment/102602/yield_after_deopt_failure.log)
>>  is prevented by this.
>> 
>> A new mode is added to the test BasicExt.java where all frames are 
>> deoptimized after a yield operation. The issue can be deterministically 
>> reproduced with the new mode. It's not worth to execute all test cases with 
>> the new mode though. Instead `ContinuationCompiledFramesWithStackArgs_3c4` 
>> is always executed a 2nd time in this mode.
>> 
>> Before this BasicExt.java was refactored for better argument processing and 
>> representation of the test modes.
>> Also the try-catch-clause in the main method had to be changed to rethrow 
>> the caught exception because without this the test would have succeeded.
>> 
>> Testing: jtreg tests tier 1-4 on standard platforms and also on ppc64le.
>
> Richard Reingruber has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Improve comment
>  - Improve comment
>  - Spelling

Thanks!

-

Marked as reviewed by mdoerr (Reviewer).

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

Re: RFR: 8299748: java/util/zip/Deinflate.java failing on s390x

[Amit Kumar]

On Wed, 15 Feb 2023 08:15:28 GMT, Alan Bateman  wrote:

>I assume you can quickly check which loop, I suspect it's the deflate loop 
>based of the previous comments/experiments.

Both of loop's are  inf-loops with this change. If somehow I terminate the 
first loop then second goes into infinite state. 

>Your initial proposal was to increase the size of out1 but I can't see any 
>cases in this test where the deflate fills out1 completely.

Alan Here are some stats (after increasing the `out1` size with a factor of 
32), where I was looking for `what size of array do we need to store compressed 
data` and `array capacity we have`: 

Capacity with out1 * 32: 557056 <-> Buffer we need 552981
Capacity with out1 * 32: 557056 <-> Buffer we need 552975

Initial size of `out1`:  525312

As you can see, the size of out1 is insufficient for the s390x. That's why I 
proposed the fix to increase the buffer size. 
Because the compressor is not deterministic, I don't want to be byte-perfect, 
so I changed the multiplier to `64` to leave some extra space here.

-

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

Withdrawn: 8296935: Arrays.asList().set() with wrong types throws undocumented ArrayStoreException

[Tingjun Yuan]

On Mon, 23 Jan 2023 02:35:19 GMT, Tingjun Yuan  wrote:

> Document `java.util.Arrays.asList` that the list will throw an 
> `ArrayStoreException` when attempting to set an element with a wrong type.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8296935: Arrays.asList().set() with wrong types throws undocumented ArrayStoreException [v8]

[Tingjun Yuan]

On Thu, 16 Feb 2023 04:39:20 GMT, Stuart Marks  wrote:

>> I've written a CSR report for this PR as below.  Could someone please help 
>> me to submit it to the JBS if it looks okay? Thank you!
>> 
>> ---
>> 
>> **Compatibility Risk:** minimum
>> 
>> **Compatibility Risk Description:** No implementation is modified
>> 
>> ## Summary
>> 
>> Document the current behavior of `java.util.Arrays.asList(Object...)` that 
>> it will throw an `ArrayStoreException` when an element of wrong type is 
>> trying to be set into the list.
>> 
>> ## Problem
>> 
>> `java.util.Arrays.asList(Object...)` is a widely used method that wraps an 
>> object array into a mutable `List`.  The specification of  methods 
>> `List.set` and `ListIterator.set` indicates that implementations should 
>> throw a `ClassCastException` when the class of the specified element 
>> prevents it from being added to this list. However when an application tries 
>> to store an element that is not compatible with the backing array component 
>> type via the returned list, an `ArrayStoreException` will be thrown instead 
>> of `ClassCastException`, which violates the `List` specification.
>> 
>> ## Solution
>> 
>> Since this method is widely used, we do not change the current behavior. 
>> Instead, we document the current behavior of 
>> `java.util.Arrays.asList(Object...)` that it will throw an 
>> `ArrayStoreException` when an element of wrong type is trying to be set into 
>> the list.
>> 
>> ## Specification
>> 
>> Add the following `@apiNote` section into the `java.util.Arrays.asList` 
>> specification:
>> 
>> 
>>  * @apiNote
>>  * The {@link List} returned by this method does not conform to the
>>  * specification for {@link List#set} and {@link ListIterator#set}.
>>  * It is possible for the type parameter {@code T} of the returned list
>>  * to differ from the array's component type. This can occur, for 
>> example,
>>  * if the array argument type has been upcast from its component type:
>>  *
>>  * {@snippet :
>>  *String[] strings = new String[1];
>>  *List objects = Arrays.asList((Object[])strings);
>>  * }
>>  *
>>  * Writing an element into the returned list may result in an
>>  * {@link ArrayStoreException} instead of {@link ClassCastException} 
>> being
>>  * thrown if the element is incompatible with the underlying array's
>>  * component type:
>>  *
>>  * {@snippet :
>>  *objects.set(0, Integer.valueOf(0)); // throws ArrayStoreException
>>  * }
>
> @yuantj 
> 
> The proposed change needs additional editing. In particular, the issue is not 
> about conformance. I'd need to suggest changes that you'd apply, in order to 
> get the text in the change to match what I'm essentially writing myself. In 
> addition, CSR also needs quite a bit of editing, which I'd write myself 
> anyway. Thus I don't think it's worth continuing with this PR. Thanks for 
> your efforts, though, in raising the issue and attempting to address it.
> 
> The main point is that this isn't a spec violation, and if it were, one part 
> of the specification can't "note" itself out of conforming to another part of 
> the specification. The issue is really a clarification that some 
> implementations might throw exceptions beyond those explicitly listed in the 
> interface.

@stuart-marks Okay, I got it. Thanks for your reviewing anyway. Going to close 
this PR.

-

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

Re: RFR: 8296935: Arrays.asList().set() with wrong types throws undocumented ArrayStoreException [v8]

[Stuart Marks]

On Wed, 8 Feb 2023 23:33:57 GMT, Tingjun Yuan  wrote:

>> Tingjun Yuan has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update Arrays.java
>
> I've written a CSR report for this PR as below.  Could someone please help me 
> to submit it to the JBS if it looks okay? Thank you!
> 
> ---
> 
> **Compatibility Risk:** minimum
> 
> **Compatibility Risk Description:** No implementation is modified
> 
> ## Summary
> 
> Document the current behavior of `java.util.Arrays.asList(Object...)` that it 
> will throw an `ArrayStoreException` when an element of wrong type is trying 
> to be set into the list.
> 
> ## Problem
> 
> `java.util.Arrays.asList(Object...)` is a widely used method that wraps an 
> object array into a mutable `List`.  The specification of  methods `List.set` 
> and `ListIterator.set` indicates that implementations should throw a 
> `ClassCastException` when the class of the specified element prevents it from 
> being added to this list. However when an application tries to store an 
> element that is not compatible with the backing array component type via the 
> returned list, an `ArrayStoreException` will be thrown instead of 
> `ClassCastException`, which violates the `List` specification.
> 
> ## Solution
> 
> Since this method is widely used, we do not change the current behavior. 
> Instead, we document the current behavior of 
> `java.util.Arrays.asList(Object...)` that it will throw an 
> `ArrayStoreException` when an element of wrong type is trying to be set into 
> the list.
> 
> ## Specification
> 
> Add the following `@apiNote` section into the `java.util.Arrays.asList` 
> specification:
> 
> 
>  * @apiNote
>  * The {@link List} returned by this method does not conform to the
>  * specification for {@link List#set} and {@link ListIterator#set}.
>  * It is possible for the type parameter {@code T} of the returned list
>  * to differ from the array's component type. This can occur, for example,
>  * if the array argument type has been upcast from its component type:
>  *
>  * {@snippet :
>  *String[] strings = new String[1];
>  *List objects = Arrays.asList((Object[])strings);
>  * }
>  *
>  * Writing an element into the returned list may result in an
>  * {@link ArrayStoreException} instead of {@link ClassCastException} being
>  * thrown if the element is incompatible with the underlying array's
>  * component type:
>  *
>  * {@snippet :
>  *objects.set(0, Integer.valueOf(0)); // throws ArrayStoreException
>  * }

@yuantj 

The proposed change needs additional editing. In particular, the issue is not 
about conformance. I'd need to suggest changes that you'd apply, in order to 
get the text in the change to match what I'm essentially writing myself. In 
addition, CSR also needs quite a bit of editing, which I'd write myself anyway. 
Thus I don't think it's worth continuing with this PR. Thanks for your efforts, 
though, in raising the issue and attempting to address it.

The main point is that this isn't a spec violation, and if it were, one part of 
the specification can't "note" itself out of conforming to another part of the 
specification. The issue is really a clarification that some implementations 
might throw exceptions beyond those explicitly listed in the interface.

-

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

Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v8]

[Ian Graves]

> This is an approach to adding a flag to jlink that will allow --compress to 
> take the same types of arguments as jmod, thus bringing the two into 
> alignment. This likely requires a CSR and a discussion on whether we should 
> deprecate or simply remove the original numeric compression arguments.

Ian Graves has updated the pull request incrementally with one additional 
commit since the last revision:

  Updating properties

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11617/files
  - new: https://git.openjdk.org/jdk/pull/11617/files/92416003..c9f4f32b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11617=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=11617=06-07

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

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

Re: RFR: 8292914: Drop the counter from lambda class names [v4]

[David M . Lloyd]

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

David M. Lloyd has updated the pull request incrementally with two additional 
commits since the last revision:

 - Fix LambdaAsm test
 - Fix stack walker test

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12579/files
  - new: https://git.openjdk.org/jdk/pull/12579/files/85620a44..d56b77a2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12579=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=12579=02-03

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

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

Re: RFR: 8292914: Drop the counter from lambda class names [v3]

[David M . Lloyd]

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

David M. Lloyd has updated the pull request incrementally with two additional 
commits since the last revision:

 - Import
 - Beter timestamp placement

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12579/files
  - new: https://git.openjdk.org/jdk/pull/12579/files/0e109beb..85620a44

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12579=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=12579=01-02

  Stats: 8 lines in 2 files changed: 4 ins; 2 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/12579.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12579/head:pull/12579

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

Re: RFR: 8292914: Drop the counter from lambda class names [v2]

[David M . Lloyd]

On Wed, 15 Feb 2023 22:44:21 GMT, David M. Lloyd  wrote:

>> The class generated for lambda proxies is now defined as a hidden class. 
>> This means that the counter, which was used to ensure a unique class name 
>> and avoid clashes, is now redundant. In addition to performing redundant 
>> work, this also impacts build reproducibility for native image generators 
>> which might already have a strategy to cope with hidden classes but cannot 
>> cope with indeterminate definition order for lambda proxy classes.
>> 
>> This solves JDK-8292914 by making lambda proxy names always be stable 
>> without any configuration needed. This would also replace #10024.
>
> David M. Lloyd has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8292914: Drop the counter from lambda class names
>   
>   The class generated for lambda proxies is now defined as a hidden class. 
> This means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
>   
>   Additionally, the lambda proxy class dumper is enhanced to prepend a raw 
> timestamp before the filename.

It looks like just prepending a timestamp is not exactly ideal since it would 
put it before the first directory segment of the path. So, I'm looking at 
wedging it in before the last segment. Also, I'm just using a raw timestamp 
value, because the date formatting infrastructure relies on lambdas.

-

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

Re: RFR: 8292914: Drop the counter from lambda class names [v2]

[Mandy Chung]

On Wed, 15 Feb 2023 22:44:21 GMT, David M. Lloyd  wrote:

>> The class generated for lambda proxies is now defined as a hidden class. 
>> This means that the counter, which was used to ensure a unique class name 
>> and avoid clashes, is now redundant. In addition to performing redundant 
>> work, this also impacts build reproducibility for native image generators 
>> which might already have a strategy to cope with hidden classes but cannot 
>> cope with indeterminate definition order for lambda proxy classes.
>> 
>> This solves JDK-8292914 by making lambda proxy names always be stable 
>> without any configuration needed. This would also replace #10024.
>
> David M. Lloyd has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8292914: Drop the counter from lambda class names
>   
>   The class generated for lambda proxies is now defined as a hidden class. 
> This means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
>   
>   Additionally, the lambda proxy class dumper is enhanced to prepend a raw 
> timestamp before the filename.

This would address my concern.  If the hidden class was defined successfully, 
use the hidden class name as the filename;  otherwise, construct the file name 
a different counter - I also like Brian's suggestion "$timestamp-$classname$" 
for the reasons he noted.

Note that the hidden class name returned by `Class::getName` is `p.Foo$Lambda$$ 
 + "/" + `.   When dumping to a file, the pathname should be 
`p/Foo$Lambda$$.` matching the type descriptor (converting '/' to '.').

-

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

Integrated: JDK-8302026: Port fdlibm inverse trig functions (asin, acos, atan) to Java

[Joe Darcy]

On Mon, 13 Feb 2023 22:03:14 GMT, Joe Darcy  wrote:

> Proceeding down the line of FDLIBM functions to be ported, next up are asin, 
> acos, and atan.
> 
> Diffs of the various versions will follow in a separate message.
> 
> There were no unusual coding idioms encountered in porting these methods.

This pull request has now been integrated.

Changeset: 3ba15608
Author:Joe Darcy 
URL:   
https://git.openjdk.org/jdk/commit/3ba156082b73c4a8e9d890a57a42fb68df2bf98f
Stats: 994 lines in 6 files changed: 986 ins; 0 del; 8 mod

8302026: Port fdlibm inverse trig functions (asin, acos, atan) to Java

Reviewed-by: bpb

-

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

Re: RFR: JDK-8301444: Port fdlibm hyperbolic transcendental functions to Java [v11]

[Joe Darcy]

> Initial pass of porting FDLIBM sinh/cosh/tanh to Java. I do intend to 
> refactor the regression tests a bit to reduce duplication, but the actual 
> ports should be ready for review.
> 
> Diff'ing the ports as before, original vs transliteration port:
> 
> 
> $ diff -w Hyperbolic.c Hyperbolic.translit.java
> 1c1
> < /* __ieee754_sinh(x)
> ---
>> /**
> 17a18,19
>> static class Sinh {
>> private static final double one = 1.0, shuge = 1.0e307;
> 19,33c21
> < #include "fdlibm.h"
> < 
> < #ifdef __STDC__
> < static const double one = 1.0, shuge = 1.0e307;
> < #else
> < static double one = 1.0, shuge = 1.0e307;
> < #endif
> < 
> < #ifdef __STDC__
> < double __ieee754_sinh(double x)
> < #else
> < double __ieee754_sinh(x)
> < double x;
> < #endif
> < {
> ---
>> private static double compute(double x) {
> 36c24
> < unsigned lx;
> ---
>> /* unsigned */ int lx;
> 51c39
> < t = expm1(fabs(x));
> ---
>> t = FdlibmTranslit.expm1(Math.abs(x));
> 57c45
> < if (ix < 0x40862E42)  return h*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42)  return h*StrictMath.exp(Math.abs(x)); // 
>> TODO switch to translit
> 60,62c48,52
> < lx = *( (((*(unsigned*))>>29)) + (unsigned*));
> < if (ix<0x408633CE || 
> ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(0.5*fabs(x));
> ---
>> // lx = *( (((*(unsigned*))>>29)) + (unsigned*));
>> // lx =  (((*(unsigned*))>>29)) + (unsigned*) ;
>> lx = __LO(x);
>> if (ix<0x408633CE || 
>> ((ix==0x408633ce)&&(Long.compareUnsigned(lx, 0x8fb9f87d) <= 0 ))) {
>> w = StrictMath.exp(0.5*Math.abs(x)); // TODO switch to 
>> translit
> 70c60,62
> < /* __ieee754_cosh(x)
> ---
>> }
>> 
>> /**
> 90,105c82,84
> < 
> < #include "fdlibm.h"
> < 
> < #ifdef __STDC__
> < static const double one = 1.0, half=0.5, huge = 1.0e300;
> < #else
> < static double one = 1.0, half=0.5, huge = 1.0e300;
> < #endif
> < 
> < #ifdef __STDC__
> < double __ieee754_cosh(double x)
> < #else
> < double __ieee754_cosh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Cosh {
>> private static final double one = 1.0, half=0.5, huge = 1.0e300;
>> private static double compute(double x) {
> 108c87
> < unsigned lx;
> ---
>> /*unsigned*/ int lx;
> 119c98
> < t = expm1(fabs(x));
> ---
>> t = expm1(Math.abs(x));
> 127c106
> < t = __ieee754_exp(fabs(x));
> ---
>> t = StrictMath.exp(Math.abs(x)); // TODO switch to translit
> 132c111
> < if (ix < 0x40862E42)  return half*__ieee754_exp(fabs(x));
> ---
>> if (ix < 0x40862E42)  return half*StrictMath.exp(Math.abs(x)); 
>> // TODO switch to translit
> 135c114
> < lx = *( (((*(unsigned*))>>29)) + (unsigned*));
> ---
>> lx = __LO(x);
> 137,138c116,117
> <   ((ix==0x408633ce)&&(lx<=(unsigned)0x8fb9f87d))) {
> < w = __ieee754_exp(half*fabs(x));
> ---
>> ((ix==0x408633ce)&&(Integer.compareUnsigned(lx, 0x8fb9f87d) 
>> <= 0))) {
>> w = StrictMath.exp(half*Math.abs(x)); // TODO switch to 
>> translit
> 146c125,127
> < /* Tanh(x)
> ---
>> }
>> 
>> /**
> 169,184c150,152
> < 
> < #include "fdlibm.h"
> < 
> < #ifdef __STDC__
> < static const double one=1.0, two=2.0, tiny = 1.0e-300;
> < #else
> < static double one=1.0, two=2.0, tiny = 1.0e-300;
> < #endif
> < 
> < #ifdef __STDC__
> < double tanh(double x)
> < #else
> < double tanh(x)
> < double x;
> < #endif
> < {
> ---
>> static class Tanh {
>> private static final double one=1.0, two=2.0, tiny = 1.0e-300;
>> static double compute(double x) {
> 203c171
> < t = expm1(two*fabs(x));
> ---
>> t = expm1(two*Math.abs(x));
> 206c174
> < t = expm1(-two*fabs(x));
> ---
>> t = expm1(-two*Math.abs(x));
> 214a183
>> }
> 
> 
> Note that the original has a in-line version of the "__LO" macro rather than 
> using the macro.
> 
> 
> And transliteration vs more idiomatic:
> 
> 
> $ diff -w Hyperbolic.translit.java Hyperbolic.fdlibm.java 
> 21c21
> < private static double compute(double x) {
> ---
>>  static double compute(double x) {
> 26c26
> < /* High word of |x|. */
> ---
>> // High word of |x|
> 28c28
> < ix = jx&0x7fff;
> ---
>> ix = jx & 0x7fff_;
> 30,31c30,33
> < /* x is INF or NaN */
> < if(ix>=0x7ff0) return x+x;
> ---
>> // x is INF or NaN
>> if ( ix >= 0x7ff0_) {
>> return x + x;
>> }
> 34,40c36,48
> < if (jx<0) h = -h;
> < /* |x| in [0,22], return sign(x)*0.5*(E+E/(E+1))) */
> < if (ix < 

Re: RFR: 8292914: Drop the counter from lambda class names [v2]

[David M . Lloyd]

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

David M. Lloyd has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  8292914: Drop the counter from lambda class names
  
  The class generated for lambda proxies is now defined as a hidden class. This 
means that the counter, which was used to ensure a unique class name and avoid 
clashes, is now redundant. In addition to performing redundant work, this also 
impacts build reproducibility for native image generators which might already 
have a strategy to cope with hidden classes but cannot cope with indeterminate 
definition order for lambda proxy classes.
  
  Additionally, the lambda proxy class dumper is enhanced to prepend a raw 
timestamp before the filename.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12579/files
  - new: https://git.openjdk.org/jdk/pull/12579/files/5ab516a5..0e109beb

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12579=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=12579=00-01

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

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

Re: RFR: JDK-8302026: Port fdlibm inverse trig functions (asin, acos, atan) to Java [v3]

[Joe Darcy]

> Proceeding down the line of FDLIBM functions to be ported, next up are asin, 
> acos, and atan.
> 
> Diffs of the various versions will follow in a separate message.
> 
> There were no unusual coding idioms encountered in porting these methods.

Joe Darcy has updated the pull request incrementally with one additional commit 
since the last revision:

  Include bug number in test.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12545/files
  - new: https://git.openjdk.org/jdk/pull/12545/files/e8195463..6c2a70af

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12545=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=12545=01-02

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

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

Integrated: 8302163: Speed up various String comparison methods with ArraysSupport.mismatch

[Claes Redestad]

On Mon, 13 Feb 2023 09:59:24 GMT, Claes Redestad  wrote:

> We can improve various String methods such as `startsWith`, `endsWith` and 
> `regionMatches` by leveraging the intrinsified mismatch methods in 
> `ArraysSupport`.

This pull request has now been integrated.

Changeset: 861e3020
Author:Claes Redestad 
URL:   
https://git.openjdk.org/jdk/commit/861e302011bb3aaf0c8431c121b58a57b78481e3
Stats: 171 lines in 5 files changed: 107 ins; 44 del; 20 mod

8302163: Speed up various String comparison methods with ArraysSupport.mismatch

Reviewed-by: stsypanov, rriggs, alanb

-

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

Re: RFR: 8302163: Speed up various String comparison methods with ArraysSupport.mismatch [v2]

[Claes Redestad]

On Mon, 13 Feb 2023 16:10:14 GMT, Claes Redestad  wrote:

>> We can improve various String methods such as `startsWith`, `endsWith` and 
>> `regionMatches` by leveraging the intrinsified mismatch methods in 
>> `ArraysSupport`.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Clarify coder shift in startsWith

Thanks for reviewing!

-

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

Re: RFR: JDK-8302026: Port fdlibm inverse trig functions (asin, acos, atan) to Java [v2]

[Brian Burkhalter]

On Wed, 15 Feb 2023 01:13:16 GMT, Joe Darcy  wrote:

>> Proceeding down the line of FDLIBM functions to be ported, next up are asin, 
>> acos, and atan.
>> 
>> Diffs of the various versions will follow in a separate message.
>> 
>> There were no unusual coding idioms encountered in porting these methods.
>
> Joe Darcy has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Add asin test case.
>  - Refactor regression tests.

Looks fine.

-

Marked as reviewed by bpb (Reviewer).

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

Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v7]

[Ian Graves]

> This is an approach to adding a flag to jlink that will allow --compress to 
> take the same types of arguments as jmod, thus bringing the two into 
> alignment. This likely requires a CSR and a discussion on whether we should 
> deprecate or simply remove the original numeric compression arguments.

Ian Graves has updated the pull request incrementally with three additional 
commits since the last revision:

 - Removing string compression plugin
 - Fixing errant compact-constant-pools
 - ShareUTF8Entries update

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11617/files
  - new: https://git.openjdk.org/jdk/pull/11617/files/2af25227..92416003

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=11617=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=11617=05-06

  Stats: 377 lines in 4 files changed: 0 ins; 373 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/11617.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11617/head:pull/11617

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

Re: RFR: 8292914: Drop the counter from lambda class names

[Brian Goetz]

On Wed, 15 Feb 2023 20:46:47 GMT, David M. Lloyd  wrote:

>> The class generated for lambda proxies is now defined as a hidden class. 
>> This means that the counter, which was used to ensure a unique class name 
>> and avoid clashes, is now redundant. In addition to performing redundant 
>> work, this also impacts build reproducibility for native image generators 
>> which might already have a strategy to cope with hidden classes but cannot 
>> cope with indeterminate definition order for lambda proxy classes.
>> 
>> This solves JDK-8292914 by making lambda proxy names always be stable 
>> without any configuration needed. This would also replace #10024.
>
> (On the debugability topic) I had thought to change the lambda class dumping 
> code so that it would dump the class bytes *after* the class was defined (to 
> capture the unique hidden class name completely). But this would exclude the 
> case where the class would fail to be defined, which would probably undermine 
> some significant percentage of the cases where you'd want a dump in the first 
> place. Maybe in this case, since the class name would never appear in any 
> stack trace anyway (and thus there is nothing to actually correlate), it 
> could be dumped with a separate counter like `..Lambda$$-error1.class` or 
> something?

@dmlloyd Exactly so.  The dumping is most useful in debugging cases where there 
are weird issues with access control or verification, or bugs in LMF that might 
cause errors on load.  I remember before we put the counter in, being bitten by 
"the first dumped $Lambda$blarg class file was overwrittten by the second, and 
I couldn't find the bug because it was in the first one, but I didn't realize 
there were two."  So the counter actually did pay its way at least once.  

That said, I do think the filename conflict was the main reason we needed it, 
and there are other ways to disambiguate file names.  In fact, it might be even 
better if we constructed the file name as "$timestamp-$classname$", so that (a) 
they would sort and you could find the most recent ones easily, and (b) dumps 
from later runs would not clobber dumps from previous runs.  I think that would 
also solve @mlchung 's concerns?

-

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

Re: RFR: 8292914: Drop the counter from lambda class names

[David M . Lloyd]

On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd  wrote:

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

(On the debugability topic) I had thought to change the lambda class dumping 
code so that it would dump the class bytes *after* the class was defined (to 
capture the unique hidden class name completely). But this would exclude the 
case where the class would fail to be defined, which would probably undermine 
some significant percentage of the cases where you'd want a dump in the first 
place. Maybe in this case, since the class name would never appear in any stack 
trace anyway (and thus there is nothing to actually correlate), it could be 
dumped with a separate counter like `..Lambda$$-error1.class` or something?

-

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

Re: RFR: 8292914: Introduce a system property that enables stable names for lambda proxy classes [v7]

[Brian Goetz]

On Mon, 21 Nov 2022 16:46:43 GMT, Strahinja Stanojevic  wrote:

>> This PR introduces an option to output stable names for the lambda classes 
>> in the JDK. A stable name consists of two parts: The first part is the 
>> predefined value `$$Lambda$` appended to the lambda capturing class, and the 
>> second is a 64-bit hash part of the name. Thus, it looks like 
>> `lambdaCapturingClass$$Lambda$hashValue`.
>> Parameters used to create a stable hash are a superset of the parameters 
>> used for lambda class archiving when the CDS dumping option is enabled. 
>> During this process, all the mutual parameters are in the same form as they 
>> are in the low-level implementation 
>> (`SystemDictionaryShared::add_lambda_proxy_class`) of the archiving process.
>> We decided to use a well-specified `CRC32` algorithm from the standard Java 
>> library. We created two 32-bit hashes from the parameters used to create 
>> stable names. Then, we combined those two 32-bit hashes into one 64-bit hash 
>> value.
>> We chose `CRC32` because it is a well-specified hash function, and we don't 
>> need to write additional code in the JDK. `SHA-256, MD5`, and all other hash 
>> functions that rely on `MessageDigest` use lambdas in the implementation, so 
>> they are unsuitable for our purpose. We also considered a few different hash 
>> functions with a low collision rate. All these functions would require at 
>> least 100 lines of additional code in the JDK. The best alternative we found 
>> is 64-bit` MurmurHash2`: 
>> https://commons.apache.org/proper/commons-codec/jacoco/org.apache.commons.codec.digest/MurmurHash2.java.html.
>>   In case adding a new hash implementation (e.g., Murmur2) to the JDK is 
>> preferred, this PR can be easily modified.
>> We found the post 
>> (https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed/145633#145633)
>>  that compares different hash functions.
>> We also tested the `CRC32` hash function against half a billion generated 
>> strings, and there were no collisions. Note that the capturing-class name is 
>> also part of the lambda class name, so the potential collisions can only 
>> appear in a single class. Thus, we do not expect to have name collisions due 
>> to a relatively low number of lambdas per class. Every tool that uses this 
>> feature should handle potential collisions on its own.  
>> We found an overall approximation of the collision rate too. You can find it 
>> here: https://preshing.com/20110504/hash-collision-probabilities/.
>> 
>> JDK currently adds an atomic integer after `$$Lambda$`, and the names of the 
>> lambdas depend on the creation order. In the `TestStableLambdaNames`, we 
>> generate all the lambdas two times. In the first run, the method 
>> createPlainLambdas generate the following lambdas:
>> 
>> - TestStableLambdaNames$$Lambda$1/0x000800c00400
>> - TestStableLambdaNames$$Lambda$2/0x000800c01800
>> - TestStableLambdaNames$$Lambda$3/0x000800c01a38
>> The same method in the second run generates lambdas with different names:
>> - TestStableLambdaNames$$Lambda$1471/0x000800d1
>> - TestStableLambdaNames$$Lambda$1472/0x000800d10238
>> - TestStableLambdaNames$$Lambda$1473/0x000800d10470
>> 
>> If we use the introduced flag, generated lambdas are:
>> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800c00400
>> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800c01800
>> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800c01a38
>> In the second run of the method, generated lambdas are:
>> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800d1
>> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800d10238
>> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800d10470
>> 
>> We can see that the introduced hash value does not change between two calls 
>> of the method `createPlainLambdas`. That was not the case in the JDK run 
>> without this change. Those lambdas are extracted directly from the test.
>
> Strahinja Stanojevic has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove address from lambda class names in test on the 32-bit architecture 
> too

David Llloyd's proposed solution is dramatically less intrusive, so it should 
be preferred over the one here.

-

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

Re: RFR: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false [v2]

[Richard Reingruber]

On Wed, 15 Feb 2023 12:35:52 GMT, Goetz Lindenmaier  wrote:

>> Richard Reingruber has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - Improve comment
>>  - Improve comment
>>  - Spelling
>
> src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp line 3094:
> 
>> 3092: 
>> 3093:   // Freezing continuation frames requires that the caller is trimmed 
>> to unextended sp if compiled.
>> 3094:   Register caller_sp = R23_tmp3;
> 
> Can the caller be interpreted? I.e., not compiled?
> Then it might be helpful to comment like
> // If not compiled this contains sp() resulting in a resize of 0.

Yes it can be interpreted. I've extended the comment for this case.

-

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

Re: RFR: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false [v2]

[Richard Reingruber]

> This fixes the linked issue by trimming the caller of a frame to be 
> deoptimized back to its `unextended_sp` iff it is compiled. The creation of 
> the section `dead after deoptimization` shown in the attachment 
> [yield_after_deopt_failure.log](https://bugs.openjdk.org/secure/attachment/102602/yield_after_deopt_failure.log)
>  is prevented by this.
> 
> A new mode is added to the test BasicExt.java where all frames are 
> deoptimized after a yield operation. The issue can be deterministically 
> reproduced with the new mode. It's not worth to execute all test cases with 
> the new mode though. Instead `ContinuationCompiledFramesWithStackArgs_3c4` is 
> always executed a 2nd time in this mode.
> 
> Before this BasicExt.java was refactored for better argument processing and 
> representation of the test modes.
> Also the try-catch-clause in the main method had to be changed to rethrow the 
> caught exception because without this the test would have succeeded.
> 
> Testing: jtreg tests tier 1-4 on standard platforms and also on ppc64le.

Richard Reingruber has updated the pull request incrementally with three 
additional commits since the last revision:

 - Improve comment
 - Improve comment
 - Spelling

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12557/files
  - new: https://git.openjdk.org/jdk/pull/12557/files/7ca17977..9d647388

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12557=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=12557=00-01

  Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/12557.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12557/head:pull/12557

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

Re: RFR: 8292914: Drop the counter from lambda class names

[David M . Lloyd]

On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd  wrote:

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

Sure, that's fine. I think debugging is a totally valid consideration; in fact 
it might be nice to have a broader facility for dumping of any hidden class 
(maybe that's what you are alluding to). But if you think it's best to wait 
then I won't pursue it further at this time.

-

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

Re: RFR: 8292914: Drop the counter from lambda class names

[Brian Goetz]

On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd  wrote:

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

While I agree with your assessment that this is a much less intrusive solution 
to JDK-8292914 than the proposed system property, I also take Mandy's concern 
seriously here.  

I suspect that as Leyden progresses, there will be multiple issues we may want 
to consider in various bootstrap implementations to make them more amenable to 
time-shifting.  I propose that we leave this PR on the table to be considered 
when we take up the issue more holistically, rather than doing it in bits and 
pieces?

-

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

Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v6]

[Ian Graves]

On Wed, 15 Feb 2023 18:23:02 GMT, Mandy Chung  wrote:

>> I think that deprecating the entire plugin could be appropriate, given the 
>> overhead, unless there's some performance data to the contrary. I haven't 
>> seen much in favor of keeping it, but I do think that breaking it apart from 
>> zip before deprecating it is also fine.
>
> If we are planning to drop this, we will not need to introduce a new plugin 
> option but instead states that `--compress 1` will be deprecated and removed 
> in a future release.

That makes sense to me. May want @AlanBateman's opinion before final action.

-

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

Re: RFR: 8292914: Drop the counter from lambda class names

[Mandy Chung]

On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd  wrote:

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

we want the generated classes to be dumped for debugging before the hidden 
class is defined e.g. to troubleshoot class loading issue (see  
`-Djdk.internal.lambda.dumpProxyClasses=` system property)

-

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

Integrated: JDK-8301460: Clean up LambdaForm to reference BasicType enums directly

[Mandy Chung]

On Mon, 13 Feb 2023 22:05:15 GMT, Mandy Chung  wrote:

> `LambdaForm` declares int constants for `BasicType::ordinal` to workaround 
> JDK-8161245.   Now these int constants are no longer needed.This removes 
> these int constants and reference `BasicType` enums directly.

This pull request has now been integrated.

Changeset: 50dcc2ae
Author:Mandy Chung 
URL:   
https://git.openjdk.org/jdk/commit/50dcc2aec5b16c0826e27d58e49a7f55a5f5ad38
Stats: 25 lines in 4 files changed: 1 ins; 10 del; 14 mod

8301460: Clean up LambdaForm to reference BasicType enums directly

Reviewed-by: jvernee

-

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

Re: RFR: 8292914: Drop the counter from lambda class names

[David M . Lloyd]

On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd  wrote:

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

Do you have an example? Could the hidden class identifier be used for this 
purpose instead?

-

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

Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v6]

[Mandy Chung]

On Wed, 15 Feb 2023 14:56:27 GMT, Ian Graves  wrote:

>> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CompactConstantPoolsPlugin.java
>>  line 48:
>> 
>>> 46: 
>>> 47: public CompactConstantPoolsPlugin() {
>>> 48: super("compact-constant-pools");
>> 
>> This plugin needs a better option name.   Maybe `--share-utf8-entries`?
>> 
>> I also wonder if anyone really uses this plugin.  It writes the shared UTF8 
>> strings into the jimage.  It incurs overhead in reconstructing the constant 
>> pool when loading classes.So I wonder if we just drop this plugin - 
>> @AlanBateman, @igraves what do you think?
>
> I think that deprecating the entire plugin could be appropriate, given the 
> overhead, unless there's some performance data to the contrary. I haven't 
> seen much in favor of keeping it, but I do think that breaking it apart from 
> zip before deprecating it is also fine.

If we are planning to drop this, we will not need to introduce a new plugin 
option but instead states that `--compress 1` will be deprecated and removed in 
a future release.

-

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

Re: RFR: 8292914: Drop the counter from lambda class names

[Mandy Chung]

On Wed, 15 Feb 2023 17:32:38 GMT, David M. Lloyd  wrote:

> The class generated for lambda proxies is now defined as a hidden class. This 
> means that the counter, which was used to ensure a unique class name and 
> avoid clashes, is now redundant. In addition to performing redundant work, 
> this also impacts build reproducibility for native image generators which 
> might already have a strategy to cope with hidden classes but cannot cope 
> with indeterminate definition order for lambda proxy classes.
> 
> This solves JDK-8292914 by making lambda proxy names always be stable without 
> any configuration needed. This would also replace #10024.

While the counter is not strictly needed, it's very important for 
diagnosability.

-

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

Re: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)

[Raffaello Giulietti]

Thanks Roger.

Let's say you have a `String` that is structured in lines, and for each line 
you need to count the occurrences (0 or more) of some character.
A way to process a line is to first search for the next '\n' from the current 
position, then to seach for that character starting from the same position but 
limiting the search up to the position of the '\n', update the current position 
after the character, iterate.
You then advance to the next line and repeat the above.

There are other ways to achieve the same result, but they are probably either 
more complicated or less efficient.

I’m proposing this addition because very similar, non-public methods already 
exist in the codebase, and which are already used to implement `indexOf()`.



As for `lastIndexOf()`, in the forward search it's natural to have `toIndex` 
excluded from the search. There are many instance of right-open ranges 
[`fromIndex`, `toIndex`) in the codebase, so the proposal is to adopt this 
convention for `indexOf(ch, fromIndex, toIndex)`.

For a backward search, however, this is less natural. The non-obvious part is 
to assign natural semantics to `toIndex` for `lastIndexOf(ch, fromIndex, 
toIndex)`. Currently, `lastIndexOf(ch, fromIndex)` operates in the closed range 
of indices [`0`, `fromIndex`].
Should the range be closed [`toIndex`, `fromIndex`] or the left-open 
(`toIndex`, `fromIndex`]? Should `lastIndexOf(ch, fromIndex)` be seen as an 
abbreviation for `lastIndexOf(ch, fromIndex, 0)` or for `lastIndexOf(ch, 
fromIndex, -1)`?

Once there's consensus about this aspect, I can of course add `lastIndexOf(ch, 
fromIndex, toIndex)` as well.



From: core-libs-dev  on behalf of Roger Riggs 

Date: Wednesday, 15 February 2023 at 18:03
To: core-libs-dev@openjdk.org 
Subject: Re: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)
Hi Raffaello,

What's the use case, can you give an example?

Seems reasonable; would you also add `lastIndexOf(int ch, int fromIndex, int 
toIndex)`?
Not intrinsified, but for symmetry.

Regards, Roger
On 2/15/23 10:47 AM, Raffaello Giulietti wrote:
Hello,

Currently `String` does not expose a method `indexOf(int ch, int fromIndex, int 
toIndex)`, where the 3rd parameter limits the search to positions up to it 
(exclusive). JBS issue [1] has been filed as an “Enhancement” to add it to the 
codebase.

Before preparing a PR and a CSR, I would like to gather opinions and 
suggestions.

Greetings
Raffaello



[1] https://bugs.openjdk.org/browse/JDK-8302590

RFR: 8292914: Drop the counter from lambda class names

[David M . Lloyd]

The class generated for lambda proxies is now defined as a hidden class. This 
means that the counter, which was used to ensure a unique class name and avoid 
clashes, is now redundant. In addition to performing redundant work, this also 
impacts build reproducibility for native image generators which might already 
have a strategy to cope with hidden classes but cannot cope with indeterminate 
definition order for lambda proxy classes.

This solves JDK-8292914 by making lambda proxy names always be stable without 
any configuration needed. This would also replace #10024.

-

Commit messages:
 - 8292914: Drop the counter from lambda class names

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

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

Re: RFR: 8292914: Introduce a system property that enables stable names for lambda proxy classes [v7]

[David M . Lloyd]

On Mon, 21 Nov 2022 16:46:43 GMT, Strahinja Stanojevic  wrote:

>> This PR introduces an option to output stable names for the lambda classes 
>> in the JDK. A stable name consists of two parts: The first part is the 
>> predefined value `$$Lambda$` appended to the lambda capturing class, and the 
>> second is a 64-bit hash part of the name. Thus, it looks like 
>> `lambdaCapturingClass$$Lambda$hashValue`.
>> Parameters used to create a stable hash are a superset of the parameters 
>> used for lambda class archiving when the CDS dumping option is enabled. 
>> During this process, all the mutual parameters are in the same form as they 
>> are in the low-level implementation 
>> (`SystemDictionaryShared::add_lambda_proxy_class`) of the archiving process.
>> We decided to use a well-specified `CRC32` algorithm from the standard Java 
>> library. We created two 32-bit hashes from the parameters used to create 
>> stable names. Then, we combined those two 32-bit hashes into one 64-bit hash 
>> value.
>> We chose `CRC32` because it is a well-specified hash function, and we don't 
>> need to write additional code in the JDK. `SHA-256, MD5`, and all other hash 
>> functions that rely on `MessageDigest` use lambdas in the implementation, so 
>> they are unsuitable for our purpose. We also considered a few different hash 
>> functions with a low collision rate. All these functions would require at 
>> least 100 lines of additional code in the JDK. The best alternative we found 
>> is 64-bit` MurmurHash2`: 
>> https://commons.apache.org/proper/commons-codec/jacoco/org.apache.commons.codec.digest/MurmurHash2.java.html.
>>   In case adding a new hash implementation (e.g., Murmur2) to the JDK is 
>> preferred, this PR can be easily modified.
>> We found the post 
>> (https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed/145633#145633)
>>  that compares different hash functions.
>> We also tested the `CRC32` hash function against half a billion generated 
>> strings, and there were no collisions. Note that the capturing-class name is 
>> also part of the lambda class name, so the potential collisions can only 
>> appear in a single class. Thus, we do not expect to have name collisions due 
>> to a relatively low number of lambdas per class. Every tool that uses this 
>> feature should handle potential collisions on its own.  
>> We found an overall approximation of the collision rate too. You can find it 
>> here: https://preshing.com/20110504/hash-collision-probabilities/.
>> 
>> JDK currently adds an atomic integer after `$$Lambda$`, and the names of the 
>> lambdas depend on the creation order. In the `TestStableLambdaNames`, we 
>> generate all the lambdas two times. In the first run, the method 
>> createPlainLambdas generate the following lambdas:
>> 
>> - TestStableLambdaNames$$Lambda$1/0x000800c00400
>> - TestStableLambdaNames$$Lambda$2/0x000800c01800
>> - TestStableLambdaNames$$Lambda$3/0x000800c01a38
>> The same method in the second run generates lambdas with different names:
>> - TestStableLambdaNames$$Lambda$1471/0x000800d1
>> - TestStableLambdaNames$$Lambda$1472/0x000800d10238
>> - TestStableLambdaNames$$Lambda$1473/0x000800d10470
>> 
>> If we use the introduced flag, generated lambdas are:
>> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800c00400
>> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800c01800
>> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800c01a38
>> In the second run of the method, generated lambdas are:
>> - TestStableLambdaNames$$Lambda$65ba26bbc6c7500d/0x000800d1
>> - TestStableLambdaNames$$Lambda$1569c8c4abe3ab18/0x000800d10238
>> - TestStableLambdaNames$$Lambda$493c0ecaaf682428/0x000800d10470
>> 
>> We can see that the introduced hash value does not change between two calls 
>> of the method `createPlainLambdas`. That was not the case in the JDK run 
>> without this change. Those lambdas are extracted directly from the test.
>
> Strahinja Stanojevic has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove address from lambda class names in test on the 32-bit architecture 
> too

Looking at the code for `InnerClassLambdaMetafactory`, I don't think that the 
numerical suffix is necessary _at all_. These lambda classes are defined as 
_hidden_ classes, in which case the class is already created with a unique 
"name" (in the form of a suffix comprising `/` and some unique identifying 
digits).

>From my own experiments, I simply removed the counter altogether and 
>everything continues to work with the classes simply being named 
>`xxx/xxx/Xxx$$Lambda`. So perhaps the entire mechanism (which consists of ` + 
>counter.incrementAndGet()` within 
>`InnerClassLambdaMetafactory#lambdaClassName`) is unneeded?

I'll also add this comment on to the original issue as well.

-

PR: 

Re: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)

[Roger Riggs]

Hi Raffaello,

What's the use case, can you give an example?

Seems reasonable; would you also add `lastIndexOf(int ch, int fromIndex, 
int toIndex)`?

Not intrinsified, but for symmetry.

Regards, Roger

On 2/15/23 10:47 AM, Raffaello Giulietti wrote:


Hello,

Currently `String` does not expose a method `indexOf(int ch, int 
fromIndex, int toIndex)`, where the 3^rd parameter limits the search 
to positions up to it (exclusive). JBS issue [1] has been filed as an 
“Enhancement” to add it to the codebase.


Before preparing a PR and a CSR, I would like to gather opinions and 
suggestions.


Greetings

Raffaello



[1] https://bugs.openjdk.org/browse/JDK-8302590

8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)

[Raffaello Giulietti]

Hello,

Currently `String` does not expose a method `indexOf(int ch, int fromIndex, int 
toIndex)`, where the 3rd parameter limits the search to positions up to it 
(exclusive). JBS issue [1] has been filed as an “Enhancement” to add it to the 
codebase.

Before preparing a PR and a CSR, I would like to gather opinions and 
suggestions.

Greetings
Raffaello



[1] https://bugs.openjdk.org/browse/JDK-8302590

Re: RFR: 8294982: Implementation of Classfile API [v19]

[Adam Sotona]

> This is root pull request with Classfile API implementation, tests and 
> benchmarks initial drop into JDK.
> 
> Following pull requests consolidating JDK class files parsing, generating, 
> and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) 
> will chain to this one.
> 
> Classfile API development is tracked at:
> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
> 
> Development branch of consolidated JDK class files parsing, generating, and 
> transforming is at:
> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
> 
> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online 
> API 
> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>  is also available.
> 
> Please take you time to review this non-trivial JDK addition.
> 
> Thank you,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  removal of MemberRefEntry::isMethod and ::isInterface

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10982/files
  - new: https://git.openjdk.org/jdk/pull/10982/files/bb295b53..dbb9e46a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10982=18
 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=17-18

  Stats: 45 lines in 3 files changed: 0 ins; 42 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/10982.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 14:05:10 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
>  line 67:
> 
>> 65:  * {@return whether this member is an interface method}
>> 66:  */
>> 67: boolean isInterface();
> 
> I'd prefer to see this to `MethodRefEntry`. But again, there's an entry for 
> interface methods, so not sure how much this is needed.

The same as above, it is possible to remove it and the two use cases (in 
`ConcreteMethodHandleEntry::asSymbol` and `ClassPrinterImpl`) replace with 
`instanceof InterfaceMethodRefEntry`.
Thanks!

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Wed, 15 Feb 2023 14:45:32 GMT, Adam Sotona  wrote:

>> src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
>>  line 62:
>> 
>>> 60:  * {@return whether this member is a method}
>>> 61:  */
>>> 62: boolean isMethod();
>> 
>> this seems surprising - after all we have separate types for methods/fields.
>
> I'll look at it - if still needed or just a relic.

Yes, it is possible to remove this method completely.

-

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

Re: RFR: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false

[Martin Doerr]

On Tue, 14 Feb 2023 14:10:08 GMT, Richard Reingruber  wrote:

> This fixes the linked issue by trimming the caller of a frame to be 
> deoptimized back to its `unextended_sp` iff it is compiled. The creation of 
> the section `dead after deoptimization` shown in the attachment 
> [yield_after_deopt_failure.log](https://bugs.openjdk.org/secure/attachment/102602/yield_after_deopt_failure.log)
>  is prevented by this.
> 
> A new mode is added to the test BasicExt.java where all frames are 
> deoptimized after a yield operation. The issue can be deterministically 
> reproduced with the new mode. It's not worth to execute all test cases with 
> the new mode though. Instead `ContinuationCompiledFramesWithStackArgs_3c4` is 
> always executed a 2nd time in this mode.
> 
> Before this BasicExt.java was refactored for better argument processing and 
> representation of the test modes.
> Also the try-catch-clause in the main method had to be changed to rethrow the 
> caught exception because without this the test would have succeeded.
> 
> Testing: jtreg tests tier 1-4 on standard platforms and also on ppc64le.

LGTM. Thanks for fixing!

src/hotspot/cpu/ppc/frame_ppc.cpp line 424:

> 422: 
> 423: intptr_t *frame::initial_deoptimization_info() {
> 424:   // `this` is the caller of the deoptee. We want to trimm it, if 
> compiled, to

I believe "to trim" should contain only one 'm' in English.

-

Marked as reviewed by mdoerr (Reviewer).

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

Re: RFR: 8293667: Align jlink's --compress option with jmod's --compress option [v6]

[Ian Graves]

On Tue, 14 Feb 2023 20:20:03 GMT, Mandy Chung  wrote:

>> Ian Graves has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixing up resources
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/CompactConstantPoolsPlugin.java
>  line 48:
> 
>> 46: 
>> 47: public CompactConstantPoolsPlugin() {
>> 48: super("compact-constant-pools");
> 
> This plugin needs a better option name.   Maybe `--share-utf8-entries`?
> 
> I also wonder if anyone really uses this plugin.  It writes the shared UTF8 
> strings into the jimage.  It incurs overhead in reconstructing the constant 
> pool when loading classes.So I wonder if we just drop this plugin - 
> @AlanBateman, @igraves what do you think?

I think that deprecating the entire plugin could be appropriate, given the 
overhead, unless there's some performance data to the contrary. I haven't seen 
much in favor of keeping it, but I do think that breaking it apart from zip 
before deprecating it is also fine.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 14:04:22 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
>  line 62:
> 
>> 60:  * {@return whether this member is a method}
>> 61:  */
>> 62: boolean isMethod();
> 
> this seems surprising - after all we have separate types for methods/fields.

I'll look at it - if still needed or just a relic.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 14:18:56 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
>  line 47:
> 
>> 45:  * {@return the start of the instruction range}
>> 46:  */
>> 47: Label startScope();
> 
> I noticed that this pattern of start/endScope is here, but also in 
> ExceptionCatch, LocalVariable and LocalVariableType. Consider using a common 
> interface for "instructions that are associated with a range".

That is interesting RFE, thanks.

> src/java.base/share/classes/jdk/internal/classfile/instruction/InvokeInstruction.java
>  line 60:
> 
>> 58:  * {@return whether the class holding the method is an interface}
>> 59:  */
>> 60: boolean isInterface();
> 
> This can also be tested with pattern matching on the MemberRefEntry?

Unfortunately the distinction is dynamic. As of my imagination pattern matching 
is not the best to model interface distinction of `InvokeInstruction`.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 14:24:58 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
>  line 63:
> 
>> 61:  * aload_0}).
>> 62:  */
>> 63: sealed interface IntrinsicConstantInstruction extends 
>> ConstantInstruction
> 
> I'm not super sure of the fine-grained distinction here. The constant pool 
> variant is interesting (as you can ask for the associated constant entry) - 
> but the distinction between intrinsics vs. argument seems rather weak.

They significantly differ in instruction formats and instruction format 
distinction is critical for some use cases.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 15:07:01 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/TypeCheckInstruction.java
>  line 39:
> 
>> 37: 
>> 38: /**
>> 39:  * Models an {@code instanceof} or {@code checkcast} instruction in the 
>> {@code
> 
> This seems to model both `instanceof` and `checkcast`. The latter seems to 
> overlap partially with `ConvertInstruction`.

`instanceof` and `checkcast` are both very similar type checking instructions. 
They have the same length, the same format, the same specification of the 
instruction parameters and almost the same behaviour.
All members of `ConvertInstruction` have no instruction parameters and source 
and target of conversion is identified from the opcode. Also the instructions 
do conversions and not type checking.
I don't see any averlap.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v18]

[Adam Sotona]

> This is root pull request with Classfile API implementation, tests and 
> benchmarks initial drop into JDK.
> 
> Following pull requests consolidating JDK class files parsing, generating, 
> and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) 
> will chain to this one.
> 
> Classfile API development is tracked at:
> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
> 
> Development branch of consolidated JDK class files parsing, generating, and 
> transforming is at:
> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
> 
> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online 
> API 
> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>  is also available.
> 
> Please take you time to review this non-trivial JDK addition.
> 
> Thank you,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  added missing factory methods to ModuleExportInfo and ModuleOpenInfo

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10982/files
  - new: https://git.openjdk.org/jdk/pull/10982/files/673887ab..bb295b53

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10982=17
 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=16-17

  Stats: 113 lines in 3 files changed: 113 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/10982.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982

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

Re: RFR: 8302158: PPC: test/jdk/jdk/internal/vm/Continuation/Fuzz.java: AssertionError: res: false shouldPin: false

[Goetz Lindenmaier]

On Tue, 14 Feb 2023 14:10:08 GMT, Richard Reingruber  wrote:

> This fixes the linked issue by trimming the caller of a frame to be 
> deoptimized back to its `unextended_sp` iff it is compiled. The creation of 
> the section `dead after deoptimization` shown in the attachment 
> [yield_after_deopt_failure.log](https://bugs.openjdk.org/secure/attachment/102602/yield_after_deopt_failure.log)
>  is prevented by this.
> 
> A new mode is added to the test BasicExt.java where all frames are 
> deoptimized after a yield operation. The issue can be deterministically 
> reproduced with the new mode. It's not worth to execute all test cases with 
> the new mode though. Instead `ContinuationCompiledFramesWithStackArgs_3c4` is 
> always executed a 2nd time in this mode.
> 
> Before this BasicExt.java was refactored for better argument processing and 
> representation of the test modes.
> Also the try-catch-clause in the main method had to be changed to rethrow the 
> caught exception because without this the test would have succeeded.
> 
> Testing: jtreg tests tier 1-4 on standard platforms and also on ppc64le.

LGTM

src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp line 3094:

> 3092: 
> 3093:   // Freezing continuation frames requires that the caller is trimmed 
> to unextended sp if compiled.
> 3094:   Register caller_sp = R23_tmp3;

Can the caller be interpreted? I.e., not compiled?
Then it might be helpful to comment like
// If not compiled this contains sp() resulting in a resize of 0.

-

Marked as reviewed by goetz (Reviewer).

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

Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v12]

[Eirik Bjorsnos]

On Wed, 15 Feb 2023 11:32:32 GMT, Lance Andersen  wrote:

>> The habit of opening resources in a TwR is hard to break, but I guess it's 
>> ok for a test like this. I have inlined the method and removed the TwR.
>
>> The habit of opening resources in a TwR is hard to break, but I guess it's 
>> ok for a test like this. I have inlined the method and removed the TwR.
> 
> Agree, thanks for addressing the suggestion as it makes the test cleaner 
> given expects/assertThrows should  react to the Exception being thrown by 
> `new ZipFile(...)`

My inlining accidentally made the shouldRejectInvalidComment test the invalid 
name file. Fixed with last commit.

-

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

Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v15]

[Eirik Bjorsnos]

> After finding a hash match, getEntryPos needs to compare the lookup name up 
> to the encoded entry name in the CEN. This comparison is done by decoding the 
> entry name into a String. The names can then be compared using the String 
> API. This decoding step adds a significat cost to this method.
> 
> This PR suggest to update the string comparison such that in the common case 
> where  both the lookup name and the entry name are encoded in 
> ASCII-compatible UTF-8,  decoding can be avoided and the byte arrays can 
> instead be compared direcly. 
> 
> ZipCoder is updated with a new method to compare a string with an encoded 
> byte array range.  The default implementation decodes to string (like the 
> current code), while the UTF-8 implementation uses 
> JavaLangAccess.getBytesNoRepl to get the  bytes. Both methods thes uses 
> Arrays.mismatch for comparison with or without matching trailing slashes. 
> 
> Additionally, this PR suggest to make the following updates to getEntryPos:
> 
> - The try/catch for IAE is redundant and can be safely removed. (initCEN 
> already checks this and will throws IAE for invalid UTF-8). This seems to 
> give a 3-4% speedup on micros)
> - A new test InvalidBytesInEntryNameOrComment is a added to verify that 
> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I 
> found no existing test coverage for this)
> - The recursion when looking for "name/" matches is replaced with iteration. 
> We keep track of any "name/" match and return it at the end of the search. (I 
> feel this is easier to follow and it also gives a ~30% speedup for addSlash 
> lookups with no regression on regular lookups)
> 
> (My though is that including these additional updates in this PR might reduce 
> reviewer overhead given that it touches the exact same code. I might be wrong 
> on this, please advise :)
> 
> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified 
> to use xalan.jar):
> 
> Baseline:
> 
> Benchmark (size)  Mode  CntScore   Error  
> Units
> ZipFileGetEntry.getEntryHit  512  avgt   15   74.941 ± 1.004  
> ns/op
> ZipFileGetEntry.getEntryHit 1024  avgt   15   84.943 ± 1.320  
> ns/op
> ZipFileGetEntry.getEntryHitUncached  512  avgt   15  120.371 ± 2.386  
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024  avgt   15  126.128 ± 1.075  
> ns/op
> ZipFileGetEntry.getEntryMiss 512  avgt   15   23.818 ± 0.838  
> ns/op
> ZipFileGetEntry.getEntryMiss1024  avgt   15   29.762 ± 5.998  
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512  avgt   15   59.405 ± 0.545  
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024  avgt   15   71.840 ± 2.455  
> ns/op
> ZipFileGetEntry.getEntrySlash512  avgt   15  135.621 ± 4.341  
> ns/op
> ZipFileGetEntry.getEntrySlash   1024  avgt   15  134.190 ± 2.141  
> ns/op
> 
> 
> 
> PR:
> 
> 
> Benchmark (size)  Mode  CntScore   Error  
> Units
> ZipFileGetEntry.getEntryHit  512  avgt   15   62.267 ± 1.329  
> ns/op
> ZipFileGetEntry.getEntryHit 1024  avgt   15   72.916 ± 2.428  
> ns/op
> ZipFileGetEntry.getEntryHitUncached  512  avgt   15  101.630 ± 1.154  
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024  avgt   15  113.161 ± 0.502  
> ns/op
> ZipFileGetEntry.getEntryMiss 512  avgt   15   23.003 ± 1.191  
> ns/op
> ZipFileGetEntry.getEntryMiss1024  avgt   15   23.236 ± 1.114  
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512  avgt   15   56.781 ± 1.505  
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024  avgt   15   67.767 ± 1.963  
> ns/op
> ZipFileGetEntry.getEntrySlash512  avgt   15   73.745 ± 2.717  
> ns/op
> ZipFileGetEntry.getEntrySlash   1024  avgt   15   75.784 ± 1.051  
> ns/op
> 
> 
> To assess the impact on startup/warmup, I made a main method class which 
> measures the total time of calling ZipFile.getEntry for all entries in the 
> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement 
> (time in micros):
> 
> 
> Percentile Baseline Patch
> 50 %  23155 21149
> 75 %  23598 21454
> 90 %  23989 21691
> 95 %  24238 21973
> 99 %  25270 22446
> STDEV   792   549
> Count   500   500

Eirik Bjorsnos has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix a regression where the invalidName was tested instead of comment in the 
invalid comment test

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12290/files
  - new: https://git.openjdk.org/jdk/pull/12290/files/9b83002a..08160a1a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12290=14
 - incr: https://webrevs.openjdk.org/?repo=jdk=12290=13-14

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/12290.diff
  Fetch: git fetch 

Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v12]

[Lance Andersen]

On Wed, 15 Feb 2023 09:55:04 GMT, Eirik Bjorsnos  wrote:

> The habit of opening resources in a TwR is hard to break, but I guess it's ok 
> for a test like this. I have inlined the method and removed the TwR.

Agree, thanks for addressing the suggestion as it makes the test cleaner given 
expects/assertThrows should  react to the Exception being thrown by `new 
ZipFile(...)`

-

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

Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v14]

[Eirik Bjorsnos]

> After finding a hash match, getEntryPos needs to compare the lookup name up 
> to the encoded entry name in the CEN. This comparison is done by decoding the 
> entry name into a String. The names can then be compared using the String 
> API. This decoding step adds a significat cost to this method.
> 
> This PR suggest to update the string comparison such that in the common case 
> where  both the lookup name and the entry name are encoded in 
> ASCII-compatible UTF-8,  decoding can be avoided and the byte arrays can 
> instead be compared direcly. 
> 
> ZipCoder is updated with a new method to compare a string with an encoded 
> byte array range.  The default implementation decodes to string (like the 
> current code), while the UTF-8 implementation uses 
> JavaLangAccess.getBytesNoRepl to get the  bytes. Both methods thes uses 
> Arrays.mismatch for comparison with or without matching trailing slashes. 
> 
> Additionally, this PR suggest to make the following updates to getEntryPos:
> 
> - The try/catch for IAE is redundant and can be safely removed. (initCEN 
> already checks this and will throws IAE for invalid UTF-8). This seems to 
> give a 3-4% speedup on micros)
> - A new test InvalidBytesInEntryNameOrComment is a added to verify that 
> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I 
> found no existing test coverage for this)
> - The recursion when looking for "name/" matches is replaced with iteration. 
> We keep track of any "name/" match and return it at the end of the search. (I 
> feel this is easier to follow and it also gives a ~30% speedup for addSlash 
> lookups with no regression on regular lookups)
> 
> (My though is that including these additional updates in this PR might reduce 
> reviewer overhead given that it touches the exact same code. I might be wrong 
> on this, please advise :)
> 
> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified 
> to use xalan.jar):
> 
> Baseline:
> 
> Benchmark (size)  Mode  CntScore   Error  
> Units
> ZipFileGetEntry.getEntryHit  512  avgt   15   74.941 ± 1.004  
> ns/op
> ZipFileGetEntry.getEntryHit 1024  avgt   15   84.943 ± 1.320  
> ns/op
> ZipFileGetEntry.getEntryHitUncached  512  avgt   15  120.371 ± 2.386  
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024  avgt   15  126.128 ± 1.075  
> ns/op
> ZipFileGetEntry.getEntryMiss 512  avgt   15   23.818 ± 0.838  
> ns/op
> ZipFileGetEntry.getEntryMiss1024  avgt   15   29.762 ± 5.998  
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512  avgt   15   59.405 ± 0.545  
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024  avgt   15   71.840 ± 2.455  
> ns/op
> ZipFileGetEntry.getEntrySlash512  avgt   15  135.621 ± 4.341  
> ns/op
> ZipFileGetEntry.getEntrySlash   1024  avgt   15  134.190 ± 2.141  
> ns/op
> 
> 
> 
> PR:
> 
> 
> Benchmark (size)  Mode  CntScore   Error  
> Units
> ZipFileGetEntry.getEntryHit  512  avgt   15   62.267 ± 1.329  
> ns/op
> ZipFileGetEntry.getEntryHit 1024  avgt   15   72.916 ± 2.428  
> ns/op
> ZipFileGetEntry.getEntryHitUncached  512  avgt   15  101.630 ± 1.154  
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024  avgt   15  113.161 ± 0.502  
> ns/op
> ZipFileGetEntry.getEntryMiss 512  avgt   15   23.003 ± 1.191  
> ns/op
> ZipFileGetEntry.getEntryMiss1024  avgt   15   23.236 ± 1.114  
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512  avgt   15   56.781 ± 1.505  
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024  avgt   15   67.767 ± 1.963  
> ns/op
> ZipFileGetEntry.getEntrySlash512  avgt   15   73.745 ± 2.717  
> ns/op
> ZipFileGetEntry.getEntrySlash   1024  avgt   15   75.784 ± 1.051  
> ns/op
> 
> 
> To assess the impact on startup/warmup, I made a main method class which 
> measures the total time of calling ZipFile.getEntry for all entries in the 
> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement 
> (time in micros):
> 
> 
> Percentile Baseline Patch
> 50 %  23155 21149
> 75 %  23598 21454
> 90 %  23989 21691
> 95 %  24238 21973
> 99 %  25270 22446
> STDEV   792   549
> Count   500   500

Eirik Bjorsnos has updated the pull request incrementally with one additional 
commit since the last revision:

  Update javadocs for ZipCoder.Comparison to first clarify that the "encoded 
string" refers to those found in the CEN byte array and to use the term "the 
encoded string" instead of the more convoluted "the string encoded in the byte 
array".

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12290/files
  - new: https://git.openjdk.org/jdk/pull/12290/files/49ad8d35..9b83002a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12290=13
 - incr: 

Re: RFR: 8294982: Implementation of Classfile API [v17]

[Adam Sotona]

> This is root pull request with Classfile API implementation, tests and 
> benchmarks initial drop into JDK.
> 
> Following pull requests consolidating JDK class files parsing, generating, 
> and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) 
> will chain to this one.
> 
> Classfile API development is tracked at:
> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
> 
> Development branch of consolidated JDK class files parsing, generating, and 
> transforming is at:
> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
> 
> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online 
> API 
> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>  is also available.
> 
> Please take you time to review this non-trivial JDK addition.
> 
> Thank you,
> Adam

Adam Sotona has updated the pull request incrementally with two additional 
commits since the last revision:

 - removed generics from Classfile.Option
 - Javadoc fixes

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/10982/files
  - new: https://git.openjdk.org/jdk/pull/10982/files/ec6829e9..673887ab

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=10982=16
 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=15-16

  Stats: 78 lines in 10 files changed: 15 ins; 4 del; 59 mod
  Patch: https://git.openjdk.org/jdk/pull/10982.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982

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

Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v12]

[Eirik Bjorsnos]

On Tue, 14 Feb 2023 23:12:38 GMT, Lance Andersen  wrote:

>> Eirik Bjorsnos has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Prefer expectThrows with asserts over test annotation regex
>
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 91:
> 
>> 89: public void shouldRejectInvalidName() throws IOException {
>> 90: ZipException ex = expectThrows(ZipException.class, () -> {
>> 91: openZipFile(invalidName);
> 
> You could just do
> 
> 
> ZipException ex = expectThrows(ZipException.class, () -> {
>  new ZipFile(invalidName.toFile())
> });
> 
> versus adding _openZipFile()_

The habit of opening resources in a TwR is hard to break, but I guess it's ok 
for a test like this. I have inlined the method and removed the TwR.

-

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

Integrated: 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp

[Severin Gehwolf]

On Mon, 13 Feb 2023 14:12:15 GMT, Severin Gehwolf  wrote:

> Could I please get a review of this trivial comment-only change? 
> `imageFile.hpp`
> describes some properties of the jimage file `lib/modules`. However, I don't 
> think
> the comment example matches current code in the JDK. 
> [`ATTRIBUTE_OFFSET`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java#L44)
>  get
> written to the image file with value `0x05` while the comment mentions it gets
> written as `0x04`. I propose to fix the comment so that it matches the code.
> 
> In passing, I've also fixed a comment related to `ATTRIBUTE_END`. I think the 
> point
> being made there is about byte values of `[0x01..0x07]`, all would represent
> `ATTRIBUTE_END`, as the upper `5` bits would be `0`. Therefore, byte `0x01` 
> would equally
> represent `ATTRIBUTE_END` as would `0x00` and `0x07` or any value in between.
> 
> Thoughts?

This pull request has now been integrated.

Changeset: 11194e8b
Author:Severin Gehwolf 
URL:   
https://git.openjdk.org/jdk/commit/11194e8b825ad2688f4ede35fdadb69d74c7a5f4
Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 mod

8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp

Reviewed-by: alanb, jlaskey

-

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

Re: RFR: 8301873: Avoid string decoding in ZipFile.Source.getEntryPos [v13]

[Eirik Bjorsnos]

> After finding a hash match, getEntryPos needs to compare the lookup name up 
> to the encoded entry name in the CEN. This comparison is done by decoding the 
> entry name into a String. The names can then be compared using the String 
> API. This decoding step adds a significat cost to this method.
> 
> This PR suggest to update the string comparison such that in the common case 
> where  both the lookup name and the entry name are encoded in 
> ASCII-compatible UTF-8,  decoding can be avoided and the byte arrays can 
> instead be compared direcly. 
> 
> ZipCoder is updated with a new method to compare a string with an encoded 
> byte array range.  The default implementation decodes to string (like the 
> current code), while the UTF-8 implementation uses 
> JavaLangAccess.getBytesNoRepl to get the  bytes. Both methods thes uses 
> Arrays.mismatch for comparison with or without matching trailing slashes. 
> 
> Additionally, this PR suggest to make the following updates to getEntryPos:
> 
> - The try/catch for IAE is redundant and can be safely removed. (initCEN 
> already checks this and will throws IAE for invalid UTF-8). This seems to 
> give a 3-4% speedup on micros)
> - A new test InvalidBytesInEntryNameOrComment is a added to verify that 
> initCEN does in fact reject invalid UTF-8 in CEN file names and comments. (I 
> found no existing test coverage for this)
> - The recursion when looking for "name/" matches is replaced with iteration. 
> We keep track of any "name/" match and return it at the end of the search. (I 
> feel this is easier to follow and it also gives a ~30% speedup for addSlash 
> lookups with no regression on regular lookups)
> 
> (My though is that including these additional updates in this PR might reduce 
> reviewer overhead given that it touches the exact same code. I might be wrong 
> on this, please advise :)
> 
> I'm seeing a ~17% saving on the micro ZipFileGetEntry.getEntryHit (modified 
> to use xalan.jar):
> 
> Baseline:
> 
> Benchmark (size)  Mode  CntScore   Error  
> Units
> ZipFileGetEntry.getEntryHit  512  avgt   15   74.941 ± 1.004  
> ns/op
> ZipFileGetEntry.getEntryHit 1024  avgt   15   84.943 ± 1.320  
> ns/op
> ZipFileGetEntry.getEntryHitUncached  512  avgt   15  120.371 ± 2.386  
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024  avgt   15  126.128 ± 1.075  
> ns/op
> ZipFileGetEntry.getEntryMiss 512  avgt   15   23.818 ± 0.838  
> ns/op
> ZipFileGetEntry.getEntryMiss1024  avgt   15   29.762 ± 5.998  
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512  avgt   15   59.405 ± 0.545  
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024  avgt   15   71.840 ± 2.455  
> ns/op
> ZipFileGetEntry.getEntrySlash512  avgt   15  135.621 ± 4.341  
> ns/op
> ZipFileGetEntry.getEntrySlash   1024  avgt   15  134.190 ± 2.141  
> ns/op
> 
> 
> 
> PR:
> 
> 
> Benchmark (size)  Mode  CntScore   Error  
> Units
> ZipFileGetEntry.getEntryHit  512  avgt   15   62.267 ± 1.329  
> ns/op
> ZipFileGetEntry.getEntryHit 1024  avgt   15   72.916 ± 2.428  
> ns/op
> ZipFileGetEntry.getEntryHitUncached  512  avgt   15  101.630 ± 1.154  
> ns/op
> ZipFileGetEntry.getEntryHitUncached 1024  avgt   15  113.161 ± 0.502  
> ns/op
> ZipFileGetEntry.getEntryMiss 512  avgt   15   23.003 ± 1.191  
> ns/op
> ZipFileGetEntry.getEntryMiss1024  avgt   15   23.236 ± 1.114  
> ns/op
> ZipFileGetEntry.getEntryMissUncached 512  avgt   15   56.781 ± 1.505  
> ns/op
> ZipFileGetEntry.getEntryMissUncached1024  avgt   15   67.767 ± 1.963  
> ns/op
> ZipFileGetEntry.getEntrySlash512  avgt   15   73.745 ± 2.717  
> ns/op
> ZipFileGetEntry.getEntrySlash   1024  avgt   15   75.784 ± 1.051  
> ns/op
> 
> 
> To assess the impact on startup/warmup, I made a main method class which 
> measures the total time of calling ZipFile.getEntry for all entries in the 
> 109 JAR file dependenies of spring-petclinic. The shows a nice improvement 
> (time in micros):
> 
> 
> Percentile Baseline Patch
> 50 %  23155 21149
> 75 %  23598 21454
> 90 %  23989 21691
> 95 %  24238 21973
> 99 %  25270 22446
> STDEV   792   549
> Count   500   500

Eirik Bjorsnos has updated the pull request incrementally with one additional 
commit since the last revision:

  Inline openZipFile and remove TwR

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12290/files
  - new: https://git.openjdk.org/jdk/pull/12290/files/4981abd3..49ad8d35

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=12290=12
 - incr: https://webrevs.openjdk.org/?repo=jdk=12290=11-12

  Stats: 7 lines in 1 file changed: 0 ins; 5 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/12290.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12290/head:pull/12290

PR: 

Re: RFR: 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp [v3]

[Severin Gehwolf]

On Tue, 14 Feb 2023 19:30:19 GMT, Severin Gehwolf  wrote:

>> Could I please get a review of this trivial comment-only change? 
>> `imageFile.hpp`
>> describes some properties of the jimage file `lib/modules`. However, I don't 
>> think
>> the comment example matches current code in the JDK. 
>> [`ATTRIBUTE_OFFSET`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java#L44)
>>  get
>> written to the image file with value `0x05` while the comment mentions it 
>> gets
>> written as `0x04`. I propose to fix the comment so that it matches the code.
>> 
>> In passing, I've also fixed a comment related to `ATTRIBUTE_END`. I think 
>> the point
>> being made there is about byte values of `[0x01..0x07]`, all would represent
>> `ATTRIBUTE_END`, as the upper `5` bits would be `0`. Therefore, byte `0x01` 
>> would equally
>> represent `ATTRIBUTE_END` as would `0x00` and `0x07` or any value in between.
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Another fixup.
>  - Correct comment in imageFile.hpp
>  - Copyright update in PerfectHashBuilder

Thanks for the reviews!

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 13:17:30 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 66:
> 
>> 64:  * @param  the type of the optional value
>> 65:  */
>> 66: public sealed interface Option permits Options.OptionValue {
> 
> After looking more at the usages of Options it is not clear to me as to why 
> generics are needed. Lookup is always performed using a non-generic 
> Option.Key - so the API code has to be unchecked anyway. In fact, I'm not 
> even sure you need a `value()` method. When looking at usages, Option is 
> mostly something you create when you need to pass it to something else (e.g. 
> a constant pool, etc.). Since the client has created the option, it is not 
> clear to me that the client has a need to access the option value (which the 
> API implementation can access internally by other means).

You are right, I've tried to remove generics from `Option` and `Option::value` 
method and it didn't have any impact on any use case we have.
All access to the `Option` value is done through `ClassReader::optionValue` or 
`ConstantPoolBuilder::optionValue`, there was no use of `Option::value` at all.
I think it is valuable API cleanup, thanks!

-

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

Integrated: JDK-8300808: Accelerate Base64 on x86 for AVX2

[Scott Gibbons]

On Sat, 21 Jan 2023 00:15:10 GMT, Scott Gibbons  wrote:

> Added code for Base64 acceleration (encode and decode) which will accelerate 
> ~4x for AVX2 platforms.
> 
> Encode performance:
> **Old:**
> 
> Benchmark  (maxNumBytes)   Mode  Cnt Score   Error   
> Units
> Base64Encode.testBase64Encode   1024  thrpt3  4309.439 ± 2.632  
> ops/ms
> 
> 
> **New:**
> 
> Benchmark  (maxNumBytes)   Mode  Cnt  Score Error 
>   Units
> Base64Encode.testBase64Encode   1024  thrpt3  24211.397 ± 102.026 
>  ops/ms
> 
> 
> Decode performance:
> **Old:**
> 
> Benchmark  (errorIndex)  (lineSize)  (maxNumBytes)   Mode 
>  Cnt ScoreError   Units
> Base64Decode.testBase64Decode   144   4   1024  thrpt 
>3  3961.768 ± 93.409  ops/ms
> 
> **New:**
> Benchmark  (errorIndex)  (lineSize)  (maxNumBytes)   Mode 
>  Cnt  ScoreError   Units
> Base64Decode.testBase64Decode   144   4   1024  thrpt 
>3  14738.051 ± 24.383  ops/ms

This pull request has now been integrated.

Changeset: 33bec207
Author:Scott Gibbons 
Committer: Claes Redestad 
URL:   
https://git.openjdk.org/jdk/commit/33bec207103acd520eb99afb093cfafa44aecfda
Stats: 234 lines in 7 files changed: 208 ins; 5 del; 21 mod

8300808: Accelerate Base64 on x86 for AVX2

Reviewed-by: jbhateja, redestad, sviswanathan

-

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

Re: RFR: JDK-8300808: Accelerate Base64 on x86 for AVX2 [v17]

[Claes Redestad]

On Tue, 14 Feb 2023 18:22:32 GMT, Scott Gibbons  wrote:

>> Added code for Base64 acceleration (encode and decode) which will accelerate 
>> ~4x for AVX2 platforms.
>> 
>> Encode performance:
>> **Old:**
>> 
>> Benchmark  (maxNumBytes)   Mode  Cnt Score   Error   
>> Units
>> Base64Encode.testBase64Encode   1024  thrpt3  4309.439 ± 2.632  
>> ops/ms
>> 
>> 
>> **New:**
>> 
>> Benchmark  (maxNumBytes)   Mode  Cnt  Score 
>> Error   Units
>> Base64Encode.testBase64Encode   1024  thrpt3  24211.397 ± 
>> 102.026  ops/ms
>> 
>> 
>> Decode performance:
>> **Old:**
>> 
>> Benchmark  (errorIndex)  (lineSize)  (maxNumBytes)   
>> Mode  Cnt ScoreError   Units
>> Base64Decode.testBase64Decode   144   4   1024  
>> thrpt3  3961.768 ± 93.409  ops/ms
>> 
>> **New:**
>> Benchmark  (errorIndex)  (lineSize)  (maxNumBytes)   
>> Mode  Cnt  ScoreError   Units
>> Base64Decode.testBase64Decode   144   4   1024  
>> thrpt3  14738.051 ± 24.383  ops/ms
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Last of review comments

No problem! Testing looks good.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 13:10:43 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
>  line 98:
> 
>> 96:  T optionValue(Classfile.Option.Key option);
>> 97: 
>> 98: boolean canWriteDirect(ConstantPool constantPool);
> 
> Missing javadoc in these two methods.

Will fix it, thanks.

> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
>  line 187:
> 
>> 185:  * {@return A {@link ModuleEntry} describing the module whose name
>> 186:  * is encoded in the provided {@linkplain Utf8Entry}}
>> 187:  * If a Module entry in the pool already describes this class,
> 
> (Here and elsewhere) - Module is capitalized. Either you use a lower case 
> name, or you use a capital name, to refer to `ModuleEntry`, or 
> `CONSTANT_Module_info` - e.g. a standalone `Module` with capital `M` is not a 
> concept in this API. (personally I think lower case is just fine).

Will fix it, thanks.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 14:01:10 GMT, Maurizio Cimadamore  
wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
>  line 537:
> 
>> 535:  * @param  the type of the entry
>> 536:  */
>> 537:  T maybeClone(T entry);
> 
> This feels a more primitive operation than the name suggests. Have you 
> considered making ConstantPool extend Consumer and call this 
> "accept" ?

I'm not quite sure what exactly do you propose.
`ConstantPool` should not accept anything as it is read-only, so "accept" would 
be confusing.
`ConstantPoolBuilder::maybeClone` is rather a `Function`, where the name might 
be changed to `ConstantPoolBuilder::apply`.
However there are so many "accepts" and "applies" across the API, that reducing 
API verbosity to just these functional terms might significantly decrease 
readability.

-

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

Re: RFR: 8299748: java/util/zip/Deinflate.java failing on s390x

[Alan Bateman]

On Wed, 15 Feb 2023 02:25:00 GMT, Amit Kumar  wrote:

> After reverting all of my changes, I applied your patch and I got time limit 
> error, probably because of some inf-loop ?
> 
> ```
> result: Error. Agent error: java.lang.Exception: Agent 2 timed out with a 
> timeout of 480 seconds; check console log for any additional details
> ```

Yes, it seems the deflate or inflate loop doesn't terminate as finished doesn't 
change to true or no space available in the buffer. I assume you can quickly 
check which loop, I suspect it's the deflate loop based of the previous 
comments/experiments.

Your initial proposal was to increase the size of out1 but I can't see any 
cases in this test where the deflate fills out1 completely. Additionally, when 
you initially tried the patch to replace deflate with a loop, it failed with 
"out1 is too small" which is very surprising, it hints that Deflater::finished 
is not returning true when the input has been consumed. You might have to dig 
into this more to see if there is a bug in this area to explain it.

For inflate, the i == 0 case (in main) will cause inflate to fill out2 
completely. This is the most likely case to loop if there was a bug somewhere.

-

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

Re: RFR: 8294982: Implementation of Classfile API [v15]

[Adam Sotona]

On Thu, 9 Feb 2023 13:01:57 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
>>  line 80:
>> 
>>> 78:  * Return a List composed by appending the additions to the base 
>>> list.
>>> 79:  * @param base The base elements for the list, must not include null
>>> 80:  * @param additions The ClassEntrys to add to the list, must not 
>>> include null
>> 
>> Perhaps we should use `{@code}` or {@link}` to surround type names (here and 
>> elsewhere). `ClassEntrys` looks particularly odd :-)
>
> It is odd to see what is essentially a list append operation in here. IMHO, 
> these helper methods, if needed (I couldn't find uses in the JDK), should 
> probably be added to Collections (which probably means in the jdktypes 
> package for now) - as I don't see anything really ClassEntry/ClassDesc 
> specific about them.

I'll make a note to deeply review javadoc for types and to wrap them, thanks.

As for the List combining methods, they had been proposed, discussed and 
approved here:
https://github.com/openjdk/jdk-sandbox/pull/35
Feel free to re-open the discussion on mailing list, maybe we can address them 
better now.
However there is no general contract between "entries" and "symbols" yet, so 
such methods could be declared generic.

-

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