Re: RFR: 8265135: Reduce work initializing VarForms
On Tue, 13 Apr 2021 18:11:37 GMT, Claes Redestad wrote: > This patch reduces work done initializing VarForms - mostly observed when > loading each VarHandle implementation class. > > - Lazily resolve MemberNames. > - Streamline MethodType creation. This reduces the number of MethodTypes > created. > > Net effect is a reduction in bytecode executed per VH class by 50-60%. Looks good to me. I also agree with Paul's suggestion to throw InternalError for errors that should never happen. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3472
Re: RFR: 8265174: Update Class.getDeclaredMethods to discuss synthetic and bridge methods [v2]
> The results from Class.getDeclaredMethods can include bridge and other > synthetic methods, which can be unexpected by users (JDK-6815786, > JDK-8142904) and appear to be inherited methods. The javadoc for > Class.getDeclaredMethods should be updated to explicitly mention the > possibility of synthetic methods appearing. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Add links to discussion in java.lang.reflect package javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3477/files - new: https://git.openjdk.java.net/jdk/pull/3477/files/696daa59..dc1a5381 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3477=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3477=00-01 Stats: 24 lines in 5 files changed: 22 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3477.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3477/head:pull/3477 PR: https://git.openjdk.java.net/jdk/pull/3477
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v2]
On Tue, 13 Apr 2021 17:56:12 GMT, Naoto Sato wrote: >Have you run regression tests in java.time? If I am not mistaken, your changes >simply seem to nullify the day period support. Hello @naotoj, my tier1 test run passed without issues locally with this change: == Test summary == TEST TOTAL PASS FAIL ERROR >> jtreg:test/hotspot/jtreg:tier1 1750 1746 4 0 << jtreg:test/jdk:tier1 2022 2022 0 0 jtreg:test/langtools:tier1 4158 4158 0 0 jtreg:test/jaxp:tier1 0 0 0 0 jtreg:test/lib-test:tier1 0 0 0 0 == (the 4 hotspot failures are unrelated environmental issues). After seeing your message, I now ran the `java/time` regression tests: jtreg -jdk:build/macosx-x86_64-server-release/images/jdk/ test/jdk/java/time/ Test results: passed: 184 and even those passed. I don't have much knowledge in this area of the code, but as far as I can see, this doesn't touch the changes introduced in the java.time to support the new day period construct. - PR: https://git.openjdk.java.net/jdk/pull/3463
ObjectMethods seems generating wrong methods for array-type field
Hello there, I'm Kengo TODA, an engineer learning about the Record feature. Today I found a nonintentional behavior, and it seems that the bug database has no info about it. Let me ask here to confirm it's by-design or not. If it's a bug, I want to try to send a patch to OpenJDK. Here is the code reproducing the nonintentional behavior: https://gist.github.com/KengoTODA/4d7ef6a5226d347ad9385241fee6bc63 In short, the ObjectMethods class in OpenJDK v16 generates code that invokes the fields' method, even when the field is an array. Please help me to understand this behavior, or make an entry in the bug database to propose a patch. Thanks in advance! :) *** Kengo TODA skypen...@gmail.com
Re: RFR: 8265135: Reduce work initializing VarForms
On Wed, 14 Apr 2021 00:35:38 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/invoke/VarForm.java line 130:
>>
>>> 128: } catch (NoSuchMethodException | IllegalAccessException e) {
>>> 129: throw new UnsupportedOperationException();
>>> 130: }
>>
>> Suggestion:
>>
>> } catch (ReflectiveOperationException e) {
>> throw newInternalError("Failed resolving VarHandle member
>> name", ex);
>> }
>
> Thanks for reviewing!
>
> Is there's a way to provoke this exception through the public API? If not
> then the suggested behavior change seems reasonable.
No, since VarHandles are not publicly extensible, the exception should not
occur unless something has gone very wrong (the correspondence between access
mode and implementing method is broken).
-
PR: https://git.openjdk.java.net/jdk/pull/3472
Re: RFR: 8265135: Reduce work initializing VarForms
On Tue, 13 Apr 2021 23:09:12 GMT, Paul Sandoz wrote:
>> This patch reduces work done initializing VarForms - mostly observed when
>> loading each VarHandle implementation class.
>>
>> - Lazily resolve MemberNames.
>> - Streamline MethodType creation. This reduces the number of MethodTypes
>> created.
>>
>> Net effect is a reduction in bytecode executed per VH class by 50-60%.
>
> src/java.base/share/classes/java/lang/invoke/VarForm.java line 130:
>
>> 128: } catch (NoSuchMethodException | IllegalAccessException e) {
>> 129: throw new UnsupportedOperationException();
>> 130: }
>
> Suggestion:
>
> } catch (ReflectiveOperationException e) {
> throw newInternalError("Failed resolving VarHandle member
> name", ex);
> }
Thanks for reviewing!
Is there's a way to provoke this exception through the public API? If not then
the suggested behavior change seems reasonable.
-
PR: https://git.openjdk.java.net/jdk/pull/3472
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v3]
On Tue, 13 Apr 2021 22:50:12 GMT, Andy Herrick wrote: >> two changes: >> One to jpackage, when recursively removing directory, when IOException >> occurs, record it and continue (deleting as much as possible) before >> throwing the exception. >> The other to tests, when running jpackage via ProcessBuilder.execute(), set >> the "TMP" environment variable to the current value of System Property >> "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp >> files to the tmp file location used by the test. (So the test harness can >> clean up after test exits). > > Andy Herrick has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8265078: jpackage tests on Windows leave large temp files This looks fine. I see that this bug is listed as Windows-specific. Are any similar changes needed for other platforms, or do they already write test files only to `java.io.tmpdir`? - Marked as reviewed by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: 8265135: Reduce work initializing VarForms
On Tue, 13 Apr 2021 18:11:37 GMT, Claes Redestad wrote:
> This patch reduces work done initializing VarForms - mostly observed when
> loading each VarHandle implementation class.
>
> - Lazily resolve MemberNames.
> - Streamline MethodType creation. This reduces the number of MethodTypes
> created.
>
> Net effect is a reduction in bytecode executed per VH class by 50-60%.
Very nice.
src/java.base/share/classes/java/lang/invoke/VarForm.java line 130:
> 128: } catch (NoSuchMethodException | IllegalAccessException e) {
> 129: throw new UnsupportedOperationException();
> 130: }
Suggestion:
} catch (ReflectiveOperationException e) {
throw newInternalError("Failed resolving VarHandle member
name", ex);
}
-
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3472
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v3]
> two changes: > One to jpackage, when recursively removing directory, when IOException > occurs, record it and continue (deleting as much as possible) before throwing > the exception. > The other to tests, when running jpackage via ProcessBuilder.execute(), set > the "TMP" environment variable to the current value of System Property > "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files > to the tmp file location used by the test. (So the test harness can clean up > after test exits). Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8265078: jpackage tests on Windows leave large temp files - Changes: - all: https://git.openjdk.java.net/jdk/pull/3473/files - new: https://git.openjdk.java.net/jdk/pull/3473/files/e78442ba..4b9e59f6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3473=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3473=01-02 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3473/head:pull/3473 PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
On Tue, 13 Apr 2021 22:00:57 GMT, Remi Forax wrote: > About your benchmark, did you test with some strings going into "default", > because it is usually in that case that you need a proper lookup switch, another way to say it is that, your results are too good when you use a cascade of guardWithTest. Yes, for the benchmarks I ran, the default case was just as likely as the other cases, so e.g. if there were 10 cases, there was a 1/11 chance the default case was hit. This might need tweaking to be more realistic but... Note that the cascading guard with test actually works more like a binary search, where each guard tests against a pivot point in the search, and then decides to go either to the left or the right side of the tree. So, when looking up the default value we don't necessarily need to do a search over all the cases. Only for hash collisions does it fall back to a linear search over all the values with the same hash code. This is also how C2 translates `lookupswitch` as far as I know (but maybe John can confirm or deny whether my reading of the C2 code is correct), so I'm not surprised to see that the if-tree approach is so close to a native `lookupswitch` instruction in performance. - PR: https://git.openjdk.java.net/jdk/pull/3401
Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
On Tue, 13 Apr 2021 22:00:57 GMT, Remi Forax wrote: > About your benchmark, did you test with some strings going into "default", > because it is usually in that case that you need a proper lookup switch, another way to say it is that, your results are too good when you use a cascade of guardWithTest. Yes, for the benchmarks I ran, the default case was just as likely as the other cases, so e.g. if there were 10 cases, there was a 1/11 chance the default case was hit. This might need tweaking to be more realistic but... Note that the cascading guard with test actually works more like a binary search, where each guard tests against a pivot point in the search, and then decides to go either to the left or the right side of the tree. So, when looking up the default value we don't necessarily need to do a search over all the cases. Only for hash collisions does it fall back to a linear search over all the values with the same hash code. This is also how C2 translates `lookupswitch` as far as I know (but maybe John can confirm or deny whether my reading of the C2 code is correct), so I'm not surprised to see that the if-tree approach is so close to a native `lookupswitch` instruction in performance. - PR: https://git.openjdk.java.net/jdk/pull/3401
RFR: 8265174: Update Class.getDeclaredMethods to discuss synethetic and bridge methods
The results from Class.getDeclaredMethods can include bridge and other synthetic methods, which can be unexpected by users (JDK-6815786, JDK-8142904) and appear to be inherited methods. The javadoc for Class.getDeclaredMethods should be updated to explicitly mention the possibility of synthetic methods appearing. - Commit messages: - 8265174: Update Class.getDeclaredMethods to discuss synethetic and bridge methods Changes: https://git.openjdk.java.net/jdk/pull/3477/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3477=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265174 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3477.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3477/head:pull/3477 PR: https://git.openjdk.java.net/jdk/pull/3477
Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
- Mail original -
> De: "Jorn Vernee"
> À: "core-libs-dev"
> Envoyé: Mardi 13 Avril 2021 16:59:58
> Objet: Re: RFR: 8263087: Add a MethodHandle combinator that switches over a
> set of MethodHandles
> On Thu, 8 Apr 2021 18:51:21 GMT, Jorn Vernee wrote:
>
>> This patch adds a `tableSwitch` combinator that can be used to switch over a
>> set
>> of method handles given an index, with a fallback in case the index is out of
>> bounds, much like the `tableswitch` bytecode. Here is a description of how it
>> works (copied from the javadoc):
>>
>> Creates a table switch method handle, which can be used to switch over
>> a set of
>> target
>> method handles, based on a given target index, called selector.
>>
>> For a selector value of {@code n}, where {@code n} falls in the range
>> {@code [0,
>> N)},
>> and where {@code N} is the number of target method handles, the table
>> switch
>> method
>> handle will invoke the n-th target method handle from the list of
>> target method
>> handles.
>>
>> For a selector value that does not fall in the range {@code [0, N)},
>> the table
>> switch
>> method handle will invoke the given fallback method handle.
>>
>> All method handles passed to this method must have the same type, with
>> the
>> additional
>> requirement that the leading parameter be of type {@code int}. The
>> leading
>> parameter
>> represents the selector.
>>
>> Any trailing parameters present in the type will appear on the returned
>> table
>> switch
>> method handle as well. Any arguments assigned to these parameters will
>> be
>> forwarded,
>> together with the selector value, to the selected method handle when
>> invoking
>> it.
>>
>> The combinator does not support specifying the starting index, so the switch
>> cases always run from 0 to however many target handles are specified. A
>> starting index can be added manually with another combination step that
>> filters
>> the input index by adding or subtracting a constant from it, which does not
>> affect performance. One of the reasons for not supporting a starting index is
>> that it allows for more lambda form sharing, but also simplifies the
>> implementation somewhat. I guess an open question is if a convenience
>> overload
>> should be added for that case?
>>
>> Lookup switch can also be simulated by filtering the input through an
>> injection
>> function that translates it into a case index, which has also proven to have
>> the ability to have comparable performance to, or even better performance
>> than,
>> a bytecode-native `lookupswitch` instruction. I plan to add such an injection
>> function to the runtime libraries in the future as well. Maybe at that point
>> it
>> could be evaluated if it's worth it to add a lookup switch combinator as
>> well,
>> but I don't see an immediate need to include it in this patch.
>>
>> The current bytecode intrinsification generates a call for each switch case,
>> which guarantees full inlining of the target method handles. Alternatively we
>> could only have 1 callsite at the end of the switch, where each case just
>> loads
>> the target method handle, but currently this does not allow for inlining of
>> the
>> handles, since they are not constant.
>>
>> Maybe a future C2 optimization could look at the receiver input for
>> invokeBasic
>> call sites, and if the input is a phi node, clone the call for each constant
>> input of the phi. I believe that would allow simplifying the bytecode without
>> giving up on inlining.
>>
>> Some numbers from the added benchmarks:
>>
>> Benchmark(numCases) (offset)
>> (sorted)
>> Mode Cnt Score Error Units
>> MethodHandlesTableSwitchConstant.testSwitch 5 0
>> N/A
>> avgt 30 4.186 � 0.054 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 5 150
>> N/A
>> avgt 30 4.164 � 0.057 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 10 0
>> N/A
>> avgt 30 4.124 � 0.023 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 10 150
>> N/A
>> avgt 30 4.126 � 0.025 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 25 0
>> N/A
>> avgt 30 4.137 � 0.042 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 25 150
>> N/A
>> avgt 30 4.113 � 0.016 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 50 0
>> N/A
>> avgt 30 4.118 � 0.028 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 50 150
>> N/A
>> avgt 30 4.127 � 0.019 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 100 0
>> N/A
>> avgt 30 4.116 � 0.013 ms/op
>> MethodHandlesTableSwitchConstant.testSwitch 100 150
Re: [External] : Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
> De: "John Rose" > À: "Remi Forax" > Cc: "Jorn Vernee" , "core-libs-dev" > > Envoyé: Samedi 10 Avril 2021 01:43:49 > Objet: Re: [External] : Re: RFR: 8263087: Add a MethodHandle combinator that > switches over a set of MethodHandles > On Apr 9, 2021, at 4:00 PM, John Rose < [ mailto:john.r.r...@oracle.com | > john.r.r...@oracle.com ] > wrote: >> The MH combinator for lookupswitch can use a data-driven >> reverse lookup in a (frozen/stable) int[] array, using binary >> search. The bytecode emitter can render such a thing as >> an internal lookupswitch, if that seems desirable. But >> the stable array with binary search scales to other types >> besides int, so it’s the right primitive. > This may be confusing on a couple of points. > First, the mapping function I’m talking about is not > a MH combinator, but rather a MH factory, which takes > a non-MH argument, probably an unsorted array or List > of items of any type. It then DTRT (internal hash map > or tree map or flat binary search or flat table lookup or > lookupswitch or any combination thereof) to get > an algorithm to classify the array or List elements > into a compact enumeration [0,N). > Second, when the input array or List is of int (or > Integer) then it *might* be a lookupswitch internally, > and I’m abusing the terminology to call this particular > case a lookupswitch. But it’s really just a classifier > factory, whose output is a MH of type K → [0,N) for > some K. The output might also be ToIntFunction > for all I care; that can be inter-converted with a MH. As you said, the classifier is either a lookup switch or a hashmap.get() or a perfect hash function like ordinal(). The last two can be already be seen as MH, that you can already compose. The only one we can not currently, without generating bytecode, is the lookup switch, so we should have a lookupswitch combinator. This does not mean we do not need the tableswitch combinator, it means we need both. Firthermore, if we do have both combinators, there is no need to a special mechanism, or am i missing something ? Rémi
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]
On Tue, 13 Apr 2021 18:56:22 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the following patch which adds additional permissions needed >> for when JTREG upgrades to a newer version of TestNG. >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > TestNG updates Permission re-organization Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3471
Integrated: 8263157: [macos]: java.library.path is being set incorrectly
On Mon, 12 Apr 2021 20:18:46 GMT, Alexander Matveev wrote: > This is regression from JDK-8242302. Fixed by setting java.library.path to > same values as it was before JDK-8242302. This pull request has now been integrated. Changeset: 55d56495 Author:Alexander Matveev URL: https://git.openjdk.java.net/jdk/commit/55d56495 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8263157: [macos]: java.library.path is being set incorrectly Reviewed-by: asemenyuk, herrick - PR: https://git.openjdk.java.net/jdk/pull/3443
Re: RFR: 8264208: Console charset API [v6]
On Tue, 13 Apr 2021 19:59:30 GMT, Naoto Sato wrote: >> Please review the changes for the subject issue. This has been suggested in >> a recent discussion thread for the JEP 400 >> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. >> A CSR has also been drafted, and comments are welcome >> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added comment to System.out/err init. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]
On Tue, 13 Apr 2021 18:56:22 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the following patch which adds additional permissions needed >> for when JTREG upgrades to a newer version of TestNG. >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > TestNG updates Permission re-organization Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8264806: Remove the experimental JIT compiler [v3]
On Mon, 12 Apr 2021 22:10:06 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Java-based JIT compiler (Graal) from JDK: >> >> - `jdk.internal.vm.compiler` — the Graal compiler >> - `jdk.internal.vm.compiler.management` — Graal's `MBean` >> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests >> >> Remove Graal related code in makefiles. >> >> Note, next two `module-info.java` files are preserved so that the JVMCI >> module `jdk.internal.vm.ci` continues to build: >> >> src/jdk.internal.vm.compiler/share/classes/module-info.java >> src/jdk.internal.vm.compiler.management/share/classes/module-info.java >> >> >> @AlanBateman suggested that we can avoid it by using Module API to export >> packages at runtime . It requires changes in GraalVM's JVMCI too so I will >> file followup RFE to implement it. >> >> Tested hs-tier1-4 > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Restore Compiler::isGraalEnabled() Approved. - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v2]
On Tue, 13 Apr 2021 21:05:26 GMT, Andy Herrick wrote:
>> two changes:
>> One to jpackage, when recursively removing directory, when IOException
>> occurs, record it and continue (deleting as much as possible) before
>> throwing the exception.
>> The other to tests, when running jpackage via ProcessBuilder.execute(), set
>> the "TMP" environment variable to the current value of System Property
>> "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp
>> files to the tmp file location used by the test. (So the test harness can
>> clean up after test exits).
>
> Andy Herrick has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8265078: jpackage tests on Windows leave large temp files
Are you sure you need an `AtomicReference` to set the exception? I don't see
any use of multiple threads, but I might be missing something. If you do need
it, you need to fix the order of arguments.
src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 96:
> 94: Files.delete(dir);
> 95: } catch (IOException ex) {
> 96: exception.compareAndSet(ex, null);
The arguments are backwards. The first argument is the one used for comparison,
and if the current reference is equal to the first, it is set to the second
value.
-
Changes requested by kcr (Author).
PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files [v2]
> two changes: > One to jpackage, when recursively removing directory, when IOException > occurs, record it and continue (deleting as much as possible) before throwing > the exception. > The other to tests, when running jpackage via ProcessBuilder.execute(), set > the "TMP" environment variable to the current value of System Property > "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files > to the tmp file location used by the test. (So the test harness can clean up > after test exits). Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8265078: jpackage tests on Windows leave large temp files - Changes: - all: https://git.openjdk.java.net/jdk/pull/3473/files - new: https://git.openjdk.java.net/jdk/pull/3473/files/69949578..e78442ba Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3473=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3473=00-01 Stats: 19 lines in 3 files changed: 3 ins; 4 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/3473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3473/head:pull/3473 PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files
On Tue, 13 Apr 2021 20:31:38 GMT, Andy Herrick wrote: >> That seems like overkill. walkFileTree must call visitFile, >> preVisitDirectory, and postVisitDirectory synchronously, because their >> return value tells walkFileTree where to go next. > > I can use AtomicReference instead of Array to hold the IOException, but must > I lock around access, there is no "setIfNull()" method Actually there is: compareAndSet(newValue, null) - PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files
On Tue, 13 Apr 2021 20:26:56 GMT, Andy Herrick wrote:
>> src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:
>>
>>> 57:
>>> 58: public static void deleteRecursive(Path directory) throws
>>> IOException {
>>> 59: final IOException [] exception = { (IOException) null };
>>
>> Do we know `Files.walkFileTree()` synchronizes calls on callback object? If
>> not, I'd use `AtomicReference` to store the first exception.
>
> That seems like overkill. walkFileTree must call visitFile,
> preVisitDirectory, and postVisitDirectory synchronously, because their return
> value tells walkFileTree where to go next.
I can use AtomicReference instead of Array to hold the IOException, but must I
lock around access, there is no "setIfNull()" method
-
PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files
On Tue, 13 Apr 2021 19:48:24 GMT, Alexey Semenyuk wrote:
>> two changes:
>> One to jpackage, when recursively removing directory, when IOException
>> occurs, record it and continue (deleting as much as possible) before
>> throwing the exception.
>> The other to tests, when running jpackage via ProcessBuilder.execute(), set
>> the "TMP" environment variable to the current value of System Property
>> "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp
>> files to the tmp file location used by the test. (So the test harness can
>> clean up after test exits).
>
> src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:
>
>> 57:
>> 58: public static void deleteRecursive(Path directory) throws
>> IOException {
>> 59: final IOException [] exception = { (IOException) null };
>
> Do we know `Files.walkFileTree()` synchronizes calls on callback object? If
> not, I'd use `AtomicReference` to store the first exception.
That seems like overkill. walkFileTree must call visitFile, preVisitDirectory,
and postVisitDirectory synchronously, because their return value tells
walkFileTree where to go next.
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java line
> 646:
>
>> 644: } else {
>> 645: exec.setExecutable(JavaTool.JPACKAGE);
>> 646: exec.setTmpDir(System.getProperty("java.io.tmpdir"));
>
> This would work only on Windows. I'd put corresponding `if` around this
> statement to avoid future confusion.
That makes sense, should this be here in JPackageCommand.CreateExecutor() or in
Executor.setTempDir() (maybe renamed setWinTempDir ?)
-
PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: 8264208: Console charset API [v6]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added comment to System.out/err init. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/cafde56a..9e323145 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 19:30:53 GMT, Joe Wang wrote: >> Although the code path is different, the logic to determine the encoding is >> not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked >> from a terminal (in fact, there's a bug where they aren't set even in a >> terminal on unix env, which I fixed in this patch) which is the same >> condition in which `System.console()` returns the console. And for both >> cases, it will default to `Charset.defaultCharset()` if they are not >> available. >> >> Having said that, one regression test started to fail with this >> implementation change as follows: >> >> Exception in thread "main" java.util.ServiceConfigurationError: Locale >> provider adapter "CLDR"cannot be instantiated. >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) >> at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) >> at >> java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) >> at java.base/java.util.Scanner.useLocale(Scanner.java:1270) >> at java.base/java.util.Scanner.(Scanner.java:543) >> at java.base/java.util.Scanner.(Scanner.java:554) >> at TestConsole.main(TestConsole.java:54) >> Caused by: java.lang.reflect.InvocationTargetException >> at >> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native >> Method) >> at >> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) >> at >> java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) >> at >> java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) >> at >> java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) >> ... 8 more >> >> I haven't looked at it in detail but it seems that calling >> `System.console()` in `System.initPhase1()` is not allowed, as the module >> system is not there yet. So I reverted the implementation to the original >> and simply retained the description in `System.out/err` with a change to >> `determined by` to `equivalent to`. > > Thanks for the explanation and updates. It's a worthwhile exercise to attempt > to align things around the new method. A short note above line 2023 to record > this might be useful as well (sth. like it's eq to console != null ? > console.charset() : Charset.defaultCharset();). No need to create another > update if you decide to add the note. Thanks. Added some explanations as suggested. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: JDK-8265078: jpackage tests on Windows leave large temp files
On Tue, 13 Apr 2021 18:57:20 GMT, Andy Herrick wrote:
> two changes:
> One to jpackage, when recursively removing directory, when IOException
> occurs, record it and continue (deleting as much as possible) before throwing
> the exception.
> The other to tests, when running jpackage via ProcessBuilder.execute(), set
> the "TMP" environment variable to the current value of System Property
> "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files
> to the tmp file location used by the test. (So the test harness can clean up
> after test exits).
Changes requested by asemenyuk (Reviewer).
src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:
> 57:
> 58: public static void deleteRecursive(Path directory) throws IOException
> {
> 59: final IOException [] exception = { (IOException) null };
Do we know `Files.walkFileTree()` synchronizes calls on callback object? If
not, I'd use `AtomicReference` to store the first exception.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/Executor.java line 92:
> 90: }
> 91:
> 92: public Executor setTmpDir(String tmp) {
As this would work only on Windows, I'd throw `IllegalStateException` if the
method is called on other platform.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java line 646:
> 644: } else {
> 645: exec.setExecutable(JavaTool.JPACKAGE);
> 646: exec.setTmpDir(System.getProperty("java.io.tmpdir"));
This would work only on Windows. I'd put corresponding `if` around this
statement to avoid future confusion.
-
PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 18:24:55 GMT, Naoto Sato wrote: >> src/java.base/share/classes/java/lang/System.java line 2020: >> >>> 2018: setIn0(new BufferedInputStream(fdIn)); >>> 2019: setOut0(newPrintStream(fdOut, cs)); >>> 2020: setErr0(newPrintStream(fdErr, cs)); >> >> It was getting from sun.stdout.encoding or sun.stderr.encoding. After the >> change, when there is no console, it would be set with >> Charset.defaultCharset(). Would that be an incompatible change? > > Although the code path is different, the logic to determine the encoding is > not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked > from a terminal (in fact, there's a bug where they aren't set even in a > terminal on unix env, which I fixed in this patch) which is the same > condition in which `System.console()` returns the console. And for both > cases, it will default to `Charset.defaultCharset()` if they are not > available. > > Having said that, one regression test started to fail with this > implementation change as follows: > > Exception in thread "main" java.util.ServiceConfigurationError: Locale > provider adapter "CLDR"cannot be instantiated. > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) > at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) > at > java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) > at java.base/java.util.Scanner.useLocale(Scanner.java:1270) > at java.base/java.util.Scanner.(Scanner.java:543) > at java.base/java.util.Scanner.(Scanner.java:554) > at TestConsole.main(TestConsole.java:54) > Caused by: java.lang.reflect.InvocationTargetException > at > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native > Method) > at > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) > at > java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) > at > java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) > at > java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) > ... 8 more > > I haven't looked at it in detail but it seems that calling `System.console()` > in `System.initPhase1()` is not allowed, as the module system is not there > yet. So I reverted the implementation to the original and simply retained the > description in `System.out/err` with a change to `determined by` to > `equivalent to`. Thanks for the explanation and updates. It's a worthwhile exercise to attempt to align things around the new method. A short note above line 2023 to record this might be useful as well (sth. like it's eq to console != null ? console.charset() : Charset.defaultCharset();). No need to create another update if you decide to add the note. - PR: https://git.openjdk.java.net/jdk/pull/3419
RFR: JDK-8265078: jpackage tests on Windows leave large temp files
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits). - Commit messages: - JDK-8265078: jpackage tests on Windows leave large temp files Changes: https://git.openjdk.java.net/jdk/pull/3473/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3473=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265078 Stats: 29 lines in 3 files changed: 27 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3473/head:pull/3473 PR: https://git.openjdk.java.net/jdk/pull/3473
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]
> Hi all, > > Please review the following patch which adds additional permissions needed > for when JTREG upgrades to a newer version of TestNG. > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: TestNG updates Permission re-organization - Changes: - all: https://git.openjdk.java.net/jdk/pull/3471/files - new: https://git.openjdk.java.net/jdk/pull/3471/files/0183f88b..20ef2bd0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3471=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3471=00-01 Stats: 20 lines in 2 files changed: 10 ins; 10 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3471/head:pull/3471 PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8265019 : Update tests for additional TestNG test permissions
On Tue, 13 Apr 2021 18:01:49 GMT, Lance Andersen wrote:
> Hi all,
>
> Please review the following patch which adds additional permissions needed
> for when JTREG upgrades to a newer version of TestNG.
>
> Best,
> Lance
test/jdk/java/lang/ProcessHandle/PermissionTest.java line 215:
> 213: permissions.add(new PropertyPermission("testng.report.xml.name",
> "read"));
> 214: permissions.add(new ReflectPermission("suppressAccessChecks"));
> 215: permissions.add(new PropertyPermission("testng.timezone",
> "read"));
might be better to group the testng.* properties so they aren't mixed up with
the other runtime permissions.
-
PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 13:04:17 GMT, Alan Bateman wrote: > 1. I think method name "charset()" is too short. It's not called frequently. > This method name should explain functionality. As for this one, I am open for suggestions. I thought `consoel()` was concise, and analogous to `Charset.defaultCharset()`. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 02:34:15 GMT, Joe Wang wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reverted PrintStream changes > > src/java.base/share/classes/java/lang/System.java line 2020: > >> 2018: setIn0(new BufferedInputStream(fdIn)); >> 2019: setOut0(newPrintStream(fdOut, cs)); >> 2020: setErr0(newPrintStream(fdErr, cs)); > > It was getting from sun.stdout.encoding or sun.stderr.encoding. After the > change, when there is no console, it would be set with > Charset.defaultCharset(). Would that be an incompatible change? Although the code path is different, the logic to determine the encoding is not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked from a terminal (in fact, there's a bug where they aren't set even in a terminal on unix env, which I fixed in this patch) which is the same condition in which `System.console()` returns the console. And for both cases, it will default to `Charset.defaultCharset()` if they are not available. Having said that, one regression test started to fail with this implementation change as follows: Exception in thread "main" java.util.ServiceConfigurationError: Locale provider adapter "CLDR"cannot be instantiated. at java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) at java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) at java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) at java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) at java.base/java.util.Scanner.useLocale(Scanner.java:1270) at java.base/java.util.Scanner.(Scanner.java:543) at java.base/java.util.Scanner.(Scanner.java:554) at TestConsole.main(TestConsole.java:54) Caused by: java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) at java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) ... 8 more I haven't looked at it in detail but it seems that calling `System.console()` in `System.initPhase1()` is not allowed, as the module system is not there yet. So I reverted the implementation to the original and simply retained the description in `System.out/err` with a change to `determined by` to `equivalent to`. - PR: https://git.openjdk.java.net/jdk/pull/3419
RFR: 8265135: Reduce work initializing VarForms
This patch reduces work done initializing VarForms - mostly observed when loading each VarHandle implementation class. - Lazily resolve MemberNames. - Streamline MethodType creation. This reduces the number of MethodTypes created. Net effect is a reduction in bytecode executed per VH class by 50-60%. - Commit messages: - Reduce work initializing VarForms Changes: https://git.openjdk.java.net/jdk/pull/3472/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3472=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265135 Stats: 77 lines in 2 files changed: 37 ins; 30 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3472.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3472/head:pull/3472 PR: https://git.openjdk.java.net/jdk/pull/3472
RFR: 8265019 : Update tests for additional TestNG test permissions
Hi all, Please review the following patch which adds additional permissions needed for when JTREG upgrades to a newer version of TestNG. Best, Lance - Commit messages: - TestNG updates Changes: https://git.openjdk.java.net/jdk/pull/3471/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3471=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265019 Stats: 21 lines in 2 files changed: 11 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3471/head:pull/3471 PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8264208: Console charset API [v5]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflected further review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/68db1a79..cafde56a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=03-04 Stats: 14 lines in 1 file changed: 6 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v2]
On Tue, 13 Apr 2021 15:03:28 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for >> https://bugs.openjdk.java.net/browse/JDK-8262108? >> >> As noted in a comment in that issue, the bug relates to the return value of >> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The >> implementation has started returning invalid values for the `AM_PM` field >> after the "day period" support was added recently in the JDK as part of >> https://bugs.openjdk.java.net/browse/JDK-8262108. >> >> The commit here adds a check in the internal implementation of the display >> name handling logic, to special case the `AM_PM` field and properly convert >> the display name array indexes (which is an internal detail) to valid values >> that represent the `AM_PM` calendar field. >> >> The commit also has a new jtreg test case `CalendarDisplayNamesTest` >> reproduces this issue and verifies the fix. >> >> After this fix was introduced, I ran the test in >> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing >> test case `NarrowNamesTest`. Looking at that test case, IMO, the current >> testing in that `NarrowNamesTest` is incorrect and is probably what hid this >> issue in the first place? To fix this, I have added an additional commit >> which updates this test case to properly test the `AM_PM` field values. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - copyright year and @bug id reference in existing test > - copyright year on source Have you run regression tests in java.time? If I am not mistaken, your changes simply seem to nullify the day period support. - PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Tue, 13 Apr 2021 09:30:23 GMT, Doug Simon wrote: >> We would definitely like to be able to continue testing of GraalVM with the >> JDK set of jtreg tests. So keeping `Compiler::isGraalEnabled()` working like >> it does today is important. > >> @dougxc I restored Compiler::isGraalEnabled(). > > Thanks. I guess this should really be named `isJVMCICompilerEnabled` now and > the `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but > maybe that's too big a change (or can be done later). @dougxc Renaming should be done separately. May be use RFE I filed: https://bugs.openjdk.java.net/browse/JDK-8265032 Do you approve these Graal removal changes? - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented
On Tue, 13 Apr 2021 16:33:21 GMT, Jim Laskey wrote: > Move makeXXXSpilterator from public (@hidden) to protected. No API ch This looks exactly like my proposed solution! +1 Thanks! Just my question: Is `@hidden` not needed to remove the documentation from protected method? Or is this automatically hidden by javadoc? - Marked as reviewed by uschindler (Author). PR: https://git.openjdk.java.net/jdk/pull/3469
RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented
Move makeXXXSpilterator from public (@hidden) to protected. No API ch - Commit messages: - Move makeXXXSpilterator from public (@hidden) to protected. No API change. Changes: https://git.openjdk.java.net/jdk/pull/3469/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3469=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265137 Stats: 107 lines in 4 files changed: 0 ins; 91 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/3469.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3469/head:pull/3469 PR: https://git.openjdk.java.net/jdk/pull/3469
Re: RFR: 8265079: Implement VarHandle invoker caching [v2]
On Tue, 13 Apr 2021 12:25:20 GMT, Jorn Vernee wrote:
>> This patch implements 2 leftover TODOs for implementing var handle invoker
>> MH caching (lambda forms for those were already shared/cached).
>>
>> This piggybacks on the existing mechanism for method handle invoker caching.
>>
>> Testing: Local testing `java/lang/invoke` tests. Tier 1-3
>>
>> Thanks,
>> Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comments:
> - Use boolean instead of index for var handle cache
test/jdk/java/lang/invoke/TestVHInvokerCaching.java line 88:
> 86: MethodHandles.Lookup lookup = lookup();
> 87:
> 88: for (Class type : TEST_TYPES) {
A simpler approach would be to iterate over the fields of `Holder` and use
`unreflectVarHandle`, then you can remove `TEST_TYPES`.
-
PR: https://git.openjdk.java.net/jdk/pull/3439
Re: RFR: 8265075: Improve and simplify Class.resolveName()
Hi, Sergey!
Have you measured the code change in the java.lang.Class itself or just
equivalent code in the JMH test as you show us?
The JMH test may show better results as it is compiled to bytecode that
uses special invokedynamic-based string concatenation with optimal MH
based underlying strategy. The code in java.lang.Class can't be compiled
to use this kind of concatenation because of bootstraping issues and is
therefore compiled to bytecode that uses StringBuilder directly (much
like the existing code of the patched method). So I'm wondering whether
the improvement in speed is actually there...
Regards, Peter
On 4/13/21 2:55 PM, Сергей Цыпанов wrote:
In mentioned method this code snippet
int len = baseName.length() + 1 + name.length();
StringBuilder sb = new StringBuilder(len);
name = sb.append(baseName.replace('.', '/'))
.append('/')
.append(name)
.toString();
can be simplified with performance improvement as
name = baseName.replace('.', '/') + '/' + name;
Also null check of `baseName` can be removed as Class.getPackageName() never
returns null.
This benchmark
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class ResolveNameBenchmark {
private final Class klazz = getClass();
@Benchmark
public Object original() {
return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
}
@Benchmark
public Object patched() {
return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
}
private String original(String name) {
if (!name.startsWith("/")) {
String baseName = getPackageName();
if (baseName != null && !baseName.isEmpty()) {
int len = baseName.length() + 1 + name.length();
StringBuilder sb = new StringBuilder(len);
name = sb.append(baseName.replace('.', '/'))
.append('/')
.append(name)
.toString();
}
} else {
name = name.substring(1);
}
return name;
}
private String patched(String name) {
if (!name.startsWith("/")) {
String baseName = getPackageName();
if (!baseName.isEmpty()) {
return baseName.replace('.', '/') + '/' + name;
}
return name;
}
return name.substring(1);
}
private String getPackageName() {
return klazz.getPackageName();
}
}
demonstrates good improvement, especially as of memory consumption:
Mode Cnt Score Error
Units
originalavgt 5057.974 ± 0.365
ns/op
original:·gc.alloc.rate avgt 50 3419.447 ± 21.154
MB/sec
original:·gc.alloc.rate.normavgt 50 312.006 ± 0.001
B/op
original:·gc.churn.G1_Eden_Spaceavgt 50 3399.396 ± 149.836
MB/sec
original:·gc.churn.G1_Eden_Space.norm avgt 50 310.198 ± 13.628
B/op
original:·gc.churn.G1_Survivor_Spaceavgt 50 0.004 ± 0.001
MB/sec
original:·gc.churn.G1_Survivor_Space.norm avgt 50≈ 10⁻³
B/op
original:·gc.count avgt 50 208.000
counts
original:·gc.time avgt 50 224.000
ms
patched avgt 5044.367 ± 0.162
ns/op
patched:·gc.alloc.rate avgt 50 2749.265 ± 10.014
MB/sec
patched:·gc.alloc.rate.norm avgt 50 192.004 ± 0.001
B/op
patched:·gc.churn.G1_Eden_Space avgt 50 2729.221 ± 193.552
MB/sec
patched:·gc.churn.G1_Eden_Space.normavgt 50 190.615 ± 13.539
B/op
patched:·gc.churn.G1_Survivor_Space avgt 50 0.003 ± 0.001
MB/sec
patched:·gc.churn.G1_Survivor_Space.normavgt 50≈ 10⁻⁴
B/op
patched:·gc.count avgt 50 167.000
counts
patched:·gc.timeavgt 50 181.000
ms
-
Commit messages:
- 8265075: Improve and simplify Class.resolveName()
Changes: https://git.openjdk.java.net/jdk/pull/3464/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3464=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8265075
Stats: 11 lines in 1 file changed: 0 ins; 7 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/3464.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3464/head:pull/3464
PR: https://git.openjdk.java.net/jdk/pull/3464
Integrated: 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64
On Tue, 13 Apr 2021 06:55:33 GMT, Mikael Vidstedt wrote: > Let's problem list java/util/concurrent/locks/Lock/TimedAcquireLeak.java (a > tier1 test) until JDK-8262897 is fixed. This pull request has now been integrated. Changeset: 2aae29c9 Author:Mikael Vidstedt URL: https://git.openjdk.java.net/jdk/commit/2aae29c9 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64 Reviewed-by: akozlov, hseigel - PR: https://git.openjdk.java.net/jdk/pull/3452
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v2]
> Can I please get a review for this proposed fix for > https://bugs.openjdk.java.net/browse/JDK-8262108? > > As noted in a comment in that issue, the bug relates to the return value of > `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation > has started returning invalid values for the `AM_PM` field after the "day > period" support was added recently in the JDK as part of > https://bugs.openjdk.java.net/browse/JDK-8262108. > > The commit here adds a check in the internal implementation of the display > name handling logic, to special case the `AM_PM` field and properly convert > the display name array indexes (which is an internal detail) to valid values > that represent the `AM_PM` calendar field. > > The commit also has a new jtreg test case `CalendarDisplayNamesTest` > reproduces this issue and verifies the fix. > > After this fix was introduced, I ran the test in > `test/jdk/java/util/Calendar/` and that showed up a failure in an existing > test case `NarrowNamesTest`. Looking at that test case, IMO, the current > testing in that `NarrowNamesTest` is incorrect and is probably what hid this > issue in the first place? To fix this, I have added an additional commit > which updates this test case to properly test the `AM_PM` field values. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - copyright year and @bug id reference in existing test - copyright year on source - Changes: - all: https://git.openjdk.java.net/jdk/pull/3463/files - new: https://git.openjdk.java.net/jdk/pull/3463/files/812d54f2..84b11879 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3463=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3463=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3463.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3463/head:pull/3463 PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
On Thu, 8 Apr 2021 18:51:21 GMT, Jorn Vernee wrote:
> This patch adds a `tableSwitch` combinator that can be used to switch over a
> set of method handles given an index, with a fallback in case the index is
> out of bounds, much like the `tableswitch` bytecode. Here is a description of
> how it works (copied from the javadoc):
>
> Creates a table switch method handle, which can be used to switch over a
> set of target
> method handles, based on a given target index, called selector.
>
> For a selector value of {@code n}, where {@code n} falls in the range
> {@code [0, N)},
> and where {@code N} is the number of target method handles, the table
> switch method
> handle will invoke the n-th target method handle from the list of target
> method handles.
>
> For a selector value that does not fall in the range {@code [0, N)}, the
> table switch
> method handle will invoke the given fallback method handle.
>
> All method handles passed to this method must have the same type, with
> the additional
> requirement that the leading parameter be of type {@code int}. The
> leading parameter
> represents the selector.
>
> Any trailing parameters present in the type will appear on the returned
> table switch
> method handle as well. Any arguments assigned to these parameters will
> be forwarded,
> together with the selector value, to the selected method handle when
> invoking it.
>
> The combinator does not support specifying the starting index, so the switch
> cases always run from 0 to however many target handles are specified. A
> starting index can be added manually with another combination step that
> filters the input index by adding or subtracting a constant from it, which
> does not affect performance. One of the reasons for not supporting a starting
> index is that it allows for more lambda form sharing, but also simplifies the
> implementation somewhat. I guess an open question is if a convenience
> overload should be added for that case?
>
> Lookup switch can also be simulated by filtering the input through an
> injection function that translates it into a case index, which has also
> proven to have the ability to have comparable performance to, or even better
> performance than, a bytecode-native `lookupswitch` instruction. I plan to add
> such an injection function to the runtime libraries in the future as well.
> Maybe at that point it could be evaluated if it's worth it to add a lookup
> switch combinator as well, but I don't see an immediate need to include it in
> this patch.
>
> The current bytecode intrinsification generates a call for each switch case,
> which guarantees full inlining of the target method handles. Alternatively we
> could only have 1 callsite at the end of the switch, where each case just
> loads the target method handle, but currently this does not allow for
> inlining of the handles, since they are not constant.
>
> Maybe a future C2 optimization could look at the receiver input for
> invokeBasic call sites, and if the input is a phi node, clone the call for
> each constant input of the phi. I believe that would allow simplifying the
> bytecode without giving up on inlining.
>
> Some numbers from the added benchmarks:
>
> Benchmark(numCases) (offset)
> (sorted) Mode Cnt Score Error Units
> MethodHandlesTableSwitchConstant.testSwitch 5 0
> N/A avgt 30 4.186 � 0.054 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 5 150
> N/A avgt 30 4.164 � 0.057 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 10 0
> N/A avgt 30 4.124 � 0.023 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 10 150
> N/A avgt 30 4.126 � 0.025 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 25 0
> N/A avgt 30 4.137 � 0.042 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 25 150
> N/A avgt 30 4.113 � 0.016 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 50 0
> N/A avgt 30 4.118 � 0.028 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 50 150
> N/A avgt 30 4.127 � 0.019 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 0
> N/A avgt 30 4.116 � 0.013 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 150
> N/A avgt 30 4.121 � 0.020 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 5 0
> N/A avgt 30 4.113 � 0.009 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 5 150
> N/A avgt 30 4.149 � 0.041 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 10 0
> N/A avgt 30 4.121 � 0.026 ms/op
>
Re: RFR: 8265075: Improve and simplify Class.resolveName()
On Tue, 13 Apr 2021 12:47:50 GMT, Сергей Цыпанов
wrote:
> In mentioned method this code snippet
>
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
>
>
> can be simplified with performance improvement as
>
> name = baseName.replace('.', '/') + '/' + name;
>
> Also null check of `baseName` can be removed as Class.getPackageName() never
> returns null.
>
> This benchmark
>
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class ResolveNameBenchmark {
>
> private final Class klazz = getClass();
>
> @Benchmark
> public Object original() {
> return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
> }
>
> @Benchmark
> public Object patched() {
> return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
> }
>
> private String original(String name) {
> if (!name.startsWith("/")) {
> String baseName = getPackageName();
> if (baseName != null && !baseName.isEmpty()) {
> int len = baseName.length() + 1 + name.length();
> StringBuilder sb = new StringBuilder(len);
> name = sb.append(baseName.replace('.', '/'))
> .append('/')
> .append(name)
> .toString();
> }
> } else {
> name = name.substring(1);
> }
> return name;
> }
>
> private String patched(String name) {
> if (!name.startsWith("/")) {
> String baseName = getPackageName();
> if (!baseName.isEmpty()) {
> return baseName.replace('.', '/') + '/' + name;
> }
> return name;
> }
> return name.substring(1);
> }
>
> private String getPackageName() {
> return klazz.getPackageName();
> }
> }
>
> demonstrates good improvement, especially as of memory consumption:
>
> Mode Cnt Score Error
> Units
>
> originalavgt 5057.974 ± 0.365
> ns/op
> original:·gc.alloc.rate avgt 50 3419.447 ± 21.154
> MB/sec
> original:·gc.alloc.rate.normavgt 50 312.006 ± 0.001
> B/op
> original:·gc.churn.G1_Eden_Spaceavgt 50 3399.396 ± 149.836
> MB/sec
> original:·gc.churn.G1_Eden_Space.norm avgt 50 310.198 ± 13.628
> B/op
> original:·gc.churn.G1_Survivor_Spaceavgt 50 0.004 ± 0.001
> MB/sec
> original:·gc.churn.G1_Survivor_Space.norm avgt 50≈ 10⁻³
> B/op
> original:·gc.count avgt 50 208.000
> counts
> original:·gc.time avgt 50 224.000
> ms
>
> patched avgt 5044.367 ± 0.162
> ns/op
> patched:·gc.alloc.rate avgt 50 2749.265 ± 10.014
> MB/sec
> patched:·gc.alloc.rate.norm avgt 50 192.004 ± 0.001
> B/op
> patched:·gc.churn.G1_Eden_Space avgt 50 2729.221 ± 193.552
> MB/sec
> patched:·gc.churn.G1_Eden_Space.normavgt 50 190.615 ± 13.539
> B/op
> patched:·gc.churn.G1_Survivor_Space avgt 50 0.003 ± 0.001
> MB/sec
> patched:·gc.churn.G1_Survivor_Space.normavgt 50≈ 10⁻⁴
> B/op
> patched:·gc.count avgt 50 167.000
> counts
> patched:·gc.timeavgt 50 181.000
> ms
src/java.base/share/classes/java/lang/Class.java line 3067:
> 3065: */
> 3066: private String resolveName(String name) {
> 3067: if (!name.startsWith("/")) {
I expect this would be more more readable if you invert it, i.e. "if
(name.startsWith("/") { return name.substring(1); } else { ... }.
Note that the baseName == null check was needed when it was originally created
because getPackageName could return null in the initial version.
-
PR: https://git.openjdk.java.net/jdk/pull/3464
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 12:54:51 GMT, Ichiroh Takiguchi wrote: > 1. I think method name "charset()" is too short. It's not called frequently. > This method name should explain functionality. > 2. Sometimes stderr may be redirected to stdout by shell. Why do we need to > set different encodings for these two (sun.stdout.encoding and > sun.stderr.encoding) ? Console's class description already covers this "If the virtual machine is started from an interactive command line without redirecting the standard input and output streams then ...". The existing reader and writer methods use the same charset. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Mon, 12 Apr 2021 23:01:24 GMT, Naoto Sato wrote: >> Please review the changes for the subject issue. This has been suggested in >> a recent discussion thread for the JEP 400 >> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. >> A CSR has also been drafted, and comments are welcome >> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted PrintStream changes I have 2 concerns: 1. I think method name "charset()" is too short. It's not called frequently. This method name should explain functionality. 2. Sometimes stderr may be redirected to stdout by shell. Why do we need to set different encodings for these two (sun.stdout.encoding and sun.stderr.encoding) ? - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64
On Tue, 13 Apr 2021 06:55:33 GMT, Mikael Vidstedt wrote: > Let's problem list java/util/concurrent/locks/Lock/TimedAcquireLeak.java (a > tier1 test) until JDK-8262897 is fixed. Looks good and trivial. Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3452
RFR: 8265075: Improve and simplify Class.resolveName()
In mentioned method this code snippet
int len = baseName.length() + 1 + name.length();
StringBuilder sb = new StringBuilder(len);
name = sb.append(baseName.replace('.', '/'))
.append('/')
.append(name)
.toString();
can be simplified with performance improvement as
name = baseName.replace('.', '/') + '/' + name;
Also null check of `baseName` can be removed as Class.getPackageName() never
returns null.
This benchmark
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class ResolveNameBenchmark {
private final Class klazz = getClass();
@Benchmark
public Object original() {
return original("com/tsypanov/ovn/ResolveNameBenchmark.class");
}
@Benchmark
public Object patched() {
return patched("com/tsypanov/ovn/ResolveNameBenchmark.class");
}
private String original(String name) {
if (!name.startsWith("/")) {
String baseName = getPackageName();
if (baseName != null && !baseName.isEmpty()) {
int len = baseName.length() + 1 + name.length();
StringBuilder sb = new StringBuilder(len);
name = sb.append(baseName.replace('.', '/'))
.append('/')
.append(name)
.toString();
}
} else {
name = name.substring(1);
}
return name;
}
private String patched(String name) {
if (!name.startsWith("/")) {
String baseName = getPackageName();
if (!baseName.isEmpty()) {
return baseName.replace('.', '/') + '/' + name;
}
return name;
}
return name.substring(1);
}
private String getPackageName() {
return klazz.getPackageName();
}
}
demonstrates good improvement, especially as of memory consumption:
Mode Cnt Score Error
Units
originalavgt 5057.974 ± 0.365
ns/op
original:·gc.alloc.rate avgt 50 3419.447 ± 21.154
MB/sec
original:·gc.alloc.rate.normavgt 50 312.006 ± 0.001
B/op
original:·gc.churn.G1_Eden_Spaceavgt 50 3399.396 ± 149.836
MB/sec
original:·gc.churn.G1_Eden_Space.norm avgt 50 310.198 ± 13.628
B/op
original:·gc.churn.G1_Survivor_Spaceavgt 50 0.004 ± 0.001
MB/sec
original:·gc.churn.G1_Survivor_Space.norm avgt 50≈ 10⁻³
B/op
original:·gc.count avgt 50 208.000
counts
original:·gc.time avgt 50 224.000
ms
patched avgt 5044.367 ± 0.162
ns/op
patched:·gc.alloc.rate avgt 50 2749.265 ± 10.014
MB/sec
patched:·gc.alloc.rate.norm avgt 50 192.004 ± 0.001
B/op
patched:·gc.churn.G1_Eden_Space avgt 50 2729.221 ± 193.552
MB/sec
patched:·gc.churn.G1_Eden_Space.normavgt 50 190.615 ± 13.539
B/op
patched:·gc.churn.G1_Survivor_Space avgt 50 0.003 ± 0.001
MB/sec
patched:·gc.churn.G1_Survivor_Space.normavgt 50≈ 10⁻⁴
B/op
patched:·gc.count avgt 50 167.000
counts
patched:·gc.timeavgt 50 181.000
ms
-
Commit messages:
- 8265075: Improve and simplify Class.resolveName()
Changes: https://git.openjdk.java.net/jdk/pull/3464/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3464=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8265075
Stats: 11 lines in 1 file changed: 0 ins; 7 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/3464.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3464/head:pull/3464
PR: https://git.openjdk.java.net/jdk/pull/3464
Re: RFR: 8263157: [macos]: java.library.path is being set incorrectly
On Mon, 12 Apr 2021 20:18:46 GMT, Alexander Matveev wrote: > This is regression from JDK-8242302. Fixed by setting java.library.path to > same values as it was before JDK-8242302. Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3443
Re: RFR: 8265079: Implement VarHandle invoker caching
On Mon, 12 Apr 2021 16:24:37 GMT, Jorn Vernee wrote: > This patch implements 2 leftover TODOs for implementing var handle invoker MH > caching (lambda forms for those were already shared/cached). > > This piggybacks on the existing mechanism for method handle invoker caching. > > Testing: Local testing `java/lang/invoke` tests. Tier 1-3 > > Thanks, > Jorn Thanks for the reviews. Comment addressed. - PR: https://git.openjdk.java.net/jdk/pull/3439
Re: RFR: 8265079: Implement VarHandle invoker caching [v2]
> This patch implements 2 leftover TODOs for implementing var handle invoker MH > caching (lambda forms for those were already shared/cached). > > This piggybacks on the existing mechanism for method handle invoker caching. > > Testing: Local testing `java/lang/invoke` tests. Tier 1-3 > > Thanks, > Jorn Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Review comments: - Use boolean instead of index for var handle cache - Changes: - all: https://git.openjdk.java.net/jdk/pull/3439/files - new: https://git.openjdk.java.net/jdk/pull/3439/files/df10dbdd..93681f77 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3439=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3439=00-01 Stats: 14 lines in 1 file changed: 4 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3439.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3439/head:pull/3439 PR: https://git.openjdk.java.net/jdk/pull/3439
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale
On Tue, 13 Apr 2021 11:42:41 GMT, Jaikiran Pai wrote:
> Can I please get a review for this proposed fix for
> https://bugs.openjdk.java.net/browse/JDK-8262108?
>
> As noted in a comment in that issue, the bug relates to the return value of
> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation
> has started returning invalid values for the `AM_PM` field after the "day
> period" support was added recently in the JDK as part of
> https://bugs.openjdk.java.net/browse/JDK-8262108.
>
> The commit here adds a check in the internal implementation of the display
> name handling logic, to special case the `AM_PM` field and properly convert
> the display name array indexes (which is an internal detail) to valid values
> that represent the `AM_PM` calendar field.
>
> The commit also has a new jtreg test case `CalendarDisplayNamesTest`
> reproduces this issue and verifies the fix.
>
> After this fix was introduced, I ran the test in
> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing
> test case `NarrowNamesTest`. Looking at that test case, IMO, the current
> testing in that `NarrowNamesTest` is incorrect and is probably what hid this
> issue in the first place? To fix this, I have added an additional commit
> which updates this test case to properly test the `AM_PM` field values.
src/java.base/share/classes/sun/util/locale/provider/CalendarNameProviderImpl.java
line 201:
> 199: switch (i) {
> 200: case 0, 2, 3, 4, 5 -> map.put(name,
> AM);
> 201: case 1, 6, 7, 8, 9, 10, 11 ->
> map.put(name, PM);
One part where I need input here, is the "midnight" and the "noon" types. I
haven't found anything in the JDK code which I can use as a reference to
classify "midnight" and "noon" as `AM` (which is what this code is doing). The
doc here https://unicode.org/reports/tr35/tr35-dates.html#Day_Period_Rules
speaks about these 2 types but I don't think it's something that I can use to
translate those types into a field value for Calendar, to represent `AM` or
`PM`. As such, right now, I've just guessed that `AM` is probably the right
value for these types, but there's no technical reason or reference backing it.
-
PR: https://git.openjdk.java.net/jdk/pull/3463
RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale
Can I please get a review for this proposed fix for https://bugs.openjdk.java.net/browse/JDK-8262108? As noted in a comment in that issue, the bug relates to the return value of `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation has started returning invalid values for the `AM_PM` field after the "day period" support was added recently in the JDK as part of https://bugs.openjdk.java.net/browse/JDK-8262108. The commit here adds a check in the internal implementation of the display name handling logic, to special case the `AM_PM` field and properly convert the display name array indexes (which is an internal detail) to valid values that represent the `AM_PM` calendar field. The commit also has a new jtreg test case `CalendarDisplayNamesTest` reproduces this issue and verifies the fix. After this fix was introduced, I ran the test in `test/jdk/java/util/Calendar/` and that showed up a failure in an existing test case `NarrowNamesTest`. Looking at that test case, IMO, the current testing in that `NarrowNamesTest` is incorrect and is probably what hid this issue in the first place? To fix this, I have added an additional commit which updates this test case to properly test the `AM_PM` field values. - Commit messages: - Fix test that now fails after fixing 8262108 - 8262108: SimpleDateFormat formatting broken for sq_MK Locale Changes: https://git.openjdk.java.net/jdk/pull/3463/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3463=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262108 Stats: 117 lines in 3 files changed: 97 ins; 1 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/3463.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3463/head:pull/3463 PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale
On Tue, 13 Apr 2021 11:42:41 GMT, Jaikiran Pai wrote:
> Can I please get a review for this proposed fix for
> https://bugs.openjdk.java.net/browse/JDK-8262108?
>
> As noted in a comment in that issue, the bug relates to the return value of
> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation
> has started returning invalid values for the `AM_PM` field after the "day
> period" support was added recently in the JDK as part of
> https://bugs.openjdk.java.net/browse/JDK-8262108.
>
> The commit here adds a check in the internal implementation of the display
> name handling logic, to special case the `AM_PM` field and properly convert
> the display name array indexes (which is an internal detail) to valid values
> that represent the `AM_PM` calendar field.
>
> The commit also has a new jtreg test case `CalendarDisplayNamesTest`
> reproduces this issue and verifies the fix.
>
> After this fix was introduced, I ran the test in
> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing
> test case `NarrowNamesTest`. Looking at that test case, IMO, the current
> testing in that `NarrowNamesTest` is incorrect and is probably what hid this
> issue in the first place? To fix this, I have added an additional commit
> which updates this test case to properly test the `AM_PM` field values.
While we are at this, the `Calendar.getDisplayNames(...)` states this:
>
> @throwsIllegalArgumentException
*if {@code field} or {@code style} is invalid,
*or if this {@code Calendar} is non-lenient and any
*of the calendar fields have invalid values
If I understand this correctly, if the `Calendar` instance was non-lenient and
this method returned invalid values for the `AM_PM` field, it should have
thrown an `IllegalArgumentException` (which is a strange exception to throw for
an invalid result, but that's a different matter). However, with the current
code in upstream master, I saw no such exception being thrown (nor do I see any
such validating code in the `Calendar` implementation) when the calendar
instance was non-lenient. Is that expected?
-
PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8265079: Implement VarHandle invoker caching
On Mon, 12 Apr 2021 16:24:37 GMT, Jorn Vernee wrote:
> This patch implements 2 leftover TODOs for implementing var handle invoker MH
> caching (lambda forms for those were already shared/cached).
>
> This piggybacks on the existing mechanism for method handle invoker caching.
>
> Testing: Local testing `java/lang/invoke` tests. Tier 1-3
>
> Thanks,
> Jorn
Looks good.
src/java.base/share/classes/java/lang/invoke/Invokers.java line 131:
> 129: }
> 130:
> 131: private MethodHandle cachedVHInvoker(int idx, VarHandle.AccessMode
> ak) {
You can turn `int idx` parameter into `boolean isExact` and choose base index
depending on its value.
-
Marked as reviewed by vlivanov (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3439
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
On Tue, 13 Apr 2021 10:04:19 GMT, Conor Cleary wrote: >> ### Description >> This fix is part of a previous effort to both cleanup/modernise JNDI code, >> the details of which can be seen in >> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number >> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where >> only a single object unique to the requirements of the method is used. The >> issues these occurrences of AICs cause are highlighted below. >> >> - AICs, in the cases concerned with this fix, are used where only one >> operation is required. While AICs can be useful for more complex >> implementations (using interfaces, multiple methods needed, local fields >> etc.), Lambdas are better suited here as they result in a more readable and >> concise solution. >> >> ### Fixes >> - Where applicable, occurrences of AICs were replaced with lambdas to >> address the issues above resulting primarily in more readable/concise code. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8048199: Cleaner syntak in getContextClassLoader Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64
On Tue, 13 Apr 2021 06:55:33 GMT, Mikael Vidstedt wrote: > Let's problem list java/util/concurrent/locks/Lock/TimedAcquireLeak.java (a > tier1 test) until JDK-8262897 is fixed. Marked as reviewed by akozlov (no project role). - PR: https://git.openjdk.java.net/jdk/pull/3452
Re: RFR: 8265061: Simplify MethodHandleNatives::canBeCalledVirtual
On Mon, 12 Apr 2021 11:47:43 GMT, Jorn Vernee wrote: >> Desugaring the single-case switch in MethodHandleNatives::canBeCalledVirtual >> is a cleanup and minimal startup improvement on apps spinning up >> MethodHandles. > > Marked as reviewed by jvernee (Committer). @JornVernee @mlchung - thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/3433
Integrated: 8265061: Simplify MethodHandleNatives::canBeCalledVirtual
On Mon, 12 Apr 2021 10:45:20 GMT, Claes Redestad wrote: > Desugaring the single-case switch in MethodHandleNatives::canBeCalledVirtual > is a cleanup and minimal startup improvement on apps spinning up > MethodHandles. This pull request has now been integrated. Changeset: 7006070f Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/7006070f Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod 8265061: Simplify MethodHandleNatives::canBeCalledVirtual Reviewed-by: jvernee, mchung - PR: https://git.openjdk.java.net/jdk/pull/3433
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
On Tue, 13 Apr 2021 10:04:19 GMT, Conor Cleary wrote: >> ### Description >> This fix is part of a previous effort to both cleanup/modernise JNDI code, >> the details of which can be seen in >> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number >> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where >> only a single object unique to the requirements of the method is used. The >> issues these occurrences of AICs cause are highlighted below. >> >> - AICs, in the cases concerned with this fix, are used where only one >> operation is required. While AICs can be useful for more complex >> implementations (using interfaces, multiple methods needed, local fields >> etc.), Lambdas are better suited here as they result in a more readable and >> concise solution. >> >> ### Fixes >> - Where applicable, occurrences of AICs were replaced with lambdas to >> address the issues above resulting primarily in more readable/concise code. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8048199: Cleaner syntak in getContextClassLoader Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8265079: Implement VarHandle invoker caching
On Mon, 12 Apr 2021 16:24:37 GMT, Jorn Vernee wrote: > This patch implements 2 leftover TODOs for implementing var handle invoker MH > caching (lambda forms for those were already shared/cached). > > This piggybacks on the existing mechanism for method handle invoker caching. > > Testing: Local testing `java/lang/invoke` tests. Tier 1-3 > > Thanks, > Jorn LGTM `MethodHandles.varHandle(Exact)Invoker` might be somewhat obscure, but caching them like this is straightforward and basically free. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3439
RFR: 8265079: Implement VarHandle invoker caching
This patch implements 2 leftover TODOs for implementing var handle invoker MH caching (lambda forms for those were already shared/cached). This piggybacks on the existing mechanism for method handle invoker caching. Testing: Local testing `java/lang/invoke` tests. Tier 1-3 Thanks, Jorn - Commit messages: - Add VarHandle invoker handle caching Changes: https://git.openjdk.java.net/jdk/pull/3439/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3439=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265079 Stats: 115 lines in 2 files changed: 110 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/3439.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3439/head:pull/3439 PR: https://git.openjdk.java.net/jdk/pull/3439
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Tue, 13 Apr 2021 10:00:51 GMT, Conor Cleary wrote: >> Good idea, also would fit in with the style of the method just after, >> `priviligedHasNext()` as that also uses a functional interface. > > Changes included as described in commit > [5d6ecd3](https://github.com/openjdk/jdk/pull/3416/commits/5d6ecd31eb6ed2a63f17b2e798e83b91cb720075) Here again this is not strictly equivalent but AFAIK `Thread::currentThread` doesn't require any permission, so this should be OK. - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
> ### Description > This fix is part of a previous effort to both cleanup/modernise JNDI code, > the details of which can be seen in > [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number > JNDI methods under `java.naming` use Anonymous Inner Classes in cases where > only a single object unique to the requirements of the method is used. The > issues these occurrences of AICs cause are highlighted below. > > - AICs, in the cases concerned with this fix, are used where only one > operation is required. While AICs can be useful for more complex > implementations (using interfaces, multiple methods needed, local fields > etc.), Lambdas are better suited here as they result in a more readable and > concise solution. > > ### Fixes > - Where applicable, occurrences of AICs were replaced with lambdas to address > the issues above resulting primarily in more readable/concise code. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8048199: Cleaner syntak in getContextClassLoader - Changes: - all: https://git.openjdk.java.net/jdk/pull/3416/files - new: https://git.openjdk.java.net/jdk/pull/3416/files/840ea35c..5d6ecd31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3416=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3416=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3416/head:pull/3416 PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Tue, 13 Apr 2021 09:34:15 GMT, Conor Cleary wrote:
>> src/java.naming/share/classes/javax/naming/ldap/StartTlsRequest.java line
>> 223:
>>
>>> 221: */
>>> 222: private final ClassLoader getContextClassLoader() {
>>> 223: PrivilegedAction pa = () ->
>>> Thread.currentThread().getContextClassLoader();
>>
>> We can use here an instance method reference to beautify code a little bit
>> more:
>> ```PrivilegedAction pa =
>> Thread.currentThread()::getContextClassLoader;```
>
> Good idea, also would fit in with the style of the method just after,
> `priviligedHasNext()` as that also uses a functional interface.
Changes included as described in commit
[5d6ecd3](https://github.com/openjdk/jdk/pull/3416/commits/5d6ecd31eb6ed2a63f17b2e798e83b91cb720075)
-
PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Mon, 12 Apr 2021 16:44:16 GMT, Aleksei Efimov wrote:
>> Conor Cleary has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Update copyright headers
>> - Tidied up lambdas
>
> src/java.naming/share/classes/javax/naming/ldap/StartTlsRequest.java line 223:
>
>> 221: */
>> 222: private final ClassLoader getContextClassLoader() {
>> 223: PrivilegedAction pa = () ->
>> Thread.currentThread().getContextClassLoader();
>
> We can use here an instance method reference to beautify code a little bit
> more:
> ```PrivilegedAction pa =
> Thread.currentThread()::getContextClassLoader;```
Good idea, also would fit in with the style of the method just after,
`priviligedHasNext()` as that also uses a functional interface.
-
PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Sun, 11 Apr 2021 10:25:47 GMT, Doug Simon wrote: >> Vladimir Kozlov has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Restore Graal Builder image makefile >> - Merge latest changes from branch 'JDK-8264805' into JDK-8264806 >> - 8264806: Remove the experimental JIT compiler > > We would definitely like to be able to continue testing of GraalVM with the > JDK set of jtreg tests. So keeping `Compiler::isGraalEnabled()` working like > it does today is important. > @dougxc I restored Compiler::isGraalEnabled(). Thanks. I guess this should really be named `isJVMCICompilerEnabled` now and the `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but maybe that's too big a change (or can be done later). - PR: https://git.openjdk.java.net/jdk/pull/3421
RFR: 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64
Let's problem list java/util/concurrent/locks/Lock/TimedAcquireLeak.java (a tier1 test) until JDK-8262897 is fixed. - Commit messages: - 8265111: ProblemList java/util/concurrent/locks/Lock/TimedAcquireLeak.java on macosx-aarch64 Changes: https://git.openjdk.java.net/jdk/pull/3452/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3452=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265111 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3452.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3452/head:pull/3452 PR: https://git.openjdk.java.net/jdk/pull/3452
