Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v2]

[Jatin Bhateja]

On Thu, 4 Jan 2024 13:41:40 GMT, Emanuel Peter  wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updating copyright year of modified files.
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5307:
> 
>> 5305: assert(bt == T_LONG || bt == T_DOUBLE, "");
>> 5306: vmovmskpd(rtmp, mask, vec_enc);
>> 5307: shlq(rtmp, 5);
> 
> Might this need to be 6? If I understand right, then you want to have a 64bit 
> stride, hence 2^6, right?
> If that is correct, then this did not show in your tests, and you need a 
> regression test anyway.

This computes the byte offset from start of the table, both integer and long 
permute table have same row sizes, 8 int elements vs 4 long elements.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1442555037

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v3]

[Jatin Bhateja]

> Hi,
> 
> Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 only 
> targets.
> Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 
> instruction set.
> These are very frequently used APIs in columnar database filter operation.
> 
> Implementation uses a lookup table to record permute indices. Table index is 
> computed using
> mask argument of compress/expand operation.
> 
> Following are the performance number of JMH micro included with the patch.
> 
> 
> System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids)
> 
> Baseline:
> Benchmark (size)   Mode  CntScore   Error 
>   Units
> ColumnFilterBenchmark.filterDoubleColumn1024  thrpt2  142.767 
>  ops/ms
> ColumnFilterBenchmark.filterDoubleColumn2047  thrpt2   71.436 
>  ops/ms
> ColumnFilterBenchmark.filterDoubleColumn4096  thrpt2   35.992 
>  ops/ms
> ColumnFilterBenchmark.filterFloatColumn 1024  thrpt2  182.151 
>  ops/ms
> ColumnFilterBenchmark.filterFloatColumn 2047  thrpt2   91.096 
>  ops/ms
> ColumnFilterBenchmark.filterFloatColumn 4096  thrpt2   44.757 
>  ops/ms
> ColumnFilterBenchmark.filterIntColumn   1024  thrpt2  184.099 
>  ops/ms
> ColumnFilterBenchmark.filterIntColumn   2047  thrpt2   91.981 
>  ops/ms
> ColumnFilterBenchmark.filterIntColumn   4096  thrpt2   45.170 
>  ops/ms
> ColumnFilterBenchmark.filterLongColumn  1024  thrpt2  148.017 
>  ops/ms
> ColumnFilterBenchmark.filterLongColumn  2047  thrpt2   73.516 
>  ops/ms
> ColumnFilterBenchmark.filterLongColumn  4096  thrpt2   36.844 
>  ops/ms
> 
> Withopt:
> Benchmark (size)   Mode  Cnt Score   
> Error   Units
> ColumnFilterBenchmark.filterDoubleColumn1024  thrpt2  2051.707
>   ops/ms
> ColumnFilterBenchmark.filterDoubleColumn2047  thrpt2   914.072
>   ops/ms
> ColumnFilterBenchmark.filterDoubleColumn4096  thrpt2   489.898
>   ops/ms
> ColumnFilterBenchmark.filterFloatColumn 1024  thrpt2  5324.195
>   ops/ms
> ColumnFilterBenchmark.filterFloatColumn 2047  thrpt2  2587.229
>   ops/ms
> ColumnFilterBenchmark.filterFloatColumn 4096  thrpt2  1278.665
>   ops/ms
> ColumnFilterBenchmark.filterIntColumn   1024  thrpt2  4149.384
>   ops/ms
> ColumnFilterBenchmark.filterIntColumn   2047  thrpt2  1791.170
>   ops/ms
> ColumnFilterBenchmark.filterIntColumn   4096...

Jatin Bhateja has updated the pull request incrementally with one additional 
commit since the last revision:

  Review comments resolutions.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17261/files
  - new: https://git.openjdk.org/jdk/pull/17261/files/6bd9b0ad..ea0aa0b4

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

  Stats: 49 lines in 4 files changed: 44 ins; 1 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/17261.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17261/head:pull/17261

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

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v2]

[Jatin Bhateja]

On Thu, 4 Jan 2024 13:30:24 GMT, Emanuel Peter  wrote:

>> test/micro/org/openjdk/bench/jdk/incubator/vector/ColumnFilterBenchmark.java 
>> line 94:
>> 
>>> 92:IntVector vec = IntVector.fromArray(ispecies, intinCol, i);
>>> 93:VectorMask pred = vec.compare(VectorOperators.GT, 
>>> ipivot);
>>> 94:vec.compress(pred).intoArray(intoutCol, j);
>> 
>> Could there be equivalent `expand` tests?
>
> And what about some result verification? Or is there another test that does 
> that?

We do have extensive functional tests for compress/expand APIs in 
[test/jdk/jdk/incubator/vector](https://github.com/openjdk/jdk/tree/master/test/jdk/jdk/incubator/vector)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1442554968

Re: RFR: 8322920: Some ProcessTools.execute* functions are declared to throw Throwable

[Leonid Mesnik]

On Wed, 3 Jan 2024 09:51:24 GMT, Stefan Karlsson  wrote:

> Most functions in ProcessTools are declared to throw Exceptions, or one of 
> its subclasses. However, there are a small number of functions that are 
> unnecessarily declared to throw Throwable instead of Exception. I propose 
> that we change them to also be declared to throw Exception.
> 
> This is a trivial patch to make it easier to refactor tests to use the 
> updated functions.
> 
> Tested manually, but will wait for GHA to verify that the change is OK.

You need to update copyrights.

-

Marked as reviewed by lmesnik (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17240#pullrequestreview-1805436752

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v2]

[Jatin Bhateja]

On Fri, 5 Jan 2024 07:03:26 GMT, Jatin Bhateja  wrote:

>> And what about some result verification? Or is there another test that does 
>> that?
>
> We do have extensive functional tests for compress/expand APIs in 
> [test/jdk/jdk/incubator/vector](https://github.com/openjdk/jdk/tree/master/test/jdk/jdk/incubator/vector)

> Could there be equivalent `expand` tests?

Here are the performance number for existing [VectorAPI JMH 
micros.](https://github.com/openjdk/panama-vector/tree/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation)

![image](https://github.com/openjdk/jdk/assets/59989778/4b260814-3d3c-4e9b-b81a-61492ea48cce)
![image](https://github.com/openjdk/jdk/assets/59989778/50048281-ad50-44f6-a875-308e02537be2)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1442556253

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v2]

[Jatin Bhateja]

On Thu, 4 Jan 2024 13:33:08 GMT, Emanuel Peter  wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updating copyright year of modified files.
>
> test/micro/org/openjdk/bench/jdk/incubator/vector/ColumnFilterBenchmark.java 
> line 76:
> 
>> 74: longinCol = new long[size];
>> 75: longoutCol = new long[size];
>> 76: lpivot = size / 2;
> 
> I'd be interested to see what happens if you move up or down the "density" of 
> elements that you accept. Would the simple branch prediction be faster if the 
> density is low enough, i.e. we almost take no element.
> 
> Though maybe that is not compiler problem but a user-problem?

Included fuzzy filter micro with varying mask density.
![image](https://github.com/openjdk/jdk/assets/59989778/a6af21cc-36c0-4503-aeb3-e66b862da2e1)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1442557565

Re: The default java.library.path on Linux does not include the library paths in the mulitarch-spec

[David Holmes]

On 5/01/2024 12:24 am, Glavo wrote:

I expect there are security reasons why the JDK tries to find
the file itself in these specific paths, rather than letting the
platform code search for it.


I think this should have nothing to do with security. If there is a 
vulnerability in the platform code, there is nothing the JDK can do to 
avoid it.


I did some archaeology and this has basically worked this way since it 
was introduced way back in 1998. As far as I can see there was never any 
discussion/suggestion that if we fail to find a library by the 
classloader mechanisms (using java.library.path etc.) that we just pass 
it through to the platform loader (e.g. dlopen) to try and find it. This 
may be an oversight, or it may relate to how the classloader has to 
maintain a mapping between library names and actual library files (which 
would be difficult if dlopen does the searching implicitly).



Well that is not something I would want to see implemented in hotspot.


This is the only way I can think of to get the JDK to behave 
consistently with ld.
Maybe we should wait and see other developers who are more familiar with 
this part.


Certainly. But IMHO neither core-libs folk, not hotspot folk, will want 
to be in the business of having to load, parse and interpret 
/etc/ld.so.conf and its related files.



I'm now sending this email to panama-dev as well.
I think this proposal is of great significance to Panama, as it will 
make it easier for developers to develop wrappers for platform libraries.


It will be interesting to see what they say.

Cheers,
David
-


Glavo

On Thu, Jan 4, 2024 at 3:12 PM David Holmes > wrote:


On 4/01/2024 1:36 pm, Glavo wrote:
 > Hey David,
 >
 >     AFAICS java.library.path (and sun.boot.library.path) are input
 >     properties that can be set by the user. There are no
"default" values
 >     for these properties as such.
 >
 >
 > Although they can be set by the user, they have default values.
 > For Linux, the default value is set here:
 >
 >

https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555
 

 
>
 >
 > The default value on Linux is the LD_LIBRARY_PATH environment
variable
 > and the paths I mentioned earlier.
 >
 >     The library loading will ultimately rely
 >     on the behaviour of dlopen, if no additional paths have been
set, so it
 >     seems to me what you really want is for dlopen to act "the
same way"
 >     that ld does. Note that dlopen will already check the contents of
 >     /etc/ld.so.cache
 >
 >
 > System.loadLibrary looks for the library in sun.boot.library.path
and
 > java.library.path and passes the full path to the library to dlopen:
 >
 >

https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254
 

 
>
 >
 > Therefore, the behavior of finding native libraries has nothing
to do
 > with the behavior of dlopen, only sun.boot.library.path and
 > java.library.path.

I stand corrected - apologies. I expected a raw name to simply get
passed through. I thought both LD_LIBRARY_PATH and java.library.path
could be used to expand the set of directories where the platform code
looks for libs, not constrain things to only those paths (plus the
defaults). I expect there are security reasons why the JDK tries to
find
the file itself in these specific paths, rather than letting the
platform code search for it.

 >     This does not seem practical. On my system ld.so.conf just
contains
 >
 >     include ld.so.conf.d/*.conf
 >
 >     so now we (hotspot) have to read the top-level file, parse
it, interpret
 >     it and then recursively read, parse and interpret more files.
 >
 >
 > Yes, this is exactly the behavior I want.

Well that is not something I would want to see implemented in hot

[jdk22] Integrated: 8322214: Return value of XMLInputFactory.getProperty() changed from boolean to String in JDK 22 early access builds

[Joe Wang]

On Thu, 4 Jan 2024 19:09:34 GMT, Joe Wang  wrote:

> backport of PR: https://github.com/openjdk/jdk/pull/17252

This pull request has now been integrated.

Changeset: b1219319
Author:Joe Wang 
URL:   
https://git.openjdk.org/jdk22/commit/b121931959e08951a8cc02eb925a77657441f175
Stats: 144 lines in 3 files changed: 140 ins; 0 del; 4 mod

8322214: Return value of XMLInputFactory.getProperty() changed from boolean to 
String in JDK 22 early access builds

Reviewed-by: naoto, iris
Backport-of: 755722ced60a686799c7f419feae61c04ce41f09

-

PR: https://git.openjdk.org/jdk22/pull/32

Re: New Java feature

[David Alayachew]

amber-...@openjdk.org

On Thu, Jan 4, 2024 at 8:29 PM Bradley Willcott 
wrote:

> Hi there.
> I am sorry if this is 'off subject'. However, where do I go to propose a
> new java feature? And please be nice :-).
>
> Thanks,
> Brad.
>

Re: New Java feature

[David Alayachew]

Try and search up the idea before posting though. A lot of people have made
feature requests before you.

On Thu, Jan 4, 2024 at 8:40 PM David Alayachew 
wrote:

> amber-...@openjdk.org
>
> On Thu, Jan 4, 2024 at 8:29 PM Bradley Willcott 
> wrote:
>
>> Hi there.
>> I am sorry if this is 'off subject'. However, where do I go to propose a
>> new java feature? And please be nice :-).
>>
>> Thanks,
>> Brad.
>>
>

New Java feature

[Bradley Willcott]

Hi there.
I am sorry if this is 'off subject'. However, where do I go to propose a 
new java feature? And please be nice :-).


Thanks,
Brad.

Integrated: 8322322: Support archived full module graph when -Xbootclasspath/a is used

[Calvin Cheung]

On Thu, 21 Dec 2023 19:10:59 GMT, Calvin Cheung  wrote:

> Please review this change for enabling full module graph even when 
> -Xbootclasspath/a is specified. The validation of -Xbootclasspath/a is 
> already being done in `FileMapInfo::validate_boot_class_paths()`. The full 
> module graph will be disabled if there's a mismatch in -Xbootclasspath/a 
> between dump time and runtime.
> 
> The changes in ClassLoaders.java is for setting up the append class path for 
> the boot loader retrieved from the CDS archive. It is required because some 
> existing tests are using the `getResourceAsStream()` api. Those tests would 
> fail without the change.
> 
> Passed tiers 1 - 4 testing.

This pull request has now been integrated.

Changeset: 3b1e56a4
Author:Calvin Cheung 
URL:   
https://git.openjdk.org/jdk/commit/3b1e56a4275addeadcefe180b5ce60d9d74cca7b
Stats: 64 lines in 4 files changed: 50 ins; 11 del; 3 mod

8322322: Support archived full module graph when -Xbootclasspath/a is used

Reviewed-by: alanb, mchung

-

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

Re: RFR: 8322322: Support archived full module graph when -Xbootclasspath/a is used [v2]

[Calvin Cheung]

On Sat, 23 Dec 2023 08:08:35 GMT, Alan Bateman  wrote:

>> Calvin Cheung has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   comments from Alan and Ioi
>
> Marked as reviewed by alanb (Reviewer).

Thanks @AlanBateman @mlchung for the reviews.
Also thanks @iklam for offline discussion and review.

-

PR Comment: https://git.openjdk.org/jdk/pull/17178#issuecomment-1877923857

Re: RFR: JDK-8322235: Split up and improve LocaleProvidersRun [v2]

[Naoto Sato]

On Thu, 4 Jan 2024 19:30:00 GMT, Justin Lu  wrote:

>> Please review this PR which splits up the _LocaleProvidersRun_ test file for 
>> performance and maintenance reasons.
>> 
>> _LocaleProvidersRun_ which tests against the various Locale Providers (CLDR, 
>> HOST, SPI, etc.) was getting rather long, as all related bugs were added to 
>> the single test file. To improve maintainability, this change splits up the 
>> single test file into separate test files focused on specific areas (ex: 
>> _j.text.Format_).  The original _LocaleProvidersRun_ test file remains and 
>> is used for more general Locale Provider testing such as the adapter loading.
>> 
>> In addition, the previously single test file used to suffer from performance 
>> issues, as each test method had to launch a new JVM (since Locale Providers 
>> are set at Java startup time). With this change, these tests files can be 
>> ran with VM flags and not cause timeout, thus `@requires vm.flagless` is no 
>> longer needed (Tiers 6-8 tested).
>> 
>> Other updates
>> - For OS/locale specific tests, the OS/locale is now checked before (not 
>> after) launching a JVM
>> - For tests that are meant to be tested against specific locales, additional 
>> run invocations were added with the appropiate locale to guarantee a run 
>> (ex: `@run junit/othervm -Duser.language=en -Duser.country=GB`)
>> - Added comments for each test method
>
> Justin Lu has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - cleanup comments
>  - provide both SPI classes under build for relevant test files

Marked as reviewed by naoto (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17257#pullrequestreview-1804861121

Re: [jdk22] RFR: 8322214: Return value of XMLInputFactory.getProperty() changed from boolean to String in JDK 22 early access builds

[Iris Clark]

On Thu, 4 Jan 2024 19:09:34 GMT, Joe Wang  wrote:

> backport of PR: https://github.com/openjdk/jdk/pull/17252

Marked as reviewed by iris (Reviewer).

-

PR Review: https://git.openjdk.org/jdk22/pull/32#pullrequestreview-1804849821

Re: [jdk22] RFR: 8322214: Return value of XMLInputFactory.getProperty() changed from boolean to String in JDK 22 early access builds

[Naoto Sato]

On Thu, 4 Jan 2024 19:09:34 GMT, Joe Wang  wrote:

> backport of PR: https://github.com/openjdk/jdk/pull/17252

Marked as reviewed by naoto (Reviewer).

-

PR Review: https://git.openjdk.org/jdk22/pull/32#pullrequestreview-1804842674

Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v6]

[Naoto Sato]

On Wed, 30 Aug 2023 22:35:43 GMT, Naoto Sato  wrote:

>> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 
>> where aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored 
>> the in-house cache with WeakHashMap, and removed the Key class as it is no 
>> longer needed (thus the original NPE will no longer be possible). Also with 
>> the new JMH test case, it gains some performance improvement:
>> 
>> (w/o fix)
>> 
>> Benchmark   Mode  Cnt  Score Error  Units
>> LocaleCache.testForLanguageTag  avgt   20   5781.275 ± 569.580  ns/op
>> LocaleCache.testLocaleOfavgt   20  62564.079 ± 406.697  ns/op
>> 
>> (w/ fix)
>> Benchmark   Mode  Cnt  Score Error  Units
>> LocaleCache.testForLanguageTag  avgt   20   4801.175 ± 371.830  ns/op
>> LocaleCache.testLocaleOfavgt   20  60394.652 ± 352.471  ns/op
>
> Naoto Sato has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 31 commits:
> 
>  - Restored the test
>  - Merge branch 'master' into JDK-8309622-Cache-BaseLocale
>  - Merge branch 'master' of https://git.openjdk.org/jdk into 
> JDK-8309622-Cache-BaseLocale
>  - small cleanup
>  - Merge branch 'pull/14684' into JDK-8309622-Cache-BaseLocale
>  - Update ReferencedKeyTest.java
>  - Simple versions of create
>  - Add flag for reference queue type
>  - Merge branch 'master' into 8310913
>  - Update to use VirtualThread friendly stale queue.
>  - ... and 21 more: https://git.openjdk.org/jdk/compare/99ea8bf2...b1f64e93

keep open

-

PR Comment: https://git.openjdk.org/jdk/pull/14404#issuecomment-1877639764

Re: RFR: JDK-8322235: Split up and improve LocaleProvidersRun [v2]

[Justin Lu]

> Please review this PR which splits up the _LocaleProvidersRun_ test file for 
> performance and maintenance reasons.
> 
> _LocaleProvidersRun_ which tests against the various Locale Providers (CLDR, 
> HOST, SPI, etc.) was getting rather long, as all related bugs were added to 
> the single test file. To improve maintainability, this change splits up the 
> single test file into separate test files focused on specific areas (ex: 
> _j.text.Format_).  The original _LocaleProvidersRun_ test file remains and is 
> used for more general Locale Provider testing such as the adapter loading.
> 
> In addition, the previously single test file used to suffer from performance 
> issues, as each test method had to launch a new JVM (since Locale Providers 
> are set at Java startup time). With this change, these tests files can be ran 
> with VM flags and not cause timeout, thus `@requires vm.flagless` is no 
> longer needed (Tiers 6-8 tested).
> 
> Other updates
> - For OS/locale specific tests, the OS/locale is now checked before (not 
> after) launching a JVM
> - For tests that are meant to be tested against specific locales, additional 
> run invocations were added with the appropiate locale to guarantee a run (ex: 
> `@run junit/othervm -Duser.language=en -Duser.country=GB`)
> - Added comments for each test method

Justin Lu has updated the pull request incrementally with two additional 
commits since the last revision:

 - cleanup comments
 - provide both SPI classes under build for relevant test files

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17257/files
  - new: https://git.openjdk.org/jdk/pull/17257/files/b38b9909..a75237dc

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

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

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

Re: RFR: JDK-8322235: Split up and improve LocaleProvidersRun [v2]

[Justin Lu]

On Thu, 4 Jan 2024 19:01:31 GMT, Naoto Sato  wrote:

>> Justin Lu has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - cleanup comments
>>  - provide both SPI classes under build for relevant test files
>
> test/jdk/java/util/Locale/LocaleProvidersFormat.java line 30:
> 
>> 28:  * @library /test/lib
>> 29:  * @build LocaleProviders
>> 30:  *providersrc.spi.src.tznp8013086
> 
> Although it is not needed in this test, I would list `tznp` to be built here, 
> as the provider's meta-info includes both classes for completeness. Applies 
> to `LocaleProvidersTimeZone` test too.

That makes sense, updated both the relevant test files as you suggested. Thank 
you for taking the time to review this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17257#discussion_r1442166693

[jdk22] RFR: 8322214: Return value of XMLInputFactory.getProperty() changed from boolean to String in JDK 22 early access builds

[Joe Wang]

backport of PR: https://github.com/openjdk/jdk/pull/17252

-

Commit messages:
 - Backport 755722ced60a686799c7f419feae61c04ce41f09

Changes: https://git.openjdk.org/jdk22/pull/32/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=32&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322214
  Stats: 144 lines in 3 files changed: 140 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk22/pull/32.diff
  Fetch: git fetch https://git.openjdk.org/jdk22.git pull/32/head:pull/32

PR: https://git.openjdk.org/jdk22/pull/32

Re: RFR: JDK-8322235: Split up and improve LocaleProvidersRun

[Naoto Sato]

On Wed, 3 Jan 2024 23:30:41 GMT, Justin Lu  wrote:

> Please review this PR which splits up the _LocaleProvidersRun_ test file for 
> performance and maintenance reasons.
> 
> _LocaleProvidersRun_ which tests against the various Locale Providers (CLDR, 
> HOST, SPI, etc.) was getting rather long, as all related bugs were added to 
> the single test file. To improve maintainability, this change splits up the 
> single test file into separate test files focused on specific areas (ex: 
> _j.text.Format_).  The original _LocaleProvidersRun_ test file remains and is 
> used for more general Locale Provider testing such as the adapter loading.
> 
> In addition, the previously single test file used to suffer from performance 
> issues, as each test method had to launch a new JVM (since Locale Providers 
> are set at Java startup time). With this change, these tests files can be ran 
> with VM flags and not cause timeout, thus `@requires vm.flagless` is no 
> longer needed (Tiers 6-8 tested).
> 
> Other updates
> - For OS/locale specific tests, the OS/locale is now checked before (not 
> after) launching a JVM
> - For tests that are meant to be tested against specific locales, additional 
> run invocations were added with the appropiate locale to guarantee a run (ex: 
> `@run junit/othervm -Duser.language=en -Duser.country=GB`)
> - Added comments for each test method

Great to see this refactoring! Much cleaner now.

test/jdk/java/util/Locale/LocaleProvidersFormat.java line 30:

> 28:  * @library /test/lib
> 29:  * @build LocaleProviders
> 30:  *providersrc.spi.src.tznp8013086

Although it is not needed in this test, I would list `tznp` to be built here, 
as the provider's meta-info includes both classes for completeness. Applies to 
`LocaleProvidersTimeZone` test too.

-

PR Review: https://git.openjdk.org/jdk/pull/17257#pullrequestreview-1804804618
PR Review Comment: https://git.openjdk.org/jdk/pull/17257#discussion_r1442145501

Withdrawn: 8314544: Matrix multiply benchmark using Vector API

[duke]

On Fri, 18 Aug 2023 03:57:24 GMT, Martin Stypinski  wrote:

> Added a bunch of different implementations for Vector API Matrix 
> Multiplications:
> 
> - Baseline
> - Blocked (Cache Local)
> - FMA
> - Vector API Simple Implementation
> - Vector API Blocked Implementation
> 
> Commit was discussed with @PaulSandoz

This pull request has been closed without being integrated.

-

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

Re: RFR: 8322877: java/io/BufferedInputStream/TransferTo.java failed with IndexOutOfBoundsException

[Brian Burkhalter]

On Thu, 4 Jan 2024 18:37:59 GMT, Markus KARG  wrote:

>> No: the third param of 
>> [Arrays.copyOfRange](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Arrays.html#copyOfRange(byte[],int,int))
>>  is `to`, not `len`.
>
> Ah, this explains why it did not fail originally, but only after adding the 
> "isTrusted" case! 🙏

See https://github.com/openjdk/jdk/pull/17250#issuecomment-1875761630 above.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17250#discussion_r1442126261

Re: RFR: 8322877: java/io/BufferedInputStream/TransferTo.java failed with IndexOutOfBoundsException

[Brian Burkhalter]

On Thu, 4 Jan 2024 18:32:55 GMT, Markus KARG  wrote:

>> The final position instead of the number of bytes to write was being passed 
>> to `ByteArrayOuputStream.write(byte[],int,int)`.
>
> src/java.base/share/classes/java/io/BufferedInputStream.java line 650:
> 
>> 648: } else {
>> 649: // Prevent poisoning and leaking of buf
>> 650: byte[] buffer = Arrays.copyOfRange(getBufIfOpen(), 
>> pos, count);
> 
> @bplb Shouldn't it be `avail` *here*, too?

No: the third param of 
[Arrays.copyOfRange](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Arrays.html#copyOfRange(byte[],int,int))
 is `to`, not `len`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17250#discussion_r1442122428

Re: RFR: 8322877: java/io/BufferedInputStream/TransferTo.java failed with IndexOutOfBoundsException

[Markus KARG]

On Thu, 4 Jan 2024 18:35:49 GMT, Brian Burkhalter  wrote:

>> src/java.base/share/classes/java/io/BufferedInputStream.java line 650:
>> 
>>> 648: } else {
>>> 649: // Prevent poisoning and leaking of buf
>>> 650: byte[] buffer = Arrays.copyOfRange(getBufIfOpen(), 
>>> pos, count);
>> 
>> @bplb Shouldn't it be `avail` *here*, too?
>
> No: the third param of 
> [Arrays.copyOfRange](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Arrays.html#copyOfRange(byte[],int,int))
>  is `to`, not `len`.

Ah, this explains why it did not fail originally, but only after adding the 
"isTrusted" case! 🙏

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17250#discussion_r1442124151

Re: RFR: 8322877: java/io/BufferedInputStream/TransferTo.java failed with IndexOutOfBoundsException

[Markus KARG]

On Wed, 3 Jan 2024 18:01:59 GMT, Brian Burkhalter  wrote:

> The final position instead of the number of bytes to write was being passed 
> to `ByteArrayOuputStream.write(byte[],int,int)`.

src/java.base/share/classes/java/io/BufferedInputStream.java line 650:

> 648: } else {
> 649: // Prevent poisoning and leaking of buf
> 650: byte[] buffer = Arrays.copyOfRange(getBufIfOpen(), 
> pos, count);

@bplb Shouldn't it be `avail` *here*, too?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17250#discussion_r1442120135

Re: RFR: 8322322: Support archived full module graph when -Xbootclasspath/a is used [v3]

[Mandy Chung]

On Wed, 3 Jan 2024 00:49:01 GMT, Calvin Cheung  wrote:

>> Please review this change for enabling full module graph even when 
>> -Xbootclasspath/a is specified. The validation of -Xbootclasspath/a is 
>> already being done in `FileMapInfo::validate_boot_class_paths()`. The full 
>> module graph will be disabled if there's a mismatch in -Xbootclasspath/a 
>> between dump time and runtime.
>> 
>> The changes in ClassLoaders.java is for setting up the append class path for 
>> the boot loader retrieved from the CDS archive. It is required because some 
>> existing tests are using the `getResourceAsStream()` api. Those tests would 
>> fail without the change.
>> 
>> Passed tiers 1 - 4 testing.
>
> Calvin Cheung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   tests update per discussion with Ioi

Looks okay to me.

-

Marked as reviewed by mchung (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17178#pullrequestreview-1804764896

Re: RFR: 8322725: (tz) Update Timezone Data to 2023d

[Naoto Sato]

On Thu, 4 Jan 2024 13:34:50 GMT, Johny Jose  wrote:

> tzdata2023d changes

LTGM too, assuming all the tests passed

-

Marked as reviewed by naoto (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17268#pullrequestreview-1804702013

[jdk22] Integrated: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

[Naoto Sato]

On Thu, 4 Jan 2024 17:26:37 GMT, Naoto Sato  wrote:

> 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

This pull request has now been integrated.

Changeset: a72afb38
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk22/commit/a72afb3845a7d245d462904e75b9368efefc0d39
Stats: 94 lines in 3 files changed: 72 ins; 2 del; 20 mod

8322647: Short name for the `Europe/Lisbon` time zone is incorrect

Reviewed-by: joehw
Backport-of: ad31ec5c5f120082cedd7b9ece45b6b44147c0c5

-

PR: https://git.openjdk.org/jdk22/pull/30

Re: [jdk22] RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

[Joe Wang]

On Thu, 4 Jan 2024 17:26:37 GMT, Naoto Sato  wrote:

> 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

Marked as reviewed by joehw (Reviewer).

-

PR Review: https://git.openjdk.org/jdk22/pull/30#pullrequestreview-1804685583

[jdk22] RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

[Naoto Sato]

8322647: Short name for the `Europe/Lisbon` time zone is incorrect

-

Commit messages:
 - Backport ad31ec5c5f120082cedd7b9ece45b6b44147c0c5

Changes: https://git.openjdk.org/jdk22/pull/30/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=30&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322647
  Stats: 94 lines in 3 files changed: 72 ins; 2 del; 20 mod
  Patch: https://git.openjdk.org/jdk22/pull/30.diff
  Fetch: git fetch https://git.openjdk.org/jdk22.git pull/30/head:pull/30

PR: https://git.openjdk.org/jdk22/pull/30

Integrated: JDK-8319626: Override toString() for ZipFile

[Justin Lu]

On Mon, 13 Nov 2023 21:59:05 GMT, Justin Lu  wrote:

> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319982) 
> which overrides and provides an implementation of `toString()` in 
> _java.util.zip.ZipFile_ (and by extension, _java.util.jar.JarFile_).

This pull request has now been integrated.

Changeset: 15cf8f85
Author:Justin Lu 
URL:   
https://git.openjdk.org/jdk/commit/15cf8f853105050ec356756d5affa153f79894fa
Stats: 14 lines in 1 file changed: 11 ins; 0 del; 3 mod

8319626: Override toString() for ZipFile

Reviewed-by: jpai, alanb, coffeys

-

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

Re: RFR: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

[Naoto Sato]

On Fri, 22 Dec 2023 18:59:20 GMT, Naoto Sato  wrote:

> This is a regression caused by the fix to 
> [JDK-8317979](https://bugs.openjdk.org/browse/JDK-8317979), where the parsing 
> of TZDB files was incorrect. With this fix, `TimeZoneNames_en.java` which is 
> generated during the build time has the following diffs from the previous 
> (incorrect) one:
> 
> --- 
> master/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-18 10:28:57
> +++ 
> tz/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-22 10:09:13
> @@ -304,11 +304,11 @@
>  };
>  final String[] Azores = new String[] {
> "Azores Standard Time",
> -   "HMT",
> +   "",
> "Azores Summer Time",
> -   "HMT",
> +   "",
> "Azores Time",
> -   "HMT",
> +   "",
>  };
>  final String[] Bhutan = new String[] {
> "Bhutan Time",
> @@ -968,11 +968,11 @@
>  };
>  final String[] Africa_Central = new String[] {
> "Central Africa Time",
> -   "SAST",
> -   "",
> -   "SAST",
> +   "CAT",
> "",
> -   "SAST",
> +   "CAT",
> +   "",
> +   "CAT",
>  };
>  final String[] Africa_Eastern = new String[] {
> "East Africa Time",
> @@ -1016,11 +1016,11 @@
>  };
>  final String[] Europe_Western = new String[] {
> "Western European Standard Time",
> -   "FMT",
> -   "Western European Summer Time",
> -   "FMT",
> +   "WET",
> +   "Western European Summer Time",
> +   "WEST",
> "Western European Time",
> -   "FMT",
> +   "WET",
>  };
>  final String[] Mexico_Pacific = new String[] {
> "Mexican Pacific Standard Time",
> @@ -1152,11 +1152,11 @@
>  };
>  final String[] Australia_Western = new String[] {
> "Australian Western Standard Time",
> -   "",
> +   "AWST",
> "Australian Western Daylight Time",
> -   "",
> +   "AWDT",
> "Western Australia Time",
> -   "",
> +   "AWT",
>  };
>  final String[] Greenland_Eastern = new String[] {
> "East Greenland Standard Time",
> 
> Previously, they all had wrong short names due to incorrect parsi...

Thanks for the reviews!

-

PR Comment: https://git.openjdk.org/jdk/pull/17187#issuecomment-1877474644

Integrated: 8322647: Short name for the `Europe/Lisbon` time zone is incorrect

[Naoto Sato]

On Fri, 22 Dec 2023 18:59:20 GMT, Naoto Sato  wrote:

> This is a regression caused by the fix to 
> [JDK-8317979](https://bugs.openjdk.org/browse/JDK-8317979), where the parsing 
> of TZDB files was incorrect. With this fix, `TimeZoneNames_en.java` which is 
> generated during the build time has the following diffs from the previous 
> (incorrect) one:
> 
> --- 
> master/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-18 10:28:57
> +++ 
> tz/build/macosx-aarch64/support/gensrc/java.base/sun/util/resources/cldr/TimeZoneNames_en.java
> 2023-12-22 10:09:13
> @@ -304,11 +304,11 @@
>  };
>  final String[] Azores = new String[] {
> "Azores Standard Time",
> -   "HMT",
> +   "",
> "Azores Summer Time",
> -   "HMT",
> +   "",
> "Azores Time",
> -   "HMT",
> +   "",
>  };
>  final String[] Bhutan = new String[] {
> "Bhutan Time",
> @@ -968,11 +968,11 @@
>  };
>  final String[] Africa_Central = new String[] {
> "Central Africa Time",
> -   "SAST",
> -   "",
> -   "SAST",
> +   "CAT",
> "",
> -   "SAST",
> +   "CAT",
> +   "",
> +   "CAT",
>  };
>  final String[] Africa_Eastern = new String[] {
> "East Africa Time",
> @@ -1016,11 +1016,11 @@
>  };
>  final String[] Europe_Western = new String[] {
> "Western European Standard Time",
> -   "FMT",
> -   "Western European Summer Time",
> -   "FMT",
> +   "WET",
> +   "Western European Summer Time",
> +   "WEST",
> "Western European Time",
> -   "FMT",
> +   "WET",
>  };
>  final String[] Mexico_Pacific = new String[] {
> "Mexican Pacific Standard Time",
> @@ -1152,11 +1152,11 @@
>  };
>  final String[] Australia_Western = new String[] {
> "Australian Western Standard Time",
> -   "",
> +   "AWST",
> "Australian Western Daylight Time",
> -   "",
> +   "AWDT",
> "Western Australia Time",
> -   "",
> +   "AWT",
>  };
>  final String[] Greenland_Eastern = new String[] {
> "East Greenland Standard Time",
> 
> Previously, they all had wrong short names due to incorrect parsi...

This pull request has now been integrated.

Changeset: ad31ec5c
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/ad31ec5c5f120082cedd7b9ece45b6b44147c0c5
Stats: 94 lines in 3 files changed: 72 ins; 2 del; 20 mod

8322647: Short name for the `Europe/Lisbon` time zone is incorrect

Reviewed-by: joehw, iris

-

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

Re: RFR: 6356745: (coll) Add PriorityQueue(Collection, Comparator) [v6]

[jmehrens]

On Thu, 4 Jan 2024 14:21:54 GMT, Valeh Hajiyev  wrote:

> why do you think the code would avoid heapify? `initFromCollection` method 
> will be called regardless of the type of the collection passed, which will 
> heapify the queue.

I simply mean to point out:

1.  That if the given comparator and the sortedset/priority queue comparator 
are the same then the call to heapify should be avoided as this is what is done 
in the [copy 
constructor.](https://github.com/openjdk/jdk/blob/139abc453f9eeeb4763076298ec44573ab94a3b6/src/java.base/share/classes/java/util/PriorityQueue.java#L196)
2. This new API has an anti-pattern not present with the other constructors.

-

PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-1877297438

Re: RFR: 8323002: test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times out on macosx-x64

[Alan Bateman]

On Thu, 4 Jan 2024 14:53:46 GMT, Patricio Chilano Mateo 
 wrote:

>> This test was recently added by JDK-8322818. On some test systems, the test 
>> passes a bit after the timeout so the test is marked as failed. The changes 
>> here dial down the iterations from 100k to 25k to reduce the execution time 
>> for release builds. No change for debug builds as this already uses a small 
>> number of iterations.
>
> Marked as reviewed by pchilanomate (Reviewer).

Thanks @pchilano.

-

PR Comment: https://git.openjdk.org/jdk/pull/17265#issuecomment-1877268522

Integrated: 8323002: test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times out on macosx-x64

[Alan Bateman]

On Thu, 4 Jan 2024 11:51:47 GMT, Alan Bateman  wrote:

> This test was recently added by JDK-8322818. On some test systems, the test 
> passes a bit after the timeout so the test is marked as failed. The changes 
> here dial down the iterations from 100k to 25k to reduce the execution time 
> for release builds. No change for debug builds as this already uses a small 
> number of iterations.

This pull request has now been integrated.

Changeset: d33dfe5c
Author:Alan Bateman 
URL:   
https://git.openjdk.org/jdk/commit/d33dfe5cb2bec682f94fbae850e167d6f437fecb
Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod

8323002: 
test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times 
out on macosx-x64

Reviewed-by: pchilanomate

-

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

Re: RFR: 8322725: (tz) Update Timezone Data to 2023d

[Sean Coffey]

On Thu, 4 Jan 2024 13:34:50 GMT, Johny Jose  wrote:

> tzdata2023d changes

LGTM

-

Marked as reviewed by coffeys (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17268#pullrequestreview-1804359390

Re: RFR: 8323002: test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times out on macosx-x64

[Patricio Chilano Mateo]

On Thu, 4 Jan 2024 11:51:47 GMT, Alan Bateman  wrote:

> This test was recently added by JDK-8322818. On some test systems, the test 
> passes a bit after the timeout so the test is marked as failed. The changes 
> here dial down the iterations from 100k to 25k to reduce the execution time 
> for release builds. No change for debug builds as this already uses a small 
> number of iterations.

Marked as reviewed by pchilanomate (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17265#pullrequestreview-1804326778

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v2]

[Emanuel Peter]

On Thu, 4 Jan 2024 05:39:01 GMT, Jatin Bhateja  wrote:

>> Hi,
>> 
>> Patch optimizes non-subword vector compress and expand APIs for x86 AVX2 
>> only targets.
>> Upcoming E-core Xeons (Sierra Forest) and Hybrid CPUs only support AVX2 
>> instruction set.
>> These are very frequently used APIs in columnar database filter operation.
>> 
>> Implementation uses a lookup table to record permute indices. Table index is 
>> computed using
>> mask argument of compress/expand operation.
>> 
>> Following are the performance number of JMH micro included with the patch.
>> 
>> 
>> System : Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids)
>> 
>> Baseline:
>> Benchmark (size)   Mode  CntScore   
>> Error   Units
>> ColumnFilterBenchmark.filterDoubleColumn1024  thrpt2  142.767
>>   ops/ms
>> ColumnFilterBenchmark.filterDoubleColumn2047  thrpt2   71.436
>>   ops/ms
>> ColumnFilterBenchmark.filterDoubleColumn4096  thrpt2   35.992
>>   ops/ms
>> ColumnFilterBenchmark.filterFloatColumn 1024  thrpt2  182.151
>>   ops/ms
>> ColumnFilterBenchmark.filterFloatColumn 2047  thrpt2   91.096
>>   ops/ms
>> ColumnFilterBenchmark.filterFloatColumn 4096  thrpt2   44.757
>>   ops/ms
>> ColumnFilterBenchmark.filterIntColumn   1024  thrpt2  184.099
>>   ops/ms
>> ColumnFilterBenchmark.filterIntColumn   2047  thrpt2   91.981
>>   ops/ms
>> ColumnFilterBenchmark.filterIntColumn   4096  thrpt2   45.170
>>   ops/ms
>> ColumnFilterBenchmark.filterLongColumn  1024  thrpt2  148.017
>>   ops/ms
>> ColumnFilterBenchmark.filterLongColumn  2047  thrpt2   73.516
>>   ops/ms
>> ColumnFilterBenchmark.filterLongColumn  4096  thrpt2   36.844
>>   ops/ms
>> 
>> Withopt:
>> Benchmark (size)   Mode  Cnt Score   
>> Error   Units
>> ColumnFilterBenchmark.filterDoubleColumn1024  thrpt2  2051.707   
>>ops/ms
>> ColumnFilterBenchmark.filterDoubleColumn2047  thrpt2   914.072   
>>ops/ms
>> ColumnFilterBenchmark.filterDoubleColumn4096  thrpt2   489.898   
>>ops/ms
>> ColumnFilterBenchmark.filterFloatColumn 1024  thrpt2  5324.195   
>>ops/ms
>> ColumnFilterBenchmark.filterFloatColumn 2047  thrpt2  2587.229   
>>ops/ms
>> ColumnFilterBenchmark.filterFloatColumn 4096  thrpt2  1278.665   
>>ops/ms
>> ColumnFilterBenchmark.filterIntColumn   1024  thrpt2  4149.384   
>>ops/ms
>> ColumnFilterBenchmark.filterIntColumn   2047  thrpt  ...
>
> Jatin Bhateja has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updating copyright year of modified files.

@jatin-bhateja this looks like a great improvement!
I have a few questions and requests below.

FYI, this feels very inspiring. I'm dreaming of a day where we could do this 
filtering in the auto-vectorizer directly.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5303:

> 5301: // Blend the results with zero vector using permute vector as mask, 
> its
> 5302: // non-participating lanes holds a -1 value.
> 5303: vblendvps(dst, dst, xtmp, permv, vec_enc);

would you mind adding a few more comments to explain what happens here? I would 
also really appreciate more expressive register/variable names.

I think you are basically converting the `mask` to a permutation `permv`, by a 
lookup in the table.
Then you permute the `src` and blend it with a -1 vector, so that the unused 
(high) lanes are -1.


xtmp -> min_one
rtmp -> table_index
rscratch -> table_adr

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 5307:

> 5305: assert(bt == T_LONG || bt == T_DOUBLE, "");
> 5306: vmovmskpd(rtmp, mask, vec_enc);
> 5307: shlq(rtmp, 5);

Might this need to be 6? If I understand right, then you want to have a 64bit 
stride, hence 2^6, right?
If that is correct, then this did not show in your tests, and you need a 
regression test anyway.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.hpp line 488:

> 486:   KRegister ktmp1, int vec_enc);
> 487: 
> 488: 

Remove useless empty line

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 957:

> 955:   __ align(CodeEntryAlignment);
> 956:   StubCodeMark mark(this, "StubRoutines", stub_name);
> 957:   address start = __ pc();

Could you please add some comments here why you are filling the data like this?
Presumably, you are emitting 32 bits and 64 bits respectively, right? So the 
cells have different size, correct?

test/micro/org/openjdk/bench/jdk/incubator/vector/ColumnFilterBenchmark.java 
line 76:

> 74: longinCol = new long[size];
> 75: longoutCol = new long[size];
> 76: lpivot = size / 2;

I'd be interested to see what happens if you move up or down the "density" of 
elements that you accept. W

RFR: 8322725: (tz) Update Timezone Data to 2023d

[Johny Jose]

tzdata2023d changes

-

Commit messages:
 - 8322725: (tz) Update Timezone Data to 2023d

Changes: https://git.openjdk.org/jdk/pull/17268/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17268&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322725
  Stats: 140 lines in 14 files changed: 94 ins; 15 del; 31 mod
  Patch: https://git.openjdk.org/jdk/pull/17268.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17268/head:pull/17268

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

Re: RFR: 6356745: (coll) Add PriorityQueue(Collection, Comparator) [v6]

[Valeh Hajiyev]

On Wed, 3 Jan 2024 20:36:46 GMT, jmehrens  wrote:

> > I think this ticket should focus on adding the new constructor as part of 
> > the API.
> 
> Okay. I would think the code would avoid heapify when the caller does foolish 
> things with this API such as:
> 
> ```
> SortedSet ss = filledSortedSet();
> PriorityQueue pq1 = new PriorityQueue(ss.comparator(), ss);
> 
> PriorityQueue pq = filledPriorityQueue();
> PriorityQueue pq2 = new PriorityQueue(pq.comparator(), pq);
> ```
> 
> I assume this constructor is going to be added to TreeSet, 
> PriorityBlockingQueue, and ConcurrentSkipListSet?

why do you think the code would avoid heapify? `initFromCollection` method will 
be called regardless of the type of the collection passed, which will heapify 
the queue.

regarding adding the constructor to the other types mentioned, I believe I can 
be done, probably as part of a different ticket.

-

PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-1877175607

Re: The default java.library.path on Linux does not include the library paths in the mulitarch-spec

[Glavo]

>
> I expect there are security reasons why the JDK tries to find
> the file itself in these specific paths, rather than letting the
> platform code search for it.
>

I think this should have nothing to do with security. If there is a
vulnerability in the platform code, there is nothing the JDK can do to
avoid it.

Well that is not something I would want to see implemented in hotspot.
>

This is the only way I can think of to get the JDK to behave consistently
with ld.
Maybe we should wait and see other developers who are more familiar with
this part.

I'm now sending this email to panama-dev as well.
I think this proposal is of great significance to Panama, as it will make
it easier for developers to develop wrappers for platform libraries.

Glavo

On Thu, Jan 4, 2024 at 3:12 PM David Holmes  wrote:

> On 4/01/2024 1:36 pm, Glavo wrote:
> > Hey David,
> >
> > AFAICS java.library.path (and sun.boot.library.path) are input
> > properties that can be set by the user. There are no "default" values
> > for these properties as such.
> >
> >
> > Although they can be set by the user, they have default values.
> > For Linux, the default value is set here:
> >
> >
> https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555
> <
> https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/hotspot/os/linux/os_linux.cpp#L532-L555
> >
> >
> > The default value on Linux is the LD_LIBRARY_PATH environment variable
> > and the paths I mentioned earlier.
> >
> > The library loading will ultimately rely
> > on the behaviour of dlopen, if no additional paths have been set, so
> it
> > seems to me what you really want is for dlopen to act "the same way"
> > that ld does. Note that dlopen will already check the contents of
> > /etc/ld.so.cache
> >
> >
> > System.loadLibrary looks for the library in sun.boot.library.path and
> > java.library.path and passes the full path to the library to dlopen:
> >
> >
> https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254
> <
> https://github.com/openjdk/jdk/blob/1cf9335b24639938aa64250d6862d9636f8605f8/src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java#L246-L254
> >
> >
> > Therefore, the behavior of finding native libraries has nothing to do
> > with the behavior of dlopen, only sun.boot.library.path and
> > java.library.path.
>
> I stand corrected - apologies. I expected a raw name to simply get
> passed through. I thought both LD_LIBRARY_PATH and java.library.path
> could be used to expand the set of directories where the platform code
> looks for libs, not constrain things to only those paths (plus the
> defaults). I expect there are security reasons why the JDK tries to find
> the file itself in these specific paths, rather than letting the
> platform code search for it.
>
> > This does not seem practical. On my system ld.so.conf just contains
> >
> > include ld.so.conf.d/*.conf
> >
> > so now we (hotspot) have to read the top-level file, parse it,
> interpret
> > it and then recursively read, parse and interpret more files.
> >
> >
> > Yes, this is exactly the behavior I want.
>
> Well that is not something I would want to see implemented in hotspot.
>
> Cheers,
> David
> -
>
> > Glavo
> >
> > On Tue, Jan 2, 2024 at 10:08 AM David Holmes  > > wrote:
> >
> > Hi Glavo,
> >
> > On 24/12/2023 4:18 am, Glavo wrote:
> >  > Hi,
> >  >
> >  > There are already many Linux distributions that are following the
> >  > multiarch-spec[1] and adding the following paths to the default
> > library
> >  > path list:
> >  >
> >  >   * /usr/local/lib/
> >  >   * /lib/
> >  >   * /usr/lib/
> >  >
> >  > But OpenJDK doesn't add these paths to the java.library.path by
> > default,
> >  > so System.loadLibrary(String) has annoying behavior differences
> > with ld.
> >
> > AFAICS java.library.path (and sun.boot.library.path) are input
> > properties that can be set by the user. There are no "default" values
> > for these properties as such. The library loading will ultimately
> rely
> > on the behaviour of dlopen, if no additional paths have been set, so
> it
> > seems to me what you really want is for dlopen to act "the same way"
> > that ld does. Note that dlopen will already check the contents of
> > /etc/ld.so.cache
> >
> >  > Many libraries already installed on the system cannot be found by
> >  > System.loadLibrary(String).
> >  >
> >  > I wish OpenJDK would parse the /etc/ld.so.conf to get the full
> > library
> >  > path list so it would be consistent with the behavior of ld.
> >  > Can anyone consider this suggestion?
> >
> > This does not seem practical. On my system ld.so.conf just contain

Re: RFR: 8322768: Optimize non-subword vector compress and expand APIs for AVX2 target. [v2]

[Emanuel Peter]

On Thu, 4 Jan 2024 13:09:30 GMT, Emanuel Peter  wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updating copyright year of modified files.
>
> test/micro/org/openjdk/bench/jdk/incubator/vector/ColumnFilterBenchmark.java 
> line 94:
> 
>> 92:IntVector vec = IntVector.fromArray(ispecies, intinCol, i);
>> 93:VectorMask pred = vec.compare(VectorOperators.GT, 
>> ipivot);
>> 94:vec.compress(pred).intoArray(intoutCol, j);
> 
> Could there be equivalent `expand` tests?

And what about some result verification? Or is there another test that does 
that?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17261#discussion_r1441750595

Re: RFR: 8322725: (tz) Update Timezone Data to 2023d

[Johny Jose]

On Thu, 4 Jan 2024 13:34:50 GMT, Johny Jose  wrote:

> tzdata2023d changes

@naotoj Can you please review the changes

-

PR Comment: https://git.openjdk.org/jdk/pull/17268#issuecomment-1877117106

Re: RFR: 8322846: Running with -Djdk.tracePinnedThreads set can hang [v2]

[Jaikiran Pai]

On Thu, 4 Jan 2024 12:53:07 GMT, Alan Bateman  wrote:

>> -Djdk.tracePinnedThreads is a debugging option that dates from early 
>> development in the loom repo to identify pinned threads. It has several 
>> issues and this tracing option will eventually be removed (use the JFR 
>> events instead). Several hangs have been reported when running with the 
>> system property set. The "hangs" stem from the onPinned callback executing 
>> while the virtual thread is in a transition state (typically parking). If 
>> the virtual parks while printing the stack trace then it works like a nested 
>> park where the thread state is never restored. Contention on the System.out 
>> can also lead to deadlock when there are platform and pinned virtual threads 
>> printing to System.out around the same time.
>> 
>> This PR brings over the changes from the loom repo to avoid these hangs. The 
>> changes mean the stack trace is only printed to System.out when the 
>> PrintStream lock can be acquired without blocking. It also restores the 
>> thread state after printing. An alternative to not printing traces would of 
>> course be to queue the traces so they are printed by another thread but this 
>> is just adding complexity for a debugging option that we want to go away.
>
> Alan Bateman 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:
> 
>  - Expand TracePinnedThreads.testContention test description
>  - Merge
>  - Initial commit

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17221#pullrequestreview-1804149476

Re: RFR: 8322846: Running with -Djdk.tracePinnedThreads set can hang [v2]

[Alan Bateman]

> -Djdk.tracePinnedThreads is a debugging option that dates from early 
> development in the loom repo to identify pinned threads. It has several 
> issues and this tracing option will eventually be removed (use the JFR events 
> instead). Several hangs have been reported when running with the system 
> property set. The "hangs" stem from the onPinned callback executing while the 
> virtual thread is in a transition state (typically parking). If the virtual 
> parks while printing the stack trace then it works like a nested park where 
> the thread state is never restored. Contention on the System.out can also 
> lead to deadlock when there are platform and pinned virtual threads printing 
> to System.out around the same time.
> 
> This PR brings over the changes from the loom repo to avoid these hangs. The 
> changes mean the stack trace is only printed to System.out when the 
> PrintStream lock can be acquired without blocking. It also restores the 
> thread state after printing. An alternative to not printing traces would of 
> course be to queue the traces so they are printed by another thread but this 
> is just adding complexity for a debugging option that we want to go away.

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

 - Expand TracePinnedThreads.testContention test description
 - Merge
 - Initial commit

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17221/files
  - new: https://git.openjdk.org/jdk/pull/17221/files/15993866..ab8a2bcb

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

  Stats: 2985 lines in 239 files changed: 1868 ins; 364 del; 753 mod
  Patch: https://git.openjdk.org/jdk/pull/17221.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17221/head:pull/17221

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

Integrated: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called

[Jim Laskey]

On Wed, 20 Dec 2023 20:25:07 GMT, Jim Laskey  wrote:

> The new repeat methods were not clearing the toStringCache.

This pull request has now been integrated.

Changeset: df22fb32
Author:Jim Laskey 
URL:   
https://git.openjdk.org/jdk/commit/df22fb322e6c4c9931a770bd0abf4c43b83c4e4a
Stats: 16 lines in 2 files changed: 15 ins; 0 del; 1 mod

8322512: StringBuffer.repeat does not work correctly after toString() was called

Reviewed-by: rriggs, jpai

-

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

RFR: 8323002: test/jdk/java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java times out on macosx-x64

[Alan Bateman]

This test was recently added by JDK-8322818. On some test systems, the test 
passes a bit after the timeout so the test is marked as failed. The changes 
here dial down the iterations from 100k to 25k to reduce the execution time for 
release builds. No change for debug builds as this already uses a small number 
of iterations.

-

Commit messages:
 - Initial commit

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

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

Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v2]

[Jim Laskey]

On Thu, 21 Dec 2023 07:55:50 GMT, Jaikiran Pai  wrote:

>> Jim Laskey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Clear sooner
>
> test/jdk/java/lang/StringBuilder/StringBufferRepeat.java line 138:
> 
>> 136: sb.repeat('*', 5);
>> 137: expected = "*";
>> 138: assertEquals(expected, sb.toString());
> 
> Hello Jim, just a minor detail - in case of testng's `Assert.assertEquals()` 
> the first param is the `actual` and the second is the `expected`. Some other 
> existing usages of this method in this test also have the incorrect order.

Updated

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17172#discussion_r1441707877

Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v4]

[Jim Laskey]

> The new repeat methods were not clearing the toStringCache.

Jim Laskey has updated the pull request incrementally with one additional 
commit since the last revision:

  Nit in test

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17172/files
  - new: https://git.openjdk.org/jdk/pull/17172/files/33dd7973..ec72e77c

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

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

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

Re: RFR: JDK-8322512 StringBuffer.repeat does not work correctly after toString() was called [v3]

[Jim Laskey]

> The new repeat methods were not clearing the toStringCache.

Jim Laskey 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 remote-tracking branch 'upstream/master' into 8322512
 - Clear sooner
 - Clear toStringCache when calling StringBuffer::repeat

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17172/files
  - new: https://git.openjdk.org/jdk/pull/17172/files/66eb7f71..33dd7973

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

  Stats: 3615 lines in 286 files changed: 2156 ins; 564 del; 895 mod
  Patch: https://git.openjdk.org/jdk/pull/17172.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17172/head:pull/17172

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

Re: RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing [v2]

[Aleksey Shipilev]

On Wed, 3 Jan 2024 21:29:57 GMT, Joshua Cao  wrote:

>> ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` -> 
>> `transfer()`. When coming from the copy constructor, the Map is empty, so 
>> there is nothing to transfer. But `transfer()` will still copy all the empty 
>> nodes from the old table to the new table.
>> 
>> This patch avoids this work by only calling `tryPresize()` if the table is 
>> already initialized. If `table` is null, the initialization is deferred to 
>> `putVal()`, which calls `initTable()`.
>> 
>> ---
>> 
>> ### JMH results for testCopyConstructor
>> 
>> before patch:
>> 
>> 
>> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor":
>>   937395.686 ±(99.9%) 99074.324 ns/op [Average]
>>   (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184
>>   CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution)
>> 
>> 
>> after patch:
>> 
>> 
>> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor":
>>   620871.469 ±(99.9%) 59195.406 ns/op [Average]
>>   (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419
>>   CI (99.9%): [561676.063, 680066.875] (assumes normal distribution)
>> 
>> 
>> Average time is decreased by about 33%.
>
> Joshua Cao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Cleanup benchmark

I am good this this version. I would like a second Reviewer to ack this too.

-

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1803734314

Re: RFR: JDK-8322782: Clean up usages of unnecessary fully qualified class name "java.util.Arrays" [v3]

[Matthias Baesken]

On Wed, 3 Jan 2024 13:55:22 GMT, Matthias Baesken  wrote:

>> In [JDK-8322772](https://bugs.openjdk.org/browse/JDK-8322772) one similar 
>> cleanup has been proposed before (and was done in the change). But there are 
>> a number of other places in the codebase where the import is done and still 
>> the unneeded fully qualified class name "java.util.Arrays" is used so more 
>> cleanups can be done.
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Adjust Invokers.java too

Hi Alexey, thanks for the review !

-

PR Comment: https://git.openjdk.org/jdk/pull/17241#issuecomment-1876673066

Integrated: JDK-8322782: Clean up usages of unnecessary fully qualified class name "java.util.Arrays"

[Matthias Baesken]

On Wed, 3 Jan 2024 11:41:20 GMT, Matthias Baesken  wrote:

> In [JDK-8322772](https://bugs.openjdk.org/browse/JDK-8322772) one similar 
> cleanup has been proposed before (and was done in the change). But there are 
> a number of other places in the codebase where the import is done and still 
> the unneeded fully qualified class name "java.util.Arrays" is used so more 
> cleanups can be done.

This pull request has now been integrated.

Changeset: 1369c545
Author:Matthias Baesken 
URL:   
https://git.openjdk.org/jdk/commit/1369c545ac51d7b5ff623d486e28c939869fecb8
Stats: 29 lines in 9 files changed: 0 ins; 0 del; 29 mod

8322782: Clean up usages of unnecessary fully qualified class name 
"java.util.Arrays"

Reviewed-by: alanb, aivanov

-

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