Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v2]

2024-01-22 Thread sendaoYan
> 8323640: [TESTBUG]testMemoryFailCount in 
> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
> OOM killed

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

  8323640: [TESTBUG]testMemoryFailCount in 
jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
OOM killed
  
  Signed-off-by: sendaoYan 

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17514/files
  - new: https://git.openjdk.org/jdk/pull/17514/files/be81665d..969b608d

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

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

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


Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed

2024-01-22 Thread sendaoYan
On Mon, 22 Jan 2024 15:03:18 GMT, Severin Gehwolf  wrote:

> `1k` increments for a total of `512k` times seems overkill. Are you sure 
> that's needed to make the test pass? How about `1MB` increments for a total 
> of `512` times?

When the docker serivice work normally on the test machine, this test will 
always fail. This test want to verify the API 
`jdk.internal.platform.Metrics.systemMetrics().getMemoryFailCount()` work 
normally or not. The API return memory allocate fail times in jvm. But, before 
this PR, everytime it allocate `1M` memory, the API has no chance the catch the 
memory allocate fail, the jvm was killed by OOM. Change `8M` increments to `1K` 
mean to avoid OOM killed for the jvm in docker container.

jvm was killed by OOM in docker container:

![image](https://github.com/openjdk/jdk/assets/24123821/c00697cc-ceef-410e-a8b9-7c401fa76134)


`1M` Increnents also can avoid OOM killed.

![image](https://github.com/openjdk/jdk/assets/24123821/bab0a753-d15c-4759-a557-b8feafaa97cb)

-

PR Comment: https://git.openjdk.org/jdk/pull/17514#issuecomment-1905139487


Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies

2024-01-22 Thread David Holmes
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev  wrote:

> Some jtreg tests require resolvable external dependencies. This resolution is 
> delegated to JIB, which is not used in vanilla OpenJDK testing. It would be 
> convenient to add a keyword that marks tests that require these external 
> dependencies, so that we could exclude those tests from runs. This would 
> allow us to: a) run all tests in hotspot:tier4, which now excludes 
> `applications/` specifically; b) make all tests runs (#17422) cleaner on many 
> environments.
> 
> I provisionally call this flag `external-dep`, but I am open for other 
> suggestions.
> 
> Note that some tests that pull `@Artifact`-s provide special paths that do 
> limited testing anyway. However, there are tests which cannot run without 
> external dependencies at all. These include at least `applications/jcstress` 
> and `applications/scimark` tests.
> 
> Ironically, I cannot run the jcstress test generator because the dependencies 
> are lacking here. I regenerated those test using a self-built jcstress 0.16 
> bundle.
> 
> Additional testing:
>  - [x] `make test TEST=applications/` fails
>  - [x]  `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, 
> skipping most of the tests

Seems quite reasonable.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17421#pullrequestreview-1837713758


Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]

2024-01-22 Thread Lance Andersen
On Mon, 22 Jan 2024 14:34:25 GMT, Eirik Bjørsnøs  wrote:

> @LanceAndersen Do you have a cent or two to spare?

Let me try and dig out from a couple of things and circle back to this again

-

PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1462519461


Re: RFR: JDK-8321545: Override toString() for Format subclasses [v4]

2024-01-22 Thread Justin Lu
On Fri, 12 Jan 2024 21:30:05 GMT, Roger Riggs  wrote:

>> Justin Lu has updated the pull request with a new target base due to a merge 
>> or a rebase. The incremental webrev excludes the unrelated changes brought 
>> in by the merge/rebase. The pull request contains eight additional commits 
>> since the last revision:
>> 
>>  - replace 'None' with 'null' for applicable classes
>>  - Merge branch 'master' into JDK-8321545-toString-j.text.Format
>>  - swap placement of decimal pattern and compact patterns. Expand on tests
>>  - add unit tests
>>  - Merge branch 'master' into JDK-8321545-toString-j.text.Format
>>  - account for null locale for SDF through deserialization
>>  - Merge branch 'master' into JDK-8321545-toString-j.text.Format
>>  - init
>
> src/java.base/share/classes/java/text/MessageFormat.java line 1195:
> 
>> 1193: """
>> 1194: MessageFormat [locale: "%s", pattern: "%s"]
>> 1195: """.formatted(locale == null ? "null" : 
>> locale.getDisplayName(), toPattern());
> 
> It would be more accurate if when locale ==null that null was not quoted in 
> the string.
> Seeing "null" would imply that the displayName of the locale was "null", when 
> it was `null`.

Hi Roger, addressed in 
https://github.com/openjdk/jdk/pull/17355/commits/70e0a175037ccd0215d76fb2fbfa8c91de291d41;
 would like to confirm the update is okay with you before integration.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17355#discussion_r1462516181


Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v3]

2024-01-22 Thread Joshua Cao
On Mon, 22 Jan 2024 17:45:45 GMT, Volker Simonis  wrote:

>> Joshua Cao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   putAll presize based on sum on both map sizes
>
> test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 122:
> 
>> 120: @Benchmark
>> 121: public ConcurrentHashMap 
>> testConcurrentHashMapPutAll() {
>> 122: ConcurrentHashMap map = new 
>> ConcurrentHashMap<>();
> 
> I think this benchmark could be made more accurate by creating the new, 
> temporary map with the right initial size (i.e. `ConcurrentHashMap<>(nkeys)`) 
> to avoid calls to `tryPresize()` in this setup step.

I updated. The numbers are surprisingly the same. I guess the benchmark compute 
time is dominated by putAll().

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1462481286


Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v4]

2024-01-22 Thread Joshua Cao
> 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%.
> 
> ### JMH results for testPutAll (size = 1)
> 
> before patch:
> 
> 
> Result 
> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll":
>   4315291.542 ±(99.9%) 336034.190 ns/op [Average]
>   (min, avg, max) = (3974688.194, 4315291.542, 4871772.209), stdev = 
> 314326.589
>   CI (99.9%): [3979257.352, 4651325.731] (assumes normal distribution)
> 
> 
> after patch:
> 
> 
> Result 
> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll":
>   3006955.723 ±(99.9%) 271757.969 ns/op [Average]
>   (min, avg, max) = (2801264.198, 3006955.723, 3553084.135), stdev = 
> 254202.573
>   CI (99.9%): [2735197.754, 3278713.692] (assumes normal distribution)
> 
> 
> Average time is decreased about 30%.

Joshua Cao has updated the pull request incrementally with one additional 
commit since the last revision:

  pass initial size to putAll benchmark map construction

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17116/files
  - new: https://git.openjdk.org/jdk/pull/17116/files/3efa593d..2a965fa9

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

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

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


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v4]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 21:26:39 GMT, Brian Burkhalter  wrote:

>> @bplb It is not used in jtreg testing.
>
> So it is only for verification purposes in the context of this PR?

@bplb Yes, it is.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17291#discussion_r1462444386


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v4]

2024-01-22 Thread Brian Burkhalter
On Mon, 22 Jan 2024 21:21:53 GMT, fabioromano1  wrote:

>> test/jdk/java/math/BigInteger/TestDivWord.java line 5:
>> 
>>> 3: import java.util.Random;
>>> 4: 
>>> 5: public class TestDivWord {
>> 
>> Where is this used in actually jtreg testing?
>
> @bplb It is not used in jtreg testing.

So it is only for verification purposes in the context of this PR?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17291#discussion_r1462441622


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v4]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 20:02:34 GMT, Brian Burkhalter  wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update TestDivWord.java
>>   
>>   Added method to check results of divWord
>
> test/jdk/java/math/BigInteger/TestDivWord.java line 5:
> 
>> 3: import java.util.Random;
>> 4: 
>> 5: public class TestDivWord {
> 
> Where is this used in actually jtreg testing?

@bplb It is not used in jtreg testing.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17291#discussion_r1462437309


Re: RFR: 8320759: Creation of random BigIntegers can be made faster [v3]

2024-01-22 Thread Brian Burkhalter
On Sat, 2 Dec 2023 15:42:18 GMT, fabioromano1  wrote:

>> A faster and simpler way to generate random BigIntegers, avoiding eventually 
>> trimming of leading zeros in magnitude array.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Create RandomBigIntegers.java
>   
>   Create a benchmark to measure the performance of BigInteger(int, Random) 
> constructor implementation.

Will more work be done on this PR?

-

PR Comment: https://git.openjdk.org/jdk/pull/16817#issuecomment-1904812784


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v4]

2024-01-22 Thread Brian Burkhalter
On Mon, 22 Jan 2024 18:52:43 GMT, fabioromano1  wrote:

>> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
>> Hacker's Delight (2nd ed), section 9.3, the same used in 
>> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
>> to get the computation faster.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update TestDivWord.java
>   
>   Added method to check results of divWord

test/jdk/java/math/BigInteger/TestDivWord.java line 5:

> 3: import java.util.Random;
> 4: 
> 5: public class TestDivWord {

Where is this used in actually jtreg testing?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17291#discussion_r1462355967


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

2024-01-22 Thread Sandhya Viswanathan
On Sat, 20 Jan 2024 09:55:45 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:
> 
>   Review comments resolution

Nice work! The patch looks good to me now.

-

Marked as reviewed by sviswanathan (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17261#pullrequestreview-1837250879


Re: Gatherer: spliterator characteristics are not propagated

2024-01-22 Thread forax
> From: "Viktor Klang" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" , "Paul Sandoz"
> 
> Sent: Monday, January 22, 2024 4:22:11 PM
> Subject: Re: Gatherer: spliterator characteristics are not propagated

> Hi Rémi,

Hello, 

> For instance, stateless is neither recessive nor dominant, since the 
> composition
> of two stateless operations is only ever stateless if they both are greedy as
> well: [
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java#L588
> |
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java#L588
> ]

Okay, so choosing SEQUENTIAL vs PARALELLIZABLE is not that important given that 
the combination is ad-hoc, reflecting the characterristics is enough. 

> So even if making it represented as ints (more like Spliterator, rather than
> Collector) makes things faster, it's still both work to track, propagate, and
> also becomes a side-channel that needs to remain in sync with the actual
> implementation of the logic.

The flags are in sync with the implementation because the only way to create a 
Gatherer if through the factory methods and those factory methods (and only 
them) compute the proper combination of SEQUENTIAL | STATELESS | GREEDY. A user 
should not be able to set those flags. Only the flags KEEP_* are settable by a 
user. 

> One could argue that logic such as: someCollection.stream().map(…).count() is 
> a
> performance bug/inefficiency in an of itself as it would be faster to do
> someCollection.size().

The stream implementation has a whole mechanism in place to propagate/preverse 
flags like SIZED, DISTINCT or SORTED. For me, discussing about the merit of 
this mechanism seems a little off topic. I would prefer the Gatherer to be a 
good citizen and works seemlessly with the other intermediary operations. 

> Cheers,
> √

regards, 
Rémi 

> Viktor Klang
> Software Architect, Java Platform Group
> Oracle

> From: fo...@univ-mlv.fr 
> Sent: Saturday, 20 January 2024 17:40
> To: Viktor Klang 
> Cc: core-libs-dev ; Paul Sandoz
> 
> Subject: [External] : Re: Gatherer: spliterator characteristics are not
> propagated

>> From: "Viktor Klang" 
>> To: "Remi Forax" 
>> Cc: "core-libs-dev" , "Paul Sandoz"
>> 
>> Sent: Thursday, January 18, 2024 5:14:38 PM
>> Subject: Re: Gatherer: spliterator characteristics are not propagated

>>> And for A.andThen(B), A.flags & B.flags should work, the stream is sorted if
>>> both gatherers keep it sorted.

>> That is unfortunately not the case. That would presume that you can implement
>> the composition such that it can preserve all the common flags. Some flags
>> could be "dominant" and some "recessive" to use genetics nomenclature.

> Some flags of the stream pipeline are "recessive", mainly SHORT_CIRCUIT, but 
> not
> the characteristics of the Gatherer which can have the corresponding 
> "dominant"
> flag, GREEDY, in this case.
> And the same for sequential, the flag should be PARALELIZABLE and not
> SEQUENTIAL.

> The idea is that the Gatherer characteristics can have the same bit set at the
> same position as the stream op flags (as defined by StreamOpFlag).
> So KEEP_DISTINCT is in position 0, KEEP_SORTED in in position 2 and KEEP_SIZED
> is in position 3.
> For GREEDY, we use the same position as SHORT_CIRCUIT and we will flip the bit
> (using XOR) when we want to extract the stream op flags from the
> characteristics

> All the factory methods call the generic of() with a combination of
> PARALELIZABLE and STATELESS and the user can adds the characteristics GREEDY,
> KEEP_DISTINCT, KEEP_SORTED and KEEP_SIZED (otherwise an exception should be
> raised).

> In StreamOpFlag, there are two unused positions (14 and 15), that's perfect 
> for
> our two new states PARALELIZABLE and STATELESS, so no problem here 
> (technically
> we can also reuse positions of the Spliterator characteristic given that those
> flags are masked before being sent to the GathererOp super constructor).

> The way to transform a Gatherer characteristics op to a stream flags op is to
> flip the bits corresponding to SHORT_CIRCUIT, add the highter bit of all flags
> but SHORT-CIRCUIT (because stream op flags are encoded using 2 bits) and mask
> to only retain SHORT_CIRCUIT, KEEP_DISTINCT, KEEP_SORTED and KEEP_SIZED.

> public static int toOpFlags ( int characteristics ) {
> return (( characteristics ^ SHORT_CIRCUIT_MASK ) | HIGHER_BITS ) &
> STREAM_OP_MASK ;
> }

> see below for a full script.

>>> I suppose that if those flags already exist, it's because they have a 
>>> purpose
>>> and i do not understand how it can make the other operations slower.

>> Extra invocations, extra storage, and extra composition overhead is not free.
>> Since Stream is one-shot you need to include the construction cost with the
>> execution cost. For something like an empty Stream construction cost scan be
>> 90+% of the total costs.

> If you create a Gatherer, the characteristics is a con

Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 18:07:49 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed trailing whitespaces
>
> As you note, that would probably require two divisions. I don't know if the 
> JIT compiler can detect that the arguments are the same and emit just one 
> division instead.
> I think your code is good enough for the purpose of [Mutable]BigInteger, and 
> better than the current one.

@rgiulietti I've added a method to check the results of tests.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904607454


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v4]

2024-01-22 Thread fabioromano1
> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
> Hacker's Delight (2nd ed), section 9.3, the same used in 
> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
> to get the computation faster.

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

  Update TestDivWord.java
  
  Added method to check results of divWord

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17291/files
  - new: https://git.openjdk.org/jdk/pull/17291/files/2460b066..67e3027f

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

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

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


Re: RFR: 8323835: Updating ASM to 9.6 for JDK 23

2024-01-22 Thread Mandy Chung
On Tue, 16 Jan 2024 21:18:55 GMT, Vicente Romero  wrote:

> Updating ASM to version 9.6,
> 
> Thanks in advance for the reviews,
> Vicente

The change looks okay to me.   Most of the changes are doc change.  I see many 
files are updated just to remove the empty line at the end of the file - is 
that intentional, imported from this upgrade? 

Any need to update the copyright header?   The new file CheckFrameAnalyzer.java 
still has the same copyright header years as other files.  I assume no need 
then?

-

PR Comment: https://git.openjdk.org/jdk/pull/17453#issuecomment-1904588248


Re: RFR: 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview) [v41]

2024-01-22 Thread Aggelos Biboudis
> This is the proposed patch for Primitive types in patterns, instanceof, and 
> switch (Preview).
> 
> Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/

Aggelos Biboudis has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 60 commits:

 - Improve Javadoc of ExactConversionsSupport
 - Merge branch 'master' into primitive-patterns
 - Update ExactConversionsSupport Javadoc and JDK version
 - Revert "Update copyright year, javadoc, JDK version"
   
   This reverts commit 676af9de90d946f64f34762a6df94dbd91bce41b.
 - Update copyright year, javadoc, JDK version
 - Merge branch 'master' into primitive-patterns
 - Merge branch 'master' into primitive-patterns
 - Cleanup
 - Merge branch 'master' into primitive-patterns
 - Remove trailing spaces
 - ... and 50 more: https://git.openjdk.org/jdk/compare/c9cacfb2...50cb9832

-

Changes: https://git.openjdk.org/jdk/pull/15638/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=40
  Stats: 3272 lines in 41 files changed: 3008 ins; 110 del; 154 mod
  Patch: https://git.openjdk.org/jdk/pull/15638.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15638/head:pull/15638

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


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 18:07:49 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed trailing whitespaces
>
> As you note, that would probably require two divisions. I don't know if the 
> JIT compiler can detect that the arguments are the same and emit just one 
> division instead.
> I think your code is good enough for the purpose of [Mutable]BigInteger, and 
> better than the current one.

@rgiulietti It seems unlikely that the JIT compiler can do that, at least to me.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904544724


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread Raffaello Giulietti
On Tue, 9 Jan 2024 18:24:49 GMT, fabioromano1  wrote:

>> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
>> Hacker's Delight (2nd ed), section 9.3, the same used in 
>> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
>> to get the computation faster.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed trailing whitespaces

As you note, that would probably require two divisions. I don't know if the JIT 
compiler can detect that the arguments are the same and emit just one division 
instead.
I think your code is good enough for the purpose of [Mutable]BigInteger, and 
better than the current one.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904538746


Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v3]

2024-01-22 Thread Volker Simonis
On Wed, 17 Jan 2024 21:16:02 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%.
>> 
>> ### JMH results for testPutAll (size = 1)
>> 
>> before patch:
>> 
>> 
>> Result 
>> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll":
>>   4315291.542 ±(99.9%) 336034.190 ns/op [Average]
>>   (min, avg, max) = (3974688.194, 4315291.542, 4871772.209), stdev = 
>> 314326.589
>>   CI (99.9%): [3979257.352, 4651325.731] (assumes normal distribution)
>> 
>> 
>> after patch:
>> 
>> 
>> Result 
>> "org.openjdk.bench.java.util.concurrent.Maps.testConcurrentHashMapPutAll":
>>   3006955.723 ±(99.9%) 271757.969 ns/op [Average]
>>   (min, avg, max) = (2801264.198, 3006955.723, 3553084.135), stdev = 
>> 254202.573
>>   CI (99.9%): [2735197.754, 3278713.692] (assumes normal distribution)
>> 
>> 
>> Average time is decreased about 30%.
>
> Joshua Cao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   putAll presize based on sum on both map sizes

Thanks for the latest changes. The patch looks good now, except that I think 
the benchmark could be made more accurate (see inline comments). This will 
probably result in an even higher improvement for `putAll()`.

test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 122:

> 120: @Benchmark
> 121: public ConcurrentHashMap 
> testConcurrentHashMapPutAll() {
> 122: ConcurrentHashMap map = new 
> ConcurrentHashMap<>();

I think this benchmark could be made more accurate by creating the new, 
temporary map with the right initial size (i.e. `ConcurrentHashMap<>(nkeys)`) 
to avoid calls to `tryPresize()` in this setup step.

-

Changes requested by simonis (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1837027167
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1462205109


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 17:23:22 GMT, Raffaello Giulietti  
wrote:

> AFAIK, IntrinsicCandidate methods are only relevant in JIT compiled code. A 
> test that checks correctness might not reach the compilation stage, and 
> execute only in the bytecode interpreter.
> 
> But IMO using the result of `divWord()` as suggested is simple enough and 
> avoids hidden dependencies.

@rgiulietti Perhaps I was not clear enough. I'm not intended to use 
`Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)` in 
the tests, but in the algorithm implementation, instead of using an hard-coded 
implementation, to use directly the assembly code, although this implies to 
calculate the approximations of `q` and `r` two times instead of one.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904488957


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread Raffaello Giulietti
On Tue, 9 Jan 2024 18:24:49 GMT, fabioromano1  wrote:

>> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
>> Hacker's Delight (2nd ed), section 9.3, the same used in 
>> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
>> to get the computation faster.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed trailing whitespaces

AFAIK, IntrinsicCandidate methods are only relevant in JIT compiled code. A 
test that checks correctness might not reach the compilation stage, and execute 
only in the bytecode interpreter.

But IMO using the result of `divWord()` as suggested is simple enough and 
avoids hidden dependencies.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904466691


Integrated: 8324065: Daylight saving information for `Africa/Casablanca` are incorrect

2024-01-22 Thread Naoto Sato
On Thu, 18 Jan 2024 19:37:33 GMT, Naoto Sato  wrote:

> Fixing incorrect std/dst transitions for time zones that have rules beyond 
> 2037. The year 2037 restriction seems to come from the old `zic` command 
> implementation and thus can be expanded in the JDK. Arbitrary I chose 2100 
> which is greater than the year 2087 which is the farthest rule at the moment.

This pull request has now been integrated.

Changeset: 0d8543d6
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/0d8543d6773a516dad54038070dce507179d0709
Stats: 44 lines in 6 files changed: 5 ins; 7 del; 32 mod

8324065: Daylight saving information for `Africa/Casablanca` are incorrect

Reviewed-by: iris, joehw, jlu

-

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


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 16:15:57 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed trailing whitespaces
>
> What about splitting the result of `divWord()` into its components `q` and 
> `r` and check that `0 <= r < d` and `n == q * d + r`? Mathematically this 
> should be sufficient. Perhaps you might need to take care about unsigned vs 
> signed arithmetic.

@rgiulietti In this case, the unsigned arithmetic is not a problem, since `(d & 
LONG_MASK) > 0` and multiplication and addition semantics is equivalent in 
signed/unsigned integers. Perhaps, it would be better to call directly 
`Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)` to 
possibly use directly assembly code, since they are annotated as 
IntrinsicCandidate.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904381418


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread Raffaello Giulietti
On Tue, 9 Jan 2024 18:24:49 GMT, fabioromano1  wrote:

>> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
>> Hacker's Delight (2nd ed), section 9.3, the same used in 
>> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
>> to get the computation faster.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed trailing whitespaces

What about splitting the result of `divWord()` into its components `q` and `r` 
and check that `0 <= r < d` and `n == q * d + r`? Mathematically this should be 
sufficient. Perhaps you might need to take care about unsigned vs signed 
arithmetic.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904341649


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 15:38:08 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed trailing whitespaces
>
> Sure. But the purpose of a test is not much to check the _current_ code but 
> to protect against erroneous changes in the future. Without a test, a later 
> modification might introduce an undetected regression.

@rgiulietti By the way, the tests for correctness of `Long.divideUnsigned(long, 
long)` and `Long.remainderUnsigned(long, long)` simply check the consistency 
between their results and those of `BigInteger.divide(BigInteger)`, so those 
tests are implicitly tests also for `MutablebigInteger.divWord(long, int)`.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904311866


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 15:38:08 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed trailing whitespaces
>
> Sure. But the purpose of a test is not much to check the _current_ code but 
> to protect against erroneous changes in the future. Without a test, a later 
> modification might introduce an undetected regression.

@rgiulietti The method `MutablebigInteger.divWord(long, int)` is used for 
division of BigIntegers, and division of BigIntegers already has several tests. 
Anyway, in case we can use the same tests which already exist for  
`Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904279676


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread Raffaello Giulietti
On Tue, 9 Jan 2024 18:24:49 GMT, fabioromano1  wrote:

>> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
>> Hacker's Delight (2nd ed), section 9.3, the same used in 
>> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
>> to get the computation faster.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed trailing whitespaces

Sure. But the purpose of a test is not much to check the _current_ code but to 
protect against erroneous changes in the future. Without a test, a later 
modification might introduce an undetected regression.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904265345


Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread fabioromano1
On Mon, 22 Jan 2024 15:14:58 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed trailing whitespaces
>
> The test does not seem to check that the results are correct, but just 
> measures elapsed time.
> 
> For this, in the context of the OpenJDK, we use 
> [JMH](https://github.com/openjdk/jmh) for performance benchmarks. It avoids 
> all the common issues with less sophisticated approaches. At first, it might 
> seem harder to use, but it is more reliable than ad-hoc measurements. While 
> not perfect, it has established itself as the "golden standard", so to say.

@rgiulietti The correctness of the algorithm simply follows by the fact that it 
is just a particularization of `Long.divideUnsigned(long, long)` and 
`Long.remainderUnsigned(long, long)`.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904256641


Re: Gatherer: spliterator characteristics are not propagated

2024-01-22 Thread Viktor Klang
Hi Rémi,

For instance, stateless is neither recessive nor dominant, since the 
composition of two stateless operations is only ever stateless if they both are 
greedy as well: 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java#L588

So even if making it represented as ints (more like Spliterator, rather than 
Collector) makes things faster, it's still both work to track, propagate, and 
also becomes a side-channel that needs to remain in sync with the actual 
implementation of the logic.

One could argue that logic such as: someCollection.stream().map(…).count() is a 
performance bug/inefficiency in an of itself as it would be faster to do 
someCollection.size().

Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle

From: fo...@univ-mlv.fr 
Sent: Saturday, 20 January 2024 17:40
To: Viktor Klang 
Cc: core-libs-dev ; Paul Sandoz 

Subject: [External] : Re: Gatherer: spliterator characteristics are not 
propagated




From: "Viktor Klang" 
To: "Remi Forax" 
Cc: "core-libs-dev" , "Paul Sandoz" 

Sent: Thursday, January 18, 2024 5:14:38 PM
Subject: Re: Gatherer: spliterator characteristics are not propagated

And for A.andThen(B), A.flags & B.flags should work, the stream is sorted if 
both gatherers keep it sorted.
That is unfortunately not the case. That would presume that you can implement 
the composition such that it can preserve all the common flags. Some flags 
could be "dominant" and some "recessive" to use genetics nomenclature.

Some flags of the stream pipeline are "recessive", mainly SHORT_CIRCUIT, but 
not the characteristics of the Gatherer which can have the corresponding 
"dominant" flag, GREEDY, in this case.
And the same for sequential, the flag should be PARALELIZABLE and not 
SEQUENTIAL.

The idea is that the Gatherer characteristics can have the same bit set at the 
same position as the stream op flags (as defined by StreamOpFlag).
So KEEP_DISTINCT is in position 0, KEEP_SORTED in in position 2 and KEEP_SIZED 
is in position 3.
For GREEDY, we use the same position as SHORT_CIRCUIT and we will flip the bit 
(using XOR) when we want to extract the stream op flags from the characteristics

All the factory methods call the generic of() with a combination of 
PARALELIZABLE and STATELESS and the user can adds the characteristics GREEDY, 
KEEP_DISTINCT, KEEP_SORTED and KEEP_SIZED (otherwise an exception should be 
raised).

In StreamOpFlag, there are two unused positions (14 and 15), that's perfect for 
our two new states PARALELIZABLE and STATELESS, so no problem here (technically 
we can also reuse positions of the Spliterator characteristic given that those 
flags are masked before being sent to the GathererOp super constructor).

The way to transform a Gatherer characteristics op to a stream flags op is to 
flip the bits corresponding to SHORT_CIRCUIT, add the highter bit of all flags 
but SHORT-CIRCUIT (because stream op flags are encoded using 2 bits) and mask 
to only retain SHORT_CIRCUIT, KEEP_DISTINCT, KEEP_SORTED and KEEP_SIZED.


public static int toOpFlags(int characteristics) {
  return  ((characteristics ^ SHORT_CIRCUIT_MASK) | HIGHER_BITS) & 
STREAM_OP_MASK;
}

see below for a full script.



I suppose that if those flags already exist, it's because they have a purpose 
and i do not understand how it can make the other operations slower.

Extra invocations, extra storage, and extra composition overhead is not free. 
Since Stream is one-shot you need to include the construction cost with the 
execution cost. For something like an empty Stream construction cost scan be 
90+% of the total costs.

If you create a Gatherer, the characteristics is a constant (so the validity 
check is removed, it's just a mask and a test) so the result of calling 
toOpFlags() is a constant too.

If the factory method is not inlined, the cost is 3 bitwise operations which is 
I believe faster than the "instanceof Greedy" used in the current code.


Cheers,
√

regards,
Rémi

---

public class GathererFlag {
  // cut and paste from StreamOpFlag
  /**
   * The bit pattern for setting/injecting a flag.
   */
  private static final int SET_BITS = 0b01;

  /**
   * The bit pattern for clearing a flag.
   */
  private static final int CLEAR_BITS = 0b10;

  /**
   * The bit pattern for preserving a flag.
   */
  private static final int PRESERVE_BITS = 0b11;


  private static int position(int opFlagSet) {
return Integer.numberOfTrailingZeros(opFlagSet) >> 1;
  }

  private static final int DISTINCT_POSITION = 
position(StreamOpFlag.IS_DISTINCT);
  private static final int SORTED_POSITION = position(StreamOpFlag.IS_SORTED);
  private static final int SIZED_POSITION = position(StreamOpFlag.IS_SIZED);

  private static final int SHORT_CIRCUIT_POSITION = 
position(StreamOpFlag.IS_SHORT_CIRCUIT);
  private static final int STATELESS_POSITION = 14;
  private static final int PARE

Re: RFR: JDK-8323186: A faster algorithm for MutablebigInteger.divWord(long, int) [v3]

2024-01-22 Thread Raffaello Giulietti
On Tue, 9 Jan 2024 18:24:49 GMT, fabioromano1  wrote:

>> The method `MutableBigInteger.divWord(long, int)` can use the algorithm of 
>> Hacker's Delight (2nd ed), section 9.3, the same used in 
>> `Long.divideUnsigned(long, long)` and `Long.remainderUnsigned(long, long)`, 
>> to get the computation faster.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed trailing whitespaces

The test does not seem to check that the results are correct, but just measures 
elapsed time.

For this, in the context of the OpenJDK, we use 
[JMH](https://github.com/openjdk/jmh) for performance benchmarks. It avoids all 
the common issues with less sophisticated approaches. At first, it might seem 
harder to use, but it is more reliable than ad-hoc measurements. While not 
perfect, it has established itself as the "golden standard", so to say.

-

PR Comment: https://git.openjdk.org/jdk/pull/17291#issuecomment-1904217509


Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed

2024-01-22 Thread Severin Gehwolf
On Mon, 22 Jan 2024 09:31:43 GMT, sendaoYan  wrote:

> 8323640: [TESTBUG]testMemoryFailCount in 
> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
> OOM killed

`1k` increments for a total of `512k` times seems overkill. Are you sure that's 
needed to make the test pass? How about `1MB` increments for a total of `512` 
times?

-

PR Review: https://git.openjdk.org/jdk/pull/17514#pullrequestreview-1836685784


Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]

2024-01-22 Thread Eirik Bjørsnøs
On Tue, 16 Jan 2024 14:55:39 GMT, Jaikiran Pai  wrote:

> I think the only role that a zip64 block should play when we are deciding how 
> to parse a data descriptor is whether or not the entry has zip64 extra field 
> set (the header id value of the extra field). Does that sound reasonable?

Are you suggesting that we ONLY look for the presence of a Zip64 (tag 0x1) 
extended field? Meaning we should ignore the following:

- The values of the 'compressed size' and 'uncompressed size' fields in the LOC 
header
- The contents/fields in the data block in the Zip64 header

The purpose of this check is to determine wheter the entry "is in the ZIP64 
format", to use parlance from `APPNOTE.TXT`. The reason stronger validation was 
added was to avoid false positives, meaning an entry would be interpreted as 
"in the Zip64 format", even when that was not the intention of the producer. 
Some of this validation was on my initiative, but I think maybe some was in 
response to concerns raised in review.

If we ignore the 'uncompressed size' and 'compresssed size' fields, then if a 
LOC has both of these set to zero and has a Zip64 extended field, then we'll 
intepret this as "in the Zip64 format", even though no field is marked using 
the Zip64 magic value `0x`.

I think I would be fine with dropping inspection of the Zip64 contents (as 
there are probably efforts underway to add validation of LOC Zip64 fields 
similar to the recently added CEN validation).

But I'm a bit worried that ignoring the LOC values will make us parse some data 
descriptors using 8 bytes, when the producer did in fact not intend to use the 
Zip64 format. (The Zip64 field just came through some kind of "pollution" or 
unintended copying.

A natural way to implement Zip64 parsing is to only look at the extended field 
if at least one of the relevant LOC headers are `0x`, otherwise one can 
short-circuit and assume there is no Zip64 field.

@LanceAndersen Do you have a cent or two to spare?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1461951695


Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v4]

2024-01-22 Thread Eirik Bjørsnøs
On Tue, 16 Jan 2024 18:18:38 GMT, Lance Andersen  wrote:

> This comment could use a bit of wordsmithing to indicate what "correct" means

It's hard to write good prose for these tricky error scenarios. But just saying 
"correct" without defining it is a bit too lazy, yes :-)

Please take a look at the last commit 39a14de where I've expanded "correct" in 
the test comments.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1461905312


Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v5]

2024-01-22 Thread Eirik Bjørsnøs
> Please consider this PR which makes `DeflaterOutputStream.close()` always 
> close its wrapped output stream exactly once.
> 
> Currently, closing of the wrapped output stream happens outside the finally 
> block where `finish()` is called. If `finish()` throws, this means the 
> wrapped stream will not be closed. This can potentially lead to leaking 
> resources such as file descriptors or sockets.
> 
> This fix is to move the closing of the wrapped stream inside the finally 
> block.
> 
> Additionally, the `closed = true;` statement is moved to the start of the 
> close method. This makes sure we only ever close the wrapped stream once 
> (this aligns with the overridden method `FilterOutputStream.close´)
> 
> Specification: This change brings the implementation of 
> `DeflaterOutputStream.close()` in line with its specification:  *Writes 
> remaining compressed data to the output stream and closes the underlying 
> stream.*
> 
> Risk: This is a behavioural change. There is a small risk that existing code 
> depends on the close method not following its specification.
> 
> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which 
> simulates the failure condition and verifies that the wrapped stream was 
> closed under failing and non-failing conditions.

Eirik Bjørsnøs has updated the pull request incrementally with one additional 
commit since the last revision:

  Spell out what is checked by each test instead of using the word "correct"

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17209/files
  - new: https://git.openjdk.org/jdk/pull/17209/files/96deca07..39a14de3

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

  Stats: 40 lines in 1 file changed: 33 ins; 1 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/17209.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17209/head:pull/17209

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


Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v3]

2024-01-22 Thread Jan Kratochvil
> The testcase requires root permissions.

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

 - Merge branch 'master' into master-cgroup
 - Fix gtest testcases compilation errors
 - 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not 
detected
 - 4c556a78: 

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17198/files
  - new: https://git.openjdk.org/jdk/pull/17198/files/395ef61f..ae58f7e3

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

  Stats: 44851 lines in 1274 files changed: 27136 ins; 11937 del; 5778 mod
  Patch: https://git.openjdk.org/jdk/pull/17198.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17198/head:pull/17198

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


Re: RFR: 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview) [v40]

2024-01-22 Thread Aggelos Biboudis
> This is the proposed patch for Primitive types in patterns, instanceof, and 
> switch (Preview).
> 
> Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/

Aggelos Biboudis has updated the pull request incrementally with two additional 
commits since the last revision:

 - Update ExactConversionsSupport Javadoc and JDK version
 - Revert "Update copyright year, javadoc, JDK version"
   
   This reverts commit 676af9de90d946f64f34762a6df94dbd91bce41b.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/15638/files
  - new: https://git.openjdk.org/jdk/pull/15638/files/676af9de..ed61d5fd

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=39
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=38-39

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

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


Re: RFR: 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview) [v39]

2024-01-22 Thread Aggelos Biboudis
> This is the proposed patch for Primitive types in patterns, instanceof, and 
> switch (Preview).
> 
> Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/

Aggelos Biboudis has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 56 commits:

 - Update copyright year, javadoc, JDK version
 - Merge branch 'master' into primitive-patterns
 - Merge branch 'master' into primitive-patterns
 - Cleanup
 - Merge branch 'master' into primitive-patterns
 - Remove trailing spaces
 - Merge branch 'primitive-patterns-and-generating-dispatch' into 
primitive-patterns
 - Fixed switch in the cases of unboxing and widening
 - Merge branch 'JDK-8319220' into primitive-patterns
 - Merge branch 'master' into JDK-8319220
 - ... and 46 more: https://git.openjdk.org/jdk/compare/9049402a...676af9de

-

Changes: https://git.openjdk.org/jdk/pull/15638/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=38
  Stats: 3281 lines in 41 files changed: 3000 ins; 110 del; 171 mod
  Patch: https://git.openjdk.org/jdk/pull/15638.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15638/head:pull/15638

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


Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed

2024-01-22 Thread sendaoYan
On Mon, 22 Jan 2024 09:31:43 GMT, sendaoYan  wrote:

> 8323640: [TESTBUG]testMemoryFailCount in 
> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
> OOM killed

The test case before this PR has a maximum heap of 64MB and applies for 8M of 
memory each time in the for loop. When applying for memory for the sixth time, 
it was killed by the docker container because of OOM, 
jdk.internal.platform.Metrics.systemMetrics().getMemoryFailCount( ) interface 
has no chance to return 1, and the Java process returns exit code 137. The 
maximum heap is also 64M, The PR is changed to 1KB each time to ensure that the 
getMemoryFailCount() interface has a chance to return 1 and the test case has a 
chance to exit the for loop of memory allocation.

## test result before this PR:

![image](https://github.com/openjdk/jdk/assets/24123821/4554dd00-6da5-4529-907a-45e2df5c902b)


## test result after this PR:

![image](https://github.com/openjdk/jdk/assets/24123821/32ea4fc8-aa04-425e-8481-a920265d2b1f)

-

PR Comment: https://git.openjdk.org/jdk/pull/17514#issuecomment-1903589872


RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed

2024-01-22 Thread sendaoYan
8323640: [TESTBUG]testMemoryFailCount in 
jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
OOM killed

-

Commit messages:
 - 8323640: [TESTBUG]testMemoryFailCount in 
jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
OOM killed

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

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


Withdrawn: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed

2024-01-22 Thread sendaoYan
On Fri, 12 Jan 2024 03:31:37 GMT, sendaoYan  wrote:

> 8323640: [TESTBUG]testMemoryFailCount in 
> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
> OOM killed

This pull request has been closed without being integrated.

-

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


Re: Does anyone have context on jdk.httpserver?

2024-01-22 Thread Alan Bateman

On 22/01/2024 08:43, Robert Engels wrote:

See github.com/robaho/httpserver  for a more capable fork of the JDK code.

Would love to create a PR to move the core changes back into the JDK but the 
net-dev folks don’t seem to be interested


As I recall, it wasn't really possible to do any assessment because the 
"contribution" wasn't covered by the OCA. In addition, I think you said 
in one of the mails that it includes code from another project (for 
websocket or HTTP upgrade?) and it's a big deal to have to import and 
work through the license issues with 3rd party code.


At a high level it seems reasonable to update the HTTP server 
implementation to work better with virtual threads but updating it to be 
a more fully featured HTTP server goes beyond what this HTTP server 
was/is intended for.


-Alan



Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v2]

2024-01-22 Thread sendaoYan
> 8323640: [TESTBUG]testMemoryFailCount in 
> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
> OOM killed

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

  8323640: [TESTBUG]testMemoryFailCount in 
jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
OOM killed
  
  Signed-off-by: sendaoYan 

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17386/files
  - new: https://git.openjdk.org/jdk/pull/17386/files/e8a99fe4..9f0aa2a1

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

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

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


Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies

2024-01-22 Thread Aleksey Shipilev
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev  wrote:

> Some jtreg tests require resolvable external dependencies. This resolution is 
> delegated to JIB, which is not used in vanilla OpenJDK testing. It would be 
> convenient to add a keyword that marks tests that require these external 
> dependencies, so that we could exclude those tests from runs. This would 
> allow us to: a) run all tests in hotspot:tier4, which now excludes 
> `applications/` specifically; b) make all tests runs (#17422) cleaner on many 
> environments.
> 
> I provisionally call this flag `external-dep`, but I am open for other 
> suggestions.
> 
> Note that some tests that pull `@Artifact`-s provide special paths that do 
> limited testing anyway. However, there are tests which cannot run without 
> external dependencies at all. These include at least `applications/jcstress` 
> and `applications/scimark` tests.
> 
> Ironically, I cannot run the jcstress test generator because the dependencies 
> are lacking here. I regenerated those test using a self-built jcstress 0.16 
> bundle.
> 
> Additional testing:
>  - [x] `make test TEST=applications/` fails
>  - [x]  `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, 
> skipping most of the tests

Any takers? Maybe the audience should include core-libs too.

-

PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1903486053