Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v3]
> This converts jpackage to use `Stream.toList()` instead of > `Stream.collect(Collectors.toList())`. One piece of code was modified to not > mutate a list in addition to one test that used a mutating sort on a list. > The rest of the changes are simple substitutions. Ian Graves has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Clarifying type argument and shorting a toArray invocation - Converting jpackage to use Stream.toList() - Changes: https://git.openjdk.java.net/jdk/pull/2997/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2997=02 Stats: 47 lines in 17 files changed: 1 ins; 3 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/2997.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2997/head:pull/2997 PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On 16/03/2021 11:58 am, Sergey Bylokhov wrote: On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS. As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu. The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set. This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine. Both issues involve a problem trying to use the canonical name, but I'd consider both fixes deficient when an alternative name could be used. But this isn't my code so ... David - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Tue, 16 Mar 2021 02:34:39 GMT, Jaikiran Pai wrote: >>> If you and others think that we can ignore this case, then your proposed >>> approach of using this lazy holder for initialization, IMO, is much cleaner >>> and natural to read and I will go ahead and update this PR to use it. >> >> For me, at least, the holder pattern is clearer. I'm happy with that >> approach. ( I don't have an objection to the alternative, just a mild >> preference for the holder ) > > Hello Chris, using the holder pattern sounds fine to me then. I've updated > this PR accordingly. The test continues to pass with this fix. > > Peter, I didn't get a chance to try out the `@Stable` for the previous > approach but given that we decided to use this holder pattern, we will no > longer need the performance tweaks. Failure in Linux x86 build in GitHub Actions is unrelated to this change and looks like an environmental issue: 2021-03-16T02:35:22.3488438Z Err:59 https://dl.bintray.com/sbt/debian Release.gpg 2021-03-16T02:35:22.3489341Z 502 Bad Gateway [IP: 35.161.90.47 443] 2021-03-16T02:35:30.2615937Z Reading package lists... 2021-03-16T02:35:30.2842714Z E: The repository 'https://dl.bintray.com/sbt/debian Release' is no longer signed. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Mon, 15 Mar 2021 17:15:59 GMT, Chris Hegarty wrote:
>>> What you have is probably fine, but has any consideration been given to
>>> using the initialization-on-demand holder idiom? e.g.
>>>
>>> ```
>>> static private class CanonicalMapHolder {
>>> static final Map,
>>> ConstantDesc>> CANONICAL_MAP
>>> = Map.ofEntries(..);
>>> }
>>> ```
>>
>> Hello Chris,
>>
>> Although I had thought of some other alternate ways to fix this, this idiom
>> isn't something that I had thought of. Now that you showed this, I thought
>> about it a bit more and it does look a lot more "natural" to read and
>> maintain as compared to the current changes in this PR which does some
>> juggling to avoid this deadlock. However, having thought about your proposed
>> solution, I think _in theory_ it still leaves open the possibility of a
>> deadlock.
>>
>> Here's what the patch looks like with your suggested change:
>>
>> diff --git
>> a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> index 976b09e5665..c7bdcf4ce32 100644
>> --- a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> +++ b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> @@ -64,8 +64,6 @@ public abstract class DynamicConstantDesc
>> private final String constantName;
>> private final ClassDesc constantType;
>>
>> -private static Map,
>> ConstantDesc>> canonicalMap;
>> -
>> /**
>> * Creates a nominal descriptor for a dynamic constant.
>> *
>> @@ -274,25 +272,7 @@ public abstract class DynamicConstantDesc
>> }
>>
>> private ConstantDesc tryCanonicalize() {
>> -var canonDescs = canonicalMap;
>> -if (canonDescs == null) {
>> -canonDescs = Map.ofEntries(
>> -Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS,
>> DynamicConstantDesc::canonicalizePrimitiveClass),
>> -Map.entry(ConstantDescs.BSM_ENUM_CONSTANT,
>> DynamicConstantDesc::canonicalizeEnum),
>> -Map.entry(ConstantDescs.BSM_NULL_CONSTANT,
>> DynamicConstantDesc::canonicalizeNull),
>> -Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD,
>> DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
>> -Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD,
>> DynamicConstantDesc::canonicalizeFieldVarHandle),
>> -Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY,
>> DynamicConstantDesc::canonicalizeArrayVarHandle));
>> -synchronized (DynamicConstantDesc.class) {
>> -if (canonicalMap == null) {
>> -canonicalMap = canonDescs;
>> -} else {
>> -canonDescs = canonicalMap;
>> -}
>> -}
>> -}
>> -
>> -Function, ConstantDesc> f =
>> canonDescs.get(bootstrapMethod);
>> +Function, ConstantDesc> f =
>> CanonicalMapHolder.CANONICAL_MAP.get(bootstrapMethod);
>> if (f != null) {
>> try {
>> return f.apply(this);
>> @@ -405,4 +385,15 @@ public abstract class DynamicConstantDesc
>> super(bootstrapMethod, constantName, constantType,
>> bootstrapArgs);
>> }
>> }
>> +
>> +private static final class CanonicalMapHolder {
>> +static final Map,
>> ConstantDesc>> CANONICAL_MAP =
>> +Map.ofEntries(
>> +Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS,
>> DynamicConstantDesc::canonicalizePrimitiveClass),
>> +Map.entry(ConstantDescs.BSM_ENUM_CONSTANT,
>> DynamicConstantDesc::canonicalizeEnum),
>> +Map.entry(ConstantDescs.BSM_NULL_CONSTANT,
>> DynamicConstantDesc::canonicalizeNull),
>> +Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD,
>> DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
>> +Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD,
>> DynamicConstantDesc::canonicalizeFieldVarHandle),
>> +Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY,
>> DynamicConstantDesc::canonicalizeArrayVarHandle));
>> +}
>> }
>>
>>
>> Please consider the following, where I try and explain the theoretical
>> possibility of a deadlock with this approach:
>>
>> 1. Let's consider 2 threads T1 and T2 doing concurrent execution
>>
>> 2. Let's for a moment assume that neither `DynamicConstantDesc` nor
>> `ConstantDescs` classes have been initialized.
>>
>> 3. T1 does a call to `DynamicConstantDesc.ofCanonical(...)` and T2 does a
>> call to something/anything on `ConstantDescs`, which triggers a class
>> initialization of `ConstantDescs`.
>>
>> 4. T1 (which called `DynamicConstantDesc.ofCanonical(...)`) will reach the
>> `tryCanonicalize` and will access `CanonicalMapHolder.CANONICAL_MAP` in the
>> implementation of that method. Because of this access (and
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v4]
> Can I please get a review for this proposed patch for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8263108? > > As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and > `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due > to the nature of the code involved in their static blocks. A thread dump of > one such deadlock (reproduced using the code attached to that issue) is as > follows: > > "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s > tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) > - waiting on the Class initialization monitor for > java.lang.constant.ConstantDescs > at Deadlock.threadA(Deadlock.java:14) > at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s > tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) > - waiting on the Class initialization monitor for > java.lang.constant.DynamicConstantDesc > at > java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) > at Deadlock.threadB(Deadlock.java:24) > at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > The commit in this PR resolves that deadlock by moving the `canonicalMap` > initialization in `DynamicConstantDesc`, from the static block, to a lazily > initialized map, into the `tryCanonicalize` (private) method of the same > class. That's the only method which uses this map. > > The implementation in `tryCanonicalize` carefully ensures that the deadlock > isn't shifted from the static block to this method, by making sure that the > `synchronization` on `DynamicConstantDesc` in this method happens only when > it's time to write the state to the shared `canonicalMap`. This has an > implication that the method local variable `canonDescs` can get initialized > by multiple threads unnecessarily but from what I can see, that's the only > way we can avoid this deadlock. This penalty of multiple threads initializing > and then throwing away that map isn't too huge because that will happen only > once when the `canonicalMap` is being initialized for the first time. > > An alternate approach that I thought of was to completely get rid of this > shared cache `canonicalMap` and instead just use method level map (that gets > initialized each time) in the `tryCanonicalize` method (thus requiring no > locks/synchronization). I ran a JMH benchmark with the current change > proposed in this PR and with the alternate approach of using the method level > map. The performance number from that run showed that the approach of using > the method level map has relatively big impact on performance (even when > there's a cache hit in that method). So I decided to not pursue that > approach. I'll include the benchmark code and the numbers in a comment in > this PR, for reference. > > The commit in this PR also includes a jtreg testcase which (consistently) > reproduces the deadlock without the fix and passes when this fix is applied. > Extra manual testing has been done to verify that the classes of interest > (noted in that testcase) are indeed getting loaded in those concurrent > threads and not in the main thread. For future maintainers, there's a > implementation note added on that testcase explaining why it cannot be moved > into testng test. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Use Chris' suggested solution of avoiding deadlock - Changes: - all: https://git.openjdk.java.net/jdk/pull/2893/files - new: https://git.openjdk.java.net/jdk/pull/2893/files/2b51ec9d..3b8e4a5d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2893=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2893=02-03 Stats: 32 lines in 1 file changed: 11 ins; 20 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2893.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2893/head:pull/2893 PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: > This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1: > 1: import java.io.IOException; Copyright? test/jdk/tools/launcher/8261785/Test8261785.java line 5: > 3: * COPYRIGHT NOTICES OR THIS FILE HEADER. > 4: * > 5: * This code is free software; you can redistribute it and/or modify it > under the terms of the GNU Looks like formatting much wider than usual - PR: https://git.openjdk.java.net/jdk/pull/2999
Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM
On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen wrote: > This patch ensure launcher won't crash JVM for the new static Methods from > local/anonymous class on MacOS. > > As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug > when the launcher trying to grab class name to be displayed as the > Application name on the menu. > > The fix is to not setting name, test shows that GUI java application shows > 'bin' as the application name. It's possible for us to set the name to > something more friendly, for example, "Java", but I am not sure that should > be launcher's responsibility to choose such a default name. It seems to me > the consumer of the JAVA_MAIN_CLASS_%d environment variable should be > responsible to pick such name in case the environment variable is not set. This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine. - PR: https://git.openjdk.java.net/jdk/pull/2999
RFR: 8261673: Move javadoc for the lookup mechanism to module-info
Consolidate and move javadoc for the lookup mechanism to the module summary. - Commit messages: - 8261673: Move javadoc for the lookup mechanism to module-info Changes: https://git.openjdk.java.net/jdk/pull/3020/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3020=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261673 Stats: 602 lines in 9 files changed: 197 ins; 361 del; 44 mod Patch: https://git.openjdk.java.net/jdk/pull/3020.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3020/head:pull/3020 PR: https://git.openjdk.java.net/jdk/pull/3020
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v6]
On Mon, 15 Mar 2021 18:04:26 GMT, Сергей Цыпанов wrote: >> In some cases wrapping of `InputStream` with `BufferedInputStream` is >> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which >> does not require any buffer having one within. >> >> Other cases are related to reading either a byte or short `byte[]`: in both >> cases `BufferedInputStream.fill()` will be called resulting in load of much >> bigger amount of data (8192 by default) than required. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > Revert HttpClient Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Mon, 15 Mar 2021 12:54:32 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Missing @since
src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 62:
> 60: @Retention(RetentionPolicy.RUNTIME)
> 61: @Target(ElementType.TYPE)
> 62: public @interface RandomGeneratorProperties {
Should the is-deprecated information be stored here?
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
On Mon, 15 Mar 2021 12:54:32 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Missing @since
src/java.base/share/classes/java/util/Random.java line 135:
> 133: * number generator which is maintained by method {@link #next}.
> 134: *
> 135: * @implSpec The invocation {@code new Random(seed)} is
> equivalent to:
This is not conventional formatting for an implSpec section. I suggest putting
implSpec on its own line and then left-justifying the text as normal. A new
paragraph tag is no needed at the beginning of implSpec.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
On Mon, 15 Mar 2021 18:53:26 GMT, Ian Graves wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Clarifying type argument and shorting a toArray invocation Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v3]
On Mon, 15 Mar 2021 21:47:28 GMT, Сергей Цыпанов
wrote:
>> Hello,
>>
>> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
>> joined Strings.
>>
>> This applies for the cases when all joined Strings as well as delimiter,
>> prefix and suffix contain only ASCII symbols.
>>
>> As a result when `StringJoiner.toString()` is called `byte[]` stored in
>> Strings is inflated in order to fill in `char[]` and after that `char[]` is
>> compressed when constructor of String is called:
>> String delimiter = this.delimiter;
>> char[] chars = new char[this.len + addLen];
>> int k = getChars(this.prefix, chars, 0);
>> if (size > 0) {
>> k += getChars(elts[0], chars, k);// inflate byte[]
>>
>> for(int i = 1; i < size; ++i) {
>> k += getChars(delimiter, chars, k);
>> k += getChars(elts[i], chars, k);
>> }
>> }
>>
>> k += getChars(this.suffix, chars, k);
>> return new String(chars);// compress char[] -> byte[]
>> This can be improved by utilizing new method `String.getBytes(byte[], int,
>> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
>> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
>> covering both cases when resulting String is Latin1 or UTF-16
>>
>> I've prepared a patch along with benchmark proving that this change is
>> correct and brings improvement.
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class StringJoinerBenchmark {
>>
>> @Benchmark
>> public String stringJoiner(Data data) {
>> String[] stringArray = data.stringArray;
>> return Joiner.joinWithStringJoiner(stringArray);
>> }
>>
>> @State(Scope.Thread)
>> public static class Data {
>>
>> @Param({"latin", "cyrillic", "mixed"})
>> private String mode;
>>
>> @Param({"8", "32", "64"})
>> private int length;
>>
>> @Param({"5", "10", "100"})
>> private int count;
>>
>> private String[] stringArray;
>>
>> @Setup
>> public void setup() {
>> RandomStringGenerator generator = new RandomStringGenerator();
>>
>> stringArray = new String[count];
>>
>> for (int i = 0; i < count; i++) {
>> String alphabet = getAlphabet(i, mode);
>> stringArray[i] = generator.randomString(alphabet, length);
>> }
>> }
>>
>> private static String getAlphabet(int index, String mode) {
>> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
>> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>>
>> String alphabet;
>> switch (mode) {
>> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
>> case "latin" -> alphabet = latin;
>> case "cyrillic" -> alphabet = cyrillic;
>> default -> throw new RuntimeException("Illegal mode " + mode);
>> }
>> return alphabet;
>> }
>> }
>> }
>>
>> public class Joiner {
>>
>> public static String joinWithStringJoiner(String[] stringArray) {
>> StringJoiner joiner = new StringJoiner(",", "[", "]");
>> for (String str : stringArray) {
>> joiner.add(str);
>> }
>> return joiner.toString();
>> }
>> }
>>
>>
>> (count) (length)(mode)
>> Java 14 patched Units
>> stringJoiner 5 8 latin 78.836
>> ± 0.20867.546 ± 0.500 ns/op
>> stringJoiner 532 latin 92.877
>> ± 0.42266.760 ± 0.498 ns/op
>> stringJoiner 564 latin 115.423
>> ± 0.88373.224 ± 0.289 ns/op
>> stringJoiner 10 8 latin 152.587
>> ± 0.429 161.427 ± 0.635 ns/op
>> stringJoiner 1032 latin 189.998
>> ± 0.478 164.099 ± 0.963 ns/op
>> stringJoiner 1064 latin 238.679
>> ± 1.419 176.825 ± 0.533 ns/op
>> stringJoiner100 8 latin 1215.612
>> ± 17.413 1541.802 ± 126.166 ns/op
>> stringJoiner10032 latin 1699.998
>> ± 28.407 1563.341 ± 4.439 ns/op
>> stringJoiner10064 latin 2289.388
>> ± 45.319 2215.931 ± 137.583 ns/op
>> stringJoiner 5 8 cyrillic 96.692
>> ± 0.94780.946 ± 0.371 ns/op
>> stringJoiner 532 cyrillic 107.806
>> ± 0.42984.717 ± 0.541 ns/op
>> stringJoiner 564 cyrillic 150.762
>> ± 2.26796.214 ± 1.251 ns/op
>> stringJoiner 10 8 cyrillic
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v3]
> Hello,
>
> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
> joined Strings.
>
> This applies for the cases when all joined Strings as well as delimiter,
> prefix and suffix contain only ASCII symbols.
>
> As a result when `StringJoiner.toString()` is called `byte[]` stored in
> Strings is inflated in order to fill in `char[]` and after that `char[]` is
> compressed when constructor of String is called:
> String delimiter = this.delimiter;
> char[] chars = new char[this.len + addLen];
> int k = getChars(this.prefix, chars, 0);
> if (size > 0) {
> k += getChars(elts[0], chars, k);// inflate byte[]
>
> for(int i = 1; i < size; ++i) {
> k += getChars(delimiter, chars, k);
> k += getChars(elts[i], chars, k);
> }
> }
>
> k += getChars(this.suffix, chars, k);
> return new String(chars);// compress char[] -> byte[]
> This can be improved by utilizing new method `String.getBytes(byte[], int,
> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
> covering both cases when resulting String is Latin1 or UTF-16
>
> I've prepared a patch along with benchmark proving that this change is
> correct and brings improvement.
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class StringJoinerBenchmark {
>
> @Benchmark
> public String stringJoiner(Data data) {
> String[] stringArray = data.stringArray;
> return Joiner.joinWithStringJoiner(stringArray);
> }
>
> @State(Scope.Thread)
> public static class Data {
>
> @Param({"latin", "cyrillic", "mixed"})
> private String mode;
>
> @Param({"8", "32", "64"})
> private int length;
>
> @Param({"5", "10", "100"})
> private int count;
>
> private String[] stringArray;
>
> @Setup
> public void setup() {
> RandomStringGenerator generator = new RandomStringGenerator();
>
> stringArray = new String[count];
>
> for (int i = 0; i < count; i++) {
> String alphabet = getAlphabet(i, mode);
> stringArray[i] = generator.randomString(alphabet, length);
> }
> }
>
> private static String getAlphabet(int index, String mode) {
> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>
> String alphabet;
> switch (mode) {
> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
> case "latin" -> alphabet = latin;
> case "cyrillic" -> alphabet = cyrillic;
> default -> throw new RuntimeException("Illegal mode " + mode);
> }
> return alphabet;
> }
> }
> }
>
> public class Joiner {
>
> public static String joinWithStringJoiner(String[] stringArray) {
> StringJoiner joiner = new StringJoiner(",", "[", "]");
> for (String str : stringArray) {
> joiner.add(str);
> }
> return joiner.toString();
> }
> }
>
>
> (count) (length)(mode)
> Java 14 patched Units
> stringJoiner 5 8 latin 78.836
> ± 0.20867.546 ± 0.500 ns/op
> stringJoiner 532 latin 92.877
> ± 0.42266.760 ± 0.498 ns/op
> stringJoiner 564 latin 115.423
> ± 0.88373.224 ± 0.289 ns/op
> stringJoiner 10 8 latin 152.587
> ± 0.429 161.427 ± 0.635 ns/op
> stringJoiner 1032 latin 189.998
> ± 0.478 164.099 ± 0.963 ns/op
> stringJoiner 1064 latin 238.679
> ± 1.419 176.825 ± 0.533 ns/op
> stringJoiner100 8 latin 1215.612
> ± 17.413 1541.802 ± 126.166 ns/op
> stringJoiner10032 latin 1699.998
> ± 28.407 1563.341 ± 4.439 ns/op
> stringJoiner10064 latin 2289.388
> ± 45.319 2215.931 ± 137.583 ns/op
> stringJoiner 5 8 cyrillic 96.692
> ± 0.94780.946 ± 0.371 ns/op
> stringJoiner 532 cyrillic 107.806
> ± 0.42984.717 ± 0.541 ns/op
> stringJoiner 564 cyrillic 150.762
> ± 2.26796.214 ± 1.251 ns/op
> stringJoiner 10 8 cyrillic 190.570
> ± 0.381 182.754 ± 0.678 ns/op
> stringJoiner 1032 cyrillic 240.239
> ± 1.110 187.991 ± 1.575 ns/op
>
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v2]
On Mon, 15 Mar 2021 20:54:54 GMT, Claes Redestad wrote: >> Сергей Цыпанов 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: >> >> - 8148937: (str) Use StringBuilder as cl4es suggests >> - 8148937: (str) Check also delimiter, prefix and suffix >> - 8148937: (str) Adapt StringJoiner for Compact Strings > > The copyright year and description of the microbenchmark needs to be updated > (my copy-paste error). I'd also consider narrowing down the parameter > combinations and iteration times so that default execution times is more > reasonable (currently ~45 minutes even though these micros stabilize in > seconds) Done. I've removed mixed mode from benchmark, anyway it falls back to non-latin1 case - PR: https://git.openjdk.java.net/jdk/pull/2627
Re: RFR: JDK-8263154: [macos] DMG builds have finder errors
On Sat, 13 Mar 2021 14:39:40 GMT, Andy Herrick wrote: > JDK-8263154: [macos] DMG builds have finder errors Can you explain what problem is and how it is fixed? Does it fails when "install-dir" is specified? "install-dir" is not relevant for DMG image and we should ignore it. - PR: https://git.openjdk.java.net/jdk/pull/2987
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v2]
On Mon, 15 Mar 2021 18:00:27 GMT, Сергей Цыпанов
wrote:
>> Hello,
>>
>> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
>> joined Strings.
>>
>> This applies for the cases when all joined Strings as well as delimiter,
>> prefix and suffix contain only ASCII symbols.
>>
>> As a result when `StringJoiner.toString()` is called `byte[]` stored in
>> Strings is inflated in order to fill in `char[]` and after that `char[]` is
>> compressed when constructor of String is called:
>> String delimiter = this.delimiter;
>> char[] chars = new char[this.len + addLen];
>> int k = getChars(this.prefix, chars, 0);
>> if (size > 0) {
>> k += getChars(elts[0], chars, k);// inflate byte[]
>>
>> for(int i = 1; i < size; ++i) {
>> k += getChars(delimiter, chars, k);
>> k += getChars(elts[i], chars, k);
>> }
>> }
>>
>> k += getChars(this.suffix, chars, k);
>> return new String(chars);// compress char[] -> byte[]
>> This can be improved by utilizing new method `String.getBytes(byte[], int,
>> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
>> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
>> covering both cases when resulting String is Latin1 or UTF-16
>>
>> I've prepared a patch along with benchmark proving that this change is
>> correct and brings improvement.
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class StringJoinerBenchmark {
>>
>> @Benchmark
>> public String stringJoiner(Data data) {
>> String[] stringArray = data.stringArray;
>> return Joiner.joinWithStringJoiner(stringArray);
>> }
>>
>> @State(Scope.Thread)
>> public static class Data {
>>
>> @Param({"latin", "cyrillic", "mixed"})
>> private String mode;
>>
>> @Param({"8", "32", "64"})
>> private int length;
>>
>> @Param({"5", "10", "100"})
>> private int count;
>>
>> private String[] stringArray;
>>
>> @Setup
>> public void setup() {
>> RandomStringGenerator generator = new RandomStringGenerator();
>>
>> stringArray = new String[count];
>>
>> for (int i = 0; i < count; i++) {
>> String alphabet = getAlphabet(i, mode);
>> stringArray[i] = generator.randomString(alphabet, length);
>> }
>> }
>>
>> private static String getAlphabet(int index, String mode) {
>> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
>> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>>
>> String alphabet;
>> switch (mode) {
>> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
>> case "latin" -> alphabet = latin;
>> case "cyrillic" -> alphabet = cyrillic;
>> default -> throw new RuntimeException("Illegal mode " + mode);
>> }
>> return alphabet;
>> }
>> }
>> }
>>
>> public class Joiner {
>>
>> public static String joinWithStringJoiner(String[] stringArray) {
>> StringJoiner joiner = new StringJoiner(",", "[", "]");
>> for (String str : stringArray) {
>> joiner.add(str);
>> }
>> return joiner.toString();
>> }
>> }
>>
>>
>> (count) (length)(mode)
>> Java 14 patched Units
>> stringJoiner 5 8 latin 78.836
>> ± 0.20867.546 ± 0.500 ns/op
>> stringJoiner 532 latin 92.877
>> ± 0.42266.760 ± 0.498 ns/op
>> stringJoiner 564 latin 115.423
>> ± 0.88373.224 ± 0.289 ns/op
>> stringJoiner 10 8 latin 152.587
>> ± 0.429 161.427 ± 0.635 ns/op
>> stringJoiner 1032 latin 189.998
>> ± 0.478 164.099 ± 0.963 ns/op
>> stringJoiner 1064 latin 238.679
>> ± 1.419 176.825 ± 0.533 ns/op
>> stringJoiner100 8 latin 1215.612
>> ± 17.413 1541.802 ± 126.166 ns/op
>> stringJoiner10032 latin 1699.998
>> ± 28.407 1563.341 ± 4.439 ns/op
>> stringJoiner10064 latin 2289.388
>> ± 45.319 2215.931 ± 137.583 ns/op
>> stringJoiner 5 8 cyrillic 96.692
>> ± 0.94780.946 ± 0.371 ns/op
>> stringJoiner 532 cyrillic 107.806
>> ± 0.42984.717 ± 0.541 ns/op
>> stringJoiner 564 cyrillic 150.762
>> ± 2.26796.214 ± 1.251 ns/op
>> stringJoiner 10 8 cyrillic
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Fri, 26 Feb 2021 13:15:28 GMT, Rémi Forax
wrote:
>> Looking for some final code reviews.
>
> I still don't like the fact that the factory uses reflection but i don't see
> how to do better
This is now looking very nicely structured.
The only thing i am unsure are the details around `RandomGenerator` being a
service provider interface. The documentation mentions this at various points
(mostly as implementation notes), but it's not really called out on
`RandomGenerator`.
Trying out the patch, I can implement `RandomGenerator` and register it as a
service:
public class AlwaysZero implements RandomGenerator {
@Override
public long nextLong() {
return 0;
}
}
...
RandomGenerator alwaysZero = RandomGenerator.of("AlwaysZero");
Is that intended? (esp. since the annotation `RandomGeneratorProperties` is not
public). If i rename the above to `L32X64MixRandom` an
`ExceptionInInitializerError` is produced due to duplicate keys.
I suspect you want to filter out the service providers to those that only
declare `RandomGeneratorProperties`, thereby restricting to providers only
supplied by the platform.
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: Comment on CSR 8251991: Hex formatting and parsing utility
Hello,
I understand your points expressed in
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-October/070317.html
On the other hand, it kind of hurts when the javadoc spec implies that
the fromXXX() methods do not logically depend on the state and yet one
has to invoke them on some instance anyway.
As for HexFormat.of() to always return the same instance, you are of
course right in observing that this is out of place for value-based
classes. The intent was to describe a static-like idiom that would deter
programmers to rewrite their own static version of these methods.
Greetings
Raffaello
Hi,
The question about static vs instance methods was raised during the review.
At the time, my rationale for the current API is summarized in this email.
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-October/070317.html
It comes down to choosing one kind of consistency vs another.
I was/am looking for additional comments to weigh one or the other
alternative.
I did receive one offline comment in support of static methods on the
rationale of
convenience.
Regards, Roger
On 3/15/21 12:06 PM, Raffaello Giulietti wrote:
Hi,
the javadoc of most of the methods listed in my previous post below
reads:
"The delimiter, prefix, suffix, and uppercase parameters are not
used."
As these eventually constitute the whole state of an instance of this
final class and as the state is not involved, it is possible to
declare the methods as static.
If, for some reason, declaring the methods as static is not a choice,
the next best thing is to clarify that
* HexFormat.of() always returns the same instance and thus
* absent any other instance, the recommended idiom for invoking any of
the methods below is, for example,
HexFormat.of().isHexDigit('F')
(because there are almost no additional costs wrt static methods.)
Note that HexFormat is specified as a value based class and
any statement related to identity is out of place.
Incidentally, its generally discouraged and can cause a warning when a
static method is invoked using a reference.
But I don't see a reason why they should be non-static. Just oversight?
Greetings
Raffaello
On 2021-03-08 11:52, Raffaello Giulietti wrote:
Hello,
it appears that all of the following API methods in [1] can be
declared *static*, which makes them more useful in contexts where
there's no instance of HexFormat available or none is desired.
As 17 has not yet entered any formal phase, the change should be
harmless.
public boolean isHexDigit(int);
public int fromHexDigit(int);
public int fromHexDigits(java.lang.CharSequence);
public int fromHexDigits(java.lang.CharSequence, int, int);
public long fromHexDigitsToLong(java.lang.CharSequence);
public long fromHexDigitsToLong(java.lang.CharSequence, int, int);
Greetings
Raffaello
[1] https://bugs.openjdk.java.net/browse/JDK-8251991
Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]
On Mon, 15 Mar 2021 19:43:12 GMT, Alexey Semenyuk wrote: >> Changes requested by iklam (Reviewer). > > @kevinrushforth I plan to resolve JDK-8263474 manually as "Delivered". Would > this work? Yes, that should work. - PR: https://git.openjdk.java.net/jdk/pull/2975
Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]
On Mon, 15 Mar 2021 19:55:03 GMT, Ioi Lam wrote: >> It is better to have `@compile` everywhere for consistency. >> >> The proper way to run tests is by passing test class name as an argument for >> test runner, test class should not have `main()` and should have `@Test` >> annotation for test methods. Some tests have not been updated to follow this >> design and adding `@compile` to them doesn't make a difference now. But >> eventually they will be updated and they will need `@compile` anyways. > > If this is the plan please update the bug description to reflect this. > Otherwise it's not clear why this is done. The `@compile` in those files > aren't really "missing" since they are not required. I would suggest changing > the bug title to "Add @compile tags to jpackage tests" > adding `@compile` to them doesn't make a difference now Also, the above is not true. `@compile` will always run javac every time you run the test. I think you should use `@build` instead -- if the class doesn't exist, it will be compiled. If it already exists (you run jtreg a second time), the class won't be compiled again, so it will run a little faster. - PR: https://git.openjdk.java.net/jdk/pull/2975
Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]
On Mon, 15 Mar 2021 19:41:06 GMT, Alexey Semenyuk wrote: >> test/jdk/tools/jpackage/windows/WinDirChooserTest.java line 44: >> >>> 42: * @requires (os.family == "windows") >>> 43: * @modules jdk.jpackage/jdk.jpackage.internal >>> 44: * @compile WinDirChooserTest.java >> >> Changes like this one is not necessary. `@run` will build WinDirChooserTest >> automatically. >> >> `@compile` should be added only when necessary, like WinInstallerUiTest.java > > It is better to have `@compile` everywhere for consistency. > > The proper way to run tests is by passing test class name as an argument for > test runner, test class should not have `main()` and should have `@Test` > annotation for test methods. Some tests have not been updated to follow this > design and adding `@compile` to them doesn't make a difference now. But > eventually they will be updated and they will need `@compile` anyways. If this is the plan please update the bug description to reflect this. Otherwise it's not clear why this is done. The `@compile` in those files aren't really "missing" since they are not required. I would suggest changing the bug title to "Add @compile tags to jpackage tests" - PR: https://git.openjdk.java.net/jdk/pull/2975
Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]
On Sat, 13 Mar 2021 08:38:16 GMT, Ioi Lam wrote: >> Alexey Semenyuk has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains one commit: >> >> 8263536: Add missing @compile tags to jpackage tests > > Changes requested by iklam (Reviewer). @kevinrushforth I plan to resolve JDK-8263474 manually as "Delivered". Would this work? > test/jdk/tools/jpackage/windows/WinDirChooserTest.java line 44: > >> 42: * @requires (os.family == "windows") >> 43: * @modules jdk.jpackage/jdk.jpackage.internal >> 44: * @compile WinDirChooserTest.java > > Changes like this one is not necessary. `@run` will build WinDirChooserTest > automatically. > > `@compile` should be added only when necessary, like WinInstallerUiTest.java It is better to have `@compile` everywhere for consistency. The proper way to run tests is by passing test class name as an argument for test runner, test class should not have `main()` and should have `@Test` annotation for test methods. Some tests have not been updated to follow this design and adding `@compile` to them doesn't make a difference now. But eventually they will be updated and they will need `@compile` anyways. - PR: https://git.openjdk.java.net/jdk/pull/2975
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
On Mon, 15 Mar 2021 18:53:26 GMT, Ian Graves wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Clarifying type argument and shorting a toArray invocation Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v28]
On Sat, 13 Mar 2021 05:49:53 GMT, David Holmes wrote:
>> Anton Kozlov has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 114 commits:
>>
>> - JDK-8262491: bsd_aarch64 part
>> - JDK-8263002: bsd_aarch64 part
>> - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
>> - Wider #ifdef block
>> - Fix most of issues in java/foreign/ tests
>>
>>Failures related to va_args are tracked in JDK-8263512.
>> - Add Azul copyright
>> - Update Oracle copyright years
>> - Use Thread::current_or_null_safe in SafeFetch
>> - 8262903: [macos_aarch64] Thread::current() called on detached thread
>> - Merge commit 'refs/pull/11/head' of https://github.com/AntonKozlov/jdk
>> into jdk-macos
>> - ... and 104 more:
>> https://git.openjdk.java.net/jdk/compare/d825198e...806fc618
>
> src/hotspot/share/runtime/safefetch.inline.hpp line 35:
>
>> 33: inline int SafeFetch32(int* adr, int errValue) {
>> 34: assert(StubRoutines::SafeFetch32_stub(), "stub not yet generated");
>> 35: Thread* thread = Thread::current_or_null_safe();
>
> Sorry but this should be MACOS_AARCH64 only. All three lines need to be
> ifdef'd if you are going to include the assertion.
>
> Thanks,
> David
Right, thanks! Fixed in
https://github.com/openjdk/jdk/pull/2200/commits/3d0f4d2342adc867eaf762fa83a9c3035d6439bd
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v21]
On Wed, 10 Mar 2021 11:21:44 GMT, Andrew Haley wrote: >> We always check for `R18_RESERVED` with `#if(n)def`, is there any reason to >> define the value for the macro? > > Robustness, clarity, maintainability, convention. Why not? I've tried to implement the suggestion, but it pulled more unnecessary changes. It makes the intended way to check the condition less clear (`#ifdef` and not `#if`). The rest of the defines in this file follows the pattern: a define without a value to be checked with `#ifdef` and define with a value to be checked with `#if`. To be consistent, I would need to add `#define R18_RESERVED false` to the `#else` clause and change every `#ifdef R18_RESERVED`/`#ifndef R18_RESERVED` to `#if R18_RESERVED`/`#if !R18_RESERVED`. I think we'll win in clarity in the long term if I will not implement the suggestion without related changes. (And related changes would introduce additional noise, which we are fighting with). - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]
On Thu, 11 Mar 2021 20:27:51 GMT, Stefan Karlsson wrote: >> The thread_bsd_aarch64.hpp describes a part of JavaThread, while this block >> belongs to Thread for now. Since W^X is an attribute of any operating system >> thread, I assumed Thread to be the right place for W^X bookkeeping. >> >> In most cases, we manage W^X state of JavaThread. But sometimes a GC thread >> needs the WXWrite state, or safefetch is called from non-JavaThread. >> Probably this can be dealt with (e.g. GCThread to always have the WXWrite >> state). But such change would be much more than a simple refactoring and it >> would require a significant amount of testing. Ideally, I would like to >> investigate this as a follow-up change, or at least after other fixes to >> this PR. > > Good point about Thread vs JavaThread. Yes, this can be looked into as > follow-up cleanups. The enhancement is tracked in https://bugs.openjdk.java.net/browse/JDK-8263492 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
On Mon, 15 Mar 2021 17:29:46 GMT, Alexey Semenyuk wrote:
>> Ian Graves has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Clarifying type argument and shorting a toArray invocation
>
> src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java
> line 200:
>
>> 198: ).map(e -> {
>> 199: e.getValue().setOutputFileName(e.getKey());
>> 200: return (WixFragmentBuilder) e.getValue();
>
> Why this explicit cast is needed here?
The explicit cast is needed because `Stream.toList()` is invariant, but the
stream is producing a covariant type (by way of the type inferencer and
`Stream.of(..)` accepting two subclasses of `WixFragmentBuilder`). We need to
introduce a hint to bring the type back to the invariant form. On second
thought I'm going to make this explicit type argument instead so it's clear
that the casting isn't part to what the code is otherwise doing.
-
PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList() [v2]
> This converts jpackage to use `Stream.toList()` instead of > `Stream.collect(Collectors.toList())`. One piece of code was modified to not > mutate a list in addition to one test that used a mutating sort on a list. > The rest of the changes are simple substitutions. Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Clarifying type argument and shorting a toArray invocation - Changes: - all: https://git.openjdk.java.net/jdk/pull/2997/files - new: https://git.openjdk.java.net/jdk/pull/2997/files/1bb46b54..ce11489b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2997=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2997=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2997.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2997/head:pull/2997 PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy
On Thu, 18 Feb 2021 07:12:34 GMT, Andrey Turbanov wrote: >> Hi @turbanoff, I'm happy to sponsor but I see two comments by @marschall - >> have they been addressed? >> https://github.com/openjdk/jdk/pull/1853#discussion_r572815422 >> https://github.com/openjdk/jdk/pull/1853#discussion_r572380746 > > @FrauBoes fixed in the last commit. Is there any way to de-_integrate_ PR (to > include last commit)? @turbanoff Tier 1-3 still all clear. If you /integrate, I will sponsor this tomorrow. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings
On Mon, 15 Mar 2021 14:07:29 GMT, Claes Redestad wrote:
>> I was thinking about `StringBuilder` at the very beginning but then decided
>> to have no bounds checks and reallocations. Indeed from maintainability
>> point of view your solution is much more attractive. I have one minor
>> comment about `append()`: I think we should do more chaining, e.g.
>> StringBuilder sb = new StringBuilder(len);
>> sb.append(elts[0]);
>> should be
>> StringBuilder sb = new StringBuilder(len).append(elts[0]);
>> etc. to have less bytecode.
>>
>> E.g. if we take
>> public String sb() {
>> StringBuilder sb = new StringBuilder();
>>
>> sb.append("a");
>> sb.append("b");
>>
>> return sb.toString();
>> }
>> `sb.append()` gets compiled into
>> L1
>> LINENUMBER 23 L1
>> ALOAD 1
>> LDC "a"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> POP
>> L2
>> LINENUMBER 24 L2
>> ALOAD 1
>> LDC "b"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> POP
>> ```
>> and in case we have
>> ```
>> sb.append("a").append("b");
>> ```
>> the amount of byte code is reduced:
>> ```
>> L1
>> LINENUMBER 23 L1
>> ALOAD 1
>> LDC "a"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> LDC "b"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> POP
>> ```
>>
>> Also I'd change
>> if (addLen == 0) {
>> compactElts();
>> return size == 0 ? "" : elts[0];
>> }
>> to
>> if (size == 0) {
>> if (addLen == 0) {
>> return "";
>> }
>> return prefix + suffix;
>> }
>> The second variant is more understandable to me and likely to be slightly
>> faster.
>>
>> And finally, should I close this PR and you'll create a new one from your
>> branch, or I need to copy your changes here?
>
> Up to you. If you adapt your PR to use a StringBuilder as suggested I can
> review and sponsor.
I'll reuse existing PR then.
-
PR: https://git.openjdk.java.net/jdk/pull/2627
Re: Comment on CSR 8251991: Hex formatting and parsing utility
Hi,
The question about static vs instance methods was raised during the review.
At the time, my rationale for the current API is summarized in this email.
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-October/070317.html
It comes down to choosing one kind of consistency vs another.
I was/am looking for additional comments to weigh one or the other
alternative.
I did receive one offline comment in support of static methods on the
rationale of
convenience.
Regards, Roger
On 3/15/21 12:06 PM, Raffaello Giulietti wrote:
Hi,
the javadoc of most of the methods listed in my previous post below
reads:
"The delimiter, prefix, suffix, and uppercase parameters are not
used."
As these eventually constitute the whole state of an instance of this
final class and as the state is not involved, it is possible to
declare the methods as static.
If, for some reason, declaring the methods as static is not a choice,
the next best thing is to clarify that
* HexFormat.of() always returns the same instance and thus
* absent any other instance, the recommended idiom for invoking any of
the methods below is, for example,
HexFormat.of().isHexDigit('F')
(because there are almost no additional costs wrt static methods.)
Note that HexFormat is specified as a value based class and
any statement related to identity is out of place.
Incidentally, its generally discouraged and can cause a warning when a
static method is invoked using a reference.
But I don't see a reason why they should be non-static. Just oversight?
Greetings
Raffaello
On 2021-03-08 11:52, Raffaello Giulietti wrote:
Hello,
it appears that all of the following API methods in [1] can be
declared *static*, which makes them more useful in contexts where
there's no instance of HexFormat available or none is desired.
As 17 has not yet entered any formal phase, the change should be
harmless.
public boolean isHexDigit(int);
public int fromHexDigit(int);
public int fromHexDigits(java.lang.CharSequence);
public int fromHexDigits(java.lang.CharSequence, int, int);
public long fromHexDigitsToLong(java.lang.CharSequence);
public long fromHexDigitsToLong(java.lang.CharSequence, int, int);
Greetings
Raffaello
[1] https://bugs.openjdk.java.net/browse/JDK-8251991
Integrated: JDK-8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException
On Sat, 20 Feb 2021 19:20:48 GMT, Craig Andrews
wrote:
> `java.net.URLClassLoader.getResource` can throw an undocumented
> `IllegalArgumentException`.
>
> According to the javadoc for the `getResource` and `findResource` methods,
> neither should be throwing `IllegalArgumentException` - they should return
> null if the resource can't be resolved.
>
> Quoting the javadoc for
> [`URLClassLoader.html#findResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URLClassLoader.html#findResource(java.lang.String))
>> Returns:
>> a URL for the resource, or null if the resource could not be found, or
>> if the loader is closed.
>
> And quoting the javadoc for
> [`ClassLoader.html#getResource`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ClassLoader.html#getResource(java.lang.String))
>> Returns:
>> URL object for reading the resource; null if the resource could not be
>> found, a URL could not be constructed to locate the resource, the resource
>> is in a package that is not opened unconditionally, or access to the
>> resource is denied by the security manager.
>
> Neither mentions throwing `IllegalArgumentException` and both are clear that
> when URL can't be constructed, `null` should be returned.
>
> Here's a stack trace:
> java.lang.IllegalArgumentException: name
> at
> java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
> at
> java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
> at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
> at java.base/java.security.AccessController.doPrivileged(Native
> Method)
> at
> java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)
>
> Looking at
> [`URLClassPath.findResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L603)
> URL findResource(final String name, boolean check) {
> URL url;
> try {
> url = new URL(base, ParseUtil.encodePath(name, false));
> } catch (MalformedURLException e) {
> throw new IllegalArgumentException("name");
> }
>
> Instead of throwing `IllegalArgumentException`, that line should simply
> return `null`.
>
> A similar issue exists at
> [`URLClassPath.getResource`](https://github.com/openjdk/jdk/blob/2b00367e1154feb2c05b84a11d62fb5750e46acf/src/java.base/share/classes/jdk/internal/loader/URLClassPath.java#L639)
> URL findResource(final String name, boolean check) {
> URL url;
> try {
> url = new URL(base, ParseUtil.encodePath(name, false));
> } catch (MalformedURLException e) {
> throw new IllegalArgumentException("name");
> }
>
> Instead of throwing `IllegalArgumentException`, that line should simply
> return `null`.
This pull request has now been integrated.
Changeset: 0c718ab2
Author:Craig Andrews
Committer: Brent Christian
URL: https://git.openjdk.java.net/jdk/commit/0c718ab2
Stats: 60 lines in 2 files changed: 57 ins; 0 del; 3 mod
8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException
Reviewed-by: alanb, bchristi, psadhukhan
-
PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8260605: Various java.lang.invoke cleanups [v5]
On Wed, 10 Mar 2021 16:34:39 GMT, Claes Redestad wrote:
>> - Remove unused code
>> - Inline and simplify the bootstrap method invocation code (remove pointless
>> reboxing checks etc)
>> - Apply pattern matching to make some code more readable
>
> Claes Redestad 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 seven additional
> commits since the last revision:
>
> - Revert pattern matching changes covered by #2913
> - Merge branch 'master' into invoke_cleanup
> - Missing .values
> - More cleanup, reduce allocations in InvokerBytecodeGenerator
> - Missing outstanding changes
> - Avoid unnecessary reboxing checks, inline and simplify class/mt invokes
> - j.l.invoke cleanups
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
342:
> 340: setClassWriter(cw);
> 341: cw.visit(Opcodes.V1_8, NOT_ACC_PUBLIC + Opcodes.ACC_FINAL +
> Opcodes.ACC_SUPER,
> 342: CLASS_PREFIX.concat(className), null,
> INVOKER_SUPER_NAME, null);
I prefer to use the existing common pattern using `+` as I believe this gain is
in the noise.
src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
151:
> 149: maybeReBoxElements(argv);
> 150: if (argv.length > 6) {
> 151: result =
> invokeWithManyArguments(bootstrapMethod, caller, name, type, argv);
it'd be cleaner to move this to the default case in line 162 and 174 instead of
having this special if-block.
src/java.base/share/classes/java/lang/invoke/MethodType.java line 418:
> 416:
> 417: /**
> 418: * Finds or creates a method type with additional parameter types.
`nptype` is never void but what about the check if `nptype` is not null?
-
PR: https://git.openjdk.java.net/jdk/pull/2300
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v28]
> Please review the implementation of JEP 391: macOS/AArch64 Port. > > It's heavily based on existing ports to linux/aarch64, macos/x86_64, and > windows/aarch64. > > Major changes are in: > * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks > JDK-8253817, JDK-8253818) > * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary > adjustments (JDK-8253819) > * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), > required on macOS/AArch64 platform. It's implemented with > pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a > thread, so W^X mode change relates to the java thread state change (for java > threads). In most cases, JVM executes in write-only mode, except when calling > a generated stub like SafeFetch, which requires a temporary switch to > execute-only mode. The same execute-only mode is enabled when a java thread > executes in java or native states. This approach of managing W^X mode turned > out to be simple and efficient enough. > * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) Anton Kozlov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 114 commits: - JDK-8262491: bsd_aarch64 part - JDK-8263002: bsd_aarch64 part - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos - Wider #ifdef block - Fix most of issues in java/foreign/ tests Failures related to va_args are tracked in JDK-8263512. - Add Azul copyright - Update Oracle copyright years - Use Thread::current_or_null_safe in SafeFetch - 8262903: [macos_aarch64] Thread::current() called on detached thread - Merge commit 'refs/pull/11/head' of https://github.com/AntonKozlov/jdk into jdk-macos - ... and 104 more: https://git.openjdk.java.net/jdk/compare/d825198e...806fc618 - Changes: https://git.openjdk.java.net/jdk/pull/2200/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2200=27 Stats: 2949 lines in 75 files changed: 2839 ins; 27 del; 83 mod Patch: https://git.openjdk.java.net/jdk/pull/2200.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200 PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v6]
> In some cases wrapping of `InputStream` with `BufferedInputStream` is > redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which does > not require any buffer having one within. > > Other cases are related to reading either a byte or short `byte[]`: in both > cases `BufferedInputStream.fill()` will be called resulting in load of much > bigger amount of data (8192 by default) than required. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: Revert HttpClient - Changes: - all: https://git.openjdk.java.net/jdk/pull/2992/files - new: https://git.openjdk.java.net/jdk/pull/2992/files/ff25cc4d..af4fcce4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2992=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2992=04-05 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2992.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2992/head:pull/2992 PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v6]
On Mon, 15 Mar 2021 18:01:20 GMT, Сергей Цыпанов wrote: >> In some cases wrapping of `InputStream` with `BufferedInputStream` is >> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which >> does not require any buffer having one within. >> >> Other cases are related to reading either a byte or short `byte[]`: in both >> cases `BufferedInputStream.fill()` will be called resulting in load of much >> bigger amount of data (8192 by default) than required. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > Revert HttpClient LGTM. Thanks! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v2]
> Hello,
>
> as of now `java.util.StringJoiner` still uses `char[]` as a storage for
> joined Strings.
>
> This applies for the cases when all joined Strings as well as delimiter,
> prefix and suffix contain only ASCII symbols.
>
> As a result when `StringJoiner.toString()` is called `byte[]` stored in
> Strings is inflated in order to fill in `char[]` and after that `char[]` is
> compressed when constructor of String is called:
> String delimiter = this.delimiter;
> char[] chars = new char[this.len + addLen];
> int k = getChars(this.prefix, chars, 0);
> if (size > 0) {
> k += getChars(elts[0], chars, k);// inflate byte[]
>
> for(int i = 1; i < size; ++i) {
> k += getChars(delimiter, chars, k);
> k += getChars(elts[i], chars, k);
> }
> }
>
> k += getChars(this.suffix, chars, k);
> return new String(chars);// compress char[] -> byte[]
> This can be improved by utilizing new method `String.getBytes(byte[], int,
> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in
> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
> covering both cases when resulting String is Latin1 or UTF-16
>
> I've prepared a patch along with benchmark proving that this change is
> correct and brings improvement.
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class StringJoinerBenchmark {
>
> @Benchmark
> public String stringJoiner(Data data) {
> String[] stringArray = data.stringArray;
> return Joiner.joinWithStringJoiner(stringArray);
> }
>
> @State(Scope.Thread)
> public static class Data {
>
> @Param({"latin", "cyrillic", "mixed"})
> private String mode;
>
> @Param({"8", "32", "64"})
> private int length;
>
> @Param({"5", "10", "100"})
> private int count;
>
> private String[] stringArray;
>
> @Setup
> public void setup() {
> RandomStringGenerator generator = new RandomStringGenerator();
>
> stringArray = new String[count];
>
> for (int i = 0; i < count; i++) {
> String alphabet = getAlphabet(i, mode);
> stringArray[i] = generator.randomString(alphabet, length);
> }
> }
>
> private static String getAlphabet(int index, String mode) {
> var latin = "abcdefghijklmnopqrstuvwxyz"; //English
> var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>
> String alphabet;
> switch (mode) {
> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
> case "latin" -> alphabet = latin;
> case "cyrillic" -> alphabet = cyrillic;
> default -> throw new RuntimeException("Illegal mode " + mode);
> }
> return alphabet;
> }
> }
> }
>
> public class Joiner {
>
> public static String joinWithStringJoiner(String[] stringArray) {
> StringJoiner joiner = new StringJoiner(",", "[", "]");
> for (String str : stringArray) {
> joiner.add(str);
> }
> return joiner.toString();
> }
> }
>
>
> (count) (length)(mode)
> Java 14 patched Units
> stringJoiner 5 8 latin 78.836
> ± 0.20867.546 ± 0.500 ns/op
> stringJoiner 532 latin 92.877
> ± 0.42266.760 ± 0.498 ns/op
> stringJoiner 564 latin 115.423
> ± 0.88373.224 ± 0.289 ns/op
> stringJoiner 10 8 latin 152.587
> ± 0.429 161.427 ± 0.635 ns/op
> stringJoiner 1032 latin 189.998
> ± 0.478 164.099 ± 0.963 ns/op
> stringJoiner 1064 latin 238.679
> ± 1.419 176.825 ± 0.533 ns/op
> stringJoiner100 8 latin 1215.612
> ± 17.413 1541.802 ± 126.166 ns/op
> stringJoiner10032 latin 1699.998
> ± 28.407 1563.341 ± 4.439 ns/op
> stringJoiner10064 latin 2289.388
> ± 45.319 2215.931 ± 137.583 ns/op
> stringJoiner 5 8 cyrillic 96.692
> ± 0.94780.946 ± 0.371 ns/op
> stringJoiner 532 cyrillic 107.806
> ± 0.42984.717 ± 0.541 ns/op
> stringJoiner 564 cyrillic 150.762
> ± 2.26796.214 ± 1.251 ns/op
> stringJoiner 10 8 cyrillic 190.570
> ± 0.381 182.754 ± 0.678 ns/op
> stringJoiner 1032 cyrillic 240.239
> ± 1.110 187.991 ± 1.575 ns/op
>
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v5]
On Mon, 15 Mar 2021 15:04:49 GMT, Daniel Fuchs wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix error message
>
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 434:
>
>> 432: int r = tmpbuf.read();
>> 433: if (r == -1) {
>> 434: logFinest("HttpClient.available(): " +
>
> There are some subtle things going on there. Using a `BufferedInputStream`
> ensures that all the bytes available on the socket will be read, up to the
> buffer capacity. Can you revert this change? I'd rather that this clean up be
> handled separately. I have logged
> https://bugs.openjdk.java.net/browse/JDK-8263599.
Sure, I'll revert this.
-
PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Mon, 15 Mar 2021 17:31:10 GMT, Alexey Semenyuk wrote: >> This converts jpackage to use `Stream.toList()` instead of >> `Stream.collect(Collectors.toList())`. One piece of code was modified to not >> mutate a list in addition to one test that used a mutating sort on a list. >> The rest of the changes are simple substitutions. > > src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java line > 151: > >> 149: components.add(BigInteger.ZERO); >> 150: } >> 151: return components.stream().toList().toArray(BigInteger[]::new); > > I guess this can be simplified down to > `components.stream().toArray(BigInteger[]::new);` Good catch! - PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: JDK-8263545: Convert jpackage to use Stream.toList()
On Sun, 14 Mar 2021 18:22:50 GMT, Ian Graves wrote:
> This converts jpackage to use `Stream.toList()` instead of
> `Stream.collect(Collectors.toList())`. One piece of code was modified to not
> mutate a list in addition to one test that used a mutating sort on a list.
> The rest of the changes are simple substitutions.
Looks good. Minor improvements suggested.
src/jdk.jpackage/windows/classes/jdk/jpackage/internal/WinMsiBundler.java line
200:
> 198: ).map(e -> {
> 199: e.getValue().setOutputFileName(e.getKey());
> 200: return (WixFragmentBuilder) e.getValue();
Why this explicit cast is needed here?
src/jdk.jpackage/share/classes/jdk/jpackage/internal/DottedVersion.java line
151:
> 149: components.add(BigInteger.ZERO);
> 150: }
> 151: return components.stream().toList().toArray(BigInteger[]::new);
I guess this can be simplified down to
`components.stream().toArray(BigInteger[]::new);`
-
PR: https://git.openjdk.java.net/jdk/pull/2997
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Mon, 15 Mar 2021 08:53:43 GMT, Jaikiran Pai wrote: > If you and others think that we can ignore this case, then your proposed > approach of using this lazy holder for initialization, IMO, is much cleaner > and natural to read and I will go ahead and update this PR to use it. For me, at least, the holder pattern is clearer. I'm happy with that approach. ( I don't have an objection to the alternative, just a mild preference for the holder ) - PR: https://git.openjdk.java.net/jdk/pull/2893
Integrated: 8263556: remove `@modules java.base` from tests
On Sat, 13 Mar 2021 20:26:42 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this trivial cleanup? > from JBS: > >> jtreg `@modules X` directive does two things: >> - exclude a test from execution if JDK under test doesn't have module X >> - if JDK under test has module X, make sure it's resolved >> >> both these things have no sense for `java.base` module as it's always >> available and is always resolved. > > > Thanks, > -- Igor This pull request has now been integrated. Changeset: d825198e Author:Igor Ignatyev URL: https://git.openjdk.java.net/jdk/commit/d825198e Stats: 21 lines in 13 files changed: 0 ins; 13 del; 8 mod 8263556: remove `@modules java.base` from tests Reviewed-by: dcubed, naoto, iris - PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: 8263556: remove `@modules java.base` from tests
On Mon, 15 Mar 2021 16:25:48 GMT, Iris Clark wrote: >> Hi all, >> >> could you please review this trivial cleanup? >> from JBS: >> >>> jtreg `@modules X` directive does two things: >>> - exclude a test from execution if JDK under test doesn't have module X >>> - if JDK under test has module X, make sure it's resolved >>> >>> both these things have no sense for `java.base` module as it's always >>> available and is always resolved. >> >> >> Thanks, >> -- Igor > > Marked as reviewed by iris (Reviewer). Iris, Naoto, Dan, thank you for your reviews! -- Igor - PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: JDK-8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException [v5]
On Mon, 15 Mar 2021 16:29:54 GMT, Craig Andrews wrote: >> You need to fix this error: >> >>> Title mismatch between PR and JBS for issue JDK-8262277 >> >> by changing the title of this PR to match the JBS title. Then you should be >> able to integrate it. > >> You need to fix this error: >> >> > Title mismatch between PR and JBS for issue JDK-8262277 >> >> by changing the title of this PR to match the JBS title. Then you should be >> able to integrate it. > > Done - how's it look now? You don't need to add yourself as a contributor. The only thing you need to do is integrate. Then it is ready to be sponsored. - PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: JDK-8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException [v5]
On Mon, 15 Mar 2021 16:26:35 GMT, Kevin Rushforth wrote: > You need to fix this error: > > > Title mismatch between PR and JBS for issue JDK-8262277 > > by changing the title of this PR to match the JBS title. Then you should be > able to integrate it. Done - how's it look now? - PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException [v5]
On Mon, 15 Mar 2021 16:19:20 GMT, Craig Andrews wrote: >> Marked as reviewed by psadhukhan (Reviewer). > > What's the next step in the process for this PR? You need to fix this error: > Title mismatch between PR and JBS for issue JDK-8262277 by changing the title of this PR to match the JBS title. Then you should be able to integrate it. - PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8263556: remove `@modules java.base` from tests
On Sat, 13 Mar 2021 20:26:42 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this trivial cleanup? > from JBS: > >> jtreg `@modules X` directive does two things: >> - exclude a test from execution if JDK under test doesn't have module X >> - if JDK under test has module X, make sure it's resolved >> >> both these things have no sense for `java.base` module as it's always >> available and is always resolved. > > > Thanks, > -- Igor Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException [v5]
On Tue, 9 Mar 2021 08:40:47 GMT, Prasanta Sadhukhan wrote: >> Craig Andrews has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> JDK-8262277: java.net.URLClassLoader.getResource throws undocumented >> IllegalArgumentException > > Marked as reviewed by psadhukhan (Reviewer). What's the next step in the process for this PR? - PR: https://git.openjdk.java.net/jdk/pull/2662
Re: RFR: 8263556: remove `@modules java.base` from tests
On Sat, 13 Mar 2021 20:26:42 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this trivial cleanup? > from JBS: > >> jtreg `@modules X` directive does two things: >> - exclude a test from execution if JDK under test doesn't have module X >> - if JDK under test has module X, make sure it's resolved >> >> both these things have no sense for `java.base` module as it's always >> available and is always resolved. > > > Thanks, > -- Igor Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2990
Re: Comment on CSR 8251991: Hex formatting and parsing utility
Hi,
the javadoc of most of the methods listed in my previous post below reads:
"The delimiter, prefix, suffix, and uppercase parameters are not used."
As these eventually constitute the whole state of an instance of this
final class and as the state is not involved, it is possible to declare
the methods as static.
If, for some reason, declaring the methods as static is not a choice,
the next best thing is to clarify that
* HexFormat.of() always returns the same instance and thus
* absent any other instance, the recommended idiom for invoking any of
the methods below is, for example,
HexFormat.of().isHexDigit('F')
(because there are almost no additional costs wrt static methods.)
But I don't see a reason why they should be non-static. Just oversight?
Greetings
Raffaello
On 2021-03-08 11:52, Raffaello Giulietti wrote:
Hello,
it appears that all of the following API methods in [1] can be declared
*static*, which makes them more useful in contexts where there's no
instance of HexFormat available or none is desired.
As 17 has not yet entered any formal phase, the change should be harmless.
public boolean isHexDigit(int);
public int fromHexDigit(int);
public int fromHexDigits(java.lang.CharSequence);
public int fromHexDigits(java.lang.CharSequence, int, int);
public long fromHexDigitsToLong(java.lang.CharSequence);
public long fromHexDigitsToLong(java.lang.CharSequence, int, int);
Greetings
Raffaello
[1] https://bugs.openjdk.java.net/browse/JDK-8251991
Re: RFR: 8263450: Simplify LambdaForm.useCount
On Thu, 11 Mar 2021 14:50:46 GMT, Claes Redestad wrote: > Simplify useCount and avoid calling lastUseIndex for a small startup win. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2940
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v5]
On Mon, 15 Mar 2021 12:19:19 GMT, Сергей Цыпанов
wrote:
>> In some cases wrapping of `InputStream` with `BufferedInputStream` is
>> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which
>> does not require any buffer having one within.
>>
>> Other cases are related to reading either a byte or short `byte[]`: in both
>> cases `BufferedInputStream.fill()` will be called resulting in load of much
>> bigger amount of data (8192 by default) than required.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix error message
Changes requested by dfuchs (Reviewer).
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 434:
> 432: int r = tmpbuf.read();
> 433: if (r == -1) {
> 434: logFinest("HttpClient.available(): " +
There are some subtle things going on there. Using a `BufferedInputStream`
ensures that all the bytes available on the socket will be read, up to the
buffer capacity. Can you revert this change? I'd rather that this clean up be
handled separately. I have logged
https://bugs.openjdk.java.net/browse/JDK-8263599.
-
PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy [v13]
> 8080272 Refactor I/O stream copying to use > InputStream.transferTo/readAllBytes and Files.copy Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy drop changes in X509CertPath - Changes: - all: https://git.openjdk.java.net/jdk/pull/1853/files - new: https://git.openjdk.java.net/jdk/pull/1853/files/1b30471d..96920ee6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1853=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1853=11-12 Stats: 25 lines in 1 file changed: 22 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1853.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1853/head:pull/1853 PR: https://git.openjdk.java.net/jdk/pull/1853
Re: java.io.Serial vs java.io.Serializable javadoc
Hi,
The serialVersionUID is recommended to be declared as "private" and
warrant a warning.
There's no value to it being public or protected and it is just noise in
the javadoc of the class.
The Serialized Form of each class documents the value of the
serialVersionUID regardless of the
access modifier.
Similarly, the writeReplace and readResolve methods that are specified to
allow ANY-ACCESS-MODIFIER are recommended to be private also, for the
same reasons.
Regards, Roger
On 3/13/21 4:07 AM, Zheka Kozlov wrote:
Hi!
The javadoc of java.io.Serial [1] says that serialVersionUID is private.
But the javadoc of Serializable [2] says it can have any access modifier.
Who is right here? When I write the following code, should there be a
warning or not?
import java.io.Serial;import java.io.Serializable;
public class SerExample implements Serializable {
@Serial // warning or not?
public static final long serialVersionUID = 2L;}
This question originated in this [3] discussion in the IDEA bugtracker.
With best regards, Zheka.
[1]
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/Serial.html
[2]
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/io/Serializable.html
[3] https://youtrack.jetbrains.com/issue/IDEA-230392
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings
On Mon, 15 Mar 2021 13:52:57 GMT, Сергей Цыпанов
wrote:
>> A less intrusive alternative would be to use a `StringBuilder`, see changes
>> in this branch:
>> https://github.com/openjdk/jdk/compare/master...cl4es:stringjoin_improvement?expand=1
>> (I adapted your StringJoinerBenchmark to work with the ascii-only build
>> constraint)
>>
>> This underperforms compared to your patch since StringBuilder.toString needs
>> to do a copy, but improves over the baseline:
>> Benchmark(count)
>> (length) (mode) Mode Cnt Score Error Units
>> StringJoinerBenchmark.stringJoiner 100
>>64 latin avgt5 5420.701 ± 1433.485 ns/op
>> StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100
>>64 latin avgt5 20640.428 ±0.130B/op
>> Patch:
>> Benchmark(count)
>> (length) (mode) Mode Cnt Score Error Units
>> StringJoinerBenchmark.stringJoiner 100
>>64 latin avgt5 4271.401 ± 677.560 ns/op
>> StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100
>>64 latin avgt5 14136.294 ±0.095B/op
>>
>> The comparative benefit is that we'd avoid punching more holes into String
>> implementation details for now. Not ruling that out indefinitely, but I
>> think it needs a stronger motivation than to improve StringJoiner alone.
>
> I was thinking about `StringBuilder` at the very beginning but then decided
> to have no bounds checks and reallocations. Indeed from maintainability point
> of view your solution is much more attractive. I have one minor comment about
> `append()`: I think we should do more chaining, e.g.
> StringBuilder sb = new StringBuilder(len);
> sb.append(elts[0]);
> should be
> StringBuilder sb = new StringBuilder(len).append(elts[0]);
> etc. to have less bytecode.
>
> E.g. if we take
> public String sb() {
> StringBuilder sb = new StringBuilder();
>
> sb.append("a");
> sb.append("b");
>
> return sb.toString();
> }
> `sb.append()` gets compiled into
> L1
> LINENUMBER 23 L1
> ALOAD 1
> LDC "a"
> INVOKEVIRTUAL java/lang/StringBuilder.append
> (Ljava/lang/String;)Ljava/lang/StringBuilder;
> POP
> L2
> LINENUMBER 24 L2
> ALOAD 1
> LDC "b"
> INVOKEVIRTUAL java/lang/StringBuilder.append
> (Ljava/lang/String;)Ljava/lang/StringBuilder;
> POP
> ```
> and in case we have
> ```
> sb.append("a").append("b");
> ```
> the amount of byte code is reduced:
> ```
> L1
> LINENUMBER 23 L1
> ALOAD 1
> LDC "a"
> INVOKEVIRTUAL java/lang/StringBuilder.append
> (Ljava/lang/String;)Ljava/lang/StringBuilder;
> LDC "b"
> INVOKEVIRTUAL java/lang/StringBuilder.append
> (Ljava/lang/String;)Ljava/lang/StringBuilder;
> POP
> ```
>
> Also I'd change
> if (addLen == 0) {
> compactElts();
> return size == 0 ? "" : elts[0];
> }
> to
> if (size == 0) {
> if (addLen == 0) {
> return "";
> }
> return prefix + suffix;
> }
> The second variant is more understandable to me and likely to be slightly
> faster.
>
> And finally, should I close this PR and you'll create a new one from your
> branch, or I need to copy your changes here?
Up to you. If you adapt your PR to use a StringBuilder as suggested I can
review and sponsor.
-
PR: https://git.openjdk.java.net/jdk/pull/2627
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings
On Mon, 15 Mar 2021 12:36:24 GMT, Claes Redestad wrote:
>> Hi @cl4es,
>>> Some of these changes conflict with #2334, which suggest removing the
>>> `coder` and `isLatin1` methods from `String`.
>>
>> I've checked out Aleksey's branch and applied my changes onto it, the only
>> thing that I changed to make it work is replacing
>> public boolean isLatin1(String str) {
>> return str.isLatin1();
>> }
>> with
>> public boolean isLatin1(String str) {
>> return str.coder == String.LATIN1;
>> }
>> The rest of the code was left intact. `jdk:tier1` is OK after the change.
>>> As a more general point I think it would be good to explore options that
>>> does not increase leakage of the implementation detail that `Strings` are
>>> latin1- or utf16-encoded outside of java.lang.
>>
>> Apart from `JavaLangAccess` the only thing that comes to my mind is
>> reflection, but it will destroy all the improvement. Otherwise I cannot
>> figure out any other way to access somehow package-private latin/non-latin
>> functionality of `j.l.String` in `java.util` package. I wonder, whether I'm
>> missing any other opportunities?
>
> A less intrusive alternative would be to use a `StringBuilder`, see changes
> in this branch:
> https://github.com/openjdk/jdk/compare/master...cl4es:stringjoin_improvement?expand=1
> (I adapted your StringJoinerBenchmark to work with the ascii-only build
> constraint)
>
> This underperforms compared to your patch since StringBuilder.toString needs
> to do a copy, but improves over the baseline:
> Benchmark(count)
> (length) (mode) Mode Cnt Score Error Units
> StringJoinerBenchmark.stringJoiner 100
> 64 latin avgt5 5420.701 ± 1433.485 ns/op
> StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100
> 64 latin avgt5 20640.428 ±0.130B/op
> Patch:
> Benchmark(count)
> (length) (mode) Mode Cnt Score Error Units
> StringJoinerBenchmark.stringJoiner 100
> 64 latin avgt5 4271.401 ± 677.560 ns/op
> StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100
> 64 latin avgt5 14136.294 ±0.095B/op
>
> The comparative benefit is that we'd avoid punching more holes into String
> implementation details for now. Not ruling that out indefinitely, but I think
> it needs a stronger motivation than to improve StringJoiner alone.
I was thinking about `StringBuilder` at the very beginning but then decided to
have no bounds checks and reallocations. Indeed from maintainability point of
view your solution is much more attractive. I have one minor comment about
`append()`: I think we should do more chaining, e.g.
StringBuilder sb = new StringBuilder(len);
sb.append(elts[0]);
should be
StringBuilder sb = new StringBuilder(len).append(elts[0]);
etc. to have less bytecode.
E.g. if we take
public String sb() {
StringBuilder sb = new StringBuilder();
sb.append("a");
sb.append("b");
return sb.toString();
}
`sb.append()` gets compiled into
L1
LINENUMBER 23 L1
ALOAD 1
LDC "a"
INVOKEVIRTUAL java/lang/StringBuilder.append
(Ljava/lang/String;)Ljava/lang/StringBuilder;
POP
L2
LINENUMBER 24 L2
ALOAD 1
LDC "b"
INVOKEVIRTUAL java/lang/StringBuilder.append
(Ljava/lang/String;)Ljava/lang/StringBuilder;
POP
```
and in case we have
```
sb.append("a").append("b");
```
the amount of byte code is reduced:
```
L1
LINENUMBER 23 L1
ALOAD 1
LDC "a"
INVOKEVIRTUAL java/lang/StringBuilder.append
(Ljava/lang/String;)Ljava/lang/StringBuilder;
LDC "b"
INVOKEVIRTUAL java/lang/StringBuilder.append
(Ljava/lang/String;)Ljava/lang/StringBuilder;
POP
```
Also I'd change
if (addLen == 0) {
compactElts();
return size == 0 ? "" : elts[0];
}
to
if (size == 0) {
if (addLen == 0) {
return "";
}
return prefix + suffix;
}
The second variant is more understandable to me and likely to be slightly
faster.
And finally, should I close this PR and you'll create a new one from your
branch, or I need to copy your changes here?
-
PR: https://git.openjdk.java.net/jdk/pull/2627
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 13:34:45 GMT, Rémi Forax wrote: >> I don't think that cast from `Object` to a raw type is unchecked, so as >> error does not seem warranted to me. >> >> However, I agree javac should produce the rawtype warning for rawtypes in >> pattern matching instanceof, because we are introducing a new variable (and >> casting). I've filled: >> https://bugs.openjdk.java.net/browse/JDK-8263590 >> >> Note the non-pattern matching instanceof does not produce the rawtype >> warning, and I don't think it should produce it. > > yes, > javac should emit a warning in that case, that also the answer of Brian. > > https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-March/002925.html Then we should use an unbounded wildcard here and in similar places to avoid "rawtype" warnings in the future. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v5]
On Mon, 15 Mar 2021 12:19:19 GMT, Сергей Цыпанов wrote: >> In some cases wrapping of `InputStream` with `BufferedInputStream` is >> redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which >> does not require any buffer having one within. >> >> Other cases are related to reading either a byte or short `byte[]`: in both >> cases `BufferedInputStream.fill()` will be called resulting in load of much >> bigger amount of data (8192 by default) than required. > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > Fix error message Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:39:20 GMT, Jan Lahoda wrote: >> We have considered generics, that why parameterized generics with another >> type arguments are forbidden, but i think we have forgotten raw types. > > I don't think that cast from `Object` to a raw type is unchecked, so as error > does not seem warranted to me. > > However, I agree javac should produce the rawtype warning for rawtypes in > pattern matching instanceof, because we are introducing a new variable (and > casting). I've filled: > https://bugs.openjdk.java.net/browse/JDK-8263590 > > Note the non-pattern matching instanceof does not produce the rawtype > warning, and I don't think it should produce it. yes, javac should emit a warning in that case, that also the answer of Brian. https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-March/002925.html - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263556: remove `@modules java.base` from tests
On Sat, 13 Mar 2021 20:26:42 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this trivial cleanup? > from JBS: > >> jtreg `@modules X` directive does two things: >> - exclude a test from execution if JDK under test doesn't have module X >> - if JDK under test has module X, make sure it's resolved >> >> both these things have no sense for `java.base` module as it's always >> available and is always resolved. > > > Thanks, > -- Igor Thumbs up. I agree that this is a trivial change. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy
On Thu, 18 Feb 2021 07:12:34 GMT, Andrey Turbanov wrote: >> Hi @turbanoff, I'm happy to sponsor but I see two comments by @marschall - >> have they been addressed? >> https://github.com/openjdk/jdk/pull/1853#discussion_r572815422 >> https://github.com/openjdk/jdk/pull/1853#discussion_r572380746 > > @FrauBoes fixed in the last commit. Is there any way to de-_integrate_ PR (to > include last commit)? @turbanoff Given that this PR has been lingering for a while, you could drop the change in `X509CertPath.java` for now and integrate the remaining changes. I'm happy to sponsor in that case. - PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v31]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Missing @since - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/ddb1a30c..9d05aa56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=30 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=29-30 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings
On Thu, 18 Feb 2021 20:14:12 GMT, Сергей Цыпанов
wrote:
>> Some of these changes conflict with #2334, which suggest removing the
>> `coder` and `isLatin1` methods from `String`.
>>
>> As a more general point I think it would be good to explore options that
>> does not increase leakage of the implementation detail that `Strings` are
>> latin1- or utf16-encoded outside of java.lang.
>
> Hi @cl4es,
>> Some of these changes conflict with #2334, which suggest removing the
>> `coder` and `isLatin1` methods from `String`.
>
> I've checked out Aleksey's branch and applied my changes onto it, the only
> thing that I changed to make it work is replacing
> public boolean isLatin1(String str) {
> return str.isLatin1();
> }
> with
> public boolean isLatin1(String str) {
> return str.coder == String.LATIN1;
> }
> The rest of the code was left intact. `jdk:tier1` is OK after the change.
>> As a more general point I think it would be good to explore options that
>> does not increase leakage of the implementation detail that `Strings` are
>> latin1- or utf16-encoded outside of java.lang.
>
> Apart from `JavaLangAccess` the only thing that comes to my mind is
> reflection, but it will destroy all the improvement. Otherwise I cannot
> figure out any other way to access somehow package-private latin/non-latin
> functionality of `j.l.String` in `java.util` package. I wonder, whether I'm
> missing any other opportunities?
A less intrusive alternative would be to use a `StringBuilder`, see changes in
this branch:
https://github.com/openjdk/jdk/compare/master...cl4es:stringjoin_improvement?expand=1
(I adapted your StringJoinerBenchmark to work with the ascii-only build
constraint)
This underperforms compared to your patch since StringBuilder.toString needs to
do a copy, but improves over the baseline:
Benchmark(count)
(length) (mode) Mode Cnt Score Error Units
StringJoinerBenchmark.stringJoiner 100
64 latin avgt5 5420.701 ± 1433.485 ns/op
StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100
64 latin avgt5 20640.428 ±0.130B/op
Patch:
Benchmark(count)
(length) (mode) Mode Cnt Score Error Units
StringJoinerBenchmark.stringJoiner 100
64 latin avgt5 4271.401 ± 677.560 ns/op
StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100
64 latin avgt5 14136.294 ±0.095B/op
The comparative benefit is that we'd avoid punching more holes into String
implementation details for now. Not ruling that out indefinitely, but I think
it needs a stronger motivation than to improve StringJoiner alone.
-
PR: https://git.openjdk.java.net/jdk/pull/2627
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v5]
> In some cases wrapping of `InputStream` with `BufferedInputStream` is > redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which does > not require any buffer having one within. > > Other cases are related to reading either a byte or short `byte[]`: in both > cases `BufferedInputStream.fill()` will be called resulting in load of much > bigger amount of data (8192 by default) than required. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: Fix error message - Changes: - all: https://git.openjdk.java.net/jdk/pull/2992/files - new: https://git.openjdk.java.net/jdk/pull/2992/files/f69c8ff4..ff25cc4d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2992=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2992=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2992.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2992/head:pull/2992 PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v5]
On Mon, 15 Mar 2021 11:41:07 GMT, Alan Bateman wrote: >> Done > >> Done > > I think I'd prefer if the exception message would say that the JMOD is > invalid or that the JMOD file is truncated (because I don't think anyone > seeing this exception will know anything about the header). Fixed - PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v4]
On Sun, 14 Mar 2021 19:32:11 GMT, Сергей Цыпанов wrote: > Done I think I'd prefer if the exception message would say that the JMOD is invalid or that the JMOD file is truncated (because I don't think anyone seeing this exception will know anything about the header). - PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:09:45 GMT, Rémi Forax wrote: >>> I think it's a bug in javac, it should not even raise a warning but an >>> error. >>> But the spec is not fully clear. >> >> If you think that it's a bug, consider providing feedback to authors of JEP >> 394: >> >>> Other refinements may be incorporated based on further feedback. >> >> I find it hard to believe that authors didn't consider generic use case >> (even though JEP 394 doesn't have examples of that). > > We have considered generics, that why parameterized generics with another > type arguments are forbidden, but i think we have forgotten raw types. I don't think that cast from `Object` to a raw type is unchecked, so as error does not seem warranted to me. However, I agree javac should produce the rawtype warning for rawtypes in pattern matching instanceof, because we are introducing a new variable (and casting). I've filled: https://bugs.openjdk.java.net/browse/JDK-8263590 Note the non-pattern matching instanceof does not produce the rawtype warning, and I don't think it should produce it. - PR: https://git.openjdk.java.net/jdk/pull/2913
Integrated: 8263552: Use String.valueOf() for char-to-String conversions
On Sat, 20 Feb 2021 12:17:32 GMT, Сергей Цыпанов wrote: > This is a very simple and trivial improvement about getting rid of pointless > char wrapping into array This pull request has now been integrated. Changeset: c0176c42 Author:Sergey Tsypanov Committer: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/c0176c42 Stats: 6 lines in 6 files changed: 0 ins; 0 del; 6 mod 8263552: Use String.valueOf() for char-to-String conversions Reviewed-by: redestad, vtewari, azvegint, chegar - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversions
On Mon, 15 Mar 2021 11:11:21 GMT, Claes Redestad wrote: >> Marked as reviewed by chegar (Reviewer). > > It seems the bots haven't seen that I changed the bug summary to match yet. @cl4es could you try `/sponsor` again? I think this time it should be fine. - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversions
On Mon, 15 Mar 2021 10:55:12 GMT, Chris Hegarty wrote: >> This is a very simple and trivial improvement about getting rid of pointless >> char wrapping into array > > Marked as reviewed by chegar (Reviewer). It seems the bots haven't seen that I changed the bug summary to match yet. - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:01:48 GMT, Pavel Rappo wrote: >> The problem is that `desc` is a raw type and raw types doesn't play well >> with the rest of the language (in particular with inference). >> >> I think it's a bug in javac, it should not even raise a warning but an error. >> But the spec is not fully clear. >> >> The spec says that the narrow conversion should not be unchecked >> https://docs.oracle.com/javase/specs/jls/se15/preview/specs/patterns-instanceof-jls.html#jls-14.30.2 >> it depends if you consider the raw conversion as unchecked or not. > >> I think it's a bug in javac, it should not even raise a warning but an error. >> But the spec is not fully clear. > > If you think that it's a bug, consider providing feedback to authors of JEP > 394: > >> Other refinements may be incorporated based on further feedback. > > I find it hard to believe that authors didn't consider generic use case (even > though JEP 394 doesn't have examples of that). We have considered generics, that why parameterized generics with another type arguments are forbidden, but i think we have forgotten raw types. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 10:47:10 GMT, Rémi Forax wrote: > I think it's a bug in javac, it should not even raise a warning but an error. > But the spec is not fully clear. If you think that it's a bug, consider providing feedback to authors of JEP 394: > Other refinements may be incorporated based on further feedback. I find it hard to believe that authors didn't consider generic use case (even though JEP 394 doesn't have examples of that). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversions
On Sat, 20 Feb 2021 12:17:32 GMT, Сергей Цыпанов wrote: > This is a very simple and trivial improvement about getting rid of pointless > char wrapping into array Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2660
Integrated: 8263508: Remove dead code in MethodHandleImpl
On Fri, 12 Mar 2021 13:27:39 GMT, Claes Redestad wrote: > Remove unused methods. This pull request has now been integrated. Changeset: fac39fe9 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/fac39fe9 Stats: 103 lines in 1 file changed: 0 ins; 103 del; 0 mod 8263508: Remove dead code in MethodHandleImpl Reviewed-by: jkuhn, mchung - PR: https://git.openjdk.java.net/jdk/pull/2969
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversions
On Mon, 15 Mar 2021 10:51:08 GMT, Claes Redestad wrote: >> Marked as reviewed by azvegint (Reviewer). > > Ok if we change the summary to "Use String.valueOf() for char-to-String > conversions"? @cl4es done - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversions
On Mon, 15 Mar 2021 09:29:00 GMT, Alexander Zvegintsev wrote: >> This is a very simple and trivial improvement about getting rid of pointless >> char wrapping into array > > Marked as reviewed by azvegint (Reviewer). Ok if we change the summary to "Use String.valueOf() for char-to-String conversions"? - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 10:18:26 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
>> 360:
>>
>>> 358: public final boolean equals(Object o) {
>>> 359: if (this == o) return true;
>>> 360: return (o instanceof DynamicConstantDesc desc)
>>
>> should be
>> `(o instanceof DynamicConstantDesc desc)`
>
> I noticed that too initially. However, `javac` does not seem to produce a
> "rawtypes" warning. Which makes sense, if you think about it.
The problem is that `desc` is a raw type and raw types doesn't play well with
the rest of the language (in particular with inference).
I think it's a bug in javac, it should not even raise a warning but an error.
But the spec is not fully clear.
The spec says that the narrow conversion should not be unchecked
https://docs.oracle.com/javase/specs/jls/se15/preview/specs/patterns-instanceof-jls.html#jls-14.30.2
it depends if you consider the raw conversion as unchecked or not.
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:32:33 GMT, Rémi Forax
wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8263358: Refactored missed equals method
>
> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
> 360:
>
>> 358: public final boolean equals(Object o) {
>> 359: if (this == o) return true;
>> 360: return (o instanceof DynamicConstantDesc desc)
>
> should be
> `(o instanceof DynamicConstantDesc desc)`
I noticed that too initially. However, `javac` does not seem to produce a
"rawtypes" warning. Which makes sense, if you think about it.
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method src/java.base/share/classes/java/lang/StackTraceElement.java line 413: > 411: && Objects.equals(moduleName, e.moduleName) > 412: && Objects.equals(moduleVersion, e.moduleVersion) > 413: && e.declaringClass.equals(declaringClass) testing the declaring class and the line number should be done first given they are primitive values, it will be a little more efficient if two StackTraceElement are not equals and one is using non interned String. return (obj instanceof StackTraceElement e) && e.lineNumber == lineNumber && e.declaringClass == declaringClass && ... - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.lang`
>> package to make use of the `instanceof` pattern variable?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8263358: Refactored missed equals method
src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
360:
> 358: public final boolean equals(Object o) {
> 359: if (this == o) return true;
> 360: return (o instanceof DynamicConstantDesc desc)
should be
`(o instanceof DynamicConstantDesc desc)`
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversion in ObjectStreamClass
On Sat, 20 Feb 2021 12:17:32 GMT, Сергей Цыпанов wrote: > This is a very simple and trivial improvement about getting rid of pointless > char wrapping into array Marked as reviewed by azvegint (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v3]
On Fri, 12 Mar 2021 17:38:30 GMT, Chris Hegarty wrote: >> Patrick Concannon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263358: Refactored equals methods > > src/java.base/share/classes/java/lang/ProcessHandleImpl.java line 525: > >> 523: return (pid == other.pid) && >> 524: (startTime == other.startTime >> 525: || startTime == 0 > > This one can be a single expression. Otherwise, looks good. Well spotted, Chris. Fixed in commit f7924d2 - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8263358: Refactored missed equals method - Changes: - all: https://git.openjdk.java.net/jdk/pull/2913/files - new: https://git.openjdk.java.net/jdk/pull/2913/files/071fe1eb..f7924d27 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2913=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2913=03-04 Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2913.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2913/head:pull/2913 PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v4]
> Hi, > > Could someone please review my code for updating the code in the `java.lang` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Patrick Concannon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8263358 - 8263358: Refactored equals methods - Merge remote-tracking branch 'origin/master' into JDK-8263358 - 8263358: Update java.lang to use instanceof pattern variable - Changes: - all: https://git.openjdk.java.net/jdk/pull/2913/files - new: https://git.openjdk.java.net/jdk/pull/2913/files/e9d91315..071fe1eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2913=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2913=02-03 Stats: 24696 lines in 1136 files changed: 20879 ins; 1644 del; 2173 mod Patch: https://git.openjdk.java.net/jdk/pull/2913.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2913/head:pull/2913 PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Fri, 12 Mar 2021 14:53:45 GMT, Chris Hegarty wrote:
> What you have is probably fine, but has any consideration been given to using
> the initialization-on-demand holder idiom? e.g.
>
> ```
> static private class CanonicalMapHolder {
> static final Map,
> ConstantDesc>> CANONICAL_MAP
> = Map.ofEntries(..);
> }
> ```
Hello Chris,
Although I had thought of some other alternate ways to fix this, this idiom
isn't something that I had thought of. Now that you showed this, I thought
about it a bit more and it does look a lot more "natural" to read and maintain
as compared to the current changes in this PR which does some juggling to avoid
this deadlock. However, having thought about your proposed solution, I think
_in theory_ it still leaves open the possibility of a deadlock.
Here's what the patch looks like with your suggested change:
diff --git
a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
index 976b09e5665..c7bdcf4ce32 100644
--- a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
+++ b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
@@ -64,8 +64,6 @@ public abstract class DynamicConstantDesc
private final String constantName;
private final ClassDesc constantType;
-private static Map,
ConstantDesc>> canonicalMap;
-
/**
* Creates a nominal descriptor for a dynamic constant.
*
@@ -274,25 +272,7 @@ public abstract class DynamicConstantDesc
}
private ConstantDesc tryCanonicalize() {
-var canonDescs = canonicalMap;
-if (canonDescs == null) {
-canonDescs = Map.ofEntries(
-Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS,
DynamicConstantDesc::canonicalizePrimitiveClass),
-Map.entry(ConstantDescs.BSM_ENUM_CONSTANT,
DynamicConstantDesc::canonicalizeEnum),
-Map.entry(ConstantDescs.BSM_NULL_CONSTANT,
DynamicConstantDesc::canonicalizeNull),
-Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD,
DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
-Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD,
DynamicConstantDesc::canonicalizeFieldVarHandle),
-Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY,
DynamicConstantDesc::canonicalizeArrayVarHandle));
-synchronized (DynamicConstantDesc.class) {
-if (canonicalMap == null) {
-canonicalMap = canonDescs;
-} else {
-canonDescs = canonicalMap;
-}
-}
-}
-
-Function, ConstantDesc> f =
canonDescs.get(bootstrapMethod);
+Function, ConstantDesc> f =
CanonicalMapHolder.CANONICAL_MAP.get(bootstrapMethod);
if (f != null) {
try {
return f.apply(this);
@@ -405,4 +385,15 @@ public abstract class DynamicConstantDesc
super(bootstrapMethod, constantName, constantType, bootstrapArgs);
}
}
+
+private static final class CanonicalMapHolder {
+static final Map,
ConstantDesc>> CANONICAL_MAP =
+Map.ofEntries(
+Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS,
DynamicConstantDesc::canonicalizePrimitiveClass),
+Map.entry(ConstantDescs.BSM_ENUM_CONSTANT,
DynamicConstantDesc::canonicalizeEnum),
+Map.entry(ConstantDescs.BSM_NULL_CONSTANT,
DynamicConstantDesc::canonicalizeNull),
+Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD,
DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
+Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD,
DynamicConstantDesc::canonicalizeFieldVarHandle),
+Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY,
DynamicConstantDesc::canonicalizeArrayVarHandle));
+}
}
Please consider the following, where I try and explain the theoretical
possibility of a deadlock with this approach:
1. Let's consider 2 threads T1 and T2 doing concurrent execution
2. Let's for a moment assume that neither `DynamicConstantDesc` nor
`ConstantDescs` classes have been initialized.
3. T1 does a call to `DynamicConstantDesc.ofCanonical(...)` and T2 does a call
to something/anything on `ConstantDescs`, which triggers a class initialization
of `ConstantDescs`.
4. T1 (which called `DynamicConstantDesc.ofCanonical(...)`) will reach the
`tryCanonicalize` and will access `CanonicalMapHolder.CANONICAL_MAP` in the
implementation of that method. Because of this access (and since
`CanonicalMapHolder` hasn't yet been initialized), T1 will obtain (an implicit)
lock on the `CanonicalMapHolder` class (for the class initialization). Let's
assume T1 is granted this lock on `CanonicalMapHolder` class and it goes ahead
into the static block of that holder class to do the initialization of
`CANONICAL_MAP`.
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v3]
> Can I please get a review for this proposed patch for the issue reported in > https://bugs.openjdk.java.net/browse/JDK-8263108? > > As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and > `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due > to the nature of the code involved in their static blocks. A thread dump of > one such deadlock (reproduced using the code attached to that issue) is as > follows: > > "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s > tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) > - waiting on the Class initialization monitor for > java.lang.constant.ConstantDescs > at Deadlock.threadA(Deadlock.java:14) > at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s > tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >java.lang.Thread.State: RUNNABLE > at > java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) > - waiting on the Class initialization monitor for > java.lang.constant.DynamicConstantDesc > at > java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) > at Deadlock.threadB(Deadlock.java:24) > at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) > at java.lang.Thread.run(java.base@16-ea/Thread.java:831) > > The commit in this PR resolves that deadlock by moving the `canonicalMap` > initialization in `DynamicConstantDesc`, from the static block, to a lazily > initialized map, into the `tryCanonicalize` (private) method of the same > class. That's the only method which uses this map. > > The implementation in `tryCanonicalize` carefully ensures that the deadlock > isn't shifted from the static block to this method, by making sure that the > `synchronization` on `DynamicConstantDesc` in this method happens only when > it's time to write the state to the shared `canonicalMap`. This has an > implication that the method local variable `canonDescs` can get initialized > by multiple threads unnecessarily but from what I can see, that's the only > way we can avoid this deadlock. This penalty of multiple threads initializing > and then throwing away that map isn't too huge because that will happen only > once when the `canonicalMap` is being initialized for the first time. > > An alternate approach that I thought of was to completely get rid of this > shared cache `canonicalMap` and instead just use method level map (that gets > initialized each time) in the `tryCanonicalize` method (thus requiring no > locks/synchronization). I ran a JMH benchmark with the current change > proposed in this PR and with the alternate approach of using the method level > map. The performance number from that run showed that the approach of using > the method level map has relatively big impact on performance (even when > there's a cache hit in that method). So I decided to not pursue that > approach. I'll include the benchmark code and the numbers in a comment in > this PR, for reference. > > The commit in this PR also includes a jtreg testcase which (consistently) > reproduces the deadlock without the fix and passes when this fix is applied. > Extra manual testing has been done to verify that the classes of interest > (noted in that testcase) are indeed getting loaded in those concurrent > threads and not in the main thread. For future maintainers, there's a > implementation note added on that testcase explaining why it cannot be moved > into testng test. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Update the testcase to improve the chances of triggering a deadlock - Changes: - all: https://git.openjdk.java.net/jdk/pull/2893/files - new: https://git.openjdk.java.net/jdk/pull/2893/files/1b59dc14..2b51ec9d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2893=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2893=01-02 Stats: 15 lines in 1 file changed: 13 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2893.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2893/head:pull/2893 PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v3]
On Sun, 14 Mar 2021 22:48:18 GMT, Sergey Bylokhov wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use InputStream.readNBytes() and fix JLinkNegativeTest
>
> src/java.desktop/share/classes/sun/awt/image/ByteArrayImageSource.java line
> 54:
>
>> 52:
>> 53: protected ImageDecoder getDecoder() {
>> 54: InputStream is = new ByteArrayInputStream(imagedata,
>> imageoffset, imagelength);
>
> This file use 80 chars per line code style.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8263560: Remove needless wrapping with BufferedInputStream [v4]
> In some cases wrapping of `InputStream` with `BufferedInputStream` is > redundant, e.g. in case the wrapped one is `ByteArrayOutputStream` which does > not require any buffer having one within. > > Other cases are related to reading either a byte or short `byte[]`: in both > cases `BufferedInputStream.fill()` will be called resulting in load of much > bigger amount of data (8192 by default) than required. Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: Fix formatting - Changes: - all: https://git.openjdk.java.net/jdk/pull/2992/files - new: https://git.openjdk.java.net/jdk/pull/2992/files/a016d2ac..f69c8ff4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2992=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2992=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2992.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2992/head:pull/2992 PR: https://git.openjdk.java.net/jdk/pull/2992
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 13:45:46 GMT, Jim Laskey wrote: >> Need rebase > > Created new PR because of forced push: > https://github.com/openjdk/jdk/pull/1292 LGTM - PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8263561: Re-examine uses of LinkedList
On Sun, 14 Mar 2021 17:26:07 GMT, Alan Bateman wrote: >> Looks like it's never specified in JavaDoc which particular implementation >> of List is used in fields of affected classes, so it's quite odd to me that >> someone would rely on that when using reflection. But your point about >> backward compatibility is reasonable, so I'll revert mentioned changes. > > Nothing outside of the JDK should be hacking into this private field of a > non-exposed class, this should not be a concern here. > @AlanBateman so is it ok to keep `ArrayLists`? One thing to look out for is usages of 2-arg add method that inserts at a specific position. This shouldn't be a concern in URLClassPath.closeLoaders (assuming this is this usage where the question arises). - PR: https://git.openjdk.java.net/jdk/pull/2744
