date:20230214

On Tue, 14 Feb 2023 11:21:36 GMT, Lance Andersen  wrote:

>> The message is already validated using `expectedExceptionsMessageRegExp` in 
>> the `@Test` annotation.
>> 
>> Would you prefer if I use expectThrows instead, or perhaps inline the 
>> `BAD_ENTRY_NAME_OR_COMMENT` constant as a literal?
>
> Sorry if this was not clear,  we have gone away from using the annotation 
> element `exepectedExceptions` for new and updated tests and  have tried to 
> standardize on `assertThrows` and  `expectThrows` instead which is the basis 
> for my suggestion.
> 
> 
> Thank you for your other updates.  I will go through them later today

No worries, this makes sense. Updated to expectThrows with assertEquals.

-

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

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

On Tue, 14 Feb 2023 22:41:47 GMT, Claes Redestad  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Last of review comments
>
> I've started tier1-5 testing internally. Will let you know if we find any 
> issues.

Thanks a lot @cl4es for doing the tests for this PR.

-

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

Re: RFR: JDK-8301460: Clean up LambdaForm to reference BasicType enums directly [v2]

2023-02-14 Thread Jorn Vernee

On Tue, 14 Feb 2023 19:30:27 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.
>
> Mandy Chung has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8301460
>  - further clean up
>  - JDK-8301460: Code in LambdaForm.java still refers to resolved JDK-8161245

Marked as reviewed by jvernee (Reviewer).

-

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

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

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

I've started tier1-5 testing internally. Will let you know if we find any 
issues.

-

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

Integrated: 8302260: VarHandle.describeConstable() fails to return a nominal descriptor for static public fields

On Mon, 13 Feb 2023 19:35:52 GMT, Mandy Chung  wrote:

> I overlooked in the fix for JDK-8297757 that it should have passed the 
> declaring class of the static fields rather than the reference class passed 
> to `Lookup::findStaticVarHandle`.

This pull request has now been integrated.

Changeset: 9c202a5a
Author:Mandy Chung 
URL:   
https://git.openjdk.org/jdk/commit/9c202a5a8fc5b0f334ea72487d079af7da275693
Stats: 231 lines in 7 files changed: 190 ins; 0 del; 41 mod

8302260: VarHandle.describeConstable() fails to return a nominal descriptor for 
static public fields

Reviewed-by: alanb, psandoz

-

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

Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v8]

2023-02-14 Thread Karl T

On Tue, 7 Feb 2023 12:53:25 GMT, Tagir F. Valeev  wrote:

>> clamp() methods added to Math and StrictMath
>> 
>> `int clamp(long, int, int)` is somewhat different, as it accepts a `long` 
>> value and safely clamps it to an `int` range. Other overloads work with a 
>> particular type (long, float and double). Using similar approach in other 
>> cases (e.g. `float clamp(double, float, float)`) may cause accidental 
>> precision loss even if the value is within range, so I decided to avoid this.
>> 
>> In all cases, `max >= min` precondition should met. For double and float we 
>> additionally order `-0.0 < 0.0`, similarly to what Math.max or 
>> Double.compare do. In double and float overloads I try to keep at most one 
>> arg-check comparison on common path, so the order of checks might look 
>> unusual.
>> 
>> For tests, I noticed that tests in java/lang/Math don't use any testing 
>> framework (even newer tests), so I somehow mimic the approach of neighbour 
>> tests.
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Comments in tests

I agree that using `var` for primitive types is maybe not the best idea, but 
there are thousands (or millions) of Java developers out there doing such 
things.
Personally, I rarely use the `var` keyword because I still prefer explicit 
types.

> In this particular case, you lose nothing, as even if the resulting variable 
> is unexpectedly int, you don't lose precision and you don't overflow.

Well there is a overflow risk if calculations are done.
Here is some "dummy" code that demonstrates an int overflow:

~~~java
var a = 1234L;
var b = Math.clamp( a, 0, 100 );
var c = b * 2_000_000_000;
System.out.println( c );
~~~

With `int clamp( long value, int min, int max )`, this outputs `-1863462912`. 
(int overflow)
But with `int clamp( int value, int min, int max )`, this outputs 
`2000`.

Still think that the API would be cleaner with `int clamp( int value, int min, 
int max )` 😉

-

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

Re: RFR: 8302260: VarHandle.describeConstable() fails to return a nominal descriptor for static public fields

2023-02-14 Thread Paul Sandoz

On Mon, 13 Feb 2023 19:35:52 GMT, Mandy Chung  wrote:

> I overlooked in the fix for JDK-8297757 that it should have passed the 
> declaring class of the static fields rather than the reference class passed 
> to `Lookup::findStaticVarHandle`.

Marked as reviewed by psandoz (Reviewer).

-

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

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

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

LGTM

src/java.base/share/native/libjimage/imageFile.hpp line 217:

> 215: //
> 216: // Notes:
> 217: //  - Even though ATTRIBUTE_END (which might be encoded with a zero 
> byte) is used to

good

-

Marked as reviewed by jlaskey (Reviewer).

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

Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v8]

2023-02-14 Thread Karl T

On Tue, 14 Feb 2023 20:13:03 GMT, Tagir F. Valeev  wrote:

> Finally, I find it extremely strange to use var to hide the type and name the 
> variable as `longValue2`, encoding the type inside variable name.

This is just an example name to document the type of the var for this post. 😉

-

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

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

On Wed, 8 Feb 2023 23:07:14 GMT, Ian Graves  wrote:

>> 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:
> 
>   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?

-

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

Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v8]

2023-02-14 Thread Tagir F . Valeev

On Tue, 7 Feb 2023 12:53:25 GMT, Tagir F. Valeev  wrote:

>> clamp() methods added to Math and StrictMath
>> 
>> `int clamp(long, int, int)` is somewhat different, as it accepts a `long` 
>> value and safely clamps it to an `int` range. Other overloads work with a 
>> particular type (long, float and double). Using similar approach in other 
>> cases (e.g. `float clamp(double, float, float)`) may cause accidental 
>> precision loss even if the value is within range, so I decided to avoid this.
>> 
>> In all cases, `max >= min` precondition should met. For double and float we 
>> additionally order `-0.0 < 0.0`, similarly to what Math.max or 
>> Double.compare do. In double and float overloads I try to keep at most one 
>> arg-check comparison on common path, so the order of checks might look 
>> unusual.
>> 
>> For tests, I noticed that tests in java/lang/Math don't use any testing 
>> framework (even newer tests), so I somehow mimic the approach of neighbour 
>> tests.
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Comments in tests

I would not use `var` with primitive types. You win nothing but may add 
confusion, as your samples show. There are many cases when implicit type 
conversion is performed with numeric types, and using `var` could be indeed 
confusing.

In this particular case, you lose nothing, as even if the resulting variable is 
unexpectedly `int`, you don't lose precision and you don't overflow. If you 
need to pass it somewhere where `long` is expected, it will be widened without 
any effort from your side. When you use `var`, you're saying that the type is 
not that important, and it indeed is. Finally, I find it extremely strange to 
use `var` to hide the type and name the variable as `longValue2`, encoding the 
type inside variable name. If the type is important, why don't use an explicit 
type declaration?

-

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

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

On Tue, 7 Feb 2023 07:07:54 GMT, Alan Bateman  wrote:

>>> Hi @AlanBateman ,
>>> with latest changes (doing inflate/deinflate) test passes over s390 and x86 
>>> as well. Please take a look now.
>> 
>> Good. One thing to try is to just deflate/inflate into out1/out2, no need 
>> for the intermediate BAOS, try this:
>> 
>> 
>> --- a/test/jdk/java/util/zip/DeInflate.java
>> +++ b/test/jdk/java/util/zip/DeInflate.java
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights 
>> reserved.
>>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>   *
>>   * This code is free software; you can redistribute it and/or modify it
>> @@ -127,11 +127,19 @@ public class DeInflate {
>>  
>>  def.setInput(in, 0, len);
>>  def.finish();
>> -int m = def.deflate(out1);
>> +int m = 0;
>> +while (!def.finished()) {
>> +int remaining = out1.length - m;
>> +m += def.deflate(out1, m, remaining);
>> +}
>>  
>>  Inflater inf = new Inflater(nowrap);
>>  inf.setInput(out1, 0, m);
>> -int n = inf.inflate(out2);
>> +int n = 0;
>> +while (!inf.finished()) {
>> +int remaining = out2.length - n;
>> +n += inf.inflate(out2, n, remaining);
>> +}
>>  
>>  if (n != len ||
>>  !Arrays.equals(Arrays.copyOf(in, len), Arrays.copyOf(out2, 
>> len)) ||
>
>> @AlanBateman should we proceed with the current changes then ?
> 
> The BAOS in your current proposal shouldn't be needed so I suspect there is 
> something else "interesting" with this zlib implementation.  For the patch 
> above, can you remove the checking for remaining == 0 and print out the value 
> from deflate at each iterate of the loop. I'm wondering if it returns 0 
> before def.finished() returns true.

> Hi @AlanBateman, what should be our next step for this PR :)

I don't think we have a good handle on the issue. It's clear that this zlib 
implementation is a bit different but I'm concerned that your proposed change 
to the test is masking an issue. I say this because the `check` method is 
changed to not use the out1 and out2 as the test intended. I'm also concerned 
that the code fragment you pasted may be the combination of two patches, I 
can't otherwise explain the ArrayIndexOutOfBoundsException you reported. Would 
it be possible to re-run with the follow change as I'm like to see again what 
the failure is:


--- a/test/jdk/java/util/zip/DeInflate.java
+++ b/test/jdk/java/util/zip/DeInflate.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -127,11 +127,19 @@ public class DeInflate {
 
 def.setInput(in, 0, len);
 def.finish();
-int m = def.deflate(out1);
+int m = 0;
+while (!def.finished()) {
+int remaining = out1.length - m;
+m += def.deflate(out1, m, remaining);
+}
 
 Inflater inf = new Inflater(nowrap);
 inf.setInput(out1, 0, m);
-int n = inf.inflate(out2);
+int n = 0;
+while (!inf.finished()) {
+int remaining = out2.length - n;
+n += inf.inflate(out2, n, remaining);
+}
 
 if (n != len ||
 !Arrays.equals(Arrays.copyOf(in, len), Arrays.copyOf(out2, len)) ||

-

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

Re: RFR: 8301226: Add clamp() methods to java.lang.Math and to StrictMath [v8]

2023-02-14 Thread Karl T

On Tue, 7 Feb 2023 12:53:25 GMT, Tagir F. Valeev  wrote:

>> clamp() methods added to Math and StrictMath
>> 
>> `int clamp(long, int, int)` is somewhat different, as it accepts a `long` 
>> value and safely clamps it to an `int` range. Other overloads work with a 
>> particular type (long, float and double). Using similar approach in other 
>> cases (e.g. `float clamp(double, float, float)`) may cause accidental 
>> precision loss even if the value is within range, so I decided to avoid this.
>> 
>> In all cases, `max >= min` precondition should met. For double and float we 
>> additionally order `-0.0 < 0.0`, similarly to what Math.max or 
>> Double.compare do. In double and float overloads I try to keep at most one 
>> arg-check comparison on common path, so the order of checks might look 
>> unusual.
>> 
>> For tests, I noticed that tests in java/lang/Math don't use any testing 
>> framework (even newer tests), so I somehow mimic the approach of neighbour 
>> tests.
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Comments in tests

Regarding the asymmetric `int clamp( long value, int min, int max )` method:
I understand the idea behind using `long`, but IMHO there is a good reason to 
use `int`instead.
When using the `var` keyword, it could easy result in unwanted type change from 
`long` to `int`.

E.g. following simple code unexpectedly changes the type from `long` to `int`:
~~~java
var longValue = 1234L;
var intValue = Math.clamp( longValue, 0, 100 );
~~~

When doing the same with `Math.min()` and `max()`, it does **not change type**:
~~~java
var longValue2 = Math.min( 100, Math.max( longValue, 0 ) );
~~~

If you don't want that type change, it is required to pass a long min or max 
value:
~~~java
var longValue = 1234L;
var longValue2 = Math.clamp( longValue, 0L, 100 );
~~~

This is confusing, isn't it?

The idea behind using `long` as first parameter is "This allows to use method 
to safely cast long value to int" (from javadoc).
But if changing to `int clamp( int value, int min, int max )`, safely casting 
long to int is still possible by passing int values to min and max an casting 
the result to int. E.g.:

~~~java
int a = (int) Math.clamp( longValue, 0, 100 );
~~~

Also not sure whether the idea behind clamp is to provide a mechanism for safe 
casting...

IMHO the benefit of "safe casting long to int" is very low compared to the risk 
of unwanted type change when using `var` keyword.

-

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

Re: RFR: JDK-8301460: Clean up LambdaForm to reference BasicType enums directly [v2]

On Tue, 14 Feb 2023 10:50:12 GMT, Jorn Vernee  wrote:

>> Mandy Chung has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8301460
>>  - further clean up
>>  - JDK-8301460: Code in LambdaForm.java still refers to resolved JDK-8161245
>
> src/java.base/share/classes/java/lang/invoke/BoundMethodHandle.java line 371:
> 
>> 369: extensions[typeNum] = sd;
>> 370: return sd;
>> 371: }
> 
> I suggest changing the existing overload to delegate to this one with 
> `BasicType.basicType(typeNum)` as the argument. (or to use some other way to 
> reduce the duplication)

I further cleaned this up.  `extendWith(byte typeNum)` is now removed.

-

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

Re: RFR: JDK-8301460: Clean up LambdaForm to reference BasicType enums directly [v2]

On Tue, 14 Feb 2023 11:16:26 GMT, Sergey Tsypanov  wrote:

>> Mandy Chung has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8301460
>>  - further clean up
>>  - JDK-8301460: Code in LambdaForm.java still refers to resolved JDK-8161245
>
> src/java.base/share/classes/java/lang/invoke/BoundMethodHandle.java line 368:
> 
>> 366: SpeciesData sd = extensions[typeNum];
>> 367: if (sd != null)  return sd;
>> 368: sd = SPECIALIZER.findSpecies(key() + 
>> basicType.basicTypeChar());
> 
> As soon as this is `java.base` I suggest to replace `key() + 
> basicType.basicTypeChar()` with 
> `key().concat(String.valueOf(basicType.basicTypeChar()))` to prevent 
> `StringBuilder` allocation and `append()` chaining.

There are only 5 elements in `extensions` arrays for 5 `BasicType`.  I don't 
think this overhead is significant.

-

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

Re: RFR: JDK-8301460: Clean up LambdaForm to reference BasicType enums directly [v2]

> `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.

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

 - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8301460
 - further clean up
 - JDK-8301460: Code in LambdaForm.java still refers to resolved JDK-8161245

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12546/files
  - new: https://git.openjdk.org/jdk/pull/12546/files/029effa0..75f07dea

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12546&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12546&range=00-01

  Stats: 57732 lines in 1241 files changed: 24871 ins; 16438 del; 16423 mod
  Patch: https://git.openjdk.org/jdk/pull/12546.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12546/head:pull/12546

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

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

On Tue, 14 Feb 2023 18:10:11 GMT, Jim Laskey  wrote:

>> Got it. How about we change this comment from:
>> 
>> //  - Even though ATTRIBUTE_END is used to mark the end of the attribute 
>> stream,
>> //  streams will contain zero byte values to represent lesser 
>> significant bits.
>> //  Thus, detecting a zero byte is not sufficient to detect the end of 
>> an attribute
>> //  stream.
>> 
>> 
>> to:
>> 
>> //  - Even though ATTRIBUTE_END (which might be encoded as a zero byte) is 
>> used to
>> //  mark the end of the attribute stream, streams will contain zero byte 
>> values in the
>> //  non-header portion of the attribute data. Thus, detecting a zero 
>> byte is not
>> //  sufficient to detect the end of an attribute stream.
>> 
>> 
>> ? The phrase "to represent lesser significant bits" and mention of 
>> `ATTRIBUTE_END` is throwing me off.
>
> Sounds good to me.

Thanks, updated.

-

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

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

On Tue, 14 Feb 2023 17:18:00 GMT, Jim Laskey  wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8302325-imageFile
>>  - Copyright dates updated.
>>  - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp
>
> Changes requested by jlaskey (Reviewer).

@JimLaskey How does this look to you now?

> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/PerfectHashBuilder.java 
> line 249:
> 
>> 247: // Attempt to pack entries until no collisions 
>> occur.
>> 248: if (!collidedEntries(bucket, count)) {
>> 249: // Failed to pack. Need to grow table.
> 
> Change copyright date to mark this change.

Fixed.

-

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

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

> 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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12533/files
  - new: https://git.openjdk.org/jdk/pull/12533/files/94b3d7e7..4cb81b43

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12533&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12533&range=01-02

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

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

Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v4]

2023-02-14 Thread Stuart Marks

On Tue, 14 Feb 2023 18:56:29 GMT, Roger Riggs  wrote:

>> It can be difficult to find the cause of calls to 
>> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java 
>> runtime exits.
>> The status value and stack trace are logged using the System Logger named 
>> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Correct System.getLogger link

src/java.base/share/classes/java/lang/Shutdown.java line 168:

> 166: Throwable throwable = new Throwable("Runtime.exit(" + 
> status + ")");
> 167: log.log(System.Logger.Level.DEBUG, "Runtime.exit() 
> called with status: " + status,
> 168: throwable);

I'd put a try/catch around the actual logging of the message, to avoid a 
situation where an error in the logger handler prevents the system from being 
shut down.

-

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

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

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

Looks good to me.

-

Marked as reviewed by sviswanathan (Reviewer).

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

Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v4]

2023-02-14 Thread Roger Riggs

> It can be difficult to find the cause of calls to 
> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java 
> runtime exits.
> The status value and stack trace are logged using the System Logger named 
> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.

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

  Correct System.getLogger link

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12517/files
  - new: https://git.openjdk.org/jdk/pull/12517/files/a8eca9e5..60123543

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12517&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12517&range=02-03

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

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

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

2023-02-14 Thread Scott Gibbons

> 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

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Last of review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12126/files
  - new: https://git.openjdk.org/jdk/pull/12126/files/5077b4e8..2adaa5da

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=16
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=15-16

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

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

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

On Tue, 14 Feb 2023 18:01:44 GMT, Severin Gehwolf  wrote:

>> I meant that an attribute can have zeros in the non-header portion of the 
>> attribute data.
>
> Got it. How about we change this comment from:
> 
> //  - Even though ATTRIBUTE_END is used to mark the end of the attribute 
> stream,
> //  streams will contain zero byte values to represent lesser significant 
> bits.
> //  Thus, detecting a zero byte is not sufficient to detect the end of an 
> attribute
> //  stream.
> 
> 
> to:
> 
> //  - Even though ATTRIBUTE_END (which might be encoded as a zero byte) is 
> used to
> //  mark the end of the attribute stream, streams will contain zero byte 
> values in the
> //  non-header portion of the attribute data. Thus, detecting a zero byte 
> is not
> //  sufficient to detect the end of an attribute stream.
> 
> 
> ? The phrase "to represent lesser significant bits" and mention of 
> `ATTRIBUTE_END` is throwing me off.

Sounds good to me.

-

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

Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v3]

2023-02-14 Thread Roger Riggs

On Tue, 14 Feb 2023 17:15:28 GMT, Daniel Fuchs  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add an @implNote to Runtime.exit to describe the java.lang.Runtime logging.
>
> src/java.base/share/classes/java/lang/Runtime.java line 160:
> 
>> 158:  *
>> 159:  * @implNote
>> 160:  * If the {@link System.Logger#getLogger(String) 
>> System.Logger.getLogger("java.lang.Runtime")}
> 
> The link looks wrong to me:
> Suggestion:
> 
>  * If the {@link System#getLogger(String) 
> System.getLogger("java.lang.Runtime")}
> 
> See 
> https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/System.html#getLogger(java.lang.String)

yep, will fix.  Oddly, the link worked. Thanks

-

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

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

On Tue, 14 Feb 2023 17:17:53 GMT, Jim Laskey  wrote:

>> To me it sounded like it wanted to say: Since the `ATTRIBUTE_END` isn't a 
>> full byte (only 5 bits in a byte), it might be represented as a non-zero 
>> value. For example a byte containing `0x07` would equally be `ATTRIBUTE_END` 
>> as would a zero byte or a `0x01` byte. `ATTRIBUTE_END` is a `kind` which is 
>> encoded with the *most* significant `5` bits. Yet, `ATTRIBUTE_END` isn't a 
>> full byte. The least significant `3` bits in the byte represent the `length 
>> - 1` - of bytes - in the attribute stream for offset values. That, to me, 
>> also would suggest that comparing it to a zero byte value is not sufficient 
>> to detect `ATTRIBUTE_END`.
>
> I meant that an attribute can have zeros in the non-header portion of the 
> attribute data.

Got it. How about we change this comment from:

//  - Even though ATTRIBUTE_END is used to mark the end of the attribute stream,
//  streams will contain zero byte values to represent lesser significant 
bits.
//  Thus, detecting a zero byte is not sufficient to detect the end of an 
attribute
//  stream.

to:

//  - Even though ATTRIBUTE_END (which might be encoded as a zero byte) is used 
to
//  mark the end of the attribute stream, streams will contain zero byte 
values in the
//  non-header portion of the attribute data. Thus, detecting a zero byte 
is not
//  sufficient to detect the end of an attribute stream.

? The phrase "to represent lesser significant bits" and mention of 
`ATTRIBUTE_END` is throwing me off.

-

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

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

On Tue, 14 Feb 2023 15:19:34 GMT, Claes Redestad  wrote:

>> Why?  There is no performance difference and the intent is clear.  Is this 
>> just a "style" thing?
>
> I think with `lessEqual` we'll jump to `L_tailProc` for the final 32-byte 
> chunk in inputs that are divisible by 32 (starting from 64), no? Using `less` 
> avoids that, while not affecting performance of any other inputs.

As Claes mentioned, this would allow us to do one more iteration of vector loop.

-

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

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

On Tue, 14 Feb 2023 15:03:49 GMT, Scott Gibbons  wrote:

>> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2658:
>> 
>>> 2656: // Check for buffer too small (for algorithm)
>>> 2657: __ subl(length, 0x2c);
>>> 2658: __ jcc(Assembler::lessEqual, L_tailProc);
>> 
>> This could be Assembler::less instead of Assembler::lessEqual.
>
> Why?  There is no performance difference and the intent is clear.  Is this 
> just a "style" thing?

The thought is that when the length is equal to 44 bytes, we could do the 
vector loop once before tail processing. The rest of the logic seems to allow 
that. 44 bytes of base64 -> 33 bytes decoded. So a 32 byte write in vector loop 
would still be ok and we wont be writing beyond.

-

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

Convert CorruptedZipFiles to testNG

2023-02-14 Thread Eirik Bjørsnøs

Hi!

CorruptedZipFiles could benefit from some modernization and a conversion to
testNG:

https://github.com/openjdk/jdk/pull/12563

Thanks,
Eirik.

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

On Tue, 14 Feb 2023 17:10:32 GMT, Severin Gehwolf  wrote:

>> @JimLaskey OK. Perhaps we can be clearer what is meant here exactly. I was 
>> having a hard time deciphering this. It does say `stream will contain zero 
>> byte values to represent lesser significant bits`. **What** are "byte values 
>> to represent lesser significant bits"?
>
> To me it sounded like it wanted to say: Since the `ATTRIBUTE_END` isn't a 
> full byte (only 5 bits in a byte), it might be represented as a non-zero 
> value. For example a byte containing `0x07` would equally be `ATTRIBUTE_END` 
> as would a zero byte or a `0x01` byte. `ATTRIBUTE_END` is a `kind` which is 
> encoded with the *most* significant `5` bits. Yet, `ATTRIBUTE_END` isn't a 
> full byte. The least significant `3` bits in the byte represent the `length - 
> 1` - of bytes - in the attribute stream for offset values. That, to me, also 
> would suggest that comparing it to a zero byte value is not sufficient to 
> detect `ATTRIBUTE_END`.

I meant that an attribute can have zeros in the non-header portion of the 
attribute data.

-

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

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

On Tue, 14 Feb 2023 14:46:58 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 with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8302325-imageFile
>  - Copyright dates updated.
>  - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp

Changes requested by jlaskey (Reviewer).

-

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

Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v3]

2023-02-14 Thread Daniel Fuchs

On Tue, 14 Feb 2023 16:46:29 GMT, Roger Riggs  wrote:

>> It can be difficult to find the cause of calls to 
>> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java 
>> runtime exits.
>> The status value and stack trace are logged using the System Logger named 
>> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add an @implNote to Runtime.exit to describe the java.lang.Runtime logging.

src/java.base/share/classes/java/lang/Runtime.java line 160:

> 158:  *
> 159:  * @implNote
> 160:  * If the {@link System.Logger#getLogger(String) 
> System.Logger.getLogger("java.lang.Runtime")}

The link looks wrong to me:
Suggestion:

 * If the {@link System#getLogger(String) 
System.getLogger("java.lang.Runtime")}

See 
https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/System.html#getLogger(java.lang.String)

-

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

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

On Tue, 14 Feb 2023 16:59:03 GMT, Severin Gehwolf  wrote:

>> src/java.base/share/native/libjimage/imageFile.hpp line 218:
>> 
>>> 216: // Notes:
>>> 217: //  - Even though ATTRIBUTE_END is used to mark the end of the 
>>> attribute stream,
>>> 218: //  streams will contain non-zero byte values to represent lesser 
>>> significant bits.
>> 
>> This change is not correct. Maybe it is badly worded but the point is that 
>> **zeroes can** occur in the stream so testing for zero is insufficient.
>
> @JimLaskey OK. Perhaps we can be clearer what is meant here exactly. I was 
> having a hard time deciphering this. It does say `stream will contain zero 
> byte values to represent lesser significant bits`. **What** are "byte values 
> to represent lesser significant bits"?

To me it sounded like it wanted to say: Since the `ATTRIBUTE_END` isn't a full 
byte (only 5 bits in a byte), it might be represented as a non-zero value. For 
example a byte containing `0x07` would equally be `ATTRIBUTE_END` as would a 
zero byte or a `0x01` byte. `ATTRIBUTE_END` is a `kind` which is encoded with 
the *most* significant `5` bits. Yet, `ATTRIBUTE_END` isn't a full byte. The 
least significant `3` bits in the byte represent the `length - 1` - of bytes - 
in the attribute stream for offset values. That, to me, also would suggest that 
comparing it to a zero byte value is not sufficient to detect `ATTRIBUTE_END`.

-

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

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

On Tue, 14 Feb 2023 16:47:15 GMT, Jim Laskey  wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8302325-imageFile
>>  - Copyright dates updated.
>>  - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp
>
> src/java.base/share/native/libjimage/imageFile.hpp line 218:
> 
>> 216: // Notes:
>> 217: //  - Even though ATTRIBUTE_END is used to mark the end of the 
>> attribute stream,
>> 218: //  streams will contain non-zero byte values to represent lesser 
>> significant bits.
> 
> This change is not correct. Maybe it is badly worded but the point is that 
> **zeroes can** occur in the stream so testing for zero is insufficient.

@JimLaskey OK. Perhaps we can be clearer what is meant here exactly. I was 
having a hard time deciphering this. It does say `stream will contain zero byte 
values to represent lesser significant bits`. **What** are "byte values to 
represent lesser significant bits"?

-

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

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

On Tue, 14 Feb 2023 14:46:58 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 with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8302325-imageFile
>  - Copyright dates updated.
>  - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp

Changes requested by jlaskey (Reviewer).

src/java.base/share/native/libjimage/imageFile.hpp line 218:

> 216: // Notes:
> 217: //  - Even though ATTRIBUTE_END is used to mark the end of the attribute 
> stream,
> 218: //  streams will contain non-zero byte values to represent lesser 
> significant bits.

This change is not correct. Maybe it is badly worded but the point is that 
**zeroes can** occur in the stream so testing for zero is insufficient.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/PerfectHashBuilder.java 
line 249:

> 247: // Attempt to pack entries until no collisions occur.
> 248: if (!collidedEntries(bucket, count)) {
> 249: // Failed to pack. Need to grow table.

Change copyright date to mark this change.

-

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

Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v3]

2023-02-14 Thread Roger Riggs

> It can be difficult to find the cause of calls to 
> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java 
> runtime exits.
> The status value and stack trace are logged using the System Logger named 
> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.

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

  Add an @implNote to Runtime.exit to describe the java.lang.Runtime logging.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12517/files
  - new: https://git.openjdk.org/jdk/pull/12517/files/76c4d61f..a8eca9e5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12517&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12517&range=01-02

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

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

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

On Tue, 14 Feb 2023 10:40:31 GMT, Alan Bateman  wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8302325-imageFile
>>  - Copyright dates updated.
>>  - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp
>
> kind = 5 looks right.  
> 
> Can you update the end date of the copyright headers as this is the first 
> change in 2023. imageFile.cpp by missed by the other change so it could be 
> done here too.

> @AlanBateman Is that what you meant?

Yes, thanks. The other one that needs update is PerfectHashBuilder.java.

-

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

Re: RFR: 8298619: java/io/File/GetXSpace.java is failing

2023-02-14 Thread Brian Burkhalter

On Sun, 12 Feb 2023 09:34:37 GMT, Alan Bateman  wrote:

>>> It seems a bit fragile to be parsing the output of `fsutil volume diskFree` 
>>> as the output seems to vary by Windows releases and maybe configuration. So 
>>> minimally, I think it should be changed to use ProcessTools so that the 
>>> command and the output show up in the .jtr file.
>>> 
>>> In passing, you might want to choose a different method name to make it 
>>> clearer what it does, maybe volumeDiskFree?
>> 
>> Another possibility would be to add a native method to the test itself to 
>> invoke 
>> [GetDiskSpaceInformationW](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getdiskspaceinformationw)
>>  to obtain the value of `CallerTotalAllocationUnits` (in bytes) from the 
>> [DISK_SPACE_INFORMATION](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-disk_space_information)
>>  structure.
>
>> Another possibility would be to add a native method to the test itself to 
>> invoke 
>> [GetDiskSpaceInformationW](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getdiskspaceinformationw)
>>  to obtain the value of `CallerTotalAllocationUnits` (in bytes) from the 
>> [DISK_SPACE_INFORMATION](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-disk_space_information)
>>  structure.
> 
> That would be more reliable than parsing the output of `fsutil volume` so 
> worth trying. You might have to dig into which of these win32 works with 
> quotas as that seems to be part of the issue. The JDK uses 
> GetDiskFreeSpaceExW. Also this test might have to do a few iterations so that 
> it captures free space info while the system is in quiescence - I suspect 
> some of the reliability issues has been concurrent activity that changes the 
> volume space usage significantly while this test is running.

There has definitely been a problem with quotas on Windows. I set up quotas on 
actual Windows 11 hardware and replicated the same failure. I am not sure how 
much lack of system quiescence is to blame, but there would be no harm in 
building in some robustness for lack of quiescence while we're at it.

-

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

Integrated: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE

2023-02-14 Thread Brian Burkhalter

On Tue, 29 Nov 2022 00:56:58 GMT, Brian Burkhalter  wrote:

> `java.io.InputStream::transferTo` could conceivably return a negative value 
> if the count of bytes transferred overflows a `long`. Modify the method to 
> limit the number of bytes transferred to `Long.MAX_VALUE` per invocation.

This pull request has now been integrated.

Changeset: 5b2d4301
Author:Brian Burkhalter 
URL:   
https://git.openjdk.org/jdk/commit/5b2d430131e8e5f6e91d449dab84b99ef6f1c880
Stats: 52 lines in 7 files changed: 39 ins; 0 del; 13 mod

8297632: InputStream.transferTo() method should specify what the return value 
should be when the number of bytes transfered is larger than Long.MAX_VALUE

Reviewed-by: alanb, lancea

-

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

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

On Tue, 14 Feb 2023 15:03:50 GMT, Scott Gibbons  wrote:

>> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2699:
>> 
>>> 2697: __ addptr(dest, 0x18);
>>> 2698: __ subl(length, 0x20);
>>> 2699: __ jcc(Assembler::lessEqual, L_tailProc);
>> 
>> This could be Assembler::less instead of Assembler::lessEqual.
>
> Why?  There is no performance difference and the intent is clear.  Is this 
> just a "style" thing?

I think with `lessEqual` we'll jump to `L_tailProc` for the final 32-byte chunk 
in inputs that are divisible by 32 (starting from 64), no? Using `less` avoids 
that, while not affecting performance of any other inputs.

-

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

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

2023-02-14 Thread Scott Gibbons

On Tue, 14 Feb 2023 01:48:37 GMT, Sandhya Viswanathan 
 wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add URL to microbenchmark
>
> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2399:
> 
>> 2397:  VM_Version::supports_avx512bw()) {
>> 2398: __ cmpl(length, 31); // 32-bytes is break-even for AVX-512
>> 2399: __ jcc(Assembler::lessEqual, L_bruteForce);
> 
> The avx2 code needs the length to be atleast 0x2c (44) bytes. We could 
> directly go to non-avx code instead of L_bruteForce here. We will save one 
> subtract/branch.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2658:
> 
>> 2656: // Check for buffer too small (for algorithm)
>> 2657: __ subl(length, 0x2c);
>> 2658: __ jcc(Assembler::lessEqual, L_tailProc);
> 
> This could be Assembler::less instead of Assembler::lessEqual.

Why?  There is no performance difference and the intent is clear.  Is this just 
a "style" thing?

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 2699:
> 
>> 2697: __ addptr(dest, 0x18);
>> 2698: __ subl(length, 0x20);
>> 2699: __ jcc(Assembler::lessEqual, L_tailProc);
> 
> This could be Assembler::less instead of Assembler::lessEqual.

Why?  There is no performance difference and the intent is clear.  Is this just 
a "style" thing?

-

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

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

2023-02-14 Thread Scott Gibbons

> 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

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Branch around for very small buffers

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12126/files
  - new: https://git.openjdk.org/jdk/pull/12126/files/424b40d7..5077b4e8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=15
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12126&range=14-15

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

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

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

On Tue, 14 Feb 2023 10:40:31 GMT, Alan Bateman  wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8302325-imageFile
>>  - Copyright dates updated.
>>  - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp
>
> kind = 5 looks right.  
> 
> Can you update the end date of the copyright headers as this is the first 
> change in 2023. imageFile.cpp by missed by the other change so it could be 
> done here too.

@AlanBateman Is that what you meant?

-

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

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

> 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 with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains three additional 
commits since the last revision:

 - Merge branch 'master' into jdk-8302325-imageFile
 - Copyright dates updated.
 - 8302325: Wrong comment in java.base/share/native/libjimage/imageFile.hpp

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12533/files
  - new: https://git.openjdk.org/jdk/pull/12533/files/bce5bb55..94b3d7e7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12533&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12533&range=00-01

  Stats: 61035 lines in 1315 files changed: 25365 ins; 16637 del; 19033 mod
  Patch: https://git.openjdk.org/jdk/pull/12533.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12533/head:pull/12533

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

Re: RFR: 8205592: BigDecimal.doubleValue() is depressingly slow

2023-02-14 Thread Andriy Plokhotnyuk

On Tue, 14 Feb 2023 14:07:22 GMT, Raffaello Giulietti  
wrote:

>> Sorry, I overlooked those checks two times :)
>> 
>> How about adding a moderate path like 
>> [this](https://github.com/plokhotnyuk/jsoniter-scala/blob/6f702ce5cae05df91b5aa6e4bd61acdf43bf18f6/jsoniter-scala-core/jvm/src/main/scala/com/github/plokhotnyuk/jsoniter_scala/core/JsonReader.scala#L1467-L1498)?
>> 
>> I think it worth do be reused for regular parsing of `double` and `float` 
>> values from `String`.
>
> @plokhotnyuk The main goal of this PR is to avoid generating a string and 
> parse it, as it happens in the current implementation. The fact that it 
> results in being faster is only a welcome byproduct.
> 
> The proposed patch for `doubleValue()` is only about 40 lines of code, not 
> counting `}`-only lines and the _extensive comments_ explaining the details 
> for the benefit of both reviewers and maintainers. Shorter, documented code 
> has a higher chance to be correct and understood. It also contributes to 
> simpler and quicker reviewing.
> 
> Adding the "moderate path" to this patch would increase the code size 
> considerably. Moreover, I would have to invest time to understand the dense, 
> uncommented code and convince myself and the reviewers that it is correct. I 
> would also have to setup benchmarks to measure the overall benefits of adding 
> it to the proposed patch. And add specific tests to cover the path.
> 
> Before that, I would prefer for this patch to be first reviewed as it is 
> (with possible corrections). I hope to have time to invest into your 
> proposals _once_ this PR is integrated into mainline. Thanks for your 
> patience.

@rgiulietti Thanks for the explanation!

I wish faster reviews of all your PRs!

I bet that investigation of the moderate path will pay itself when that path 
will be reused for improving `java.lang.Double.parseDouble` and 
`java.lang.Float.parseFloat` methods. 

You can roughly estimate the moderate path speed up comparing the throughput of 
`borer` and `jsoniterScala` in the following chart. Both of them use the same 
fast path and fallback to `java.lang.Double.parseDouble`, the difference is 
that `jsoniterScala` (and `smithy4sJson` that is based on it) adds using of the 
moderate path:

![image](https://user-images.githubusercontent.com/890289/218768844-86d53c5f-2b4e-4326-80ae-b6301ee3c67f.png)

BTW, the `jacksonScala` uses a Java port of Daniel Lemire's 
[fast_float](https://github.com/fastfloat/fast_float) project.

-

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

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

2023-02-14 Thread 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.

Commit messages:
- Trimm compiled caller of deoptee to unextended sp
- Deoptimize after yield
- BasicExt.java test: use enums for parameters
- BasicExt.java test: rethrow caught exception to indicate failure

Changes: https://git.openjdk.org/jdk/pull/12557/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12557&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8302158
Stats: 203 lines in 3 files changed: 148 ins; 13 del; 42 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: 8205592: BigDecimal.doubleValue() is depressingly slow

2023-02-14 Thread Raffaello Giulietti

On Tue, 14 Feb 2023 08:02:08 GMT, Andriy Plokhotnyuk  wrote:

>> A reimplementation of `BigDecimal.[double|float]Value()` to enhance 
>> performance, avoiding an intermediate string and its subsequent parsing on 
>> the slow path.
>
> Sorry, I overlooked those checks two times :)
> 
> How about adding a moderate path like 
> [this](https://github.com/plokhotnyuk/jsoniter-scala/blob/6f702ce5cae05df91b5aa6e4bd61acdf43bf18f6/jsoniter-scala-core/jvm/src/main/scala/com/github/plokhotnyuk/jsoniter_scala/core/JsonReader.scala#L1467-L1498)?
> 
> I think it worth do be reused for regular parsing of `double` and `float` 
> values from `String`.

@plokhotnyuk The main goal of this PR is to avoid generating a string and parse 
it, as it happens in the current implementation. The fact that it results in 
being faster is only a welcome byproduct.

The proposed patch for `doubleValue()` is only about 40 lines of code, not 
counting `}`-only lines and the _extensive comments_ explaining the details for 
the benefit of both reviewers and maintainers. Shorter, documented code has a 
higher chance to be correct and understood. It also contributes to simpler and 
quicker reviewing.

Adding the "moderate path" to this patch would increase the code size 
considerably. Moreover, I would have to invest time to understand the dense, 
uncommented code and convince myself and the reviewers that it is correct. I 
would also have to setup benchmarks to measure the overall benefits of adding 
it to the proposed patch. And add specific tests to cover the path.

Before that, I would prefer for this patch to be first reviewed as it is (with 
possible corrections). I hope to have time to invest into your proposals _once_ 
this PR is integrated into mainline. Thanks for your patience.

-

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

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

On Tue, 14 Feb 2023 10:40:31 GMT, Alan Bateman  wrote:

> Can you update the end date of the copyright headers as this is the first 
> change in 2023. imageFile.cpp by missed by the other change so it could be 
> done here too.

Absolutely! Thanks for the review.

-

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

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

2023-02-14 Thread Raffaello Giulietti

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.

Otherwise LGTM

test/jdk/java/lang/StrictMath/InverseTrigTests.java line 96:

> 94: 
> 95:0.975,
> 96:   -0.975,

I think other decision points for asin are 2**-27 and -2**-27 (0x1p-27 and 
-0x1p-27)

-

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

Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v2]

2023-02-14 Thread Daniel Fuchs

On Tue, 14 Feb 2023 07:45:07 GMT, Alan Bateman  wrote:

>> FINE is not a level supported by the System.Logger, it is the level to which 
>> DEBUG is mapped when the backend is java.util.logging. I suggest to remove 
>> FINE from this description and add an `{@link Loger.Level#DEBUG DEBUG}` 
>> around DEBUG.
>
> Roger has updated this but it's still a comment on a non-public class. I 
> think the main question here is whether there should be a note in the 
> System.exit and Runtime.exit to document that these methods log? If not, will 
> it be documented anywhere, maybe a troubleshooting guide to help track down 
> what/who is calling System.exit?

Another way to document that could be to add the commented logger name, and  a 
comment, to 
https://github.com/openjdk/jdk/blob/master/src/java.logging/share/conf/logging.properties
 
A new section at the end with some explanation on what this logger prints and 
how to enable it - with the appropriate wording to make it clear that it's 
JDK-implementation specific and not part of the spec.

-

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

Re: RFR: 8302260: VarHandle.describeConstable() fails to return a nominal descriptor for static public fields

On Mon, 13 Feb 2023 19:35:52 GMT, Mandy Chung  wrote:

> I overlooked in the fix for JDK-8297757 that it should have passed the 
> declaring class of the static fields rather than the reference class passed 
> to `Lookup::findStaticVarHandle`.

Looks right and you've added good test coverage.

test/jdk/java/lang/invoke/VarHandles/describeConstable/DescribeConstableTest.java
 line 53:

> 51: // resolved to the one defined in the direct 
> superinterface of C
> 52: Arguments.of(p.C.class, "stringField", String.class, 
> p.I.class, "I"),
> 53: Arguments.of(p.C.class, "longField", long.class, 
> p.I.class, 10L),

This looks right - when not declared in p.C, it should look next in the direct 
superinterfaces before looking in the super classes.

-

Marked as reviewed by alanb (Reviewer).

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

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

> 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:

  Prefer expectThrows with asserts over test annotation regex

-

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

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=11
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=10-11

  Stats: 18 lines in 1 file changed: 7 ins; 1 del; 10 mod
  Patch: https://git.openjdk.org/jdk/pull/12290.diff
  Fetch: git fetch https://git.openjdk.org/jd

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

2023-02-14 Thread Lance Andersen

On Tue, 14 Feb 2023 08:47:15 GMT, Eirik Bjorsnos  wrote:

>> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 90:
>> 
>>> 88: expectedExceptionsMessageRegExp = BAD_ENTRY_NAME_OR_COMMENT)
>>> 89: public void shouldRejectInvalidName() throws IOException {
>>> 90: try (ZipFile zf = new ZipFile(invalidName.toFile())) {
>> 
>> If you could please convert to use `expectThrows` to get to validate the 
>> message name
>
> The message is already validated using `expectedExceptionsMessageRegExp` in 
> the `@Test` annotation.
> 
> Would you prefer if I use expectThrows instead, or perhaps inline the 
> `BAD_ENTRY_NAME_OR_COMMENT` constant as a literal?

Sorry if this was not clear,  we have gone away from using the annotation 
element `exepectedExceptions` for new and updated tests and  have tried to 
standardize on `assertThrows` and  `expectThrows` instead which is the basis 
for my suggestion.

Thank you for your other updates.  I will go through them later today

-

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

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

2023-02-14 Thread Sergey Tsypanov

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.

src/java.base/share/classes/java/lang/invoke/BoundMethodHandle.java line 368:

> 366: SpeciesData sd = extensions[typeNum];
> 367: if (sd != null)  return sd;
> 368: sd = SPECIALIZER.findSpecies(key() + 
> basicType.basicTypeChar());

As soon as this is `java.base` I suggest to replace `key() + 
basicType.basicTypeChar()` with 
`key().concat(String.valueOf(basicType.basicTypeChar()))` to prevent 
`StringBuilder` allocation and `append()` chaining.

-

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

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

2023-02-14 Thread Jorn Vernee

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.

src/java.base/share/classes/java/lang/invoke/BoundMethodHandle.java line 371:

> 369: extensions[typeNum] = sd;
> 370: return sd;
> 371: }

I suggest changing the existing overload to delegate to this one with 
`BasicType.basicType(typeNum)` as the argument. (or to use some other way to 
reduce the duplication)

-

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

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

On Fri, 10 Feb 2023 23:18:47 GMT, Claes Redestad  wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add URL to microbenchmark
>
> Marked as reviewed by redestad (Reviewer).

> @cl4es Can you please initiate the in-depth testing required for this fix? 
> Thanks.

I've checked out the sources and fired off a sanity run of the first couple of 
tiers. Holding off on testing higher tiers until @sviswa7's concerns has been 
resolved.

-

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

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

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?

kind = 5 looks right.  

Can you update the end date of the copyright headers as this is the first 
change in 2023. imageFile.cpp by missed by the other change so it could be done 
here too.

-

Marked as reviewed by alanb (Reviewer).

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

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

> 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:

  Improve documentation of ZipCoder.compare and the Comparison enum values. 
Describe rationale for the tests added to TestZipFileEncodings. Revert 
unintended change in setTime in TestZipFileEncodings. Rename 
Comparison.SLASH_MATCH to DIRECTORY_MATCH.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12290/files
  - new: https://git.openjdk.org/jdk/pull/12290/files/d94cbe38..a633d270

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=09
 - incr: https://webrevs.openjdk.

Integrated: 8302337: JDK crashes if lib/modules contains non-zero byte containing ATTRIBUTE_END

On Mon, 13 Feb 2023 16:57:17 GMT, Severin Gehwolf  wrote:

> The `jimage` location attributes are terminated with `ATTRIBUTE_END`-kinds. 
> However,
> the byte containing `ATTRIBUTE_END` (most significant 5 bits, represent 
> `kind`), might
> be non-zero in the lower 3 bits (values up to `0x07` represent 
> `ATTRIBUTE_END`). The JDK code
> handles this case correctly in 
> [`ImageLocation.decompress()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java#L69..L71).
>  However, the `libjimage`
> code in `java.base` doesn't. That can lead to segfaults reading random bytes 
> and offsets.
> 
> I propose to break the loop if `ATTRIBUTE_END` is being encountered so that 
> reading stops.
> 
> Testing:
>  - [x] `test/jdk/tools/jimage` and `test/jdk/jdk/internal/jimage` tests.
>  - [x] Manual testing with a patched JDK to write non-zero bytes containing 
> `ATTRIBUTE_END` into the jimage. Segfaults before, runs fine after.
>  - [x] GHA.
> 
> Thoughts?

This pull request has now been integrated.

Changeset: ee5f6e15
Author:Severin Gehwolf 
URL:   
https://git.openjdk.org/jdk/commit/ee5f6e156de0fd3d78adf60951866f43c492725b
Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod

8302337: JDK crashes if lib/modules contains non-zero byte containing 
ATTRIBUTE_END

Reviewed-by: stuefe, jlaskey, alanb

-

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

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

On Tue, 14 Feb 2023 08:34:15 GMT, Eirik Bjorsnos  wrote:

>> 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:
> 
>   Improve documentation of ZipCoder.compare and the Comparison enum values. 
> Describe rationale for the tests added to TestZipFileEncodings. Revert 
> unintended change in setTime in TestZipFileEncodings. Rename 
> Comparison.SLASH_MATCH to DIRECTORY_MATCH.

Thanks for your thorough and helpful review, Lance.

-

PR: https://gi

Re: RFR: 8302337: JDK crashes if lib/modules contains non-zero byte containing ATTRIBUTE_END

On Mon, 13 Feb 2023 16:57:17 GMT, Severin Gehwolf  wrote:

> The `jimage` location attributes are terminated with `ATTRIBUTE_END`-kinds. 
> However,
> the byte containing `ATTRIBUTE_END` (most significant 5 bits, represent 
> `kind`), might
> be non-zero in the lower 3 bits (values up to `0x07` represent 
> `ATTRIBUTE_END`). The JDK code
> handles this case correctly in 
> [`ImageLocation.decompress()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java#L69..L71).
>  However, the `libjimage`
> code in `java.base` doesn't. That can lead to segfaults reading random bytes 
> and offsets.
> 
> I propose to break the loop if `ATTRIBUTE_END` is being encountered so that 
> reading stops.
> 
> Testing:
>  - [x] `test/jdk/tools/jimage` and `test/jdk/jdk/internal/jimage` tests.
>  - [x] Manual testing with a patched JDK to write non-zero bytes containing 
> `ATTRIBUTE_END` into the jimage. Segfaults before, runs fine after.
>  - [x] GHA.
> 
> Thoughts?

Thanks for the reviews!

-

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

Re: RFR: 8302337: JDK crashes if lib/modules contains non-zero byte containing ATTRIBUTE_END

On Mon, 13 Feb 2023 19:50:24 GMT, Alan Bateman  wrote:

> The change looks good, I'm just curious whether you observed a crash or 
> whether this was noticed by inspection.

Noticed by exploration. Changed the java code on the jlink side and observed 
the crash.

-

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

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

On Mon, 13 Feb 2023 16:43:12 GMT, Eirik Bjorsnos  wrote:

> Case-insensitive regionMatches could be improved by using 
> ArraysSupport.mismatch to skip over long common substrings:

I considered this but saw regressions similar to what you're getting for size = 
6 and backed off. I think this might be a good future enhancement, with some 
care, but didn't want to encumber this RFE with a case that regresses small 
inputs (which tend to be more common in actual applications).

In similar constructs we have avoided doing the vectorized call in a loop since 
this could regress worst case inputs severely (an adversary example might be 
something like `regionMatches(true, "aa", 0, 
"aAaAaAaAaAaAaAa", 0, 15)` which will call mismatch 8 times on 15 byte of 
input).

-

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

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

> 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:

  Remove accidentally introduced trailing whitespace

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/12290/files
  - new: https://git.openjdk.org/jdk/pull/12290/files/a633d270..21cdb732

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=10
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12290&range=09-10

  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 https://git.openjdk.org/jdk pull/12290

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

On Mon, 13 Feb 2023 20:20:22 GMT, Lance Andersen  wrote:

>> Eirik Bjorsnos has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Revert accidental removal of UTF8ZipCoder.compare
>
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 90:
> 
>> 88: expectedExceptionsMessageRegExp = BAD_ENTRY_NAME_OR_COMMENT)
>> 89: public void shouldRejectInvalidName() throws IOException {
>> 90: try (ZipFile zf = new ZipFile(invalidName.toFile())) {
> 
> If you could please convert to use `expectThrows` to get to validate the 
> message name

The message is already validated using `expectedExceptionsMessageRegExp` in the 
`@Test` annotation.

Would you prefer if I use expectThrows instead, or perhaps inline the 
`BAD_ENTRY_NAME_OR_COMMENT` constant as a literal?

-

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

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