RFR: 8272120: Avoid looking for standard encodings in "java." modules
This is the continuation of JDK-8233884 and JDK-8271456. This change affects
fewer cases so I fix all "java." modules at once.
In many places standard charsets are looked up via their names, for example:
absolutePath.getBytes("UTF-8");
This could be done more efficiently(up to x20 time faster) with use of
java.nio.charset.StandardCharsets:
absolutePath.getBytes(StandardCharsets.UTF_8);
The later variant also makes the code cleaner, as it is known not to throw
UnsupportedEncodingException in contrary to the former variant.
tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
-
Commit messages:
- Initial fix of JDK-8272120
Changes: https://git.openjdk.java.net/jdk/pull/5063/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5063=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272120
Stats: 127 lines in 15 files changed: 24 ins; 53 del; 50 mod
Patch: https://git.openjdk.java.net/jdk/pull/5063.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5063/head:pull/5063
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup There is no user-visible aspect. I plan to file a CSR to document the workaround to enable the old implementation when running into the issues described in the risks and assumption section of JEP 416. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 20:15:59 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/ref/Reference.java line 374:
>>
>>> 372: * to call the native implementation over the intrinsic.
>>> 373: */
>>> 374: boolean refersToImpl(T obj) {
>>
>> I'm curious why can't you get rid of `refersToImpl` (virtual method) and
>> just override `refersTo` on `PhantomReference`. Am I missing something
>> important about keeping `refersTo` final?
>
> We don't want user-defined subclass of `Reference` overriding the `refersTo`
> implementation accidentally.
Got it. Thanks for the clarification!
-
PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v2]
On Sat, 7 Aug 2021 02:07:24 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor test cleanup Are there any user-visible aspects of this re-implementation that would merit a CSR for behavioral changes? - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 19:58:48 GMT, Vladimir Ivanov wrote:
>> While fixing JDK-8270626 it was discovered that the C2 intrinsic for
>> `Reference.refersTo()` is not used as often as one would think. Instead C2
>> often prefers using the native implementation, which is much slower which
>> defeats the purpose of having an intrinsic in the first place.
>>
>> The problem arise from having `refersTo0()` be virtual, native and
>> @IntrinsicCandidate. This can be worked around by introducing an virtual
>> method layer and so that `refersTo0()` can be made `final`. That's what this
>> patch does, by adding a virtual `refersToImpl()` which in turn calls the now
>> final `refersTo0()`.
>>
>> It should be noted that `Object.clone()` and `Object.hashCode()` are also
>> virtual, native and @IntrinsicCandidate and these methods get special
>> treatment by C2 to get the intrinsic generated more optimally. We might want
>> to do a similar optimization for `Reference.refersTo0()`. In that case, it
>> is suggested that such an improvement could come later and be handled as a
>> separate enhancement, and @kimbarrett has already filed JDK-8272140 to track
>> that.
>>
>> Testing:
>> I found this hard to test without instrumenting Hotspot to tell me that the
>> intrinsic was called instead of the native method. So, for that reason
>> testing was ad-hoc, using an instrumented Hotspot in combination with the
>> test
>> (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`)
>> that initially uncovered the problem. See JDK-8270626 for additional
>> information.
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 374:
>
>> 372: * to call the native implementation over the intrinsic.
>> 373: */
>> 374: boolean refersToImpl(T obj) {
>
> I'm curious why can't you get rid of `refersToImpl` (virtual method) and just
> override `refersTo` on `PhantomReference`. Am I missing something important
> about keeping `refersTo` final?
We don't want user-defined subclass of `Reference` overriding the `refersTo`
implementation accidentally.
-
PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden wrote:
> While fixing JDK-8270626 it was discovered that the C2 intrinsic for
> `Reference.refersTo()` is not used as often as one would think. Instead C2
> often prefers using the native implementation, which is much slower which
> defeats the purpose of having an intrinsic in the first place.
>
> The problem arise from having `refersTo0()` be virtual, native and
> @IntrinsicCandidate. This can be worked around by introducing an virtual
> method layer and so that `refersTo0()` can be made `final`. That's what this
> patch does, by adding a virtual `refersToImpl()` which in turn calls the now
> final `refersTo0()`.
>
> It should be noted that `Object.clone()` and `Object.hashCode()` are also
> virtual, native and @IntrinsicCandidate and these methods get special
> treatment by C2 to get the intrinsic generated more optimally. We might want
> to do a similar optimization for `Reference.refersTo0()`. In that case, it is
> suggested that such an improvement could come later and be handled as a
> separate enhancement, and @kimbarrett has already filed JDK-8272140 to track
> that.
>
> Testing:
> I found this hard to test without instrumenting Hotspot to tell me that the
> intrinsic was called instead of the native method. So, for that reason
> testing was ad-hoc, using an instrumented Hotspot in combination with the
> test
> (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`)
> that initially uncovered the problem. See JDK-8270626 for additional
> information.
src/java.base/share/classes/java/lang/ref/Reference.java line 374:
> 372: * to call the native implementation over the intrinsic.
> 373: */
> 374: boolean refersToImpl(T obj) {
I'm curious why can't you get rid of `refersToImpl` (virtual method) and just
override `refersTo` on `PhantomReference`. Am I missing something important
about keeping `refersTo` final?
src/java.base/share/classes/java/lang/ref/Reference.java line 379:
> 377:
> 378: @IntrinsicCandidate
> 379: private native final boolean refersTo0(Object o);
No need to have it both `private` and `final`. Just `private` should be enough.
-
PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden wrote: > While fixing JDK-8270626 it was discovered that the C2 intrinsic for > `Reference.refersTo()` is not used as often as one would think. Instead C2 > often prefers using the native implementation, which is much slower which > defeats the purpose of having an intrinsic in the first place. > > The problem arise from having `refersTo0()` be virtual, native and > @IntrinsicCandidate. This can be worked around by introducing an virtual > method layer and so that `refersTo0()` can be made `final`. That's what this > patch does, by adding a virtual `refersToImpl()` which in turn calls the now > final `refersTo0()`. > > It should be noted that `Object.clone()` and `Object.hashCode()` are also > virtual, native and @IntrinsicCandidate and these methods get special > treatment by C2 to get the intrinsic generated more optimally. We might want > to do a similar optimization for `Reference.refersTo0()`. In that case, it is > suggested that such an improvement could come later and be handled as a > separate enhancement, and @kimbarrett has already filed JDK-8272140 to track > that. > > Testing: > I found this hard to test without instrumenting Hotspot to tell me that the > intrinsic was called instead of the native method. So, for that reason > testing was ad-hoc, using an instrumented Hotspot in combination with the > test > (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) > that initially uncovered the problem. See JDK-8270626 for additional > information. Looks good to me too. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden wrote: > While fixing JDK-8270626 it was discovered that the C2 intrinsic for > `Reference.refersTo()` is not used as often as one would think. Instead C2 > often prefers using the native implementation, which is much slower which > defeats the purpose of having an intrinsic in the first place. > > The problem arise from having `refersTo0()` be virtual, native and > @IntrinsicCandidate. This can be worked around by introducing an virtual > method layer and so that `refersTo0()` can be made `final`. That's what this > patch does, by adding a virtual `refersToImpl()` which in turn calls the now > final `refersTo0()`. > > It should be noted that `Object.clone()` and `Object.hashCode()` are also > virtual, native and @IntrinsicCandidate and these methods get special > treatment by C2 to get the intrinsic generated more optimally. We might want > to do a similar optimization for `Reference.refersTo0()`. In that case, it is > suggested that such an improvement could come later and be handled as a > separate enhancement, and @kimbarrett has already filed JDK-8272140 to track > that. > > Testing: > I found this hard to test without instrumenting Hotspot to tell me that the > intrinsic was called instead of the native method. So, for that reason > testing was ad-hoc, using an instrumented Hotspot in combination with the > test > (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) > that initially uncovered the problem. See JDK-8270626 for additional > information. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment [v2]
On Mon, 9 Aug 2021 18:19:03 GMT, Alexey Semenyuk wrote: >> Added jtreg test to verify jpackage launcher adds app dir to >> `java.library.path` system property. >> Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. >> Moved test/jdk/tools/jpackage/apps/image/Hello.java to >> test/jdk/tools/jpackage/apps/Hello.java. > > Alexey Semenyuk has updated the pull request incrementally with one > additional commit since the last revision: > > Handle duplicate elements in `java.library.path` system property value. Marked as reviewed by herrick (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5056
Re: RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment [v2]
On Mon, 9 Aug 2021 18:19:03 GMT, Alexey Semenyuk wrote: >> Added jtreg test to verify jpackage launcher adds app dir to >> `java.library.path` system property. >> Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. >> Moved test/jdk/tools/jpackage/apps/image/Hello.java to >> test/jdk/tools/jpackage/apps/Hello.java. > > Alexey Semenyuk has updated the pull request incrementally with one > additional commit since the last revision: > > Handle duplicate elements in `java.library.path` system property value. Marked as reviewed by almatvee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5056
Re: RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment [v2]
> Added jtreg test to verify jpackage launcher adds app dir to > `java.library.path` system property. > Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. > Moved test/jdk/tools/jpackage/apps/image/Hello.java to > test/jdk/tools/jpackage/apps/Hello.java. Alexey Semenyuk has updated the pull request incrementally with one additional commit since the last revision: Handle duplicate elements in `java.library.path` system property value. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5056/files - new: https://git.openjdk.java.net/jdk/pull/5056/files/bda38d39..4c0891dd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5056=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5056=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5056/head:pull/5056 PR: https://git.openjdk.java.net/jdk/pull/5056
Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]
On Sat, 7 Aug 2021 06:45:21 GMT, Markus KARG wrote: > I think I fixed all requested changes. Anymore comments on this PR? I hope to get to this soon. - PR: https://git.openjdk.java.net/jdk/pull/4263
RFR: 8271170: Add unit test for what jpackage app launcher puts in the environment
Added jtreg test to verify jpackage launcher adds app dir to `java.library.path` system property. Removed unused test/jdk/tools/jpackage/apps/installer/Hello.java. Moved test/jdk/tools/jpackage/apps/image/Hello.java to test/jdk/tools/jpackage/apps/Hello.java. - Commit messages: - dummy commit - Whitespace clean up - 8271170: Add unit test for what jpackage app launcher puts in the environment Changes: https://git.openjdk.java.net/jdk/pull/5056/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5056=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271170 Stats: 342 lines in 7 files changed: 212 ins; 116 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/5056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5056/head:pull/5056 PR: https://git.openjdk.java.net/jdk/pull/5056
Integrated: 8264792: The NumberFormat for locale sq_XK formats price incorrectly.
On Fri, 6 Aug 2021 16:39:34 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. The root cause of this problem is > that the currency for the country code `XK` is undefined because the country > code is user-defined in the ISO 3166 standard. However, it is commonly used > to represent the region `Kosovo`, which CLDR supports and subsequently is > supported by the JDK since JDK9. Adding the data `EUR` for the country code > `XK` will fix the issue. This pull request has now been integrated. Changeset: 41dc795d Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/41dc795d6c08af84aa6544cc5a5704dcf99386cf Stats: 13 lines in 3 files changed: 6 ins; 0 del; 7 mod 8264792: The NumberFormat for locale sq_XK formats price incorrectly. Reviewed-by: joehw, iris - PR: https://git.openjdk.java.net/jdk/pull/5033
RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used
While fixing JDK-8270626 it was discovered that the C2 intrinsic for `Reference.refersTo()` is not used as often as one would think. Instead C2 often prefers using the native implementation, which is much slower which defeats the purpose of having an intrinsic in the first place. The problem arise from having `refersTo0()` be virtual, native and @IntrinsicCandidate. This can be worked around by introducing an virtual method layer and so that `refersTo0()` can be made `final`. That's what this patch does, by adding a virtual `refersToImpl()` which in turn calls the now final `refersTo0()`. It should be noted that `Object.clone()` and `Object.hashCode()` are also virtual, native and @IntrinsicCandidate and these methods get special treatment by C2 to get the intrinsic generated more optimally. We might want to do a similar optimization for `Reference.refersTo0()`. In that case, it is suggested that such an improvement could come later and be handled as a separate enhancement, and @kimbarrett has already filed JDK-8272140 to track that. Testing: I found this hard to test without instrumenting Hotspot to tell me that the intrinsic was called instead of the native method. So, for that reason testing was ad-hoc, using an instrumented Hotspot in combination with the test (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) that initially uncovered the problem. See JDK-8270626 for additional information. - Commit messages: - 8271862: C2 intrinsic for Reference.refersTo() is often not used Changes: https://git.openjdk.java.net/jdk/pull/5052/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5052=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271862 Stats: 14 lines in 2 files changed: 11 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5052.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5052/head:pull/5052 PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v8]
On Thu, 5 Aug 2021 11:21:59 GMT, Matthias Baesken wrote:
>> Hello, please review this PR; it extend the OSContainer API in order to also
>> support the pids controller of cgroups.
>>
>> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct",
>> "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids
>> controller might not be there (or not fully supported) so it was added as
>> optional , see the coding
>>
>>
>> if (!cg_infos[PIDS_IDX]._data_complete) {
>> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
>> // keep the other controller info, pids is optional
>> }
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Adjust tests, unlimited pids value can lead on some setups to high number
The changes look good to me.
I have one question/suggestion re. copyright comments. The files
test/hotspot/jtreg/containers/docker/TestPids.java
and
test/jdk/jdk/internal/platform/docker/TestPidsLimit.java
are new with this PR. Shouldn't the copyright then just specify the current
year and not a range?
And how about adding the author's company as additional copyright holder?
-
Marked as reviewed by lucy (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4518
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 26 Jul 2021 19:15:55 GMT, Brett Okken wrote: >> I found few places, where code initially perform `Object[] >> Colleciton.toArray()` call and then manually copy array into another array >> with required type. >> This PR cleanups such places to more shorter call `T[] >> Collection.toArray(T[])`. > > src/java.base/share/classes/java/security/Security.java line 656: > >> 654: return null; >> 655: >> 656: return candidates.toArray(new Provider[0]); > > Is this called often enough to warrant creating a constant of `new > Provider[0]` (benefits of this covered in the _Wisdom of the Ancients_ blog > linked)? May be yes, may be not. I like `0`-sized array approach more. 1. It looks clearer and easier to read 2. It should be faster than original code anyway - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 26 Jul 2021 19:55:09 GMT, Brett Okken wrote: >> I found few places, where code initially perform `Object[] >> Colleciton.toArray()` call and then manually copy array into another array >> with required type. >> This PR cleanups such places to more shorter call `T[] >> Collection.toArray(T[])`. > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/SystemDictionaryHelper.java > line 92: > >> 90: } >> 91: >> 92: InstanceKlass[] searchResult = tmp.toArray(new InstanceKlass[0]); > > Is it too far outside the scope of these changes to make `tmp` an `ArrayList` > rather than a `Vector`? I'll create separate PRs to migrate Vector usages to ArrayList. - PR: https://git.openjdk.java.net/jdk/pull/4487
