Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v8]
> This reimplements core reflection with method handles. > > For `Constructor::newInstance` and `Method::invoke`, the new implementation > uses `MethodHandle`. For `Field` accessor, the new implementation uses > `VarHandle`.For the first few invocations of one of these reflective > methods on a specific reflective object we invoke the corresponding method > handle directly. After that we spin a dynamic bytecode stub defined in a > hidden class which loads the target `MethodHandle` or `VarHandle` from its > class data as a dynamically computed constant. Loading the method handle from > a constant allows JIT to inline the method-handle invocation in order to > achieve good performance. > > The VM's native reflection methods are needed during early startup, before > the method-handle mechanism is initialized. That happens soon after > System::initPhase1 and before System::initPhase2, after which we switch to > using method handles exclusively. > > The core reflection and method handle implementation are updated to handle > chained caller-sensitive method calls [1] properly. A caller-sensitive method > can define with a caller-sensitive adapter method that will take an > additional caller class parameter and the adapter method will be annotated > with `@CallerSensitiveAdapter` for better auditing. See the detailed > description from [2]. > > Ran tier1-tier8 tests. > > [1] https://bugs.openjdk.java.net/browse/JDK-8013527 > [2] > https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: minor cleanup and more test case. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/68bb9efe..32e7f340 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5027=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5027=06-07 Stats: 56 lines in 8 files changed: 33 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: 8214761: Bug in parallel Kahan summation implementation [v3]
> 8214761: Bug in parallel Kahan summation implementation Ian Graves 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 six additional commits since the last revision: - Changing some comments. - Merge branch 'master' into kahan-summation-bug - Updates with more test coverage - stashing - Stashing - 8214761: Bug in parallel Kahan summation implementation - Changes: - all: https://git.openjdk.java.net/jdk/pull/4674/files - new: https://git.openjdk.java.net/jdk/pull/4674/files/10b8dcda..905450d8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4674=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4674=01-02 Stats: 107423 lines in 2079 files changed: 73519 ins; 22961 del; 10943 mod Patch: https://git.openjdk.java.net/jdk/pull/4674.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4674/head:pull/4674 PR: https://git.openjdk.java.net/jdk/pull/4674
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Fri, 27 Aug 2021 13:12:33 GMT, Maurizio Cimadamore
wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove separate accessor for static vs instance method
>>
>> There is no effective difference when using MethodHandle::dropArgument for
>> static method. Removing Static*Accessor and Instance*Accessor simplifies
>> the implementation.
>
> src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java
> line 106:
>
>> 104: Object invokeImpl(Object[] args) throws Throwable {
>> 105: var mhInvoker = mhInvoker();
>> 106: return switch (paramCount) {
>
> As @plevart observed, I'm also a bit surprised to see this, but I note your
> comments regarding performance - especially in cold case. Every adaptation
> counts, I guess, if you're not in the hot path. But let's make sure that the
> pay off to add the extra hand-specialized cases is worth it - I'd be
> surprised if spreading arguments is that expensive.
While I recall it was motivated primarily for startup (note startup numbers for
`-Djdk.reflect.useSpreader=true` in @mlchung reply earlier in this PR), the
specialization to avoid the spreader for low-arity arguments also improve
performance on throughput microbenchmarks. Surprisingly also reduce the per
invocation allocation rate.
Allocation profiling suggests that with the specialization a varargs array is
entirely eliminated. This is of course somewhat fragile and dependent on a
number of things - and might ultimately be an artifact of a rather synthetic
microbenchmark that will have little benefit in practice, but it's a rather
consistent gain for various number of arguments - even when actually going into
a spreader (I have some hypotheses as to why this might happen, but most likely
we help the JIT to narrow things down for each kind of invocation scheme with
the selector):
18 baseline
Benchmark Mode
CntScore Error Units
ReflectionMethods.static_methodavgt
10 57.329 ± 4.217 ns/op
ReflectionMethods.static_method:·gc.alloc.rate.normavgt
10 72.006 ± 0.001B/op
ReflectionMethods.static_method_3arg avgt
10 70.940 ± 7.457 ns/op
ReflectionMethods.static_method_3arg:·gc.alloc.rate.norm avgt
10 96.008 ± 0.002B/op
ReflectionMethods.static_method_4arg avgt
10 90.453 ± 4.252 ns/op
ReflectionMethods.static_method_4arg:·gc.alloc.rate.norm avgt
10 112.010 ± 0.001B/op
pull/5027
Benchmark Mode
CntScore Error Units
ReflectionMethods.static_methodavgt
10 41.518 ± 2.444 ns/op
ReflectionMethods.static_method:·gc.alloc.rate.normavgt
10 48.004 ± 0.001B/op
ReflectionMethods.static_method_3arg avgt
10 57.603 ± 3.299 ns/op
ReflectionMethods.static_method_3arg:·gc.alloc.rate.norm avgt
10 64.006 ± 0.001B/op
ReflectionMethods.static_method_4arg avgt
10 56.772 ± 3.971 ns/op
ReflectionMethods.static_method_4arg:·gc.alloc.rate.norm avgt
10 80.007 ± 0.001B/op
On a patch to remove the specialization on top of pull/5027, performance
reverts back to numbers very similar to the baseline:
remove-spreader-patch:
Benchmark Mode
CntScore Error Units
ReflectionMethods.static_methodavgt
10 56.644 ± 4.322 ns/op
ReflectionMethods.static_method:·gc.alloc.rate.normavgt
10 72.006 ± 0.001B/op
ReflectionMethods.static_method_3arg avgt
10 72.353 ± 6.815 ns/op
ReflectionMethods.static_method_3arg:·gc.alloc.rate.norm avgt
10 96.008 ± 0.001B/op
ReflectionMethods.static_method_4arg avgt
10 82.820 ± 8.303 ns/op
ReflectionMethods.static_method_4arg:·gc.alloc.rate.norm avgt
10 112.009 ± 0.002B/op
I think the cold start reduction alone is worth the extra 100 lines or so of
code, but if the gain on these microbenchmarks translates to real throughput
gains then I think the complexity is more than paid for. Simplifying it while
retaining similar characteristics would be great of course, but I think such an
exploration should be done as a follow-up.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v7]
On Tue, 31 Aug 2021 17:14:05 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Rename InvokerBuilder to ReflectionInvokerBuilder Thanks for the changes - they look good and they make the code initialization logic easier to follow. Looks good to me. - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5027
Integrated: 8273197: ProblemList 2 jtools tests due to JDK-8273187
On Tue, 31 Aug 2021 19:44:08 GMT, Daniel D. Daugherty wrote: > Trivial fixes to reduce the noise in the JDK18 CI: > JDK-8273197 ProblemList 2 jtools tests due to JDK-8273187 > JDK-8273198 ProblemList > java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 > > These failures happen in Tier5 so I'm ProblemListing them now to give @naotoj > time to > work on the issues introduced by JDK-8260265 UTF-8 by Default. This pull request has now been integrated. Changeset: 9c392d00 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/9c392d008a5a34cdc2ed6339a63f1a0d06efe619 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod 8273197: ProblemList 2 jtools tests due to JDK-8273187 8273198: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/5321
Re: Integrated: 8273197: ProblemList 2 jtools tests due to JDK-8273187
On Tue, 31 Aug 2021 19:44:08 GMT, Daniel D. Daugherty wrote: > Trivial fixes to reduce the noise in the JDK18 CI: > JDK-8273197 ProblemList 2 jtools tests due to JDK-8273187 > JDK-8273198 ProblemList > java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 > > These failures happen in Tier5 so I'm ProblemListing them now to give @naotoj > time to > work on the issues introduced by JDK-8260265 UTF-8 by Default. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5321
Integrated: 8273197: ProblemList 2 jtools tests due to JDK-8273187
Trivial fixes to reduce the noise in the JDK18 CI: JDK-8273197 ProblemList 2 jtools tests due to JDK-8273187 JDK-8273198 ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 These failures happen in Tier5 so I'm ProblemListing them now to give @naotoj time to work on the issues introduced by JDK-8260265 UTF-8 by Default. - Commit messages: - 8273198: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188 - 8273197: ProblemList 2 jtools tests due to JDK-8273187 Changes: https://git.openjdk.java.net/jdk/pull/5321/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5321=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273197 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5321.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5321/head:pull/5321 PR: https://git.openjdk.java.net/jdk/pull/5321
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v3]
On Mon, 30 Aug 2021 19:28:11 GMT, Kevin Rushforth 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/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,` Right! Fixed > 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. Done - PR: https://git.openjdk.java.net/jdk/pull/5303
Re: RFR: 8273140: Replace usages of Enum.class.getEnumConstants() with Enum.values() where possible [v4]
> 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/171ecd4e..a5f58fe2
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=5303=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=5303=02-03
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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
RFR: 6957241: ClassLoader.getResources() returns only 1 instance when using jar indexing
Using jarIndex for Hibench, there is an unexpected behavior with the exception
"Exception in thread "main"
org.apache.hadoop.fs.UnsupportedFileSystemException: No FileSystem for scheme
"hdfs"".
After investigating it, it is related to the usage of ServiceLoader with
JarIndex.
The below stack shows the issue with JDK11:
getResource:1016, URLClassPath$JarLoader (jdk.internal.loader)
getResource:937, URLClassPath$JarLoader (jdk.internal.loader)
findResource:912, URLClassPath$JarLoader (jdk.internal.loader)
next:341, URLClassPath$1 (jdk.internal.loader)
hasMoreElements:351, URLClassPath$1 (jdk.internal.loader)
hasNext:355, BuiltinClassLoader$1 (jdk.internal.loader)
hasMoreElements:363, BuiltinClassLoader$1 (jdk.internal.loader)
next:3032, CompoundEnumeration (java.lang)
hasMoreElements:3041, CompoundEnumeration (java.lang)
nextProviderClass:1203, ServiceLoader$LazyClassPathLookupIterator (java.util)
hasNextService:1221, ServiceLoader$LazyClassPathLookupIterator (java.util)
hasNext:1265, ServiceLoader$LazyClassPathLookupIterator (java.util)
hasNext:1300, ServiceLoader$2 (java.util)
hasNext:1385, ServiceLoader$3 (java.util)
The below API tries to get all the resources with the same name.
public Enumeration findResources(final String name,
final boolean check)
```
After using JarIndex, URLClassPath.findResources only returns 1 URL.
It is the same as the description in JDK-6957241.
The issue still exists in JDK18.
Root cause:
public Enumeration findResources(final String name,
final boolean check) {
return new Enumeration<>() {
private int index = 0;
private URL url = null;
private boolean next() {
if (url != null) {
return true;
} else {
Loader loader;
while ((loader = getLoader(index++)) != null) {
url = loader.findResource(name, check);
if (url != null) {
return true;
}
}
return false;
}
}
...
};
}
With the JarIndex, there is only one loader which is corresponding to the jar
with the index due to the implementation in JarLoader.getResource(final String
name, boolean check, Set visited).
Loaders corresponding to other jar packages will not appear in this while.
So it only returns 1 instance.
To solve the issue, I change the implementation "private boolean next()".
If the loader has index, traverse the index and get all the resource from the
loader.
-
Commit messages:
- solve Whitespace error
- 6957241: ClassLoader.getResources() returns only 1 instance when using jar
indexing
Changes: https://git.openjdk.java.net/jdk/pull/5316/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5316=00
Issue: https://bugs.openjdk.java.net/browse/JDK-6957241
Stats: 769 lines in 8 files changed: 763 ins; 0 del; 6 mod
Patch: https://git.openjdk.java.net/jdk/pull/5316.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5316/head:pull/5316
PR: https://git.openjdk.java.net/jdk/pull/5316
RFR: 8269039: Disable SHA-1 Signed JARs
This change will disable JARs signed with algorithms using SHA-1 by default, and treat them as unsigned. This applies to the algorithms used to digest, sign, and optionally timestamp the JAR. It also applies to the signature and digest algorithms of the certificates in the certificate chain of the code signer and the Timestamp Authority, and any CRLs or OCSP responses that are used to verify if those certificates have been revoked. The specific details are more fully described in the CSR: https://bugs.openjdk.java.net/browse/JDK-8272155. Some additional notes about the fix: - This change was previously backed out of JDK 17 and delayed because of performance regressions. The overall performance is still to be verified, but the primary bottlenecks were addressed as follows: - `sun.security.util.DisabledAlgorithmConstraints` no longer depends on `java.text.SimpleDateFormat` to format date fields which is expensive. - the `jdkCA` constraint has been removed as this caused the `cacerts` keystore to be loaded. Applications using SHA-1 JARs signed by certificates that chain back to private CAs and are impacted by the restrictions can, at their own risk, adjust the properties and add back in the `jdkCA` constraint. - `jarsigner` has been enhanced to more accurately warn about algorithms that are disabled based on the constraints specified in the security properties. Previously it had used a simpler scheme which did not take into account constraints such as `Usage` or `DenyAfter`. Similar changes should also be made to `keytool` but that will be addressed in a separate issue. - Some SHA-1 JARs used by tests where it does not affect the results have been re-signed with SHA-2 algorithms. - Commit messages: - Fix trailing whitespace. - Initial revision. Changes: https://git.openjdk.java.net/jdk/pull/5320/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5320=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269039 Stats: 643 lines in 27 files changed: 301 ins; 214 del; 128 mod Patch: https://git.openjdk.java.net/jdk/pull/5320.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5320/head:pull/5320 PR: https://git.openjdk.java.net/jdk/pull/5320
Re: Proposal: JDK-8231640 - (prop) Canonical property storage
There's one important to keep in mind: escaping. To ensure we're reading the properties correctly, we want to write them using Properties. Although the /META-INF/maven///pom.properties is very predictable, others are not. For example the maven-shade-plugin can rewrite properties files for class relocation. (off-topic: this could use a per-line reader, so comments are preserved) These are just examples I know of, there are much more out there other more complex usage of properties. The Maven Project has several places where Properties is extended for reproducibility, whereas this ticket is about making it part of the API itself, so not everybody needs to write their own extended class for the same purpose. thanks, Robert -- Origineel bericht -- Van: "Bernd Eckenfels" Aan: "core-libs-dev@openjdk.java.net" Verzonden: 31-8-2021 16:02:50 Onderwerp: Re: Proposal: JDK-8231640 - (prop) Canonical property storage BTW it is probably not a good idea to overwrite Properties (for example to get a defined store order). Especially since changes in the past already broke this. However the attached discussion shows that people do need insert-order and/or alphabetical ordered properties — maybe a more general solution would help (and also make sure that existing implementations which subtype Properties and overwrite entrySet() would continue to work? After all properties is/was not final. https://stackoverflow.com/questions/17011108/how-can-i-write-java-properties-in-a-defined-order On a somewhat related note, I think if the Properties class is not deterministic, preserves order and comments, it’s maybe not a good file,creation APi for Maven anyway. What Plug-ins are affected for which operation? Gruss Bernd -- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von Roger Riggs Gesendet: Monday, August 30, 2021 5:00:14 PM An: Jaikiran Pai ; core-libs-dev@openjdk.java.net Betreff: 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
Integrated: 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
On Tue, 31 Aug 2021 09:40:14 GMT, xpbob wrote: > 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 This pull request has now been integrated. Changeset: 683e30db Author:bobpengxie Committer: Sergey Bylokhov URL: https://git.openjdk.java.net/jdk/commit/683e30db79789ee44b3cc0b44c085de4615bca7b Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 Reviewed-by: jiefu, serb - PR: https://git.openjdk.java.net/jdk/pull/5315
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Tue, 31 Aug 2021 16:01:39 GMT, Maurizio Cimadamore wrote: > Please throw a "Reflection" somewhere :-) - e.g. "ReflectionInvokerBuilder" Done. I'm fine with that. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v7]
> 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: Rename InvokerBuilder to ReflectionInvokerBuilder - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/df6fb0a2..68bb9efe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5027=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5027=05-06 Stats: 12 lines in 3 files changed: 1 ins; 2 del; 9 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: 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
On Tue, 31 Aug 2021 09:40:14 GMT, xpbob wrote: > 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5315
Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation
On Thu, 26 Aug 2021 15:35:44 GMT, Paul Sandoz wrote: > May i suggest that we add some JavaDoc to `ensureClassInitialized` describing > the two cases of the calling thread is the initialization thread or not. This is a good suggestion that also applies to `Lookup::ensureInitialized`. I created https://bugs.openjdk.java.net/browse/JDK-8273194 to improve the javadoc for `Lookup::ensureInitialized`. - PR: https://git.openjdk.java.net/jdk/pull/5258
Re: RFR: 8273000: Remove WeakReference-based class initialisation barrier implementation
On Wed, 25 Aug 2021 22:05:24 GMT, Vladimir Ivanov wrote: > Get rid of WeakReference-based logic in > DirectMethodHandle::checkInitialized() and reimplement it with > `Unsafe::ensureClassInitialized()`/`shouldBeInitialized()`. > > The key observation is that `Unsafe::ensureClassInitialized()` does not block > the initializing thread. > > Also, removed `Unsafe::shouldBeInitialized()` in > `DMH::shouldBeInitialized(MemberName)` to save on calling into the VM. > `Unsafe::ensureClassInitialized()` already has a fast-path check which checks > whether the class is fully initialized or not. > > Testing: tier1 - tier6 Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5258
Re: RFR: 8273092: Sort classlist in JDK image [v2]
On Sat, 28 Aug 2021 13:18:37 GMT, Claes Redestad wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @dfuch comments > > Seems OK. Thanks @cl4es @magicus @dfuch for the review! - PR: https://git.openjdk.java.net/jdk/pull/5288
Integrated: 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 This pull request has now been integrated. Changeset: 1996f649 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/1996f649a3a30b7ac4b547a762417f807f5fa414 Stats: 114 lines in 3 files changed: 102 ins; 6 del; 6 mod 8273092: Sort classlist in JDK image Reviewed-by: redestad, ihse, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
On Tue, 31 Aug 2021 07:22:17 GMT, Raffaello Giulietti
wrote:
>> 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.
>
> Yes, I tend to return as soon as possible (btw, it's a q (for quotient)
> rather than a r (for remainder).
> I can of course modify the floorDiv() family to match this convention but I
> would like not to open an issue just for that. What is best?
I think it's fine to make small changes like this without opening another issue.
>> 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"?
>
> OK, but for consistency then even floorMod() should be changed. Would this
> require another JBS issue (or CSR)?
If the specifications of the new methods are consistent with their respective
`floorX()` analogs then they are fine as-is and no need to change the analogs
to match.
-
PR: https://git.openjdk.java.net/jdk/pull/5285
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]
On Mon, 30 Aug 2021 16:56:20 GMT, Mandy Chung wrote: > What about `InvokerBuilder` (it creates either MHInvoker or VHInvoker)? Please throw a "Reflection" somewhere :-) - e.g. "ReflectionInvokerBuilder" - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 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). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v6]
> 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: Fix NativeAccessor.c build issue for the renamed classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5027/files - new: https://git.openjdk.java.net/jdk/pull/5027/files/475a1be6..df6fb0a2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5027=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5027=04-05 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 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
Integrated: 8271225: Add floorDivExact() method to java.lang.[Strict]Math
On Thu, 29 Jul 2021 23:27:32 GMT, Brian Burkhalter wrote: > Add methods `floorDivExact(int,int)` and `floorDivExact(long,long)` to > `java.lang.Math` and `java.lang.StrictMath`. This pull request has now been integrated. Changeset: e5518528 Author:Brian Burkhalter URL: https://git.openjdk.java.net/jdk/commit/e55185280126e450e31eb65aa8342aebe6f31606 Stats: 205 lines in 3 files changed: 202 ins; 0 del; 3 mod 8271225: Add floorDivExact() method to java.lang.[Strict]Math Reviewed-by: darcy - PR: https://git.openjdk.java.net/jdk/pull/4941
Re: Proposal: JDK-8231640 - (prop) Canonical property storage
BTW it is probably not a good idea to overwrite Properties (for example to get a defined store order). Especially since changes in the past already broke this. However the attached discussion shows that people do need insert-order and/or alphabetical ordered properties — maybe a more general solution would help (and also make sure that existing implementations which subtype Properties and overwrite entrySet() would continue to work? After all properties is/was not final. https://stackoverflow.com/questions/17011108/how-can-i-write-java-properties-in-a-defined-order On a somewhat related note, I think if the Properties class is not deterministic, preserves order and comments, it’s maybe not a good file,creation APi for Maven anyway. What Plug-ins are affected for which operation? Gruss Bernd -- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von Roger Riggs Gesendet: Monday, August 30, 2021 5:00:14 PM An: Jaikiran Pai ; core-libs-dev@openjdk.java.net Betreff: 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: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 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). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: Proposal: JDK-8231640 - (prop) Canonical property storage
Hello Magnus, On 30/08/21 5:29 pm, Magnus Ihse Bursie wrote: 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. I do agree that it would be good to just get rid of the date comment and let callers control what exact comments (if any) get written out. However, I think that having both - a new method (overloaded or named differently) and at the same time changing the implementation of the existing store(...) methods to make the date comment reproducible is a bit of a overkill for a comment that doesn't even play a functional role in that API. I listed that option of a new overloaded API and would have been in favour of it, if that was the only addition/change that we did to support the date comment disabling/reproducibility. Having said that, I will gladly go ahead and include/work on that additional new API, if there is some general agreement on doing so. -Jaikiran
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 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). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Integrated: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings
On Sat, 28 Aug 2021 13:21:24 GMT, Claes Redestad wrote: > Refactor to improve inlining, which helps some microbenchmarks exer > StringBuilder.append(String) This pull request has now been integrated. Changeset: 98fa5335 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/98fa53357a66f474090304e53959be5d433d5e5f Stats: 17 lines in 1 file changed: 11 ins; 2 del; 4 mod 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings Reviewed-by: rriggs, alanb - 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 Thanks for reviews, everyone! - PR: https://git.openjdk.java.net/jdk/pull/5291
Re: RFR: 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
On Tue, 31 Aug 2021 09:40:14 GMT, xpbob wrote: > 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 LGTM Thanks for fixing it. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5315
RFR: 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 - Commit messages: - 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 Changes: https://git.openjdk.java.net/jdk/pull/5315/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5315=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273169 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5315.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5315/head:pull/5315 PR: https://git.openjdk.java.net/jdk/pull/5315
Re: RFR: 8271302: Regex Test Refresh [v5]
On Mon, 30 Aug 2021 15:52:05 GMT, Ian Graves wrote: >> 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 https://bugs.openjdk.java.net/browse/JDK-8273169 I'd like to fix it. --- a/test/jdk/java/util/regex/NegativeArraySize.java +++ b/test/jdk/java/util/regex/NegativeArraySize.java @@ -25,7 +25,7 @@ * @test * @bug 8223174 * @summary Pattern.compile() can throw confusing NegativeArraySizeException - * @requires os.maxMemory >= 5g + * @requires os.maxMemory >= 5g & vm.bits == 64 * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize */ - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8273092: Sort classlist in JDK image [v2]
On Tue, 31 Aug 2021 01:05:05 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 > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > @dfuch comments Thanks Ioi! These changes look good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8271302: Regex Test Refresh [v5]
On Mon, 30 Aug 2021 15:52:05 GMT, Ian Graves wrote: >> 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 Looks like it was missed that the test fails oi a github actions: https://github.com/igraves/jdk/runs/3463998089 Run if ! grep --include=test-summary.txt -lqr build/*/test-results -e "TEST SUCCESS" ; then newfailures.txt java/util/regex/NegativeArraySize.java Error: Process completed with exit code 1. Invalid initial heap size: -Xms5G The specified size exceeds the maximum representable size. Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. - PR: https://git.openjdk.java.net/jdk/pull/5092
Re: RFR: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
On Mon, 30 Aug 2021 22:25:09 GMT, Brian Burkhalter 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 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.
Yes, I tend to return as soon as possible (btw, it's a q (for quotient) rather
than a r (for remainder).
I can of course modify the floorDiv() family to match this convention but I
would like not to open an issue just for that. What is best?
> 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"?
This is the same wording as in floorMod().
"If and only if", "exactly when", "precisely when" are usually regarded the
same afaik. (e.g., see https://en.wikipedia.org/wiki/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"?
OK, but for consistency then even floorMod() should be changed. Would this
require another JBS issue (or CSR)?
> 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: `floorMod()` does `mod += y` and
> there is only one return.
Similar comment as for floorDiv() as well.
-
PR: https://git.openjdk.java.net/jdk/pull/5285
