Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). New commit pushed. Sections added. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
> This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: sections etc - Changes: - all: https://git.openjdk.java.net/jdk/pull/5204/files - new: https://git.openjdk.java.net/jdk/pull/5204/files/63b1b7c9..08635b91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5204=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5204=00-01 Stats: 7 lines in 2 files changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5204.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5204/head:pull/5204 PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Tue, 24 Aug 2021 19:06:55 GMT, Roger Riggs wrote:
>> test/jdk/java/lang/ProcessBuilder/Basic.java line 30:
>>
>>> 28: * 6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
>>> 29: * 4947220 7018606 7034570 4244896 5049299 8003488 8054494 8058464
>>> 30: * 8067796 8224905 8263729 8265173 8272600 8231297
>>
>> The test should also be modified to use `@run
>> main/othervm/native/timeout=300` so that this test will be flagged by jtreg
>> if `-nativepath:` is not specified.
>
> It should be possible to run this test as a main, without the overhead of
> building the native image.
> The use of a Java child greatly reduces the complexity of the test and
> improves its maintainability.
> Requiring a native special built program raises the overhead considerably.
> And all because the VM can't or won't allow its output to be managed.
In the same way the test uses:
` private static final String[] winEnvCommand = {"cmd.exe", "/c", "set"};`
you could also have:
` private static final String[] winSleepCommand = {"cmd.exe", "/c", "timeout",
"/T", "60", "/NOBREAK"};`
-
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Sat, 28 Aug 2021 02:34:48 GMT, Roger Riggs wrote:
>> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified
>> unexpected messages from a child Java VM
>> as the cause of the test failure. Attempts to control the output of the
>> child VM have failed, the VM is unrepentant .
>>
>> There is no functionality in the child except to wait long enough for the
>> test to finish and the child is destroyed.
>> The fix is to switch from using a Java child to using a native child; a new
>> executable `sleepmillis`.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revised to use native /bin/sleep program on Unix* (non-Windows).
> For Windows, a native "BasicSleep" program is used.
Hi Roger,
I think this can be simplified - see comments.
Thanks,
David
test/jdk/java/lang/ProcessBuilder/Basic.java line 2646:
> 2644: if (exePath.toFile().canExecute()) {
> 2645: return exePath;
> 2646: }
Not sure why this is so elaborate when elsewhere in the test we just assume
`/usr/bin/env` exists?
test/jdk/java/lang/ProcessBuilder/Basic.java line 2662:
> 2660: // Fallback to the JavaChild sleep does a sleep for 10
> minutes.
> 2661: // The fallback the Java$Child is used if the test is run
> without building
> 2662: // the BasicSleep native executable (for Windows).
The comment doesn't read correctly.
test/jdk/java/lang/ProcessBuilder/Basic.java line 2665:
> 2663:
> 2664: Path exePath =
> Path.of(TEST_NATIVEPATH).resolve("BasicSleep.exe");
> 2665: System.out.println("exePath: " + exePath + ", canExec: " +
> exePath.toFile().canExecute());
What is this for??
-
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8273092: Sort classlist in JDK image [v2]
> When the classlist is generated using build.tools.classlist.HelloClasslist, > its contents may be non-deterministic due to Java thread execution order. > > We should sort the generated classlist to make the JDK image's contents more > deterministic. > > Tested with Mach5 tier1, tier2, builds-tier5 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dfuch comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5288/files - new: https://git.openjdk.java.net/jdk/pull/5288/files/dc170ec0..ee710895 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5288=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5288=00-01 Stats: 15 lines in 1 file changed: 7 ins; 7 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5288.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5288/head:pull/5288 PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8273092: Sort classlist in JDK image [v2]
On Mon, 30 Aug 2021 12:51:43 GMT, Daniel Fuchs wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dfuch comments
>
> make/jdk/src/classes/build/tools/classlist/SortClasslist.java line 58:
>
>> 56: String line = scanner.nextLine();
>> 57: Matcher m = p.matcher(line);
>> 58: if (m.find()) {
>
> Are we sure that a comment line will not match this regexp, or that if it
> matches, it is not a comment line?
Thanks for the comments. I've swapper the matching order to check for leading
`#` and `@` characters first.
-
PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v3]
On Sat, 28 Aug 2021 02:34:48 GMT, Roger Riggs wrote: >> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified >> unexpected messages from a child Java VM >> as the cause of the test failure. Attempts to control the output of the >> child VM have failed, the VM is unrepentant . >> >> There is no functionality in the child except to wait long enough for the >> test to finish and the child is destroyed. >> The fix is to switch from using a Java child to using a native child; a new >> executable `sleepmillis`. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Revised to use native /bin/sleep program on Unix* (non-Windows). > For Windows, a native "BasicSleep" program is used. test/jdk/java/lang/ProcessBuilder/exeBasicSleep.c line 37: > 35: * returned from sleep has limited accuracy. > 36: * > 37: * Note: the file name prefix "exe" identifies the source should be built > into SleepMillis(.exe). You mean BasicSleep(.exe) - PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v5]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Shorten the class name - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/ba099fb4..475a1be6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5027=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5027=03-04 Stats: 23 lines in 4 files changed: 0 ins; 0 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Thu, 26 Aug 2021 17:57:26 GMT, Paul Sandoz wrote:
>> `MethodHandle.asTypeCache` keeps a strong reference to adapted
>> `MethodHandle` and it can introduce a class loader leak through its
>> `MethodType`.
>>
>> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
>> only contain `MethodHandle`s which are guaranteed to not introduce any
>> dependencies on new class loaders compared to the original `MethodHandle`.
>> 2nd level is backed by a `SoftReference` and is used as a backup when the
>> result of `MethodHandle.asType()` conversion can't populate the higher level
>> cache.
>>
>> The fix is based on [the
>> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
>> made by Peter Levart @plevart back in 2015.
>>
>> Testing: tier1 - tier6
>
> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 953:
>
>> 951:
>> 952: /* Determine whether {@code descendant} keeps {@code ancestor}
>> alive through the loader delegation chain. */
>> 953: private static boolean keepsAlive(ClassLoader ancestor, ClassLoader
>> descendant) {
>
> Might be clearer to name the method by what it is e.g. isAncestor
> // Returns true if ancestor can be found descendant's delegation chain.
This method is not exactly doing `isAncestor` check. It returns true if
`ancestor` is a builtin loader even it's not an ancestor of `descendent`. I
agree that it would be helpful if the method is named by what it is.
Maybe naming it `isAncestor` but move the `isSystemLoader(ancestor)` check out
to the caller. Just a thought.
-
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8078641: MethodHandle.asTypeCache can retain classes from unloading
On Wed, 25 Aug 2021 09:31:51 GMT, Vladimir Ivanov wrote:
> `MethodHandle.asTypeCache` keeps a strong reference to adapted `MethodHandle`
> and it can introduce a class loader leak through its `MethodType`.
>
> Proposed fix introduces a 2-level cache (1 element each) where 1st level can
> only contain `MethodHandle`s which are guaranteed to not introduce any
> dependencies on new class loaders compared to the original `MethodHandle`.
> 2nd level is backed by a `SoftReference` and is used as a backup when the
> result of `MethodHandle.asType()` conversion can't populate the higher level
> cache.
>
> The fix is based on [the
> work](http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/)
> made by Peter Levart @plevart back in 2015.
>
> Testing: tier1 - tier6
Thanks for fixing this. JEP 416 depends on this.
src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 966:
> 964: }
> 965:
> 966: private static boolean isSystemLoader(ClassLoader loader) {
These are builtin loaders. This method may be better to rename to
`isBuiltinLoader`.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5246
Re: RFR: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
On Fri, 27 Aug 2021 18:53:23 GMT, Raffaello Giulietti
wrote:
> Please review this PR to add officially endorsed `ceilDiv()` and `ceilMod()`
> methods do `j.l.[Strict]Math`.
>
> Beside adding fresh tests to `test/jdk/java/lang/Math/DivModTests.java`, this
> PR also corrects small typos in it and exercises tests that were already
> present but which were never invoked.
> Let me know if this is acceptable for a test or if there's a need of a
> separate issue in the JBS.
src/java.base/share/classes/java/lang/Math.java line 1589:
> 1587: *
> 1588: * Regardless of the signs of the arguments, {@code
> ceilMod}(x, y)
> 1589: * is zero exactly when {@code x % y} is zero as well.
Do you intend to say "`ceilMod(x, y)` is zero if and only if `x % y` is zero"?
That is to say "exactly when" intends "if and only if"?
src/java.base/share/classes/java/lang/Math.java line 1591:
> 1589: * is zero exactly when {@code x % y} is zero as well.
> 1590: * If neither of {@code ceilMod}(x, y) or {@code x % y} is
> zero,
> 1591: * their results differ exactly when the signs of the
> arguments are the same.
I would say "If neither `ceilMod(x, y)`nor `x % y` is zero". Also same
question as above: by "exactly when" do you intend "if any only if"?
-
PR: https://git.openjdk.java.net/jdk/pull/5285
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v4]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with four additional commits since the last revision: - Rename the accessor classes to make it explicit for method handles - Add a new test for testing methods whose parameter types and return type not visible to the caller - Move the handling of native accessor to the factory method for method/constructor accessor - DirectConstructorAccessorImpl should take the MHInvoker parameter - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/cff856f9..ba099fb4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5027=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5027=02-03 Stats: 658 lines in 12 files changed: 401 ins; 202 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/5027.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5027/head:pull/5027 PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
On Fri, 27 Aug 2021 18:53:23 GMT, Raffaello Giulietti
wrote:
> Please review this PR to add officially endorsed `ceilDiv()` and `ceilMod()`
> methods do `j.l.[Strict]Math`.
>
> Beside adding fresh tests to `test/jdk/java/lang/Math/DivModTests.java`, this
> PR also corrects small typos in it and exercises tests that were already
> present but which were never invoked.
> Let me know if this is acceptable for a test or if there's a need of a
> separate issue in the JBS.
I don't think a separate JBS issue for the extra test changes here is needed.
src/java.base/share/classes/java/lang/Math.java line 1501:
> 1499: // if the signs are the same and modulo not zero, round up
> 1500: if ((x ^ y) >= 0 && (q * y != x)) {
> 1501: return q + 1;
In `floorDiv()` this line is `r--` and there is only one return statement in
the method. I think the accepted convention is to return as soon as possible
like is done here, so perhaps it would be good to change `floorDiv()` to match?
In any cases the two should be consistent.
src/java.base/share/classes/java/lang/Math.java line 1612:
> 1610: // if the signs are the same and modulo not zero, adjust result
> 1611: if ((x ^ y) >= 0 && r != 0) {
> 1612: return r - y;
Similar comment as for `floorDIv()` above: `ceilMod()` does `mod += y` and
there is only one return.
-
PR: https://git.openjdk.java.net/jdk/pull/5285
Re: RFR: 8271225: Add floorDivExact() method to java.lang.[Strict]Math [v3]
On Thu, 5 Aug 2021 18:57:42 GMT, Brian Burkhalter wrote: >> Add methods `floorDivExact(int,int)` and `floorDivExact(long,long)` to >> `java.lang.Math` and `java.lang.StrictMath`. > > Brian Burkhalter has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - 8271225: Verbiage update after 8271599 > - Merge > - 8271225: Revert drive-by verbiage updates > - 8271225: Some verbiage updates > - 8271225: Add floorDivExact() method to java.lang.[Strict]Math Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4941
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). In the class description of `java/lang/SecurityManager` I think it would be useful to add a couple of sub-sections, 1. **Setting a Security Manager** just before the paragraph that starts with "Environments using a security manager will typically set the security manager at startup." and ends with "The current security manager is returned by the getSecurityManager method." 2. **Checking permissions** which starts after the section above and continues to the end. The reason I think this is useful is that you can then add a link from `System.setSecurityManager` to the subsection on **Setting a Security Manager** as I think it will be useful to link those together. The best place for that link is probably in the `@implNote` where it describes the JDK behavior for the `java.security.manager` system property. - PR: https://git.openjdk.java.net/jdk/pull/5204
Integrated: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
On Mon, 23 Aug 2021 18:08:02 GMT, Vicente Romero wrote: > Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) This pull request has now been integrated. Changeset: 0609421d Author:Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/0609421d4b824c5642ca75d525bad3edd72cd23a Stats: 32 lines in 2 files changed: 18 ins; 0 del; 14 mod 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null Reviewed-by: mchung, chegar - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8214761: Bug in parallel Kahan summation implementation [v2]
On Wed, 21 Jul 2021 20:19:31 GMT, Ian Graves wrote: >> 8214761: Bug in parallel Kahan summation implementation > > Ian Graves has updated the pull request incrementally with three additional > commits since the last revision: > > - Updates with more test coverage > - stashing > - Stashing The code changes look fine, but IMO the comments should be re-worded a bit. Rather text like // Negating this value because low-order bits are in negated form I suggest something like // Subtract compensation bits The main compensation loop also subtracts the compensation bits. A comment like "subtract compensation bits" makes the corrected handling of them seem less anomalous. - Marked as reviewed by darcy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4674
Integrated: 8260265: UTF-8 by Default
On Thu, 8 Jul 2021 21:23:00 GMT, Naoto Sato wrote: > This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of > the changes is `Charset.defaultCharset()` returning `UTF-8` and > `file.encoding` system property being added in the spec, but another notable > modification is in `java.io.PrintStream` where it continues to use the > `Console` encoding as the default charset instead of `UTF-8`. Other changes > are mostly clarification of the term "default charset" and their links. > Corresponding CSR has also been drafted. > > JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041 > CSR: https://bugs.openjdk.java.net/browse/JDK-8260266 This pull request has now been integrated. Changeset: 7fc85409 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/7fc8540907e8e7483ad5729ea416167810aa8747 Stats: 409 lines in 22 files changed: 208 ins; 24 del; 177 mod 8260265: UTF-8 by Default Reviewed-by: alanb, rriggs - PR: https://git.openjdk.java.net/jdk/pull/4733
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v4]
> Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into JDK-8272347 - adding pattern matching - clarifying that the names parameter is ignored in some cases - addressing review comments - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null - Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/102cd1aa..e0a7f5b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5226=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5226=02-03 Stats: 6008 lines in 269 files changed: 4342 ins; 755 del; 911 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v3]
On Mon, 30 Aug 2021 01:45:49 GMT, Mandy Chung wrote: >> Vicente Romero has updated the pull request incrementally with one >> additional commit since the last revision: >> >> clarifying that the names parameter is ignored in some cases > > src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 345: > >> 343: Arrays.stream(getters).forEach(Objects::requireNonNull); >> 344: MethodType methodType; >> 345: if (type instanceof MethodType) > > Since you are modifying this file, do you mind taking Jesper's suggestion [1] > posted in another PR to use pattern matching. > > Suggestion: > > if (type instanceof MethodType mt) > methodType = mt; > > > [1] https://github.com/openjdk/valhalla/pull/528#discussion_r688100918 sure I will do this before pushing, thanks - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v3]
On Mon, 30 Aug 2021 19:23:57 GMT, Сергей Цыпанов
wrote:
>> Just a very tiny clean-up.
>>
>> There are some places in JDK code base where we call
>> `Enum.class.getEnumConstants()` to get all the values of the referenced
>> `enum`. This is excessive, less-readable and slower than just calling
>> `Enum.values()` as in `getEnumConstants()` we have volatile access:
>>
>> public T[] getEnumConstants() {
>> T[] values = getEnumConstantsShared();
>> return (values != null) ? values.clone() : null;
>> }
>>
>> private transient volatile T[] enumConstants;
>>
>> T[] getEnumConstantsShared() {
>> T[] constants = enumConstants;
>> if (constants == null) { /* ... */ }
>> return constants;
>> }
>>
>> Calling values() method is slightly faster:
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class EnumBenchmark {
>>
>> @Benchmark
>> public Enum[] values() {
>> return Enum.values();
>> }
>>
>> @Benchmark
>> public Enum[] getEnumConstants() {
>> return Enum.class.getEnumConstants();
>> }
>>
>> private enum Enum {
>> A,
>> B
>> }
>> }
>>
>>
>> BenchmarkMode Cnt
>> Score Error Units
>> EnumBenchmark.getEnumConstants avgt 15
>> 6,265 ± 0,051 ns/op
>> EnumBenchmark.getEnumConstants:·gc.alloc.rateavgt 15
>> 2434,075 ± 19,568 MB/sec
>> EnumBenchmark.getEnumConstants:·gc.alloc.rate.norm avgt 15
>> 24,002 ± 0,001B/op
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space avgt 15
>> 2433,709 ± 70,216 MB/sec
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space.norm avgt 15
>> 23,998 ± 0,659B/op
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space avgt 15
>> 0,009 ± 0,003 MB/sec
>> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space.norm avgt 15
>> ≈ 10⁻⁴ B/op
>> EnumBenchmark.getEnumConstants:·gc.count avgt 15
>> 210,000counts
>> EnumBenchmark.getEnumConstants:·gc.time avgt 15
>> 119,000ms
>>
>> EnumBenchmark.values avgt 15
>> 4,164 ± 0,134 ns/op
>> EnumBenchmark.values:·gc.alloc.rate avgt 15
>> 3665,341 ± 120,721 MB/sec
>> EnumBenchmark.values:·gc.alloc.rate.norm avgt 15
>> 24,002 ± 0,001B/op
>> EnumBenchmark.values:·gc.churn.G1_Eden_Space avgt 15
>> 3660,512 ± 137,250 MB/sec
>> EnumBenchmark.values:·gc.churn.G1_Eden_Space.normavgt 15
>> 23,972 ± 0,529B/op
>> EnumBenchmark.values:·gc.churn.G1_Survivor_Space avgt 15
>> 0,017 ± 0,003 MB/sec
>> EnumBenchmark.values:·gc.churn.G1_Survivor_Space.normavgt 15
>> ≈ 10⁻⁴ B/op
>> EnumBenchmark.values:·gc.count avgt 15
>> 262,000counts
>> EnumBenchmark.values:·gc.timeavgt 15
>> 155,000ms
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8273140: Fix copyright year
You missed two of the copyright year problems.
src/java.desktop/share/classes/sun/font/EAttribute.java line 2:
> 1: /*
> 2: * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
You need to also fix this copyright year. It must be `2005, 2021,`
src/java.sql/share/classes/java/sql/JDBCType.java line 2:
> 1: /*
> 2: * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
This one, too.
-
PR: https://git.openjdk.java.net/jdk/pull/5303
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v3]
> Just a very tiny clean-up.
>
> There are some places in JDK code base where we call
> `Enum.class.getEnumConstants()` to get all the values of the referenced
> `enum`. This is excessive, less-readable and slower than just calling
> `Enum.values()` as in `getEnumConstants()` we have volatile access:
>
> public T[] getEnumConstants() {
> T[] values = getEnumConstantsShared();
> return (values != null) ? values.clone() : null;
> }
>
> private transient volatile T[] enumConstants;
>
> T[] getEnumConstantsShared() {
> T[] constants = enumConstants;
> if (constants == null) { /* ... */ }
> return constants;
> }
>
> Calling values() method is slightly faster:
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class EnumBenchmark {
>
> @Benchmark
> public Enum[] values() {
> return Enum.values();
> }
>
> @Benchmark
> public Enum[] getEnumConstants() {
> return Enum.class.getEnumConstants();
> }
>
> private enum Enum {
> A,
> B
> }
> }
>
>
> BenchmarkMode Cnt
> Score Error Units
> EnumBenchmark.getEnumConstants avgt 15
> 6,265 ± 0,051 ns/op
> EnumBenchmark.getEnumConstants:·gc.alloc.rateavgt 15
> 2434,075 ± 19,568 MB/sec
> EnumBenchmark.getEnumConstants:·gc.alloc.rate.norm avgt 15
> 24,002 ± 0,001B/op
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space avgt 15
> 2433,709 ± 70,216 MB/sec
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space.norm avgt 15
> 23,998 ± 0,659B/op
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space avgt 15
> 0,009 ± 0,003 MB/sec
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space.norm avgt 15
> ≈ 10⁻⁴ B/op
> EnumBenchmark.getEnumConstants:·gc.count avgt 15
> 210,000counts
> EnumBenchmark.getEnumConstants:·gc.time avgt 15
> 119,000ms
>
> EnumBenchmark.values avgt 15
> 4,164 ± 0,134 ns/op
> EnumBenchmark.values:·gc.alloc.rate avgt 15
> 3665,341 ± 120,721 MB/sec
> EnumBenchmark.values:·gc.alloc.rate.norm avgt 15
> 24,002 ± 0,001B/op
> EnumBenchmark.values:·gc.churn.G1_Eden_Space avgt 15
> 3660,512 ± 137,250 MB/sec
> EnumBenchmark.values:·gc.churn.G1_Eden_Space.normavgt 15
> 23,972 ± 0,529B/op
> EnumBenchmark.values:·gc.churn.G1_Survivor_Space avgt 15
> 0,017 ± 0,003 MB/sec
> EnumBenchmark.values:·gc.churn.G1_Survivor_Space.normavgt 15
> ≈ 10⁻⁴ B/op
> EnumBenchmark.values:·gc.count avgt 15
> 262,000counts
> EnumBenchmark.values:·gc.timeavgt 15
> 155,000ms
Сергей Цыпанов has updated the pull request incrementally with one additional
commit since the last revision:
8273140: Fix copyright year
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5303/files
- new: https://git.openjdk.java.net/jdk/pull/5303/files/1cfb0af3..171ecd4e
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=5303=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=5303=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5303.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5303/head:pull/5303
PR: https://git.openjdk.java.net/jdk/pull/5303
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v2]
On Mon, 30 Aug 2021 17:54:45 GMT, Joe Darcy wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273140: Fix copyright year > > src/java.desktop/share/classes/sun/font/AttributeValues.java line 2: > >> 1: /* >> 2: * Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights >> reserved. > > Per OpenJDK conventions, the original copyright year must be preserved when a > file is updated so > > "Copyright (c) 2004, 2021," > > in this case. Done - PR: https://git.openjdk.java.net/jdk/pull/5303
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v2]
> Just a very tiny clean-up.
>
> There are some places in JDK code base where we call
> `Enum.class.getEnumConstants()` to get all the values of the referenced
> `enum`. This is excessive, less-readable and slower than just calling
> `Enum.values()` as in `getEnumConstants()` we have volatile access:
>
> public T[] getEnumConstants() {
> T[] values = getEnumConstantsShared();
> return (values != null) ? values.clone() : null;
> }
>
> private transient volatile T[] enumConstants;
>
> T[] getEnumConstantsShared() {
> T[] constants = enumConstants;
> if (constants == null) { /* ... */ }
> return constants;
> }
>
> Calling values() method is slightly faster:
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class EnumBenchmark {
>
> @Benchmark
> public Enum[] values() {
> return Enum.values();
> }
>
> @Benchmark
> public Enum[] getEnumConstants() {
> return Enum.class.getEnumConstants();
> }
>
> private enum Enum {
> A,
> B
> }
> }
>
>
> BenchmarkMode Cnt
> Score Error Units
> EnumBenchmark.getEnumConstants avgt 15
> 6,265 ± 0,051 ns/op
> EnumBenchmark.getEnumConstants:·gc.alloc.rateavgt 15
> 2434,075 ± 19,568 MB/sec
> EnumBenchmark.getEnumConstants:·gc.alloc.rate.norm avgt 15
> 24,002 ± 0,001B/op
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space avgt 15
> 2433,709 ± 70,216 MB/sec
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space.norm avgt 15
> 23,998 ± 0,659B/op
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space avgt 15
> 0,009 ± 0,003 MB/sec
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space.norm avgt 15
> ≈ 10⁻⁴ B/op
> EnumBenchmark.getEnumConstants:·gc.count avgt 15
> 210,000counts
> EnumBenchmark.getEnumConstants:·gc.time avgt 15
> 119,000ms
>
> EnumBenchmark.values avgt 15
> 4,164 ± 0,134 ns/op
> EnumBenchmark.values:·gc.alloc.rate avgt 15
> 3665,341 ± 120,721 MB/sec
> EnumBenchmark.values:·gc.alloc.rate.norm avgt 15
> 24,002 ± 0,001B/op
> EnumBenchmark.values:·gc.churn.G1_Eden_Space avgt 15
> 3660,512 ± 137,250 MB/sec
> EnumBenchmark.values:·gc.churn.G1_Eden_Space.normavgt 15
> 23,972 ± 0,529B/op
> EnumBenchmark.values:·gc.churn.G1_Survivor_Space avgt 15
> 0,017 ± 0,003 MB/sec
> EnumBenchmark.values:·gc.churn.G1_Survivor_Space.normavgt 15
> ≈ 10⁻⁴ B/op
> EnumBenchmark.values:·gc.count avgt 15
> 262,000counts
> EnumBenchmark.values:·gc.timeavgt 15
> 155,000ms
Сергей Цыпанов has updated the pull request incrementally with one additional
commit since the last revision:
8273140: Fix copyright year
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5303/files
- new: https://git.openjdk.java.net/jdk/pull/5303/files/90319b2f..1cfb0af3
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=5303=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=5303=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5303.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5303/head:pull/5303
PR: https://git.openjdk.java.net/jdk/pull/5303
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote: >> Refactor to improve inlining, which helps some microbenchmarks exer >> StringBuilder.append(String) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify and call getBytes(String, int, byte) when possible Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible
On Mon, 30 Aug 2021 14:26:56 GMT, Сергей Цыпанов
wrote:
> Just a very tiny clean-up.
>
> There are some places in JDK code base where we call
> `Enum.class.getEnumConstants()` to get all the values of the referenced
> `enum`. This is excessive, less-readable and slower than just calling
> `Enum.values()` as in `getEnumConstants()` we have volatile access:
>
> public T[] getEnumConstants() {
> T[] values = getEnumConstantsShared();
> return (values != null) ? values.clone() : null;
> }
>
> private transient volatile T[] enumConstants;
>
> T[] getEnumConstantsShared() {
> T[] constants = enumConstants;
> if (constants == null) { /* ... */ }
> return constants;
> }
>
> Calling values() method is slightly faster:
>
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class EnumBenchmark {
>
> @Benchmark
> public Enum[] values() {
> return Enum.values();
> }
>
> @Benchmark
> public Enum[] getEnumConstants() {
> return Enum.class.getEnumConstants();
> }
>
> private enum Enum {
> A,
> B
> }
> }
>
>
> BenchmarkMode Cnt
> Score Error Units
> EnumBenchmark.getEnumConstants avgt 15
> 6,265 ± 0,051 ns/op
> EnumBenchmark.getEnumConstants:·gc.alloc.rateavgt 15
> 2434,075 ± 19,568 MB/sec
> EnumBenchmark.getEnumConstants:·gc.alloc.rate.norm avgt 15
> 24,002 ± 0,001B/op
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space avgt 15
> 2433,709 ± 70,216 MB/sec
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space.norm avgt 15
> 23,998 ± 0,659B/op
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space avgt 15
> 0,009 ± 0,003 MB/sec
> EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space.norm avgt 15
> ≈ 10⁻⁴ B/op
> EnumBenchmark.getEnumConstants:·gc.count avgt 15
> 210,000counts
> EnumBenchmark.getEnumConstants:·gc.time avgt 15
> 119,000ms
>
> EnumBenchmark.values avgt 15
> 4,164 ± 0,134 ns/op
> EnumBenchmark.values:·gc.alloc.rate avgt 15
> 3665,341 ± 120,721 MB/sec
> EnumBenchmark.values:·gc.alloc.rate.norm avgt 15
> 24,002 ± 0,001B/op
> EnumBenchmark.values:·gc.churn.G1_Eden_Space avgt 15
> 3660,512 ± 137,250 MB/sec
> EnumBenchmark.values:·gc.churn.G1_Eden_Space.normavgt 15
> 23,972 ± 0,529B/op
> EnumBenchmark.values:·gc.churn.G1_Survivor_Space avgt 15
> 0,017 ± 0,003 MB/sec
> EnumBenchmark.values:·gc.churn.G1_Survivor_Space.normavgt 15
> ≈ 10⁻⁴ B/op
> EnumBenchmark.values:·gc.count avgt 15
> 262,000counts
> EnumBenchmark.values:·gc.timeavgt 15
> 155,000ms
src/java.desktop/share/classes/sun/font/AttributeValues.java line 2:
> 1: /*
> 2: * Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
> reserved.
Per OpenJDK conventions, the original copyright year must be preserved when a
file is updated so
"Copyright (c) 2004, 2021,"
in this case.
-
PR: https://git.openjdk.java.net/jdk/pull/5303
Integrated: 8271302: Regex Test Refresh
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves wrote: > 8271302: Regex Test Refresh This pull request has now been integrated. Changeset: fecefb85 Author:Ian Graves URL: https://git.openjdk.java.net/jdk/commit/fecefb8541d5056b1a8b105126ac9c566875e056 Stats: 2253 lines in 3 files changed: 220 ins; 989 del; 1044 mod 8271302: Regex Test Refresh Reviewed-by: bchristi, smarks - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Fri, 27 Aug 2021 13:28:08 GMT, Maurizio Cimadamore
wrote:
> Overall, seems like a solid piece of work. I did not review in full the
> intricacies with caller sensitive (as I don't know that area too much), but
> the general rewrite seems solid.
Thanks for the review.
> One thing I had trouble with was the naming of the various method accessors -
> for instance, DirectMethodAccessorImpl vs. NativeMethodAccessorImpl vs.
> DelegatingMethodAccessorImpl vs. AdaptiveMethodAccessor (and there's more).
> It's not always easy to grasp from the method name which one is new and which
> one is old. Maybe sprinkling the `Handle` word on any accessor impl would
> help a bit with that.
I can see how this could be confusing. Sprinkling the `Handle` word may help.
I will try that.
> Similarly, I found the `ClassByteBuilder` name a bit weak, since that is
> meant to only create instance of custom MHInvokers. A more apt name will help
> there too.
What about `InvokerBuilder` (it creates either MHInvoker or VHInvoker)?
> I also had some issues parsing the flow in
> `ReflectionFactory::newMethodAccessor` (and constructor, has similar issue):
>
> ```
>if (useNativeAccessor(method)) {
> return DirectMethodAccessorImpl.nativeAccessor(method,
> callerSensitive);
> }
> return MethodHandleAccessorFactory.newMethodAccessor(method,
> callerSensitive);
> ```
>
> Why can't logic like this be encapsulated in a single factory call? E.g. it
> seems like the code is would like to abstract the differences between the
> various accessor implementations, but it doesn't seem to get all the way
> there (it's also possible I'm missing some constraint here).
We have to avoid to initialize the `MethodHandleAccessorFactory` class until
`java.lang.invoke` is initialized. because
`MethodHandleAccessorFactory::` creates lookup object and method
handles. I agree it's cleaner to encapsulate these in one single factory
method.I think moving the the static fields and the initialization to a
holder class may be a way to do it. I will give it a try.
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
> 200:
>
>> 198: return
>> MethodHandleAccessorFactory.newMethodAccessor(method, callerSensitive);
>> 199: } else {
>> 200: if (!useDirectMethodHandle && noInflation
>
> seems to me that the `!useDirectMethodHandle` is useless here (always false) ?
Good catch! Will fix.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Integrated: 8272861: Add a micro benchmark for vector api
On Mon, 23 Aug 2021 23:18:28 GMT, Sandhya Viswanathan wrote: > This pull request adds a micro benchmark for Vector API. > The Black Scholes algorithm is implemented with and without Vector API. > We see about ~6x gain with Vector API for this micro benchmark using 256 bit > vectors. This pull request has now been integrated. Changeset: 5aaa20f8 Author:Sandhya Viswanathan URL: https://git.openjdk.java.net/jdk/commit/5aaa20f898e8679ef1c2c36bd01d48c17be0aacf Stats: 189 lines in 1 file changed: 189 ins; 0 del; 0 mod 8272861: Add a micro benchmark for vector api Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/5234
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote: >> Refactor to improve inlining, which helps some microbenchmarks exer >> StringBuilder.append(String) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify and call getBytes(String, int, byte) when possible LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8271302: Regex Test Refresh [v5]
> 8271302: Regex Test Refresh Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Removing some notes re JUnit5 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5092/files - new: https://git.openjdk.java.net/jdk/pull/5092/files/3f01c172..0845a56f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5092=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5092=03-04 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271302: Regex Test Refresh [v4]
On Fri, 27 Aug 2021 23:18:34 GMT, Stuart Marks wrote: >> Ian Graves has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Additional cleanup > > test/jdk/java/util/regex/RegExTest.java line 85: > >> 83: import static org.testng.Assert.fail; >> 84: import static org.testng.Assert.expectThrows; //Replaced by assertThrows >> in JUnit5 >> 85: > > Slightly odd to mention JUnit 5 here, since this is being converted to a > TestNG test. Are these notes for yourself for future work? Ah yes. I'll pull that. - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 14:52:12 GMT, Claes Redestad wrote:
>> What about lines 582, 1003 and 1175? E.g. 582
>>
>> public AbstractStringBuilder append(String str) {
>> if (str == null) {
>> return appendNull();
>> }
>> int len = str.length();
>> ensureCapacityInternal(count + len);
>> putStringAt(count, str); // couldn't it be
>> putStringAt(count, str, 0, len);
>> count += len;
>> return this;
>> }
>>
>> Doing this here and in other places allows to rid `private void
>> putStringAt(int index, String str)` completely and reduce one nested method
>> call, right?
>
> I think you've got the wrong idea: We _want_ to use `putStringAt(int,
> String)` now since it can call into `String.getBytes(String, int, byte)`,
> which has a simpler and more efficient implementation than
> `String.getBytes(String, int, int, byte, int)`, since it avoids a couple of
> `<< coder` operations. This makes up for most of the improvement between my
> initial and the current version of this patch.
>
> (There's also no nested delegation from `putStringAt(int, String)` to
> `putStringAt(int, String, int, int)` in my proposed patch.)
You are right, I've applied your patch locally incompletely and misunderstood
it, my bad :)
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote: >> Refactor to improve inlining, which helps some microbenchmarks exer >> StringBuilder.append(String) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify and call getBytes(String, int, byte) when possible Marked as reviewed by stsypa...@github.com (no known OpenJDK username). - PR: https://git.openjdk.java.net/jdk/pull/5291
Re: Proposal: JDK-8231640 - (prop) Canonical property storage
Hi Jaikiran, System properties, especially new ones, should be only settable on the command line and read once. It makes them visible to developers and avoids state-full dependencies and concurrency issues. Retiring system properties is quite difficult because there's no way to know if they are still being used or by whom. The technique using system properties to revert to previous behavior has been used when API changes are unavoidable and the impact on existing applications is not known. It isn't a good solution but does provide a workaround when an issue is discovered. Better to not introduce them in the first place. The use of SOURCE_DATE_EPOCH as proposed seems better than most, as its definition has a wider scope and longer expected life than most properties. Since SOURCE_DATE_EPOCH is an environment variable, not a system property, it will be less visible to developers, but is already read-once at first use of any environment variable as per System.getenv(). $.02, Roger On 8/28/21 12:45 AM, Jaikiran Pai wrote: Hello Roger, On 28/08/21 12:16 am, Roger Riggs wrote: Hi, I'm finding the idea of removing the hardcoded timestamp and adding a property to restore compatibility strangely attractive. I don't think we've yet found a case where the timestamp was needed (but need to keep looking). (Adding a timestamp to the comment by the caller of store() is already possible) It will reveal where the timestamp is needed (via some kind of failure, though perhaps not a timely one) and includes a fallback mechanism when needed. It will generally cleanup up the behavior of an old API. The other approaches make new work for developers based on unclear requirements. So this is essentially the proposal 1d that I listed in one of my mails, with the added advantage of allowing users to switch back to the old behaviour with a system property setting. I hadn't considered the system property approach to switch to old behaviour in my proposals, so this is a very good input and I personally think the most logical proposals so far. One question that however remains is, how long (how many releases) do we support this new system property? The --illegal-access option (although not a system property) seems to be one such example where after a few releases, that option will no longer be supported and won't play any role. Perhaps this system property too will follow a similar lifetime? One other thing - I believe this new system property must be "set once at launch time" kind of property, whose value can be set at launch time and cannot be changed dynamically in the runtime. That would provide consistency in how the Properties class behaves globally within that runtime, instead of potentially behaving differently in different parts of the code, depending on how the callers set/reset the system property value before calling the "store(...)" APIs. -Jaikiran
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 14:26:23 GMT, Сергей Цыпанов
wrote:
>> No, I don't think so. The only use of this I can find is at line 1298 which
>> effectively adds a substring: `putStringAt(dstOffset, (String) s, start,
>> end);`
>
> What about lines 582, 1003 and 1175? E.g. 582
>
> public AbstractStringBuilder append(String str) {
> if (str == null) {
> return appendNull();
> }
> int len = str.length();
> ensureCapacityInternal(count + len);
> putStringAt(count, str); // couldn't it be putStringAt(count,
> str, 0, len);
> count += len;
> return this;
> }
>
> Doing this here and in other places allows to rid `private void
> putStringAt(int index, String str)` completely and reduce one nested method
> call, right?
I think you've got the wrong idea: We _want_ to use `putStringAt(int, String)`
now since it can call into `String.getBytes(String, int, byte)`, which has a
simpler and more efficient implementation than `String.getBytes(String, int,
int, byte, int)`, since it avoids a couple of `<< coder` operations. This makes
up for most of the improvement between my initial and the current version of
this patch.
(There's also no nested delegation from `putStringAt(int, String)` to
`putStringAt(int, String, int, int)` in my proposed patch.)
-
PR: https://git.openjdk.java.net/jdk/pull/5291
RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible
Just a very tiny clean-up.
There are some places in JDK code base where we call
`Enum.class.getEnumConstants()` to get all the values of the referenced `enum`.
This is excessive, less-readable and slower than just calling `Enum.values()`
as in `getEnumConstants()` we have volatile access:
public T[] getEnumConstants() {
T[] values = getEnumConstantsShared();
return (values != null) ? values.clone() : null;
}
private transient volatile T[] enumConstants;
T[] getEnumConstantsShared() {
T[] constants = enumConstants;
if (constants == null) { /* ... */ }
return constants;
}
Calling values() method is slightly faster:
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class EnumBenchmark {
@Benchmark
public Enum[] values() {
return Enum.values();
}
@Benchmark
public Enum[] getEnumConstants() {
return Enum.class.getEnumConstants();
}
private enum Enum {
A,
B
}
}
BenchmarkMode Cnt
Score Error Units
EnumBenchmark.getEnumConstants avgt 15
6,265 ± 0,051 ns/op
EnumBenchmark.getEnumConstants:·gc.alloc.rateavgt 15
2434,075 ± 19,568 MB/sec
EnumBenchmark.getEnumConstants:·gc.alloc.rate.norm avgt 15
24,002 ± 0,001B/op
EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space avgt 15
2433,709 ± 70,216 MB/sec
EnumBenchmark.getEnumConstants:·gc.churn.G1_Eden_Space.norm avgt 15
23,998 ± 0,659B/op
EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space avgt 15
0,009 ± 0,003 MB/sec
EnumBenchmark.getEnumConstants:·gc.churn.G1_Survivor_Space.norm avgt 15≈
10⁻⁴ B/op
EnumBenchmark.getEnumConstants:·gc.count avgt 15
210,000counts
EnumBenchmark.getEnumConstants:·gc.time avgt 15
119,000ms
EnumBenchmark.values avgt 15
4,164 ± 0,134 ns/op
EnumBenchmark.values:·gc.alloc.rate avgt 15
3665,341 ± 120,721 MB/sec
EnumBenchmark.values:·gc.alloc.rate.norm avgt 15
24,002 ± 0,001B/op
EnumBenchmark.values:·gc.churn.G1_Eden_Space avgt 15
3660,512 ± 137,250 MB/sec
EnumBenchmark.values:·gc.churn.G1_Eden_Space.normavgt 15
23,972 ± 0,529B/op
EnumBenchmark.values:·gc.churn.G1_Survivor_Space avgt 15
0,017 ± 0,003 MB/sec
EnumBenchmark.values:·gc.churn.G1_Survivor_Space.normavgt 15≈
10⁻⁴ B/op
EnumBenchmark.values:·gc.count avgt 15
262,000counts
EnumBenchmark.values:·gc.timeavgt 15
155,000ms
-
Commit messages:
- 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values()
where possible
Changes: https://git.openjdk.java.net/jdk/pull/5303/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5303=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8273140
Stats: 13 lines in 4 files changed: 0 ins; 2 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/5303.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5303/head:pull/5303
PR: https://git.openjdk.java.net/jdk/pull/5303
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 13:54:13 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1730:
>>
>>> 1728: }
>>> 1729:
>>> 1730: private void putStringAt(int index, String str) {
>>
>> Can we replace all the calls to this method with calls to previous method as
>> `putStringAt(index, str, 0, str.length())` taking into account that in all
>> usecases `str.length()` is already calculated into a local var?
>
> No, I don't think so. The only use of this I can find is at line 1298 which
> effectively adds a substring: `putStringAt(dstOffset, (String) s, start,
> end);`
What about lines 582, 1003 and 1175? E.g. 582
public AbstractStringBuilder append(String str) {
if (str == null) {
return appendNull();
}
int len = str.length();
ensureCapacityInternal(count + len);
putStringAt(count, str); // couldn't it be putStringAt(count,
str, 0, len);
count += len;
return this;
}
Doing this here and in other places allows to rid `private void
putStringAt(int index, String str)` completely and reduce one nested method
call, right?
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 13:35:23 GMT, Сергей Цыпанов
wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Simplify and call getBytes(String, int, byte) when possible
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1714:
>
>> 1712:
>> 1713: private void inflateIfNeededFor(String input) {
>> 1714: if (COMPACT_STRINGS && (coder != input.coder())) {
>
> I'm not completely sure whether it's a good idea in terms of maintainability,
> but I think this can be simplified a bit more. Currently in both `String` and
> `ASB` we have implementations of `coder()` very much alike:
>
> // ASB
> final byte getCoder() {
> return COMPACT_STRINGS ? coder : UTF16;
> }
>
> //String
> byte getCoder() {
> return COMPACT_STRINGS ? coder : UTF16;
> }
>
> Here we have this condition
>
> if (COMPACT_STRINGS && (coder != input.getCoder())) {}
>
> where the right operand of `&&` is evaluated only when `COMPACT_STRINGS` is
> `true` and hence it always returns the value of `coder` field. This means we
> can reference it directly as
>
> if (COMPACT_STRINGS && (coder != input.coder)) {}
I'm not sure if this'd give us enough to motivate the refactor, especially
since we'd have to widen the visibility of `String.coder`. Maybe startup could
be helped a little. Either way it feels out of scope for this change, don't you
think?
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 14:04:27 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1714:
>>
>>> 1712:
>>> 1713: private void inflateIfNeededFor(String input) {
>>> 1714: if (COMPACT_STRINGS && (coder != input.coder())) {
>>
>> I'm not completely sure whether it's a good idea in terms of
>> maintainability, but I think this can be simplified a bit more. Currently in
>> both `String` and `ASB` we have implementations of `coder()` very much alike:
>>
>> // ASB
>> final byte getCoder() {
>> return COMPACT_STRINGS ? coder : UTF16;
>> }
>>
>> //String
>> byte getCoder() {
>> return COMPACT_STRINGS ? coder : UTF16;
>> }
>>
>> Here we have this condition
>>
>> if (COMPACT_STRINGS && (coder != input.getCoder())) {}
>>
>> where the right operand of `&&` is evaluated only when `COMPACT_STRINGS` is
>> `true` and hence it always returns the value of `coder` field. This means we
>> can reference it directly as
>>
>> if (COMPACT_STRINGS && (coder != input.coder)) {}
>
> I'm not sure if this'd give us enough to motivate the refactor, especially
> since we'd have to widen the visibility of `String.coder`. Maybe startup
> could be helped a little. Either way it feels out of scope for this change,
> don't you think?
Agree
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 13:29:56 GMT, Сергей Цыпанов
wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Simplify and call getBytes(String, int, byte) when possible
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1730:
>
>> 1728: }
>> 1729:
>> 1730: private void putStringAt(int index, String str) {
>
> Can we replace all the calls to this method with calls to previous method as
> `putStringAt(index, str, 0, str.length())` taking into account that in all
> usecases `str.length()` is already calculated into a local var?
No, I don't think so. The only use of this I can find is at line 1298 which
effectively adds a substring: `putStringAt(dstOffset, (String) s, start, end);`
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote:
>> Refactor to improve inlining, which helps some microbenchmarks exer
>> StringBuilder.append(String)
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify and call getBytes(String, int, byte) when possible
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1714:
> 1712:
> 1713: private void inflateIfNeededFor(String input) {
> 1714: if (COMPACT_STRINGS && (coder != input.coder())) {
I'm not completely sure whether it's a good idea in terms of maintability, but
I think this can be simplified a bit more. Currently in both `String` and `ASB`
we have implementation of `coder()` very much alike:
// ASB
final byte getCoder() {
return COMPACT_STRINGS ? coder : UTF16;
}
//String
byte getCoder() {
return COMPACT_STRINGS ? coder : UTF16;
}
Here we have this condition
if (COMPACT_STRINGS && (coder != input.getCoder())) {}
where the right operand of `&&` is evaluated only when `COMPACT_STRINGS` is
`true` and hence it always returns the value of `coder` field. This means we
can reference it directly as
if (COMPACT_STRINGS && (coder != input.coder)) {}
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote:
>> Refactor to improve inlining, which helps some microbenchmarks exer
>> StringBuilder.append(String)
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify and call getBytes(String, int, byte) when possible
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1730:
> 1728: }
> 1729:
> 1730: private void putStringAt(int index, String str) {
Can we replace all the calls to this method with calls to previous method as
`putStringAt(index, str, 0, str.length())`?
-
PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 13:15:20 GMT, Сергей Цыпанов wrote: > Hi, just curious how have you found out that the code should be extracted > into a separate methods? Profiler? I saw that `String::length` calls appeared more than once with async-profiler, then did some experiments to see if manually inlining so that only one call was needed helped (and it did). - PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote: >> Refactor to improve inlining, which helps some microbenchmarks exer >> StringBuilder.append(String) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify and call getBytes(String, int, byte) when possible Hi, just curious how have you found out that the code should be extracted into a separate methods? Profiler? - PR: https://git.openjdk.java.net/jdk/pull/5291
Integrated: 8273098: Unnecessary Vector usage in java.naming
On Thu, 26 Aug 2021 07:04:45 GMT, Andrey Turbanov wrote: > Usage of thread-safe collection Vector is unnecessary. It's recommended to > use ArrayList/array if a thread-safe implementation is not needed. In > post-BiasedLocking times, this is gets worse, as every access is synchronized. > I checked only places where Vector was used as local variable. This pull request has now been integrated. Changeset: 5185dbde Author:Andrey Turbanov Committer: Aleksei Efimov URL: https://git.openjdk.java.net/jdk/commit/5185dbde67f07ff876305a9568bb5cebb7a7b384 Stats: 30 lines in 3 files changed: 1 ins; 2 del; 27 mod 8273098: Unnecessary Vector usage in java.naming Reviewed-by: aefimov, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/5262
Re: RFR: 8273092: Sort classlist in JDK image
On Fri, 27 Aug 2021 23:12:52 GMT, Ioi Lam wrote:
> When the classlist is generated using build.tools.classlist.HelloClasslist,
> its contents may be non-deterministic due to Java thread execution order.
>
> We should sort the generated classlist to make the JDK image's contents more
> deterministic.
>
> Tested with Mach5 tier1, tier2, builds-tier5
make/jdk/src/classes/build/tools/classlist/SortClasslist.java line 58:
> 56: String line = scanner.nextLine();
> 57: Matcher m = p.matcher(line);
> 58: if (m.find()) {
Are we sure that a comment line will not match this regexp, or that if it
matches, it is not a comment line?
-
PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
On Mon, 30 Aug 2021 11:49:49 GMT, Claes Redestad wrote: >> Refactor to improve inlining, which helps some microbenchmarks exer >> StringBuilder.append(String) > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify and call getBytes(String, int, byte) when possible Simplified further after realizing `putStringAt(int, String)` can use `String.getBytes(String, int, byte)`, improving performance for both modes: # +CompactStrings BenchmarkMode CntScoreError Units StringBuilders.appendLoop16 avgt 15 681.778 ± 14.978 ns/op StringBuilders.appendLoop8 avgt 15 358.008 ± 9.230 ns/op # -CompactStrings BenchmarkMode CntScoreError Units StringBuilders.appendLoop16 avgt 15 664.591 ± 10.505 ns/op StringBuilders.appendLoop8 avgt 15 351.175 ± 13.282 ns/op - PR: https://git.openjdk.java.net/jdk/pull/5291
Re: Proposal: JDK-8231640 - (prop) Canonical property storage
On 2021-08-28 17:16, Alan Bateman wrote: On 28/08/2021 05:45, Jaikiran Pai wrote: I hadn't considered the system property approach to switch to old behaviour in my proposals, so this is a very good input and I personally think the most logical proposals so far. Roger may be right that few would care but it would be changing behavior that goes back to JDK 1.0. In your list (and thanks for writing down the options with pros/cons) then your 1a where SOURCE_DATE_EPOCH changes to write the epoch date rather than the current date seems to be most consistent with other tools and the wider eco system. It seems the least disruptive in that it doesn't change existing behavior and would be opt-in when reproducibility is required. Previous discussion was leading towards your option 2 (or variants of) but isn't option doesn't help the cases where build tools use libraries that rely on the older methods. If I understood the numbering and alternatives correctly, I don't think there is a conflict between 1a and 2, and I think both should be implemented, for different reasons. This is my understanding of the options, with the rationale I see for choosing them: * 1a says that we change the store() method to write the date from SOURCE_DATE_EPOCH, if it is set -- otherwise it keeps the old behavior. This allows users who want to use old Java code (or code they can't modify) to produce output in a reproducible way to do so without changing the source code of the product. * 2 says that we add a new override to store() which also takes an Instant. This way programmers who write new code and want to make sure it is always reproducible can send in Instant.EPOCH, and not rely on the user to configure SOURCE_DATE_EPOCH. (In fact, when thinking of it, this seems overly complex. There is no real need to send in a generic Instant, just an override with a boolean skipTimestamp, or something like that. Basically, any solution which allows programmers who write new code to get property files without timestamps easily, is what's needed to fulfill this spot. /Magnus -Alan
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v3]
> Refactor to improve inlining, which helps some microbenchmarks exer > StringBuilder.append(String) Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Simplify and call getBytes(String, int, byte) when possible - Changes: - all: https://git.openjdk.java.net/jdk/pull/5291/files - new: https://git.openjdk.java.net/jdk/pull/5291/files/e5052256..6dfaff5a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5291=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5291=01-02 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5291/head:pull/5291 PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273092: Sort classlist in JDK image
On Fri, 27 Aug 2021 23:12:52 GMT, Ioi Lam wrote: > When the classlist is generated using build.tools.classlist.HelloClasslist, > its contents may be non-deterministic due to Java thread execution order. > > We should sort the generated classlist to make the JDK image's contents more > deterministic. > > Tested with Mach5 tier1, tier2, builds-tier5 Looks great. Thank you for helping to make the build deterministic! - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8273098: Unnecessary Vector usage in java.naming
On Thu, 26 Aug 2021 07:04:45 GMT, Andrey Turbanov wrote: > Usage of thread-safe collection Vector is unnecessary. It's recommended to > use ArrayList/array if a thread-safe implementation is not needed. In > post-BiasedLocking times, this is gets worse, as every access is synchronized. > I checked only places where Vector was used as local variable. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5262
Re: RFR: 8273098: Unnecessary Vector usage in java.naming
On Thu, 26 Aug 2021 07:04:45 GMT, Andrey Turbanov wrote: > Usage of thread-safe collection Vector is unnecessary. It's recommended to > use ArrayList/array if a thread-safe implementation is not needed. In > post-BiasedLocking times, this is gets worse, as every access is synchronized. > I checked only places where Vector was used as local variable. Hi Andrey, The changes look good to me. I've run your patch through our CI and all JNDI tests are green. Best, Aleksei - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/5262
Re: RFR: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings [v2]
> Refactor to improve inlining, which helps some microbenchmarks exer > StringBuilder.append(String) 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 two additional commits since the last revision: - Merge branch 'master' into asb_inlining - 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings - Changes: - all: https://git.openjdk.java.net/jdk/pull/5291/files - new: https://git.openjdk.java.net/jdk/pull/5291/files/56e55c57..e5052256 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5291=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5291=00-01 Stats: 152 lines in 4 files changed: 143 ins; 5 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5291/head:pull/5291 PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8268231: Aarch64: Use ldp in intrinsics for String.compareTo [v7]
> Dear all, > Can you do me a favor to review this patch. This patch use `ldp` to > implement String.compareTo. > > * We add a JMH test case > * Here is the result of this test case > > Benchmark|(size)| Mode| Cnt|Score | Error |Units > -|--|-||--||- > StringCompare.compareLL | 64 | avgt| 5 |7.992 | ±0.005|us/op > StringCompare.compareLL | 72 | avgt| 5 |15.029| ±0.006|us/op > StringCompare.compareLL | 80 | avgt| 5 |14.655| ±0.011|us/op > StringCompare.compareLL | 91 | avgt| 5 |16.363| ±0.12 |us/op > StringCompare.compareLL | 101 | avgt| 5 |16.966| ±0.007|us/op > StringCompare.compareLL | 121 | avgt| 5 |19.276| ±0.006|us/op > StringCompare.compareLL | 181 | avgt| 5 |19.002| ±0.417|us/op > StringCompare.compareLL | 256 | avgt| 5 |24.707| ±0.041|us/op > StringCompare.compareLLWithLdp| 64 | avgt| 5 |8.001| ± > 0.121|us/op > StringCompare.compareLLWithLdp| 72 | avgt| 5 |11.573| ±0.003|us/op > StringCompare.compareLLWithLdp| 80 | avgt| 5 |6.861 | ±0.004|us/op > StringCompare.compareLLWithLdp| 91 | avgt| 5 |12.774| ±0.201|us/op > StringCompare.compareLLWithLdp| 101 | avgt| 5 |8.691 | ±0.004|us/op > StringCompare.compareLLWithLdp| 121 | avgt| 5 |11.091| ±1.342|us/op > StringCompare.compareLLWithLdp| 181 | avgt| 5 |14.64 | ±0.581|us/op > StringCompare.compareLLWithLdp| 256 | avgt| 5 |25.879| ±1.775|us/op > StringCompare.compareUU | 64 | avgt| 5 |13.476| ±0.01 |us/op > StringCompare.compareUU | 72 | avgt| 5 |15.078| ±0.006|us/op > StringCompare.compareUU | 80 | avgt| 5 |23.512| ±0.011|us/op > StringCompare.compareUU | 91 | avgt| 5 |24.284| ±0.008|us/op > StringCompare.compareUU | 101 | avgt| 5 |20.707| ±0.017|us/op > StringCompare.compareUU | 121 | avgt| 5 |29.302| ±0.011|us/op > StringCompare.compareUU | 181 | avgt| 5 |39.31| ± > 0.016|us/op > StringCompare.compareUU | 256 | avgt| 5 |54.592| ±0.392|us/op > StringCompare.compareUUWithLdp| 64 | avgt| 5 |16.389| ±0.008|us/op > StringCompare.compareUUWithLdp| 72 | avgt| 5 |10.71 | ±0.158|us/op > StringCompare.compareUUWithLdp| 80 | avgt| 5 |11.488| ±0.024|us/op > StringCompare.compareUUWithLdp| 91 | avgt| 5 |13.412| ±0.006|us/op > StringCompare.compareUUWithLdp| 101 | avgt| 5 |16.245| ±0.434|us/op > StringCompare.compareUUWithLdp| 121 | avgt| 5 |16.597| ±0.016|us/op > StringCompare.compareUUWithLdp| 181 | avgt| 5 |27.373| ±0.017|us/op > StringCompare.compareUUWithLdp| 256 | avgt| 5 |41.74 | ±3.5 |us/op > > From this table, we can see that in most cases, our patch is better than old > one. > > Thank you for your review. Any suggestions are welcome. Wang Huang has updated the pull request incrementally with one additional commit since the last revision: fix windows build failed - Changes: - all: https://git.openjdk.java.net/jdk/pull/4722/files - new: https://git.openjdk.java.net/jdk/pull/4722/files/2f756261..8cf3b2c7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4722=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4722=05-06 Stats: 97 lines in 2 files changed: 0 ins; 96 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4722.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4722/head:pull/4722 PR: https://git.openjdk.java.net/jdk/pull/4722
