Re: RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file
On Thu, 18 Jan 2024 00:58:49 GMT, Daniel D. Daugherty wrote: > A trivial fix to disable core file generation in > java/foreign/critical/TestCriticalUpcall.java. Good and trivial. Copyright year needs updating. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17476#pullrequestreview-1829051168
Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v3]
On Thu, 18 Jan 2024 05:32:26 GMT, jmehrens 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
>
> src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
> 1088:
>
>> 1086: public void putAll(Map m) {
>> 1087: if (table != null) {
>> 1088: tryPresize(size() + m.size());
>
> Is overflow not an issue here because calling tryPresize with a negative
> value will invoke tableSizeFor?
When `tableSizeFor` is called with a negative value greater than
`Integer.MIN_VALUE`, it’ll just return `+1`:
https://github.com/openjdk/jdk/blob/ff8cc268fdaaf85299c94088a226b73e7eaf6bdb/src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java#L704-L707
This will in turn cause `tryPresize` to do nothing when `table` is already
initialized.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1457023596
Re: RFR: 8322149: ConcurrentHashMap smarter presizing for copy constructor and putAll [v3]
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
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
1088:
> 1086: public void putAll(Map m) {
> 1087: if (table != null) {
> 1088: tryPresize(size() + m.size());
Is overflow not an issue here because calling tryPresize with a negative value
will invoke tableSizeFor?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1456919954
RFR: 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file
A trivial fix to disable core file generation in java/foreign/critical/TestCriticalUpcall.java. - Commit messages: - 8321938: java/foreign/critical/TestCriticalUpcall.java does not need a core file Changes: https://git.openjdk.org/jdk/pull/17476/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17476&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321938 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17476.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17476/head:pull/17476 PR: https://git.openjdk.org/jdk/pull/17476
Re: RFR: 8324053: Use the blessed modifier order for sealed in java.base
On Wed, 17 Jan 2024 21:22:07 GMT, Pavel Rappo wrote: > Please review this trivial PR to reorder the `sealed` class or interface > modifier. For context of this change see: > https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396. Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17468#pullrequestreview-1828378857
[jdk22] Integrated: 8322799: Test JPKG003-013: ServiceTest fails because the user cannot uninstall the "servicetest" package on OEL 9.2 x64 and OEL 9.2 64-bit Arm (aarch64)
On Wed, 17 Jan 2024 18:33:27 GMT, Alexey Semenyuk wrote: > Clean backport This pull request has now been integrated. Changeset: b5ed8cca Author:Alexey Semenyuk URL: https://git.openjdk.org/jdk22/commit/b5ed8cca77ab3d4303dde691d6ccb113f3ce0a65 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8322799: Test JPKG003-013: ServiceTest fails because the user cannot uninstall the "servicetest" package on OEL 9.2 x64 and OEL 9.2 64-bit Arm (aarch64) Reviewed-by: almatvee Backport-of: 8e12053e0352a26ecd7f2b9bc298ddb8fb4bb61b - PR: https://git.openjdk.org/jdk22/pull/88
Re: RFR: 8324053: Use the blessed modifier order for sealed in java.base
On Wed, 17 Jan 2024 21:22:07 GMT, Pavel Rappo wrote: > Please review this trivial PR to reorder the `sealed` class or interface > modifier. For context of this change see: > https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396. Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17468#pullrequestreview-1828357538
Re: RFR: 8311864: Add ArraysSupport.hashCode(int[] a, fromIndex, length, initialValue) [v4]
On Tue, 2 Jan 2024 14:37:16 GMT, Pavel Rappo wrote: >> This PR adds an internal method to calculate hash code from the provided >> integer array, offset and length into that array, and the initial hash code >> value. >> >> Current options for calculating a hash code for int[] are inflexible. It's >> either ArraysSupport.vectorizedHashCode with an offset, length and initial >> value, or Arrays.hashCode with just an array. >> >> For an arbitrary int[], unconditional vectorization might be unwarranted or >> puzzling. Unfortunately, it's the only hash code method that operates on an >> array subrange or accepts the initial hash code value. >> >> A more convenient method could be added and then used, for example, here: >> >> * >> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083 >> >> * >> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680 >> >> * >> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/sun/security/pkcs10/PKCS10.java#L356-L362 >> >> This PR adds such a method and provides tests for it. Additionally, this PR >> adds tests for `null` passed to `java.util.Arrays.hashCode` overloads, >> behaviour which was specified but untested. > > Pavel Rappo 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 8311864 > - Merge remote-tracking branch 'jdk/master' into 8311864 > - Merge branch 'master' into 8311864 > - Initial commit To Skara bots: keep this PR alive. - PR Comment: https://git.openjdk.org/jdk/pull/14831#issuecomment-1896855868
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern [v2]
> Please consider this fix to ensure that going from `MessageFormat` to pattern
> string via `toPattern()` and then back via `new MessageFormat()` results in a
> format that is equivalent to the original.
>
> The quoting and escaping rules for `MessageFormat` pattern strings are really
> tricky. I admit not completely understanding them. At a high level, they work
> like this: The normal way one would "nest" strings containing special
> characters is with straightforward recursive escaping like with the `bash`
> command line. For example, if you want to echo `a "quoted string" example`
> then you enter `echo "a "quoted string" example"`. With this scheme it's
> always the "outer" layer's job to (un)escape special characters as needed.
> That is, the echo command never sees the backslash characters.
>
> In contrast, with `MessageFormat` and friends, nested subformat pattern
> strings are always provided "pre-escaped". So to build an "outer" string
> (e.g., for `ChoiceFormat`) the "inner" subformat pattern strings are more or
> less just concatenated, and then only the `ChoiceFormat` option separator
> characters (e.g., `<`, `#`, `|`, etc.) are escaped.
>
> The "pre-escape" escaping algorithm escapes `{` characters, because `{`
> indicates the beginning of a format argument. However, it doesn't escape `}`
> characters. This is OK because the format string parser treats any "extra"
> closing braces (where "extra" means not matching an opening brace) as plain
> characters.
>
> So far, so good... at least, until a format string containing an extra
> closing brace is nested inside a larger format string, where the extra
> closing brace, which was previously "extra", can now suddenly match an
> opening brace in the outer pattern containing it, thus truncating it by
> "stealing" the match from some subsequent closing brace.
>
> An example is the `MessageFormat` string `"{0,choice,0.0#option A:
> {1}|1.0#option B: {1}'}'}"`. Note the second option format string has a
> trailing closing brace in plain text. If you create a `MessageFormat` with
> this string, you see a trailing `}` only with the second option.
>
> However, if you then invoke `toPattern()`, the result is
> `"{0,choice,0.0#option A: {1}|1.0#option B: {1}}}"`. Oops, now because the
> "extra" closing brace is no longer quoted, it matches the opening brace at
> the beginning of the string, and the following closing brace, which was the
> previous match, is now just plain text in the outer `MessageFormat` string.
>
> As a result, invoking `f.format(new Object{} { 0, 5 })` will retur...
Archie Cobbs has updated the pull request incrementally with one additional
commit since the last revision:
Quote '{' and '}' in subformat patterns, but only it not already quoted.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17416/files
- new: https://git.openjdk.org/jdk/pull/17416/files/688f5748..a9d78c76
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17416&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17416&range=00-01
Stats: 344 lines in 4 files changed: 300 ins; 18 del; 26 mod
Patch: https://git.openjdk.org/jdk/pull/17416.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17416/head:pull/17416
PR: https://git.openjdk.org/jdk/pull/17416
Re: RFR: 8323699: MessageFormat.toPattern() generates non-equivalent MessageFormat pattern
On Tue, 16 Jan 2024 22:05:03 GMT, Justin Lu wrote:
>> Please consider this fix to ensure that going from `MessageFormat` to
>> pattern string via `toPattern()` and then back via `new MessageFormat()`
>> results in a format that is equivalent to the original.
>>
>> The quoting and escaping rules for `MessageFormat` pattern strings are
>> really tricky. I admit not completely understanding them. At a high level,
>> they work like this: The normal way one would "nest" strings containing
>> special characters is with straightforward recursive escaping like with the
>> `bash` command line. For example, if you want to echo `a "quoted string"
>> example` then you enter `echo "a "quoted string" example"`. With this scheme
>> it's always the "outer" layer's job to (un)escape special characters as
>> needed. That is, the echo command never sees the backslash characters.
>>
>> In contrast, with `MessageFormat` and friends, nested subformat pattern
>> strings are always provided "pre-escaped". So to build an "outer" string
>> (e.g., for `ChoiceFormat`) the "inner" subformat pattern strings are more or
>> less just concatenated, and then only the `ChoiceFormat` option separator
>> characters (e.g., `<`, `#`, `|`, etc.) are escaped.
>>
>> The "pre-escape" escaping algorithm escapes `{` characters, because `{`
>> indicates the beginning of a format argument. However, it doesn't escape `}`
>> characters. This is OK because the format string parser treats any "extra"
>> closing braces (where "extra" means not matching an opening brace) as plain
>> characters.
>>
>> So far, so good... at least, until a format string containing an extra
>> closing brace is nested inside a larger format string, where the extra
>> closing brace, which was previously "extra", can now suddenly match an
>> opening brace in the outer pattern containing it, thus truncating it by
>> "stealing" the match from some subsequent closing brace.
>>
>> An example is the `MessageFormat` string `"{0,choice,0.0#option A:
>> {1}|1.0#option B: {1}'}'}"`. Note the second option format string has a
>> trailing closing brace in plain text. If you create a `MessageFormat` with
>> this string, you see a trailing `}` only with the second option.
>>
>> However, if you then invoke `toPattern()`, the result is
>> `"{0,choice,0.0#option A: {1}|1.0#option B: {1}}}"`. Oops, now because the
>> "extra" closing brace is no longer quoted, it matches the opening brace at
>> the beginning of the string, and the following closing brace, which was the
>> previous match, is now just plain text in the outer `MessageFormat` string.
>>
>> As a result, invoking `f.format(new ...
>
> Hi Archie, thanks for the proposed fix. I am still taking a look, but I
> wanted to demonstrate a current issue,
>
> (Jshell with your patch)
>
>
> var pattIn = "Test: {0,number,foo'{'#.00}";
> MessageFormat mFmt = new MessageFormat(pattIn);
> var pattOut = mFmt.toPattern(); // returns "Test: {0,number,foo{#.00}";
>
>
>
> var pattIn = "Test: {0,number,foo'}'#.00}";
> MessageFormat mFmt = new MessageFormat(pattIn);
> var pattOut = mFmt.toPattern(); // returns "Test: {0,number,foo'}'#.00}";
>
>
> As it stands, it would be inconsistent to have the closing bracket quoted and
> the opening bracket not quoted.
>
> Also in response to your earlier question on core-libs-dev, ideally invoking
> toPattern() can roundtrip, but there are known issues, such as a custom user
> defined Format subclass, or one of the newer Format subclasses that do not
> implement the toPattern() method. I am working on making this apparent in the
> specification of the method in a separate issue.
Hi @justin-curtis-lu,
Thanks a lot for taking a look, I am glad for any other set of eyes on this
tricky stuff!
> As it stands, it would be inconsistent to have the closing bracket quoted and
> the opening bracket not quoted.
You're right - the problem exists with both `{` and `}`, as is shown here
(unmodified jshell):
$ jshell
| Welcome to JShell -- Version 17.0.9
| For an introduction type: /help intro
jshell> import java.text.*;
jshell> new MessageFormat("Test: {0,number,foo'{'#.00}");
$2 ==> java.text.MessageFormat@951c5b58
jshell> $2.toPattern()
$3 ==> "Test: {0,number,foo{#.00}"
jshell> new MessageFormat($3)
| Exception java.lang.IllegalArgumentException: Unmatched braces in the
pattern.
|at MessageFormat.applyPattern (MessageFormat.java:521)
|at MessageFormat. (MessageFormat.java:371)
|at (#4:1)
I've been missing the forest for the trees a bit and I think the fix can be
simpler now.
For the record, here is my latest understanding of what's going on...
1. When `MessageFormat.toPattern()` constructs the combined pattern string, it
concatenates the plain text bits, suitably escaped, and the pattern strings
from each subformat.
1. The subformat strings are already escaped, in the sense that you can take
(for example) a `ChoiceFormat` format
RFR: 8324053: Use the blessed modifier order for sealed in java.base
Please review this trivial PR to reorder the `sealed` class or interface modifier. For context of this change see: https://github.com/openjdk/jdk/pull/17242#issuecomment-1887338396. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/17468/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17468&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324053 Stats: 8 lines in 7 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17468.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17468/head:pull/17468 PR: https://git.openjdk.org/jdk/pull/17468
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]
On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber wrote: >> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for >> correct (Dekker scheme) synchronization with concurrent execution of >> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). >> >> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot >> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance >> Suite, and SAP specific tests. >> Testing was done with fastdebug and release builds on the main platforms and >> also on Linux/PPC64le and AIX. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Review Alan test/jdk/java/nio/channels/Selector/LotsOfInterrupts.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. I guess you should credit SAP here. - PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1456496044
Re: RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing [v2]
On Fri, 12 Jan 2024 08:51:16 GMT, Volker Simonis wrote: >>> tryPresize(int size) is doing and if its size argument is supposed to >>> contain the additional number of elements which will be inserted into the >>> hash map or if it is a hint for the new total size of the hash map >> >> Argument `size` for `tryPresize()` is a hint for the **new total size** of >> the hash map. If the size is too small compared to the current size of the >> map, there will be no resizing. > > Thanks. In that case, calling `tryPresize()` with `size < this.size()` > doesn't make sense and we should call it with `this.size() + m.size()`. Thanks. Just committed this change. Also added a new benchmark for `putAll()` and the results in the top post. - PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1456490539
Re: RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/17116/files - new: https://git.openjdk.org/jdk/pull/17116/files/3632649c..3efa593d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17116&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17116&range=01-02 Stats: 11 lines in 2 files changed: 10 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: [jdk22] RFR: 8322799: Test JPKG003-013: ServiceTest fails because the user cannot uninstall the "servicetest" package on OEL 9.2 x64 and OEL 9.2 64-bit Arm (aarch64)
On Wed, 17 Jan 2024 18:33:27 GMT, Alexey Semenyuk wrote: > Clean backport Looks good. I verified that it is a clean backport. - Marked as reviewed by almatvee (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/88#pullrequestreview-1828180526
RFR: 8159927: Add a test to verify JMOD files created in the images do not have debug symbols
The build excludes the native debug symbols in JMOD files created for JDK modules (see make/CreateJmods.gmk). This PR adds a test to verify that native debug symbols are excluded as expected. - Commit messages: - 8159927: Add a test to verify JMOD files created in the images do not have debug symbols Changes: https://git.openjdk.org/jdk/pull/17467/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17467&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8159927 Stats: 76 lines in 1 file changed: 76 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17467.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17467/head:pull/17467 PR: https://git.openjdk.org/jdk/pull/17467
Re: Seing a Collector as a Gatherer
> From: "Viktor Klang"
> To: "Remi Forax"
> Sent: Wednesday, January 17, 2024 6:01:38 PM
> Subject: Re: Seing a Collector as a Gatherer
> Hi Rémi,
> Yes, this was something I was hoping to get into the preview, but I wasn't
> sure
> where that conversion should end up.
> There are a few different places where it might go:
> Gatherer.of(Collector)
> Gatherers.collect(Collector)
> Collector.asGatherer()
> Collectors.gather(Collector)
> I wasn't really convinced where it should go, and I was hesitant to making any
> changes to existing public interfaces as a "nice to have", so I opted to leave
> it out for now.
> I think people would prefer to see it on Collector as a default method, but
> as I
> said before, making changes to Collector wasn't something I was ready to
> prioritize for the (first) JEP.
I think this method is also important pedagogically, there should be a place
that describe the relationship between a Collector and a Gatherer.
For Gatherer.of(), this one seems alien compared to the other overloads of
of()/ofSequential() and to a lesser extend I do not like too much to have
overloads with one parameter with two different interfaces, because someone can
comes with a class that implements both Collector and Integrator (as stupid as
it is),
For Gatherers.collect(Collector) is fine,
For Collector.asGatherer(), a default method has the disadvantage that usually
a Collector is typed from left to right so calling an instance method requires
an intermediary variable
Collector> collector = Collector.toList(); // ok
Gatherer> gatherer = Collector.toList().asGatherer();
// we are in trouble here
that's why Collectors.collectingAndThen() (aka compose the finisher) is a
static method in Collectors and not an instance method in Collector
(finishAndThen ?),
For Collectors.gather(), methods inside Collectors tend to return a Collector.
> Cheers,
> √
regards,
Rémi
> Viktor Klang
> Software Architect, Java Platform Group
> Oracle
> From: core-libs-dev on behalf of Remi Forax
>
> Sent: Wednesday, 17 January 2024 17:08
> To: core-libs-dev
> Subject: Seing a Collector as a Gatherer
> Hello,
> I may have overlook that, but it seems there is no method to see a Collector
> as
> a Gatherer.
> A Gatherer is more general than a Collector and a Gatherer with a greedy
> integrator that does not call Downstream.push in the intergator and only once
> is the finisher is basicaly a Collector.
> In code:
> Gatherer asGatherer(Collector
> collector) {
> var supplier = collector.supplier();
> var accumulator = collector.accumulator();
> var combiner = collector.combiner();
> var finisher = collector.finisher();
> return Gatherer.of(supplier,
> Gatherer.Integrator.ofGreedy((state, element, _) -> {
> accumulator.accept(state, element);
> return true;
> }),
> combiner,
> (state, downstream) -> downstream.push(finisher.apply(state)));
> }
> This is eaxctly how Gatherer.fold() works.
> Is there a reason why such method does not exist ?
> regards,
> Rémi
[jdk22] Integrated: 8322512: StringBuffer.repeat does not work correctly after toString() was called
On Wed, 17 Jan 2024 14:21:22 GMT, Jim Laskey wrote: > The new repeat methods were not clearing the toStringCache. This pull request has now been integrated. Changeset: 60c68a13 Author:Jim Laskey URL: https://git.openjdk.org/jdk22/commit/60c68a13639fe79cc2510d551b8c1c7d7e1a02f3 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: prappo, redestad Backport-of: df22fb322e6c4c9931a770bd0abf4c43b83c4e4a - PR: https://git.openjdk.org/jdk22/pull/87
Re: Gatherer API : wildcards complaint
Ah, now I see what you mean! Thank you 👍
The reason for the signature of `Gatherer.of` was to mirror as much as possible
of `Collector.of`[1] so I would argue that if we tweak the variance of one then
we should consider tweaking it for both.
1:https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Collector.java#L264
Cheers,
√
Viktor Klang
Software Architect, Java Platform Group
Oracle
From: fo...@univ-mlv.fr
Sent: Wednesday, 17 January 2024 20:49
To: Viktor Klang
Cc: core-libs-dev
Subject: [External] : Re: Gatherer API : wildcards complaint
From: "Viktor Klang"
To: "Remi Forax" , "core-libs-dev"
Sent: Wednesday, January 17, 2024 5:49:01 PM
Subject: Re: Gatherer API : wildcards complaint
Hi Rémi,
Thank you for the feedback—examples would be much appreciated!
Here is an example with an interface and a class,
interface Counter {
void increment();
int value();
}
Gatherer count() {
class CounterImpl implements Counter {
int counter;
@Override
public void increment() {
counter++;
}
@Override
public int value() {
return counter;
}
}
Supplier initializer = CounterImpl::new;
Gatherer.Integrator>
integrator = (counter, _, _) -> {
counter.increment();
return true;
};
BiConsumer> finisher =
(counter, downstream) -> {
downstream.push(counter.value());
};
return Gatherer.ofSequential(initializer, integrator, finisher);// does
not compile :(
}
void main() {
System.out.println(Stream.of("foo").gather(count()).findFirst().orElseThrow());
}
if instead of explicitly typing each functions, we directly call ofSequential,
it works
return Gatherer.ofSequential(
CounterImpl::new,
(Counter counter, String _, Gatherer.Downstream _) -> {
counter.increment();
return true;
},
(Counter counter, Gatherer.Downstream downstream) -> {
downstream.push(counter.value());
}
);
because here, CounterImpl::new is inferred as Supplier.
Cheers,
√
Viktor Klang
Software Architect, Java Platform Group
Oracle
From: core-libs-dev on behalf of Remi Forax
Sent: Wednesday, 17 January 2024 16:55
To: core-libs-dev
Subject: Gatherer API : wildcards complaint
Hello,
this is a minor complaint but I do not see a raison to not getting this right.
Currently the Gatherer API does not use the wildcard correctly, which is not
fully an issue because there is "enough" wildcards that if you rely on the
inference, it will work.
The problem is that when you write code, you make mistakes and usually when you
have a typing issue, a way to debug it is to fix the type arguments
de-activating the inference.
But because there are some missing wildcards, this debugging strategy just fail
flat with more typing errors.
I think that fixing the missing wildcards will hep users (or a least me) to
have a better experience when using the Gatherer API.
I can help and propose a PR if you want.
regards,
Rémi
Re: Gatherer API : wildcards complaint
> From: "Viktor Klang"
> To: "Remi Forax" , "core-libs-dev"
>
> Sent: Wednesday, January 17, 2024 5:49:01 PM
> Subject: Re: Gatherer API : wildcards complaint
> Hi Rémi,
> Thank you for the feedback—examples would be much appreciated!
Here is an example with an interface and a class,
interface Counter {
void increment ();
int value ();
}
Gatherer < String , Counter , Integer > count () {
class CounterImpl implements Counter {
int counter ;
@Override
public void increment () {
counter ++;
}
@Override
public int value () {
return counter ;
}
}
Supplier < CounterImpl > initializer = CounterImpl :: new ;
Gatherer . Integrator < Counter , String , Gatherer . Downstream > integrator = ( counter , _ , _ ) -> {
counter .increment();
return true ;
};
BiConsumer < Counter , Gatherer . Downstream > finisher = (
counter , downstream ) -> {
downstream .push( counter .value());
};
return Gatherer . ofSequential ( initializer , integrator , finisher ); // does
not compile :(
}
void main () {
System . out .println( Stream . of ( "foo"
).gather(count()).findFirst().orElseThrow());
}
if instead of explicitly typing each functions, we directly call ofSequential,
it works
return Gatherer . ofSequential (
CounterImpl :: new ,
( Counter counter , String _ , Gatherer . Downstream _ ) ->
{
counter .increment();
return true ;
},
( Counter counter , Gatherer . Downstream downstream ) -> {
downstream .push( counter .value());
}
);
because here, CounterImpl::new is inferred as Supplier.
> Cheers,
> √
> Viktor Klang
> Software Architect, Java Platform Group
> Oracle
> From: core-libs-dev on behalf of Remi Forax
>
> Sent: Wednesday, 17 January 2024 16:55
> To: core-libs-dev
> Subject: Gatherer API : wildcards complaint
> Hello,
> this is a minor complaint but I do not see a raison to not getting this right.
> Currently the Gatherer API does not use the wildcard correctly, which is not
> fully an issue because there is "enough" wildcards that if you rely on the
> inference, it will work.
> The problem is that when you write code, you make mistakes and usually when
> you
> have a typing issue, a way to debug it is to fix the type arguments
> de-activating the inference.
> But because there are some missing wildcards, this debugging strategy just
> fail
> flat with more typing errors.
> I think that fixing the missing wildcards will hep users (or a least me) to
> have
> a better experience when using the Gatherer API.
> I can help and propose a PR if you want.
> regards,
> Rémi
Re: Gatherer: spliterator characteristics are not propagated
Hi Rémi, You can find some of my benches here: https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/util/stream/ops/ref Initially I had Characteristics such as ORDERED etc on Gatherer but it just didn't end up worth it when looking at the bench results over a wide array of stream sizes and number of operations. Cheers, √ Viktor Klang Software Architect, Java Platform Group Oracle From: core-libs-dev on behalf of Remi Forax Sent: Wednesday, 17 January 2024 16:48 To: core-libs-dev Subject: Gatherer: spliterator characteristics are not propagated While doing some benchmarking of the Gatherer API, i've found that the characteristics of the spliterator was not propagated by the method Stream.gather() making the stream slower than it should. As an example, there is no way when reimplementing map() using a Gatherer to say that this intermediate operation keep the size, which is important if the terminal operation is toList() because if the size is known, toList() will presize the List and avoid the creation of an intermediary ArrayList. See https://github.com/forax/we_are_all_to_gather/blob/master/src/main/java/com/gihtub/forax/wearealltogather/bench/MapGathererBenchmark.java I think that adding a way to propagate the spliterator characteristics through a Gatherer would greatly improve the performance of commons streams (at least all the ones that end with a call to toList). I have some idea of how to do that, but I prefer first to hear if i've overlook something and if improving the performance is something worth changing the API. regards, Rémi
Re: [jdk22] RFR: 8322799: Test JPKG003-013: ServiceTest fails because the user cannot uninstall the "servicetest" package on OEL 9.2 x64 and OEL 9.2 64-bit Arm (aarch64)
On Wed, 17 Jan 2024 18:33:27 GMT, Alexey Semenyuk wrote: > Clean backport @alexeysemenyukoracle please review - PR Comment: https://git.openjdk.org/jdk22/pull/88#issuecomment-1896427625
[jdk22] RFR: 8322799: Test JPKG003-013: ServiceTest fails because the user cannot uninstall the "servicetest" package on OEL 9.2 x64 and OEL 9.2 64-bit Arm (aarch64)
Clean backport - Commit messages: - Backport 8e12053e0352a26ecd7f2b9bc298ddb8fb4bb61b Changes: https://git.openjdk.org/jdk22/pull/88/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=88&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322799 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk22/pull/88.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/88/head:pull/88 PR: https://git.openjdk.org/jdk22/pull/88
Re: RFR: 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
On Tue, 16 Jan 2024 11:12:44 GMT, Andrey Turbanov wrote:
>> 8291027: Some of TimeZone methods marked 'synchronized' unnecessarily
>
> src/java.base/share/classes/java/util/TimeZone.java line 629:
>
>> 627: */
>> 628: public static String[] getAvailableIDs(int rawOffset) {
>> 629: return ZoneInfo.getAvailableIDs(rawOffset);
>
> BTW can we call `ZoneInfoFile.getZoneIds` here directly?
> `ZoneInfo.getAvailableIDs` bridge seems redudnant.
`TimeZone` is an abstract class, and `ZoneInfo` is an implementation. So to me,
it is clearer to call the overriden method in the implementation even though
the reason you mentioned.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17441#discussion_r1456249396
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Thanks for the reminder. The new alias is nice, easier to run all tier tests. I
often run xml-only as well that includes jaxp_all plus a small set of jaxp
tests in jdk_all (test/jdk/javax/xml/jaxp). But that's just me, jdk_all already
covers those tests.
-
Marked as reviewed by joehw (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17422#pullrequestreview-1827838226
Re: [jdk22] RFR: 8322512: StringBuffer.repeat does not work correctly after toString() was called [v2]
On Wed, 17 Jan 2024 15:01:07 GMT, Jim Laskey wrote: >> 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 two additional commits since > the last revision: > > - Merge branch 'master' into backport-JimLaskey-df22fb32 > - Backport df22fb322e6c4c9931a770bd0abf4c43b83c4e4a Marked as reviewed by redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/87#pullrequestreview-1827802455
Re: Improving the Gatherer public API
Hi Rémi,
Thanks for your thoughts!
Improving the documentation of Gatherer / Gatherers is definitely something I
am intending to have inspired by feedback received as people try it out, so
your thoughts here are most welcome.
For the longest time Gatherer had Characteristics (just like Collector, which
Gatherer borrows its design from) but in the end it didn't carry its weight and
tracking the characteristics separately from the actual behavior turned out to
be a source of both poor performance (since characteristics need to be
composed) and a source of subtle defects.
The concern around the identity-equals of a lambda is a bit of a non-issue
since we are comparing instances of the interfaces, and the default values are
not
lambdaform:https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java#L453
PS. Forgetting ofGreedy shouldn't affect semantics, it's just an evaluation
hint which can result in higher performance.
Cheers,
√
Cheers,
√
Viktor Klang
Software Architect, Java Platform Group
Oracle
From: core-libs-dev on behalf of Remi Forax
Sent: Wednesday, 17 January 2024 16:38
To: core-libs-dev
Subject: Improving the Gatherer public API
Hello,
i've played quite a lot with the Gatherer API and overall, i quite like how
things work conceptually.
But i think the public API can be improved.
First, the documentation is not fully clear that a Gatherer has 3
characteristics,
1/ Is it sequential or parallelizable (can be parallel is ask)
2/ Is it stateless or stateful
3/ Is the integrator greedy or not.
There is also, is there a finisher or not, but this is less important.
When creating a Gatherer, a user can find the right method "Gatherer.of" by
following this state diagram
// if sequential
// if stateless
// if greedy
Gatherer.ofSequential(Gatherer.Integrator.ofGreedy(integrator),
finisher?)
// otherwise short-circuit
Gatherer.ofSequential(integrator, finisher?)
// otherwise statefull
// if greedy
Gatherer.ofSequential(initializer,
Gatherer.Integrator.ofGreedy(integrator), finisher?)
// otherwise short-circuit
Gatherer.ofSequential(initializer, integrator, finisher?)
// otherwise parallelizable
// if stateless
// if greedy
Gatherer.of(Gatherer.Integrator.ofGreedy(integrator), finisher?)
// otherwise short-circuit
Gatherer.of(integrator, finisher?)
// otherwise stateful
// if greedy
Gatherer.of(initializer, Gatherer.Integrator.ofGreedy(integrator),
combiner, finisher)
// otherwise short-circuit
Gatherer.of(initializer, integrator, combiner, finisher)
The first issue I stumble several time is that i've forgotten to use
Integrator.ofGreedy(). I'm not the only one, at least both Viktor and Nicolai
Parlog had the same issue. It's to easy to forget to call Integrator.ofGreedy().
I think the API should be slightly modified to force the user to make a choice
between greedy or short-cuircuit.
I'm proposing to add a new parameter for all the factory methods, in front of
the integrator, forcing the user to make a choice
enum IntegratorKind {
GREEDY, SHORT_CIRCUIT
}
so the state diagram becomes
// if sequential
// if stateless
// if greedy
Gatherer.ofSequential(GREEDY, integrator, finisher?)
// otherwise short-circuit
Gatherer.ofSequential(SHORT_CIRCUIT, integrator, finisher?)
// otherwise stateful
// if greedy
Gatherer.ofSequential(initializer, GREEDY, integrator, finisher?)
// otherwise short-circuit
Gatherer.ofSequential(initializer, SHORT_CIRCUIT, integrator, finisher?)
// otherwise parallelizable
// if stateless
// if greedy
Gatherer.of(GREEDY, integrator, finisher?)
// otherwise short-circuit
Gatherer.of(SHORT_CIRCUIT, integrator, finisher?)
// otherwise stateful
// if greedy
Gatherer.of(initializer, GREEDY, integrator, combiner, finisher)
// otherwise short-circuit
Gatherer.of(initializer, SHORT_CIRCUIT, integrator, combiner, finisher)
The second issue is that it's hard to implement a Gatherer that itself need a
Gatherer as parameter (like andThen/compose). This is due to the fact that
querying if a Gatherer is SEQUENTIAL, STATELESS or GREEDY is far for obvious.
Instead of having a method characteristics() like a Collector, the way to query
the characteristics is very add-hoc, using either the default
combiner/initializer or instanceof on the integrator.
SEQUENTIAL: combiner == defaultCombiner
STATELESS: initializer == defaultInitializer
GREEDY: integrator instanceof Greedy
I think the API should be simple if the default combiner/initializer and
finisher were removed a method characteristics (and a default method
hasCHaracteristics) were added.
It would be mor
Re: Seing a Collector as a Gatherer
Hi Rémi,
Yes, this was something I was hoping to get into the preview, but I wasn't sure
where that conversion should end up.
There are a few different places where it might go:
Gatherer.of(Collector)
Gatherers.collect(Collector)
Collector.asGatherer()
Collectors.gather(Collector)
I wasn't really convinced where it should go, and I was hesitant to making any
changes to existing public interfaces as a "nice to have", so I opted to leave
it out for now.
I think people would prefer to see it on Collector as a default method, but as
I said before, making changes to Collector wasn't something I was ready to
prioritize for the (first) JEP.
Cheers,
√
Viktor Klang
Software Architect, Java Platform Group
Oracle
From: core-libs-dev on behalf of Remi Forax
Sent: Wednesday, 17 January 2024 17:08
To: core-libs-dev
Subject: Seing a Collector as a Gatherer
Hello,
I may have overlook that, but it seems there is no method to see a Collector as
a Gatherer.
A Gatherer is more general than a Collector and a Gatherer with a greedy
integrator that does not call Downstream.push in the intergator and only once
is the finisher is basicaly a Collector.
In code:
Gatherer asGatherer(Collector
collector) {
var supplier = collector.supplier();
var accumulator = collector.accumulator();
var combiner = collector.combiner();
var finisher = collector.finisher();
return Gatherer.of(supplier,
Gatherer.Integrator.ofGreedy((state, element, _) -> {
accumulator.accept(state, element);
return true;
}),
combiner,
(state, downstream) -> downstream.push(finisher.apply(state)));
}
This is eaxctly how Gatherer.fold() works.
Is there a reason why such method does not exist ?
regards,
Rémi
Re: Gatherer API : wildcards complaint
Hi Rémi, Thank you for the feedback―examples would be much appreciated! Cheers, √ Viktor Klang Software Architect, Java Platform Group Oracle From: core-libs-dev on behalf of Remi Forax Sent: Wednesday, 17 January 2024 16:55 To: core-libs-dev Subject: Gatherer API : wildcards complaint Hello, this is a minor complaint but I do not see a raison to not getting this right. Currently the Gatherer API does not use the wildcard correctly, which is not fully an issue because there is "enough" wildcards that if you rely on the inference, it will work. The problem is that when you write code, you make mistakes and usually when you have a typing issue, a way to debug it is to fix the type arguments de-activating the inference. But because there are some missing wildcards, this debugging strategy just fail flat with more typing errors. I think that fixing the missing wildcards will hep users (or a least me) to have a better experience when using the Gatherer API. I can help and propose a PR if you want. regards, Rémi
Seing a Collector as a Gatherer
Hello,
I may have overlook that, but it seems there is no method to see a Collector as
a Gatherer.
A Gatherer is more general than a Collector and a Gatherer with a greedy
integrator that does not call Downstream.push in the intergator and only once
is the finisher is basicaly a Collector.
In code:
Gatherer asGatherer(Collector
collector) {
var supplier = collector.supplier();
var accumulator = collector.accumulator();
var combiner = collector.combiner();
var finisher = collector.finisher();
return Gatherer.of(supplier,
Gatherer.Integrator.ofGreedy((state, element, _) -> {
accumulator.accept(state, element);
return true;
}),
combiner,
(state, downstream) -> downstream.push(finisher.apply(state)));
}
This is eaxctly how Gatherer.fold() works.
Is there a reason why such method does not exist ?
regards,
Rémi
RFR: 8323835: Updating ASM to 9.6 for JDK 23
Updating ASM to version 9.6, Thanks in advance for the reviews, Vicente - Commit messages: - some additional minor changes - more changes - additional changes - 8323835: Updating ASM to 9.6 for JDK 23 Changes: https://git.openjdk.org/jdk/pull/17453/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17453&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323835 Stats: 1536 lines in 127 files changed: 1033 ins; 181 del; 322 mod Patch: https://git.openjdk.org/jdk/pull/17453.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17453/head:pull/17453 PR: https://git.openjdk.org/jdk/pull/17453
Gatherer API : wildcards complaint
Hello, this is a minor complaint but I do not see a raison to not getting this right. Currently the Gatherer API does not use the wildcard correctly, which is not fully an issue because there is "enough" wildcards that if you rely on the inference, it will work. The problem is that when you write code, you make mistakes and usually when you have a typing issue, a way to debug it is to fix the type arguments de-activating the inference. But because there are some missing wildcards, this debugging strategy just fail flat with more typing errors. I think that fixing the missing wildcards will hep users (or a least me) to have a better experience when using the Gatherer API. I can help and propose a PR if you want. regards, Rémi
Gatherer: spliterator characteristics are not propagated
While doing some benchmarking of the Gatherer API, i've found that the characteristics of the spliterator was not propagated by the method Stream.gather() making the stream slower than it should. As an example, there is no way when reimplementing map() using a Gatherer to say that this intermediate operation keep the size, which is important if the terminal operation is toList() because if the size is known, toList() will presize the List and avoid the creation of an intermediary ArrayList. See https://github.com/forax/we_are_all_to_gather/blob/master/src/main/java/com/gihtub/forax/wearealltogather/bench/MapGathererBenchmark.java I think that adding a way to propagate the spliterator characteristics through a Gatherer would greatly improve the performance of commons streams (at least all the ones that end with a call to toList). I have some idea of how to do that, but I prefer first to hear if i've overlook something and if improving the performance is something worth changing the API. regards, Rémi
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]
On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber wrote: >> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for >> correct (Dekker scheme) synchronization with concurrent execution of >> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). >> >> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot >> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance >> Suite, and SAP specific tests. >> Testing was done with fastdebug and release builds on the main platforms and >> also on Linux/PPC64le and AIX. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Review Alan Thanks for your help Alan. - PR Comment: https://git.openjdk.org/jdk/pull/17444#issuecomment-1896080567
Integrated: 8323794: Remove unused jimage compressor plugin configuration
On Tue, 16 Jan 2024 10:55:07 GMT, Claes Redestad wrote: > There's an unused concept of a pluginConfig that is passed into the jlink > compress plugins, however we always pass null here and the code seems broken > (the pluginConfig wouldn't have been stored properly). This seem to be a > left-over from a more generic plugin design that never came to fruition. > > This PR cleans out this code from the plugins and decompressors, while > keeping the compressed header format intact for backwards compatibility (and > in case we want to revisit this in the future) This pull request has now been integrated. Changeset: 8b29e127 Author:Claes Redestad URL: https://git.openjdk.org/jdk/commit/8b29e127c2b030a2f63840b56c5bdecd5ee18cab Stats: 94 lines in 13 files changed: 10 ins; 47 del; 37 mod 8323794: Remove unused jimage compressor plugin configuration Reviewed-by: jlaskey, mchung - PR: https://git.openjdk.org/jdk/pull/17443
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]
On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber wrote: >> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for >> correct (Dekker scheme) synchronization with concurrent execution of >> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). >> >> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot >> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance >> Suite, and SAP specific tests. >> Testing was done with fastdebug and release builds on the main platforms and >> also on Linux/PPC64le and AIX. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Review Alan Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17444#pullrequestreview-1827598012
Improving the Gatherer public API
Hello,
i've played quite a lot with the Gatherer API and overall, i quite like how
things work conceptually.
But i think the public API can be improved.
First, the documentation is not fully clear that a Gatherer has 3
characteristics,
1/ Is it sequential or parallelizable (can be parallel is ask)
2/ Is it stateless or stateful
3/ Is the integrator greedy or not.
There is also, is there a finisher or not, but this is less important.
When creating a Gatherer, a user can find the right method "Gatherer.of" by
following this state diagram
// if sequential
// if stateless
// if greedy
Gatherer.ofSequential(Gatherer.Integrator.ofGreedy(integrator),
finisher?)
// otherwise short-circuit
Gatherer.ofSequential(integrator, finisher?)
// otherwise statefull
// if greedy
Gatherer.ofSequential(initializer,
Gatherer.Integrator.ofGreedy(integrator), finisher?)
// otherwise short-circuit
Gatherer.ofSequential(initializer, integrator, finisher?)
// otherwise parallelizable
// if stateless
// if greedy
Gatherer.of(Gatherer.Integrator.ofGreedy(integrator), finisher?)
// otherwise short-circuit
Gatherer.of(integrator, finisher?)
// otherwise stateful
// if greedy
Gatherer.of(initializer, Gatherer.Integrator.ofGreedy(integrator),
combiner, finisher)
// otherwise short-circuit
Gatherer.of(initializer, integrator, combiner, finisher)
The first issue I stumble several time is that i've forgotten to use
Integrator.ofGreedy(). I'm not the only one, at least both Viktor and Nicolai
Parlog had the same issue. It's to easy to forget to call Integrator.ofGreedy().
I think the API should be slightly modified to force the user to make a choice
between greedy or short-cuircuit.
I'm proposing to add a new parameter for all the factory methods, in front of
the integrator, forcing the user to make a choice
enum IntegratorKind {
GREEDY, SHORT_CIRCUIT
}
so the state diagram becomes
// if sequential
// if stateless
// if greedy
Gatherer.ofSequential(GREEDY, integrator, finisher?)
// otherwise short-circuit
Gatherer.ofSequential(SHORT_CIRCUIT, integrator, finisher?)
// otherwise stateful
// if greedy
Gatherer.ofSequential(initializer, GREEDY, integrator, finisher?)
// otherwise short-circuit
Gatherer.ofSequential(initializer, SHORT_CIRCUIT, integrator, finisher?)
// otherwise parallelizable
// if stateless
// if greedy
Gatherer.of(GREEDY, integrator, finisher?)
// otherwise short-circuit
Gatherer.of(SHORT_CIRCUIT, integrator, finisher?)
// otherwise stateful
// if greedy
Gatherer.of(initializer, GREEDY, integrator, combiner, finisher)
// otherwise short-circuit
Gatherer.of(initializer, SHORT_CIRCUIT, integrator, combiner, finisher)
The second issue is that it's hard to implement a Gatherer that itself need a
Gatherer as parameter (like andThen/compose). This is due to the fact that
querying if a Gatherer is SEQUENTIAL, STATELESS or GREEDY is far for obvious.
Instead of having a method characteristics() like a Collector, the way to query
the characteristics is very add-hoc, using either the default
combiner/initializer or instanceof on the integrator.
SEQUENTIAL: combiner == defaultCombiner
STATELESS: initializer == defaultInitializer
GREEDY: integrator instanceof Greedy
I think the API should be simple if the default combiner/initializer and
finisher were removed a method characteristics (and a default method
hasCHaracteristics) were added.
It would be more like a Collector, simpler to user, and the default
implementation of the combiner/initializer/finisher could be an impleemntation
detail instead of being part of the API.
As a side note: the exact semantics of == on a lambda is not specified so the
actual implementation ask users to rely on the way the current OpenJDK
implementation works.
They are more mails to come on two other issues not fully realated to the
public API of the Gatherer, so i'm trying to keep the mail short.
regards,
Rémi
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]
> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for > correct (Dekker scheme) synchronization with concurrent execution of > [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). > > The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot > and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance > Suite, and SAP specific tests. > Testing was done with fastdebug and release builds on the main platforms and > also on Linux/PPC64le and AIX. Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Review Alan - Changes: - all: https://git.openjdk.org/jdk/pull/17444/files - new: https://git.openjdk.org/jdk/pull/17444/files/2c1d3835..ab3513c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17444&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17444&range=00-01 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17444.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17444/head:pull/17444 PR: https://git.openjdk.org/jdk/pull/17444
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin
On Tue, 16 Jan 2024 10:57:46 GMT, Richard Reingruber wrote:
> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for
> correct (Dekker scheme) synchronization with concurrent execution of
> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176).
>
> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot
> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance
> Suite, and SAP specific tests.
> Testing was done with fastdebug and release builds on the main platforms and
> also on Linux/PPC64le and AIX.
Thanks for finding this issue. It duplicates on JDK 8, and looking at the
ancient history, it looks like it's been there since JDK 1.4 but just not
noticed or diagnosed.
Reading the nioBlocker after setting the interrupt status is good.
There are about 20 tests for async interrupt of I/O ops in tier2. I see you've
run tier1-4 so you've run them.
src/java.base/share/classes/java/lang/Thread.java line 1706:
> 1704: checkAccess();
> 1705: }
> 1706: // Write interrupted before reading nioBlocker for correct
> synchronization.
I think I'd prefer if this said that it sets the interrupt status, and must be
done before reading the nioBlocker.
src/java.base/share/classes/java/lang/Thread.java line 1710:
> 1708: interrupt0(); // inform VM of interrupt
> 1709: if (this != Thread.currentThread()) {
> 1710: // thread may be blocked in an I/O operation
I think the existing comment "thread may be blocked in an I/O operation" can
move to before the `if` statement so that it's a bit clearer than this is code
for I/O operations.
-
PR Review: https://git.openjdk.org/jdk/pull/17444#pullrequestreview-1827537664
PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1455826929
PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1455825649
Re: [jdk22] RFR: 8322512: StringBuffer.repeat does not work correctly after toString() was called [v2]
> 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 two additional commits since the last revision: - Merge branch 'master' into backport-JimLaskey-df22fb32 - Backport df22fb322e6c4c9931a770bd0abf4c43b83c4e4a - Changes: - all: https://git.openjdk.org/jdk22/pull/87/files - new: https://git.openjdk.org/jdk22/pull/87/files/e9817489..0ae24358 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk22&pr=87&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk22&pr=87&range=00-01 Stats: 7075 lines in 237 files changed: 4658 ins; 1464 del; 953 mod Patch: https://git.openjdk.org/jdk22/pull/87.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/87/head:pull/87 PR: https://git.openjdk.org/jdk22/pull/87
Re: [jdk22] RFR: 8322512: StringBuffer.repeat does not work correctly after toString() was called
On Wed, 17 Jan 2024 14:21:22 GMT, Jim Laskey wrote: > The new repeat methods were not clearing the toStringCache. Jim, note that the bot said this: > At the time when this comment was updated there had been 64 new commits > pushed to the master branch: I'm not sure why there's such a big divergence, but if I were you, I'd be testing this change extra carefully before integrating. - PR Comment: https://git.openjdk.org/jdk22/pull/87#issuecomment-1895943189
Re: [jdk22] RFR: 8322512: StringBuffer.repeat does not work correctly after toString() was called
On Wed, 17 Jan 2024 14:21:22 GMT, Jim Laskey wrote: > The new repeat methods were not clearing the toStringCache. Seems like a clean backport of a P3 bug from mainline to jdk 22, just in time before RDP 2. - Marked as reviewed by prappo (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/87#pullrequestreview-1827429912
[jdk22] RFR: 8322512: StringBuffer.repeat does not work correctly after toString() was called
The new repeat methods were not clearing the toStringCache. - Commit messages: - Backport df22fb322e6c4c9931a770bd0abf4c43b83c4e4a Changes: https://git.openjdk.org/jdk22/pull/87/files Webrev: https://webrevs.openjdk.org/?repo=jdk22&pr=87&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322512 Stats: 16 lines in 2 files changed: 15 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk22/pull/87.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/87/head:pull/87 PR: https://git.openjdk.org/jdk22/pull/87
Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v6]
On Tue, 16 Jan 2024 15:16:45 GMT, Jaikiran Pai wrote: >> Glavo 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 six additional commits since >> the last revision: >> >> - Merge branch 'openjdk:master' into zip-utils >> - Merge branch 'openjdk:master' into zip-utils >> - Merge branch 'openjdk:master' into zip-utils >> - Merge branch 'openjdk:master' into zip-utils >> - Merge branch 'openjdk:master' into zip-utils >> - use ByteArrayLittleEndian in ZipUtils > > Hello Glavo, I see that you are interested in pursuing this change further. > Would you mind getting the latest micro benchmark numbers which this proposed > change? I see that your PR description has a run from some time back, getting > a latest one would be useful. > > Additionally, I see that https://github.com/openjdk/jdk/pull/14636 where you > had proposed a test case for the `ByteArrayLittleEndian` class (in addition > to other things) got closed without being integrated. Would you mind adding a > new test case for that class as part of this current PR since you have a few > more new methods being added to that class? @jaikiran Can you review this PR again? - PR Comment: https://git.openjdk.org/jdk/pull/14632#issuecomment-1895683407
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
jtreg:test/hotspot/jtreg:all 6731 670229 0
<<
jtreg:test/jdk:all 9962 995111 0
<<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Any other reviews needed for this? Nominally, this changes the test groups in
langtools, so maybe @lahodaj or @biboudis want to take a look. For jaxp,
@JoeWang-Java, maybe?
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1895680732
Re: RFR: 8275338: Add JFR events for notable serialization situations [v15]
On Mon, 15 Jan 2024 14:17:40 GMT, Raffaello Giulietti wrote: >> Adds serialization misdeclaration events to JFR. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > Removed useless event settings in test. I plan to integrate the last commit by tomorrow, provided `security` folks do not disagree. - PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1895579331
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v5]
On Wed, 17 Jan 2024 10:41:07 GMT, Claes Redestad wrote: >> There's an unused concept of a pluginConfig that is passed into the jlink >> compress plugins, however we always pass null here and the code seems broken >> (the pluginConfig wouldn't have been stored properly). This seem to be a >> left-over from a more generic plugin design that never came to fruition. >> >> This PR cleans out this code from the plugins and decompressors, while >> keeping the compressed header format intact for backwards compatibility (and >> in case we want to revisit this in the future) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Missed a Properties argument in CompressorPluginTest Thanks for reviewing and re-reviewing! I caught and fixed a few issues in testing (tier1-4), extending pre-integration testing to tier5 and taking another manual pass over all related tests before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/17443#issuecomment-1895560774
Re: RFR: 8323794: Remove unused jimage compressor plugin configuration [v5]
> There's an unused concept of a pluginConfig that is passed into the jlink > compress plugins, however we always pass null here and the code seems broken > (the pluginConfig wouldn't have been stored properly). This seem to be a > left-over from a more generic plugin design that never came to fruition. > > This PR cleans out this code from the plugins and decompressors, while > keeping the compressed header format intact for backwards compatibility (and > in case we want to revisit this in the future) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Missed a Properties argument in CompressorPluginTest - Changes: - all: https://git.openjdk.org/jdk/pull/17443/files - new: https://git.openjdk.org/jdk/pull/17443/files/d1917182..0dad1af3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17443&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17443.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17443/head:pull/17443 PR: https://git.openjdk.org/jdk/pull/17443
[jdk22] Integrated: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
On Mon, 15 Jan 2024 16:14:15 GMT, Per Minborg wrote: > 8323159: Consider adding some text re. memory zeroing in Arena::allocate This pull request has now been integrated. Changeset: 887a93b7 Author:Per Minborg URL: https://git.openjdk.org/jdk22/commit/887a93b7c949308b83d4feba714682e8962a3556 Stats: 51 lines in 2 files changed: 49 ins; 0 del; 2 mod 8323159: Consider adding some text re. memory zeroing in Arena::allocate 8323745: Missing comma in copyright header in TestScope Reviewed-by: mcimadamore, alanb Backport-of: f5b757ced6b672010ea10575d644d3f9d1728923 - PR: https://git.openjdk.org/jdk22/pull/77
RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin
Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for correct (Dekker scheme) synchronization with concurrent execution of [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance Suite, and SAP specific tests. Testing was done with fastdebug and release builds on the main platforms and also on Linux/PPC64le and AIX. - Commit messages: - New version of LotsOfInterrupts.java supporting virtual threads - Add Alan's LotsOfInterrupts.java test - Must checkAccess before changing interrupt state of other thread - 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin Changes: https://git.openjdk.org/jdk/pull/17444/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17444&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323782 Stats: 101 lines in 2 files changed: 95 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17444.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17444/head:pull/17444 PR: https://git.openjdk.org/jdk/pull/17444
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin
On Tue, 16 Jan 2024 10:57:46 GMT, Richard Reingruber wrote: > Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for > correct (Dekker scheme) synchronization with concurrent execution of > [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). > > The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot > and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance > Suite, and SAP specific tests. > Testing was done with fastdebug and release builds on the main platforms and > also on Linux/PPC64le and AIX. Thanks for the test @AlanBateman The new test LotsOfInterrupts.java hangs after a few repetitions (using jtreg's REPEAT_COUNT). With the fix it always terminates successfully. - PR Comment: https://git.openjdk.org/jdk/pull/17444#issuecomment-1894154593 PR Comment: https://git.openjdk.org/jdk/pull/17444#issuecomment-1894211061
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v11]
On Sun, 17 Dec 2023 23:11:10 GMT, Chen Liang wrote: >> Summaries: >> 1. A few recommendations about updating the constant API is made at >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html >> and I may update this patch shall the API changes be integrated before >> 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their >> code generation infrastructure upgraded from ASM to Classfile API. >> 3. Most tests are included in tier1, but some are not: >> In `:jdk_io`: (tier2, part 2) >> >> test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java >> test/jdk/java/io/Serializable/records/ProhibitedMethods.java >> test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java >> >> In `:jdk_instrument`: (tier 3) >> >> test/jdk/java/lang/instrument/RetransformAgent.java >> test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java >> test/jdk/java/lang/instrument/asmlib/Instrumentor.java >> >> >> @asotona Would you mind reviewing? > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix the 2 failing tests and add notes Yes, CodeBuilder method renames will require some refactoring. This is good. - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13009#pullrequestreview-1826743185
