Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов wrote: >> The code in `Integer.decode()` and `Long.decode()` might allocate two >> instances of Integer/Long for the negative values less than -127: >> >> Integer result; >> >> result = Integer.valueOf(nm.substring(index), radix); >> result = negative ? Integer.valueOf(-result.intValue()) : result; >> >> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` >> method. Same applicable for `Long` and some other classes. > > Сергей Цыпанов 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 five additional > commits since the last revision: > > - 8267844: Add benchmarks for Integer/Long.decode() > - 8267844: Rid useless substring when calling Integer/Long.parse*() > - Merge branch 'master' into 8267844 > - Merge branch 'master' into 8267844 > - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where > applicable Nice! - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
> The code in `Integer.decode()` and `Long.decode()` might allocate two > instances of Integer/Long for the negative values less than -127: > > Integer result; > > result = Integer.valueOf(nm.substring(index), radix); > result = negative ? Integer.valueOf(-result.intValue()) : result; > > To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` > method. Same applicable for `Long` and some other classes. Сергей Цыпанов 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 five additional commits since the last revision: - 8267844: Add benchmarks for Integer/Long.decode() - 8267844: Rid useless substring when calling Integer/Long.parse*() - Merge branch 'master' into 8267844 - Merge branch 'master' into 8267844 - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable - Changes: - all: https://git.openjdk.java.net/jdk/pull/5068/files - new: https://git.openjdk.java.net/jdk/pull/5068/files/a1b993d4..7486b13f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5068=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5068=00-01 Stats: 149574 lines in 2453 files changed: 97455 ins; 39648 del; 12471 mod Patch: https://git.openjdk.java.net/jdk/pull/5068.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5068/head:pull/5068 PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 17:39:01 GMT, Сергей Цыпанов
wrote:
>> src/java.base/share/classes/java/lang/Integer.java line 1450:
>>
>>> 1448:
>>> 1449: try {
>>> 1450: result = parseInt(nm.substring(index), radix);
>>
>> Possibly a follow-up, but I think using `parseInt/-Long(nm, index,
>> nm.length(), radix)` could give an additional speed-up in these cases (by
>> avoiding a substring allocation).
>
> Good point! Let me check this.
Indeed, looks like getting rid of `substring()` call makes things faster:
before
Benchmark(size) Mode Cnt Score Error Units
Integers.decode 500 avgt 15 11.444 ? 0.949 us/op
Integers.parseInt 500 avgt 15 8.669 ? 0.152 us/op
Integers.toStringBig500 avgt 15 10.295 ? 0.612 us/op
Integers.toStringSmall 500 avgt 15 7.020 ? 0.581 us/op
Benchmark(size) Mode Cnt Score Error Units
Longs.decode500 avgt 15 29.568 ? 9.785 us/op
Longs.repetitiveSubtraction 500 avgt 15 0.820 ? 0.153 us/op
Longs.toStringBig 500 avgt 15 13.418 ? 0.744 us/op
Longs.toStringSmall 500 avgt 15 8.180 ? 0.780 us/op
after
Benchmark(size) Mode Cnt Score Error Units
Integers.decode 500 avgt 15 7.377 ? 0.040 us/op
Integers.parseInt 500 avgt 15 8.720 ? 0.230 us/op
Integers.toStringBig500 avgt 15 10.328 ? 0.266 us/op
Integers.toStringSmall 500 avgt 15 6.913 ? 0.178 us/op
Benchmark(size) Mode Cnt Score Error Units
Longs.decode500 avgt 15 8.373 ? 0.708 us/op
Longs.repetitiveSubtraction 500 avgt 15 0.771 ? 0.003 us/op
Longs.toStringBig 500 avgt 15 13.126 ? 0.079 us/op
Longs.toStringSmall 500 avgt 15 6.915 ? 0.259 us/op
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> 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.
Yes, while I don't know exactly which changes resolved JDK-6764325, it's clear
from the microbenchmarks added for #2102 that it's no longer an issue - at
least not in the mainline.
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 09:18:39 GMT, Alan Bateman wrote: > It would be useful to get up to date performance data on using Charset vs. > charset name. At least historically, the charset name versions were faster > (see [JDK-6764325](https://bugs.openjdk.java.net/browse/JDK-6764325)). The code in question was changed a few times since then, the last change was done by the https://github.com/openjdk/jdk/pull/2102. So currently the code for string.getBytes String/Charset uses the same code paths, except that the string version has an additional call to lookup the charset. The string version: https://github.com/openjdk/jdk/blob/66d1faa7847b645f20ab2e966adf0a523e3ffeb2/src/java.base/share/classes/java/lang/String.java#L1753 The charset version: https://github.com/openjdk/jdk/blob/66d1faa7847b645f20ab2e966adf0a523e3ffeb2/src/java.base/share/classes/java/lang/String.java#L1777 I checked the performance and the charset is always faster, depending on the string size, up to x20. @cl4es please confirm my observation since you did it already for https://github.com/openjdk/jdk/pull/2102 - PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 14:56:11 GMT, Claes Redestad wrote:
>> The code in `Integer.decode()` and `Long.decode()` might allocate two
>> instances of Integer/Long for the negative values less than -127:
>>
>> Integer result;
>>
>> result = Integer.valueOf(nm.substring(index), radix);
>> result = negative ? Integer.valueOf(-result.intValue()) : result;
>>
>> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
>> method. Same applicable for `Long` and some other classes.
>
> src/java.base/share/classes/java/lang/Integer.java line 1450:
>
>> 1448:
>> 1449: try {
>> 1450: result = parseInt(nm.substring(index), radix);
>
> Possibly a follow-up, but I think using `parseInt/-Long(nm, index,
> nm.length(), radix)` could give an additional speed-up in these cases (by
> avoiding a substring allocation).
Good point! Let me check this.
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Integrated: 8271601: Math.floorMod(int, int) and Math.floorMod(long, long) differ in their logic
On Mon, 2 Aug 2021 19:59:57 GMT, Raffaello Giulietti wrote: > Hello, > > please review this tiny change in the implementation of > j.l.Math.floorMod(int, int). > > While the results are unaffected, all of > floorDiv(int, int) > floorDiv(long, long) > floorMod(long, long) > use x ^ y in the tests to correct the result if needed. > > Not only is the proposed change more consistent with the other methods, but > it might benefit later stages in the cpu to proceed with the evaluation of x > ^ y in parallel with the previous x % y and, depending of the outcome, even > further down. > > > Greetings > Raffaello This pull request has now been integrated. Changeset: 66d1faa7 Author:Raffaello Giulietti Committer: Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/66d1faa7847b645f20ab2e966adf0a523e3ffeb2 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8271601: Math.floorMod(int, int) and Math.floorMod(long, long) differ in their logic Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/4962
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 13:16:42 GMT, Сергей Цыпанов
wrote:
> The code in `Integer.decode()` and `Long.decode()` might allocate two
> instances of Integer/Long for the negative values less than -127:
>
> Integer result;
>
> result = Integer.valueOf(nm.substring(index), radix);
> result = negative ? Integer.valueOf(-result.intValue()) : result;
>
> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
> method. Same applicable for `Long` and some other classes.
Looks fine to me.
Could you consider adding microbenchmarks for Integer/Long.decode?
src/java.base/share/classes/java/lang/Integer.java line 1450:
> 1448:
> 1449: try {
> 1450: result = parseInt(nm.substring(index), radix);
Possibly a follow-up, but I think using `parseInt/-Long(nm, index, nm.length(),
radix)` could give an additional speed-up in these cases (by avoiding a
substring allocation).
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5068
Integrated: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov 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[])`. This pull request has now been integrated. Changeset: 35b399ac Author:Andrey Turbanov Committer: Jayathirth D V URL: https://git.openjdk.java.net/jdk/commit/35b399aca810db63371ff65046f047ef0b955161 Stats: 70 lines in 8 files changed: 0 ins; 54 del; 16 mod 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying Reviewed-by: mullan, serb - PR: https://git.openjdk.java.net/jdk/pull/4487
RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
The code in `Integer.decode()` and `Long.decode()` might allocate two instances of Integer/Long for the negative values less than -127: Integer result; result = Integer.valueOf(nm.substring(index), radix); result = negative ? Integer.valueOf(-result.intValue()) : result; To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` method. Same applicable for `Long` and some other classes. - Commit messages: - Merge branch 'master' into 8267844 - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable Changes: https://git.openjdk.java.net/jdk/pull/5068/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5068=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267844 Stats: 12 lines in 4 files changed: 0 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/5068.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5068/head:pull/5068 PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> 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.
It would be useful to get up to date performance data on using Charset vs.
charset name. At least historically, the charset name versions were faster (see
[JDK-6764325](https://bugs.openjdk.java.net/browse/JDK-6764325)).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used [v2]
On Mon, 9 Aug 2021 19:59:21 GMT, Vladimir Ivanov wrote: >> Per Liden has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Private implies 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. Good point. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/5052
Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used [v2]
> 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. Per Liden has updated the pull request incrementally with one additional commit since the last revision: Private implies final - Changes: - all: https://git.openjdk.java.net/jdk/pull/5052/files - new: https://git.openjdk.java.net/jdk/pull/5052/files/66743f76..3bc3d5e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5052=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5052=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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
Integrated: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups
On Thu, 17 Jun 2021 12:27:25 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
> }
This pull request has now been integrated.
Changeset: 089e83bf
Author:Matthias Baesken
URL:
https://git.openjdk.java.net/jdk/commit/089e83bf1bf6f28cec8dd30288720b6d066301f0
Stats: 555 lines in 22 files changed: 476 ins; 28 del; 51 mod
8266490: Extend the OSContainer API to support the pids controller of cgroups
Reviewed-by: sgehwolf, lucy
-
PR: https://git.openjdk.java.net/jdk/pull/4518
Re: Issue with BuiltinClassLoader.ucp field not being visible
On Tue, Aug 10, 2021 at 1:34 AM Luke Hutchison wrote: > Maybe that should be part of the Instrumentation class, so that it is only > accessible to Java agent code that is able to get a reference to an > Instrumentation object. > Actually I take that last part back, that would not be very helpful to libraries like ClassGraph. It would be much better if there were simply a public method in `AppClassLoader` that could be used to get all classpath elements that have been added for instrumentation.
Re: Issue with BuiltinClassLoader.ucp field not being visible
On 10/08/2021 07:58, Luke Hutchison wrote: On Tue, Aug 10, 2021 at 12:51 AM Luke Hutchison wrote: Could a getURLClassPath() method please be added to BuiltInClassLoader? For security reasons, the getURLClassPath() method should return a copy of ucp, not the reference to ucp, to prevent the caller from modifying the system classpath. I think you need more context here. This a JDK internal class so shouldn't be used directly. It reads like the original message was looking for the value of java.class.path as a sequence of URLs, is that right? -Alan
Re: Issue with BuiltinClassLoader.ucp field not being visible
On Tue, Aug 10, 2021 at 12:51 AM Luke Hutchison wrote: > Could a getURLClassPath() method please be added to BuiltInClassLoader? > For security reasons, the getURLClassPath() method should return a copy of ucp, not the reference to ucp, to prevent the caller from modifying the system classpath.
Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v9]
> 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:
Update test Copyright headers
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4518/files
- new: https://git.openjdk.java.net/jdk/pull/4518/files/0dd58e00..217ffb26
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4518=08
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4518=07-08
Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4518.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4518/head:pull/4518
PR: https://git.openjdk.java.net/jdk/pull/4518
