Re: RFR: 8309196: Remove Thread.countStackFrames
On Thu, 1 Jun 2023 06:40:04 GMT, Alan Bateman wrote: > Thread.countStackFrames is/was an ill-defined method that dates from JDK 1.0 > for counting the stack frames of a suspended Thread. The method was > deprecated in JDK 1.2 (1998), deprecated for removal in Java 9, and > re-specified/degraded to throw UOE unconditionally in Java 14. Java 22 would > be a fine time to finally remove this method. > > Code that wants to count stack frames should be directed to use > j.l.StackWalker, or if is a a tool, then it can use JVM TI and other tool > APIs. > > A corpus analysis of 30M classes in 131k artifacts from Maven Central found > only 1 usages to this method. The 1 usage appears to be a class that just > wraps all methods, it doesn't actually make use of it. Happy to see this being removed early in JDK 22! - Marked as reviewed by iris (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14257#pullrequestreview-1456700929
Re: RFR: 8309196: Remove Thread.countStackFrames
On Thu, 1 Jun 2023 06:40:04 GMT, Alan Bateman wrote: > Thread.countStackFrames is/was an ill-defined method that dates from JDK 1.0 > for counting the stack frames of a suspended Thread. The method was > deprecated in JDK 1.2 (1998), deprecated for removal in Java 9, and > re-specified/degraded to throw UOE unconditionally in Java 14. Java 22 would > be a fine time to finally remove this method. > > Code that wants to count stack frames should be directed to use > j.l.StackWalker, or if is a a tool, then it can use JVM TI and other tool > APIs. > > A corpus analysis of 30M classes in 131k artifacts from Maven Central found > only 1 usages to this method. The 1 usage appears to be a class that just > wraps all methods, it doesn't actually make use of it. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14257#pullrequestreview-1456650687
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
On Fri, 26 May 2023 20:37:39 GMT, Joe Darcy wrote: > > > Looks fine. This version matches the current implementation where > > > `getCanonicalName` returns non-null. When the implementation of > > > `getCanonicalName` is updated to return null, the spec should be updated. > > > Nit: `` is needed assuming the section of "Unnamed Classes" has 3 > > > paragraphs. > > > > > > Thanks @mlchung. > > Yes, the current spec matches what the implementation in @JimLaskey 's PR > > #13689 does at the time of writing. > > Discussions are on-going with the JEP 445 team regarding whether and how to > > implement a "isUnnamedClass" predicate for core reflection which would > > allow the `getCanonicalName` method to screen for unnamed classes and > > return null, as would reasonably be implied by a reading of the most > > current draft JLS updates. > > PS Depending on the result of those discussion, the specification for > `getCanonicalName` may be changed to return `null`. PPS PR updated along those line; the changes will likely end up being integrated through Jim's existing JEP 445 PR. - PR Comment: https://git.openjdk.org/jdk/pull/14165#issuecomment-1573138645
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview) [v4]
> Explain in java.lang.Class how unnamed classes are modeled in core reflection. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update reflective support. - Changes: - all: https://git.openjdk.org/jdk/pull/14165/files - new: https://git.openjdk.org/jdk/pull/14165/files/05244653..eead727d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14165=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14165=02-03 Stats: 36 lines in 1 file changed: 29 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/14165.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14165/head:pull/14165 PR: https://git.openjdk.org/jdk/pull/14165
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v8]
On Fri, 2 Jun 2023 03:36:35 GMT, Chen Liang wrote: >> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with >> proper privileges, with tests. >> >> This addition is useful in case classes at runtime are not loaded from the >> system class loader, such as Proxy. This is also useful to APIs that >> generate bytecode with a `Lookup` object, such as a custom >> single-abstract-method class implementations from a method handle. >> >> See >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html >> as well. >> >> Current questions, which I wish to discuss with @asotona: >> 1. Should the resolver fail fast on `IllegalAccessException` from the >> lookup? This usually indicates the hierarchy resolver is set up improperly, >> and proceeding may simply yield verification errors in class loading that >> are hard to track. For bytecode-generating APIs, throwing access errors for >> the Lookup eagerly is also more preferable than later silent generation >> failure. >> 2. Whether the default resolver should be reading from jrt alone, reflection >> alone, or jrt then reflection. I personally believe reflection alone is more >> reliable, for classes may redefined with instrumentation or jfr, which may >> not be reflected in the system resources. >> 3. In addition, I don't think chaining system class loader reflection after >> system resource retrieval is really meaningful: is there any case where >> reflection always works while the system resource retrieval always fails? > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - 1. Moved the default resolver to a static method, in anticipation of > future changes >2. Removed SecurityManager related content >3. Changed ClassHierarchyInfo into an interface >4. Moved caching method from static to instance method > - Merge branch 'master' into hierarchy-resolve > - rename to ofClassLoading/ofResourceParsing >convert the default class provider to bypass security manager restrictions > - Merge branch 'master' into hierarchy-resolve > - Merge branch 'master' into hierarchy-resolve > - Test both cached and uncached resolvers > - Update the class hierarchy resolver api as per mailing list last week > - Merge branch 'master' into hierarchy-resolve > - Update > src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java > >Co-authored-by: Andrey Turbanov > - Make lookup based resolver throw on illegal access eagerly > - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb A brief summary of the latest update: 1. I've moved to implement all 3 API changes to ClassHierarchyResolver discussed on classfile-api-dev 2. I've dropped SecurityManager content; if they are needed by another patch, those patches can add on their own. This will be in conflict with the Classfile object update; I can update accordingly if that one is integrated first. - PR Comment: https://git.openjdk.org/jdk/pull/13082#issuecomment-1573096991
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v8]
> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with > proper privileges, with tests. > > This addition is useful in case classes at runtime are not loaded from the > system class loader, such as Proxy. This is also useful to APIs that generate > bytecode with a `Lookup` object, such as a custom single-abstract-method > class implementations from a method handle. > > See > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html > as well. > > Current questions, which I wish to discuss with @asotona: > 1. Should the resolver fail fast on `IllegalAccessException` from the lookup? > This usually indicates the hierarchy resolver is set up improperly, and > proceeding may simply yield verification errors in class loading that are > hard to track. For bytecode-generating APIs, throwing access errors for the > Lookup eagerly is also more preferable than later silent generation failure. > 2. Whether the default resolver should be reading from jrt alone, reflection > alone, or jrt then reflection. I personally believe reflection alone is more > reliable, for classes may redefined with instrumentation or jfr, which may > not be reflected in the system resources. > 3. In addition, I don't think chaining system class loader reflection after > system resource retrieval is really meaningful: is there any case where > reflection always works while the system resource retrieval always fails? Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - 1. Moved the default resolver to a static method, in anticipation of future changes 2. Removed SecurityManager related content 3. Changed ClassHierarchyInfo into an interface 4. Moved caching method from static to instance method - Merge branch 'master' into hierarchy-resolve - rename to ofClassLoading/ofResourceParsing convert the default class provider to bypass security manager restrictions - Merge branch 'master' into hierarchy-resolve - Merge branch 'master' into hierarchy-resolve - Test both cached and uncached resolvers - Update the class hierarchy resolver api as per mailing list last week - Merge branch 'master' into hierarchy-resolve - Update src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java Co-authored-by: Andrey Turbanov - Make lookup based resolver throw on illegal access eagerly - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb - Changes: https://git.openjdk.org/jdk/pull/13082/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13082=07 Stats: 388 lines in 8 files changed: 294 ins; 29 del; 65 mod Patch: https://git.openjdk.org/jdk/pull/13082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13082/head:pull/13082 PR: https://git.openjdk.org/jdk/pull/13082
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v5]
On Fri, 2 Jun 2023 02:12:30 GMT, Chen Liang wrote:
>> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`.
>>
>> A few implementation-detail methods in VarHandle are now documented with the
>> implied constraints to avoid subtle problems in the future. Changed
>> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy
>> VarHandle changes. Also changed VarHandleBaseTest to report the whole
>> incorrect type of exception caught than swallow it and leaving only a
>> message.
>>
>> Current problems:
>> - [ ] The lazy var handle is quite slow on the first invocation.
>>- As seen in the benchmark, users can first call
>> `Lookup::ensureInitialized` to create an eager handle.
>>- After that, the lazy handle has a performance on par with the regular
>> var handle.
>> - [ ] The class-loading-based test is not in a unit test
>>- The test frameworks don't seem to offer fine-grained control for
>> class-loading detection or reliable unloading
>>
>>
>> BenchmarkMode Cnt Score
>> Error Units
>> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 12.668 ±
>> 0.069 ns/op
>> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 12.683 ±
>> 0.069 ns/op
>>
>>
>> BenchmarkMode Cnt Score
>> Error Units
>> LazyStaticColdStart.methodHandleCreateEagerss 1050.980 ±
>> 9.454 us/op
>> LazyStaticColdStart.methodHandleCreateLazy ss 1024.350 ±
>> 6.701 us/op
>> LazyStaticColdStart.methodHandleInitializeCallEagerss 1065.140 ±
>> 7.552 us/op
>> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 118.360 ±
>> 20.320 us/op
>> LazyStaticColdStart.varHandleCreateEager ss 1049.500 ±
>> 4.277 us/op
>> LazyStaticColdStart.varHandleCreateLazyss 1026.690 ±
>> 5.157 us/op
>> LazyStaticColdStart.varHandleInitializeCallEager ss 1087.930 ±
>> 12.643 us/op
>> LazyStaticColdStart.varHandleInitializeCallLazyss 10 1057.120 ±
>> 189.810 us/op
>
> Chen Liang 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 11 additional commits since
> the last revision:
>
> - Compute base and offset right after linking, simplify code
> - Merge branch 'master' into lazy-static-varhandle
> - Fix exact swap
>
>Co-authored-by: Mandy Chung
> - Remove export for removed package
> - Merge branch 'master' into lazy-static-varhandle
> - Test the creation performance of handles, lazy one indeed better
> - Merge branch 'master' into lazy-static-varhandle
> - copyright year
> - Benchmarks. lazy initialize is SLOW
> - nuke meaningless overrides
> - ... and 1 more: https://git.openjdk.org/jdk/compare/b9ba7a85...27e18b7c
Not quite. I mean scenarios like this:
import java.lang.invoke.*;
class Parent {
public static final int[] value = {34};
}
class Child extends Parent {}
public class Test {
public static void main(String... args) throws Throwable {
System.out.println(Child.value[0]);
var lookup = MethodHandles.lookup();
var getter = lookup.findStaticVarHandle(Child.class, "value",
int[].class);
System.out.println(((int[]) getter.getAcquire())[0]);
}
}
Here, the `value` field is in parent, but it's accessed via a subclass and is
resolved successfully. Are the refc and declaring class still equal here? I
think we only need to initialize Parent without initializing Child in this case.
I've also updated the benchmark for the latest patch. Unfortunately the regular
invocations and lazy var handle initial call performance saw a decrease.
-
PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1573054150
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v4]
On Fri, 2 Jun 2023 01:19:26 GMT, Chen Liang wrote: > In addition, should we initialize the ref class or the declaring class only? > I don't think we need to initialize the ref class when declaring class > initialization should be sufficient, but that is a change in behavior. Declaring class will be initialized [1]. refc should be equals to the declaring class because the static field to be looked up can only be found in the declaring class. There should be no behavioral change. [1] https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStaticVarHandle(java.lang.Class,java.lang.String,java.lang.Class) - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1573031355
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v5]
> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. > > A few implementation-detail methods in VarHandle are now documented with the > implied constraints to avoid subtle problems in the future. Changed > `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy > VarHandle changes. Also changed VarHandleBaseTest to report the whole > incorrect type of exception caught than swallow it and leaving only a message. > > Current problems: > - [ ] The lazy var handle is quite slow on the first invocation. >- As seen in the benchmark, users can first call > `Lookup::ensureInitialized` to create an eager handle. >- After that, the lazy handle has a performance on par with the regular > var handle. > - [ ] The class-loading-based test is not in a unit test >- The test frameworks don't seem to offer fine-grained control for > class-loading detection or reliable unloading > > > BenchmarkMode Cnt Score > Error Units > VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± > 0.003 ns/op > VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± > 0.002 ns/op > > > BenchmarkMode Cnt Score > Error Units > LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± > 7735.276 ns/op > LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± > 8482.883 ns/op > LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± > 9776.343 ns/op > LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± > 72894.865 ns/op > LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± > 3513.909 ns/op > LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± > 5144.540 ns/op > LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± > 10816.983 ns/op > LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± > 189370.448 ns/op Chen Liang 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 11 additional commits since the last revision: - Compute base and offset right after linking, simplify code - Merge branch 'master' into lazy-static-varhandle - Fix exact swap Co-authored-by: Mandy Chung - Remove export for removed package - Merge branch 'master' into lazy-static-varhandle - Test the creation performance of handles, lazy one indeed better - Merge branch 'master' into lazy-static-varhandle - copyright year - Benchmarks. lazy initialize is SLOW - nuke meaningless overrides - ... and 1 more: https://git.openjdk.org/jdk/compare/a32cfd30...27e18b7c - Changes: - all: https://git.openjdk.org/jdk/pull/13821/files - new: https://git.openjdk.org/jdk/pull/13821/files/d347ee7e..27e18b7c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13821=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13821=03-04 Stats: 66891 lines in 1457 files changed: 47041 ins; 10823 del; 9027 mod Patch: https://git.openjdk.org/jdk/pull/13821.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13821/head:pull/13821 PR: https://git.openjdk.org/jdk/pull/13821
Re: RFR: 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale cache being cleared [v3]
On Thu, 1 Jun 2023 08:58:23 GMT, SUN Guoyun wrote:
>> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
>> TEST=gc/TestAllocHumongousFragment.java
>> error info:
>>
>> Caused by: java.lang.NullPointerException: Cannot invoke
>> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
>> at
>> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
>> ... 24 more
>>
>> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
>> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
>> (LocaleObjectCache uses SoftReferences, used by printf method called in
>> getRandomInstance(Utils.java:511)).
>>
>> Maybe we have to deal with the case where the getBaseLocale() return value
>> is null. the call stack is:
>>
>> at
>> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
>> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
>> at
>> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>>
>> in LocaleObjectCache.java:64
>>
>> 62 if (key == null || newVal == null) {
>>
>> 63 // subclass must return non-null key/value object
>>
>> 64 return null; // run here
>> 65 }
>
> SUN Guoyun has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale
> cache being cleared
`make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp -XX:+UseShenandoahGC
-XX:ShenandoahGCHeuristics=aggressive -XX:+ShenandoahOOMDuringEvacALot"
TEST=java/util/Locale/SoftKeys.java` can trigger same error:
Caused by: java.lang.NullPointerException: Cannot invoke
"sun.util.locale.BaseLocale.getVariant()" because "base" is null
at java.base/java.util.Locale.forLanguageTag(Locale.java:1876)
at
java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:45)
... 19 more
this patch can fix it.
-
PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1573032287
Re: RFR: 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale cache being cleared [v3]
On Thu, 1 Jun 2023 09:03:23 GMT, SUN Guoyun wrote: >> SUN Guoyun has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale >> cache being cleared > > I've done basic testing jtreg tier1-3 and partial shenandoah testing with > vmoptions `-Xcomp -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=aggressive > -XX:+ShenandoahOOMDuringEvacALot`, the results are all OK. > @sunny868 Would you be able to add a JMH test to make sure that your change > would not affect the startup time? All JMH tests or some of them? I had trigger JMH test, waiting for run results. - PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1572999255
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v4]
On Thu, 1 Jun 2023 21:26:25 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix exact swap > > Co-authored-by: Mandy Chung In addition, should we initialize the ref class or the declaring class only? I don't think we need to initialize the ref class when declaring class initialization should be sufficient, but that is a change in behavior. - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1572991154
Re: RFR: 8307149: MethodHandles.arrayConstructor can be cached
On Thu, 4 May 2023 23:29:17 GMT, Chen Liang wrote: > This patch migrates `MethodHandles::arrayConstructor`, added in Java 9 as a > hotfix to the incorrect constructor found on arrays via Lookup, to share the > array access caching features. The result is that calling > `MethodHandles.arrayConstructor` to create method handles is much faster. > > Oracle JDK 20: > > Benchmark Mode Cnt Score > Error Units > MethodHandlesArrayConstructor.createWithAnewarray avgt 15 2.739 ± > 0.058 ns/op > MethodHandlesArrayConstructor.createWithMethodHandle avgt 15 3.313 ± > 0.041 ns/op > MethodHandlesArrayConstructor.methodHandleCreationavgt 15 61.874 ± > 0.397 ns/op > > > This patch: > > Benchmark Mode Cnt Score > Error Units > MethodHandlesArrayConstructor.createWithAnewarray avgt 15 3.067 ± > 0.026 ns/op > MethodHandlesArrayConstructor.createWithMethodHandle avgt 15 3.699 ± > 0.042 ns/op > MethodHandlesArrayConstructor.methodHandleCreationavgt 15 1.447 ± > 0.004 ns/op keep-alive. - PR Comment: https://git.openjdk.org/jdk/pull/13819#issuecomment-1572983914
Re: RFR: 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale cache being cleared [v3]
On Thu, 1 Jun 2023 08:58:23 GMT, SUN Guoyun wrote:
>> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
>> TEST=gc/TestAllocHumongousFragment.java
>> error info:
>>
>> Caused by: java.lang.NullPointerException: Cannot invoke
>> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
>> at
>> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
>> ... 24 more
>>
>> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
>> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
>> (LocaleObjectCache uses SoftReferences, used by printf method called in
>> getRandomInstance(Utils.java:511)).
>>
>> Maybe we have to deal with the case where the getBaseLocale() return value
>> is null. the call stack is:
>>
>> at
>> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
>> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
>> at
>> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>>
>> in LocaleObjectCache.java:64
>>
>> 62 if (key == null || newVal == null) {
>>
>> 63 // subclass must return non-null key/value object
>>
>> 64 return null; // run here
>> 65 }
>
> SUN Guoyun has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale
> cache being cleared
src/java.base/share/classes/sun/util/locale/BaseLocale.java line 369:
> 367: BaseLocale l = key.holder;
> 368: BaseLocale locale = new BaseLocale(l.getLanguage(),
> l.getScript(), l.getRegion(), l.getVariant(), true);
> 369: return (new Key(locale)).getBaseLocale();
Perhaps a more rigorous approach would look like this：
BaseLocale locale = new BaseLocale(l.getLanguage(), l.getScript(),
l.getRegion(), l.getVariant(), true);
BaseLocal value = (new Key(locale)).getBaseLocale();
Reference.reachabilityFence(locale);
return value;
But the current patch has passed the tests with
`-XX:ShenandoahGCHeuristics=aggressive -XX:+ShenandoahOOMDuringEvacALot`, so I
think the current patch is OK.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14211#discussion_r1213813143
Re: RFR: 8309196: Remove Thread.countStackFrames
On Thu, 1 Jun 2023 06:40:04 GMT, Alan Bateman wrote: > Thread.countStackFrames is/was an ill-defined method that dates from JDK 1.0 > for counting the stack frames of a suspended Thread. The method was > deprecated in JDK 1.2 (1998), deprecated for removal in Java 9, and > re-specified/degraded to throw UOE unconditionally in Java 14. Java 22 would > be a fine time to finally remove this method. > > Code that wants to count stack frames should be directed to use > j.l.StackWalker, or if is a a tool, then it can use JVM TI and other tool > APIs. > > A corpus analysis of 30M classes in 131k artifacts from Maven Central found > only 1 usages to this method. The 1 usage appears to be a class that just > wraps all methods, it doesn't actually make use of it. Yes :) - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14257#pullrequestreview-1456506531
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v4]
On Thu, 1 Jun 2023 21:26:25 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix exact swap > > Co-authored-by: Mandy Chung `staticFieldBase` and `staticFieldOffset` can be called before initialization once a class has been linked. They are unsafe operations and so the code has to ensure class initialization before it accesses a static field. - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1572912754
RFR: 8309216: Cast from jchar* to char* in test java/io/GetXSpace.java
For Unix, copy the `jchar`s into an allocated `char` array. - Commit messages: - 8309216: Cast from jchar* to char* in test java/io/GetXSpace.java Changes: https://git.openjdk.org/jdk/pull/14276/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14276=00 Issue: https://bugs.openjdk.org/browse/JDK-8309216 Stats: 23 lines in 1 file changed: 19 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14276.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14276/head:pull/14276 PR: https://git.openjdk.org/jdk/pull/14276
Re: RFR: 8309216: Cast from jchar* to char* in test java/io/GetXSpace.java
On Fri, 2 Jun 2023 00:00:07 GMT, Brian Burkhalter wrote: > For Unix, copy the `jchar`s into an allocated `char` array. Preliminary test run passes. - PR Comment: https://git.openjdk.org/jdk/pull/14276#issuecomment-1572936688
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v4]
On Thu, 1 Jun 2023 16:18:24 GMT, Erik Joelsson wrote: > > > Another possibility might be the user provided `BUILD_LDCXX` includes > > > extra options in the testing build (?). If that's the case, we probably > > > could define a separate `BUILD_LD_PARTIAL` with no added options. In our > > > prototype on JDK 11, we define a separate one for partial linking. > > > > > > I tried building `static-libs-image` for linux-aarch64 on Ubuntu machine > > using `devkit` to reproduce the failure. Also tried building for > > `linux-ppc64le-server-release` target using `devkit` on Ubuntu. Both built > > successfully with using `devkit`. I could not build a `devkit` for arm32 > > (with `make TARGETS="arm-linux-gnueabihf" BASE_OS=Fedora > > BASE_OS_VERSION=17`, also tried with BASE_OS_VERSION=17). > > @erikj79 Could you please help provide additional insight for the build > > failure you found for arm32? `BUILD_LIBJVM_partial_link.cmdline` might help > > shed some light. > > There are no extra options. I suspect it's an issue with the older GCC > version. Here is one failing command line: > > ``` > $ ( cd /home/erik/git/jdk/build/linux-arm32 ; > /var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/arm-linux-gnueabihf-gcc > -r > --sysroot=/var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/arm-linux-gnueabihf/sysroot > -o > /home/erik/git/jdk/build/linux-arm32/support/native/java.rmi/librmi/static/librmi_relocatable.o > > /home/erik/git/jdk/build/linux-arm32/support/native/java.rmi/librmi/static/GC.o > ) > /var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/bin/ld: > fatal error: cannot mix -r with dynamic object > /var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/lib/libgcc_s.so.1 > ``` > Thanks a lot for looking into the issue. > > The partial linking was originally suggested by C++/Clang toolchain folks > > to mitigate linker overhead that was observed during final executable link > > time. For a static library containing any object file (`.o`) that was > > compiled with ThinLTO (https://clang.llvm.org/docs/ThinLTO.html) enabled, > > linking an executable using the static library without distributed ThinLTO > > could experience more overhead and slow down linking. Solving the macosx > > `ar` limitation is a side-effect/benefit of using partial linking. We > > probably would want to include the partial linking even without the `ar` > > limitation. > > This seems to be Clang specific, so maybe only apply this to Clang and not > GCC? Have you measured the difference in link time and concluded that this > workaround is needed? If so, how big were the differences? It was noticeably slower (which prompted me looked into it at that time) when linking the executable using the static libraries created without partial linking workaround. In my experiments, it took >2m when linking final executable. With the mitigation the final linking took a few seconds. > Given that it prints a lot of warnings on mac, I would suggest leaving the > partial linking out of this patch to get something in that is actually > working. It was included in this patch because of the side effect it had with > handling long path names. However, since looking closer at that issue, we > were using a different workaround for Clang already and that same workaround > should work fine for AR on mac as well. Sounds good for solving the macosx `ar` limitation differently. I'll change that. We can also exclude the partial linking part for gcc (due the older tool issue that you've found). Any concerns with including partial linking step for clang on Linux? - PR Comment: https://git.openjdk.org/jdk/pull/14064#issuecomment-1572922666
Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]
On Tue, 23 May 2023 09:04:11 GMT, Severin Gehwolf wrote: >> Please review these test changes which implement automatic testing of >> container resource updates without JVM restart. Note that this merely tests >> container detection code handling this case. It doesn't do anything special >> for the JVM itself, though it might make sense to add some sanity checks >> should we detect certain limits changing. In another PR, though. >> >> As to the test design, it works similar to the shared temp tests: Interact >> between the two containers by virtue of a shared filesystem `/tmp` and >> creating marker files there in order to make them cooperate. Note that the >> new test needs `podman` version `4.3.0` and better (`4.5` is current). >> >> Testing: >> - [x] GHA >> - [x] Linux x86_64 container tests on cg v1 and cg v2 system >> - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and >> `docker`) > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into jdk-8308090-tests-container-on-fly-updates > - Fix whitespace > - 8308090: Add container tests for on-the-fly resource quota updates Changes look good to me. Thank you for adding these tests. - Marked as reviewed by mseledtsov (Committer). PR Review: https://git.openjdk.org/jdk/pull/14090#pullrequestreview-1456374316
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 20:57:25 GMT, Maurizio Cimadamore wrote: >> On a related note: should we even allow sequences of padding layouts? > > I let sequence of padding slide on the basis that groups of padding are > allowed. But it's a somewhat arbitrary line. That said, we do use stuff like > that in jextract, where we model types we don't support (or bitfields) as > padding. I think starting to ban padding in certain places would result in a > less compositional API - and perhaps tools would have to workaround some of > these limitations. Thanks, I agree with that analogy - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213714285
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:00:33 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 470:
>>
>>> 468: * @throws IllegalArgumentException if the layout path contains
>>> one or more dereference path elements.
>>> 469: * @throws IllegalArgumentException if the layout path contains
>>> one or more path elements that select one or more
>>> 470: * sequence element indices, such as {@link
>>> PathElement#sequenceElement(long)} and {@link
>>> PathElement#sequenceElement(long, long)}).
>>
>> Looks like the first method link should like to
>> `PathElement#sequenceElement()` instead? (I think using a bound
>> `sequenceElement` is fine right?)
>
> This is deliberate (but subtle). Basically, for `select` we just want to
> select a target layout (we don't care which). So the method has a preference
> for open sequence layout paths: there's no access or offset to model here,
> the method just needs to end up into some layout at the end of the path. One
> might argue that these restrictions are unnecessary - but I didn't feel
> strongly enough to drop them.
Ok, thanks for explaining.
>> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 388:
>>
>>> 386: * knows that they have fully processed the contents of the
>>> allocated segment before the subsequent allocation request
>>> 387: * takes place.
>>> 388: * @implNote While a prefix allocator is thread-safe,
>>> concurrent access on the same recycling
>>
>> Is the term "thread-safe" defined any where? Should it be?
>
> I think the term is used pretty much all over the javadoc (not just FFM's) -
> e.g. look for this preamble:
>
> * href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based,
> * immutable and thread-safe
> ```
Right, I guess I'm asking: can/should we do better? ;) It might be nice to
define the term once and for all for the entire JDK, similar to how
'reachability' also has a central definition. (and then add a link from here).
But, arguably, that seems like too much scope creep for this PR, so let's save
it for a potential followup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213714496
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213716230
Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently [v3]
On Thu, 1 Jun 2023 08:58:23 GMT, SUN Guoyun wrote:
>> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
>> TEST=gc/TestAllocHumongousFragment.java
>> error info:
>>
>> Caused by: java.lang.NullPointerException: Cannot invoke
>> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
>> at
>> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
>> ... 24 more
>>
>> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
>> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
>> (LocaleObjectCache uses SoftReferences, used by printf method called in
>> getRandomInstance(Utils.java:511)).
>>
>> Maybe we have to deal with the case where the getBaseLocale() return value
>> is null. the call stack is:
>>
>> at
>> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
>> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
>> at
>> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>>
>> in LocaleObjectCache.java:64
>>
>> 62 if (key == null || newVal == null) {
>>
>> 63 // subclass must return non-null key/value object
>>
>> 64 return null; // run here
>> 65 }
>
> SUN Guoyun has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale
> cache being cleared
As to the patch, would you please elaborate on your changes more? To me, it is
simply inlining `Key.normalize(key)` content into `Cache.createObject()`, and
not sure how it prevents the issue in which the referent got GC'ed during the
reference creation and use.
-
PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1572826469
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v4]
> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. > > A few implementation-detail methods in VarHandle are now documented with the > implied constraints to avoid subtle problems in the future. Changed > `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy > VarHandle changes. Also changed VarHandleBaseTest to report the whole > incorrect type of exception caught than swallow it and leaving only a message. > > Current problems: > - [ ] The lazy var handle is quite slow on the first invocation. >- As seen in the benchmark, users can first call > `Lookup::ensureInitialized` to create an eager handle. >- After that, the lazy handle has a performance on par with the regular > var handle. > - [ ] The class-loading-based test is not in a unit test >- The test frameworks don't seem to offer fine-grained control for > class-loading detection or reliable unloading > > > BenchmarkMode Cnt Score > Error Units > VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± > 0.003 ns/op > VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± > 0.002 ns/op > > > BenchmarkMode Cnt Score > Error Units > LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± > 7735.276 ns/op > LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± > 8482.883 ns/op > LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± > 9776.343 ns/op > LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± > 72894.865 ns/op > LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± > 3513.909 ns/op > LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± > 5144.540 ns/op > LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± > 10816.983 ns/op > LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± > 189370.448 ns/op Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Fix exact swap Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/13821/files - new: https://git.openjdk.org/jdk/pull/13821/files/6ed6b805..d347ee7e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13821=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13821=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13821.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13821/head:pull/13821 PR: https://git.openjdk.org/jdk/pull/13821
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Thu, 1 Jun 2023 19:39:27 GMT, Mandy Chung wrote: > As for the implementation of `LazyStaticVarHandle`, I expect that > `makeFieldHandle` can always create the direct var handle (i.e. > `FieldStaticReadXXX` var handle) and then wrapped by `LazyStaticVarHandle` > similar to how `IndirectVarHandle` does; Since I am not familiar with hotspot internals, I assumed that staticFieldBase and staticFieldOffset shouldn't be used until the class is initialized (rather than linked). This for certain can be done if they are ready once a class is linked, and we should probably update unsafe's comment as well. Can you confirm that they staticFieldBase/Offset are safe to be called before initialization? If this change is safe, we can significantly reduce the initial-call overhead of the lazy VH significantly as well. - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1572801392
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:04:42 GMT, Maurizio Cimadamore wrote: >> the idea behind this is to connect with the javadoc of `SymbolLookup` which >> defines and then uses symbol all over the place. > > Maybe a linkplan could help? Okay, but `SymbolLookup` also says this: "A symbol lookup retrieves the address of a symbol in one or more libraries." and "This symbol lookup, which is known as a default lookup, helps clients to quickly find addresses of well-known symbols." This seems to imply that "symbol" and "address" are two different things. I get the connection between the address and the SymbolLookup, but I'll note that it's also not necessary for a function address to come from a SymbolLookup (e.g. the address could be returned from native code, or be the address of an upcall stub). So, I feel like sticking with "address" is better. Maybe a paragraph (or just a `@see` tag) could be added to refer to SymbolLookup instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213693435
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:00:58 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Arena.java line 269:
>
>> 267: * @throws IllegalStateException if this arena has already been
>> {@linkplain #close() closed}.
>> 268: * @throws WrongThreadException if this arena is confined, and this
>> method is called from a thread {@code T}
>> 269: * other than the arena's owner thread.
>
> "thread T" hints that "T" will be used later, maybe it's not needed.
Correct - this is mostly a copy/paste issue for similar throws clauses in
methods outside arena.
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 363:
>
>> 361: * The returned allocator throws {@link IndexOutOfBoundsException}
>> when a slice of the provided
>> 362: * segment with the requested size and alignment cannot be found.
>> 363: * @implNote A slicing allocator is not thread-safe.
>
> The implNote about thread safety makes me wonder if the SegmentAllocator
> should say something about thread safety, e.g. should it specify that the
> allocate methods are thread safe?
I think SegmentAllocator should be agnostic re. thread safety. Allocation is a
world of compromises, where if you give up (thread) safety you can gain more
performance (and viceversa). So I think having a "one size fits all"
thread-safety blanket might not work very well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213686844
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213685702
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:04:27 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/java/lang/foreign/Linker.java line 473: >> >>> 471: >>> 472: /** >>> 473: * Creates a method handle which is used to call a foreign >>> function with the given signature and symbol. >> >> I always think of a function "symbol" mostly as the _name_ of the function, >> so it seems that "address" would be more correct here. Though, I might be >> wrong on that. It's hard to find a clear definition of "symbol" that applies >> to this specific use case. > > the idea behind this is to connect with the javadoc of `SymbolLookup` which > defines and then uses symbol all over the place. Maybe a linkplan could help? - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679476
Re: RFR: JDK-8306584: Start of release updates for JDK 22 [v5]
> Time to get JDK 22 underway... Joe Darcy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits: - Update symbol files for JDK 21 build 25. - Merge branch 'master' into JDK-8306584 - Refactor a few more enums. - Respond to review comments. - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Sync in symbol changes for JDK 21 build 24. - Merge branch 'master' into JDK-8306584 - ... and 19 more: https://git.openjdk.org/jdk/compare/0ab09630...075a2ab6 - Changes: https://git.openjdk.org/jdk/pull/13567/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13567=04 Stats: 5956 lines in 69 files changed: 5902 ins; 0 del; 54 mod Patch: https://git.openjdk.org/jdk/pull/13567.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13567/head:pull/13567 PR: https://git.openjdk.org/jdk/pull/13567
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 18:12:28 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 444:
>
>> 442: *
>> 443: * When an upcall stub is passed to a foreign function, a JVM crash
>> might occur, if the foreign code casts the function pointer
>> 444: * associated with the upcall stub to a type that is incompatible with
>> the type of the upcall stub. Moreover, if the method
>
> This kind of makes it sound like a crash can occur at the time of the cast. I
> think we should mention that the crash can occur when the function is invoked.
> Suggestion:
>
> * When an upcall stub is passed to a foreign function, a JVM crash might
> occur, if the foreign code casts the function pointer
> * associated with the upcall stub to a type that is incompatible with the
> type of the upcall stub, and then attempts to invoke the function through the
> resulting function pointer. Moreover, if the method
I agree that's better - I was also puzzled by the text (I think it's there from
before, just shuffled?)
> src/java.base/share/classes/java/lang/foreign/Linker.java line 473:
>
>> 471:
>> 472: /**
>> 473: * Creates a method handle which is used to call a foreign function
>> with the given signature and symbol.
>
> I always think of a function "symbol" mostly as the _name_ of the function,
> so it seems that "address" would be more correct here. Though, I might be
> wrong on that. It's hard to find a clear definition of "symbol" that applies
> to this specific use case.
the idea behind this is to connect with the javadoc of `SymbolLookup` which
defines and then uses symbol all over the place.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 393:
>
>> 391: *
>> 392: * Multiple paths can be chained, by using > href=#deref-path-elements>dereference path elements.
>> 393: * A dereference path element allows to obtain a native memory
>> segment whose base address is the address value
>
> "allows to obtain" doesn't sound right to me. I think it should either be
> "allows _subject_ to obtain" (where _subject_ is for instance "the user"), or
> it could also be "allows obtaining" (the the former seems more natural to me).
> Suggestion:
>
> * A dereference path element allows the user to obtain a native memory
> segment whose base address is the address value
I will also look for some other way to say this - typically when I end up with
such convoluted sentences there's a better way to write it :-)
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 418:
>
>> 416: *
>> 417: * @param elements the layout path elements.
>> 418: * @return a var handle that accesses a memory segment at the
>> offset selected by the given layout path.
>
> This doesn't seem quite right. It is not the memory segment which is found at
> the offset given by the layout path, it is the base address.
>
> Maybe:
> Suggestion:
>
> * @return a var handle that accesses a memory segment whose base address
> is found at the offset selected by the given layout path.
Yeah - I meant what you said - but now that you said it, I also saw how what
I've written can be prone to an alternate (and wrong) interpretation. I'll
clarify.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 470:
>
>> 468: * @throws IllegalArgumentException if the layout path contains one
>> or more dereference path elements.
>> 469: * @throws IllegalArgumentException if the layout path contains one
>> or more path elements that select one or more
>> 470: * sequence element indices, such as {@link
>> PathElement#sequenceElement(long)} and {@link
>> PathElement#sequenceElement(long, long)}).
>
> Looks like the first method link should like to
> `PathElement#sequenceElement()` instead? (I think using a bound
> `sequenceElement` is fine right?)
This is deliberate (but subtle). Basically, for `select` we just want to select
a target layout (we don't care which). So the method has a preference for open
sequence layout paths: there's no access or offset to model here, the method
just needs to end up into some layout at the end of the path. One might argue
that these restrictions are unnecessary - but I didn't feel strongly enough to
drop them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213681006
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679251
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213677679
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213676964
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213675483
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:25:38 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 645:
>>
>>> 643: * is 1. As such, regardless of its size, in the absence of an
>>> {@linkplain #withByteAlignment(long) explicit}
>>> 644: * alignment constraint, a padding layout does not affect the
>>> alignment constraint of the group or sequence layout
>>> 645: * it is nested into.
>>
>> It is possible to override the alignment constraints of group and sequence
>> layouts, so maybe this should say _natural alignment_.
>> Suggestion:
>>
>> * alignment constraint, a padding layout does not affect the natural
>> alignment of the group or sequence layout
>> * it is nested into.
>
> On a related note: should we even allow sequences of padding layouts?
I let sequence of padding slide on the basis that groups of padding are
allowed. But it's a somewhat arbitrary line. That said, we do use stuff like
that in jextract, where we model types we don't support (or bitfields) as
padding. I think starting to ban padding in certain places would result in a
less compositional API - and perhaps tools would have to workaround some of
these limitations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213672976
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:36:48 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 938:
>
>> 936: * the properties of this segment. For instance, if this segment is
>> a {@linkplain #isReadOnly() read-only segment},
>> 937: * then the resulting buffer is also {@linkplain
>> ByteBuffer#isReadOnly() read-only}. Additionally, if this is a native
>> 938: * segment, the resulting buffer is a {@linkplain
>> ByteBuffer#isDirect() direct buffer}.
>
> (Pre-existing, but seemed useful to mention) Rather than giving a few
> examples with 'For instance', perhaps this should more explicitly list _all_
> the properties that are reflected in the returned buffer (not sure if there
> are more).
Good idea - it is not trivial to capture all the properties in a single para.
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 388:
>
>> 386: * knows that they have fully processed the contents of the
>> allocated segment before the subsequent allocation request
>> 387: * takes place.
>> 388: * @implNote While a prefix allocator is thread-safe,
>> concurrent access on the same recycling
>
> Is the term "thread-safe" defined any where? Should it be?
I think the term is used pretty much all over the javadoc (not just FFM's) -
e.g. look for this preamble:
* value-based,
* immutable and thread-safe
```
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213670809
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213669418
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:16:44 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/SequenceLayout.java line 75:
>
>> 73: * @return a sequence layout with the same characteristics of this
>> layout, but with the given element count.
>> 74: * @throws IllegalArgumentException if {@code elementCount} is
>> negative.
>> 75: * @throws IllegalArgumentException if {@code
>> elementLayout.bitSize() * elementCount} overflows.
>
> Shouldn't the exception descriptions be combined to avoid IAE being listed
> twice in the generated javadoc?
I think @PaulSandoz suggested this idiom in other places where the `@throws`
clause was very hard to break up, so I thought that would have been preferrable
than having a single para. But if there are strong opinions, I could tweak in
different ways, perhaps with bullet lists (but that will require a lot of work).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213660125
RFR: 8307840: SequencedMap view method specification and implementation adjustments
Adjust the specification of the `SequencedMap` sequenced-view methods, and adjust implementations to match. - Commit messages: - Minor wording adjustment. - 8307840: SequencedMap view method specification and implementation adjustments Changes: https://git.openjdk.org/jdk/pull/14267/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14267=00 Issue: https://bugs.openjdk.org/browse/JDK-8307840 Stats: 113 lines in 3 files changed: 64 ins; 3 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/14267.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14267/head:pull/14267 PR: https://git.openjdk.org/jdk/pull/14267
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:24:35 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 645:
>
>> 643: * is 1. As such, regardless of its size, in the absence of an
>> {@linkplain #withByteAlignment(long) explicit}
>> 644: * alignment constraint, a padding layout does not affect the
>> alignment constraint of the group or sequence layout
>> 645: * it is nested into.
>
> It is possible to override the alignment constraints of group and sequence
> layouts, so maybe this should say _natural alignment_.
> Suggestion:
>
> * alignment constraint, a padding layout does not affect the natural
> alignment of the group or sequence layout
> * it is nested into.
On a related note: should we even allow sequences of padding layouts?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213585708
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore
wrote:
>> As the FFM API evolved over time, some parts of the javadoc went out of
>> sync. Now that most of the API is stable, it is a good time to look again at
>> the javadoc as a whole, and bring some more consistency.
>>
>> While most of the changes in this PR are stylistic, I'd like to call out few
>> things which resulted in API changes:
>>
>> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified
>> requirement that its alignment parameter must be a power of two.
>>
>> * `MemoryLayout::sliceHandle` did not require the absence of dereference
>> paths in the provided layout path. While that is technically possible to
>> allow, currently the method is specified as returning a "slice"
>> corresponding to some "nested layout", so following pointers seems
>> incompatible with the spec. I have decided to disallow for now - we can
>> always compatibly enable it later, if required.
>>
>> * `MemorySegment::copy` - most of the overloads did not specify that
>> `UnsupportedOperationException` is thrown if the destination segment is
>> read-only.
>>
>> * In several places, an extra `@throws IllegalArgumentException` or `@throws
>> IllegalArgumentException` has been added to capture cases where element size
>> * index computation can overflow. This is true for all the element-wise
>> segment copy methods, for the indexed accessors in memory segment (e.g.
>> `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for
>> `SegmentAllocator::allocateArray(MemoryLayout, long)`.
>>
>> In all these cases (except for overflows), new tests have been added to
>> cover the additional API changes (a CSR will also be filed to cover these).
>>
>> The class with most changes is `MemoryLayout`. I realized that the javadoc
>> there was a bit sloppy around the definition of "layout paths". Now there
>> are several subsection in the class javadoc, which explain how different
>> kinds of paths can be used. I have introduced the notion of "open path
>> elements" to denote those path elements that are not fully specified, and
>> result in additional var handle coordinates to be added. There is also now a
>> definition of what it means for a layout path to be "well-formed", so that
>> all methods accepting a layout path can simply refer to it (this definition
>> is tricky to give inline in the javadoc of the various path-accepting
>> methods, as it depends on many factors).
>>
>> Another biggie change was in `MemorySegment` as now all dereference methods
>> state precisely their spatial checks requirements, as well also specifying
>> when the API can th...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix wrong link in layout well-formedness doc
Overall a great cleanup, nice work!
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 91:
> 89: /**
> 90: * Returns a function descriptor with no return layout.
> 91: * @return a new function descriptor, with no return layout.
Maybe this should be collapsed into a single `{@return ...}` block.
src/java.base/share/classes/java/lang/foreign/Linker.java line 201:
> 199: *
> 200: * All native linker implementations operate on a subset of memory
> layouts. More formally, a layout {@code L}
> 201: * is supported by a native linker {@code NL} iff:
I think using `iff` (if-and-only-if) is incorrect here, since certain linkers
might impose additional constraints. For instance, the fallback linker doesn't
support union layouts. Also, we want to further restrict variadic argument
layouts as well as part of https://github.com/openjdk/jdk/pull/14225
Maybe we could say that all layouts passed to a linker must _at least_ adhere
to the following constraints.
src/java.base/share/classes/java/lang/foreign/Linker.java line 203:
> 201: * is supported by a native linker {@code NL} iff:
> 202: *
> 203: * {@code L} is a value layout {@code V} and {@code V.withoutName()}
> is equal to one of the following layout constants:
I think the equivalence this is talking about is MemoryLayout::equals? Maybe a
plain link should be added to that method as well?
Suggestion:
* {@code L} is a value layout {@code V} and {@code V.withoutName()} is
{@linkplain MemoryLayout#equals(Object) equal} to one of the following layout
constants:
src/java.base/share/classes/java/lang/foreign/Linker.java line 444:
> 442: *
> 443: * When an upcall stub is passed to a foreign function, a JVM crash
> might occur, if the foreign code casts the function pointer
> 444: * associated with the upcall stub to a type that is incompatible with
> the type of the upcall stub. Moreover, if the method
This kind of makes it sound like a crash can occur at the time of the cast. I
think we should mention that the crash can occur when the function is invoked.
Suggestion:
* When an
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v29]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 45 commits: - Merge branch 'master' into 8306112 - Integrating JDK-8308913 - Integrate JDK-8308916 and JDK-8308831 - javax.model - Missing Preview feature enum - Restrict access to unnamed class members when doing separate compilation. - Merge branch 'master' into 8306112 - Remove mandated flag - Remove trailing whitespace - Add main tests for inferface/enum/record - Improving error recovery in presence of less important syntactic errors in top-level methods and fields. Author: Jan Lahoda - ... and 35 more: https://git.openjdk.org/jdk/compare/2bb19724...e39e3fe9 - Changes: https://git.openjdk.org/jdk/pull/13689/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13689=28 Stats: 1648 lines in 41 files changed: 1471 ins; 32 del; 145 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote:
>> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`.
>>
>> A few implementation-detail methods in VarHandle are now documented with the
>> implied constraints to avoid subtle problems in the future. Changed
>> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy
>> VarHandle changes. Also changed VarHandleBaseTest to report the whole
>> incorrect type of exception caught than swallow it and leaving only a
>> message.
>>
>> Current problems:
>> - [ ] The lazy var handle is quite slow on the first invocation.
>>- As seen in the benchmark, users can first call
>> `Lookup::ensureInitialized` to create an eager handle.
>>- After that, the lazy handle has a performance on par with the regular
>> var handle.
>> - [ ] The class-loading-based test is not in a unit test
>>- The test frameworks don't seem to offer fine-grained control for
>> class-loading detection or reliable unloading
>>
>>
>> BenchmarkMode Cnt Score
>> Error Units
>> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ±
>> 0.003 ns/op
>> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ±
>> 0.002 ns/op
>>
>>
>> BenchmarkMode Cnt Score
>> Error Units
>> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ±
>> 7735.276 ns/op
>> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ±
>> 8482.883 ns/op
>> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ±
>> 9776.343 ns/op
>> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ±
>> 72894.865 ns/op
>> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ±
>> 3513.909 ns/op
>> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ±
>> 5144.540 ns/op
>> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ±
>> 10816.983 ns/op
>> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ±
>> 189370.448 ns/op
>
> Chen Liang 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 eight additional commits since
> the last revision:
>
> - Remove export for removed package
> - Merge branch 'master' into lazy-static-varhandle
> - Test the creation performance of handles, lazy one indeed better
> - Merge branch 'master' into lazy-static-varhandle
> - copyright year
> - Benchmarks. lazy initialize is SLOW
> - nuke meaningless overrides
> - make static field var handles lazy and fix NPE in isAccessModeSupported
I think the exact argument for `withInvokeExactBehavior` and
`withInvokeBehavior` is swapped.
I studied the patch and it seems a clever idea. I will do another pass to
check if `VarHandle::asDirect` is only invoked when the var handle is doing
field access.
As for the implementation of `LazyStaticVarHandle`, I expect that
`makeFieldHandle` can always create the direct var handle (i.e.
`FieldStaticReadXXX` var handle) and then wrapped by `LazyStaticVarHandle`
similar to how `IndirectVarHandle` does; something like this:
LazyStaticVarHandle(VarHandle target, Class refc, boolean exact) {
super(target.vform, exact);
this.target = target;
this.refc = refc;
}
Some overridden methods for example `describeConstable` can simply retrun
`target.describeConstable()`. Do I miss some issue that it can't be
implemented that way?
src/java.base/share/classes/java/lang/invoke/LazyStaticVarHandle.java line 126:
> 124: var delegate = this.delegate;
> 125: return delegate == null ? (hasInvokeExactBehavior() ? this
> 126: : new LazyStaticVarHandle(refc, field,
> writeAllowedOnFinalFields, false))
Suggestion:
: new LazyStaticVarHandle(refc, field,
writeAllowedOnFinalFields, true))
src/java.base/share/classes/java/lang/invoke/LazyStaticVarHandle.java line 134:
> 132: var delegate = this.delegate;
> 133: return delegate == null ? (!hasInvokeExactBehavior() ? this
> 134: : new LazyStaticVarHandle(refc, field,
> writeAllowedOnFinalFields, true))
Suggestion:
: new LazyStaticVarHandle(refc, field,
writeAllowedOnFinalFields, false))
-
PR Review: https://git.openjdk.org/jdk/pull/13821#pullrequestreview-1456188245
PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1572667645
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1213602428
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1213602313
Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v2]
On Thu, 1 Jun 2023 15:14:52 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed benchmarks
>
> src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java
> line 840:
>
>> 838: try {
>> 839: //clone SplitConstantPool with alternate context
>> 840: var cc =
>> Classfile.of(Classfile.StackMapsOption.DO_NOT_GENERATE_STACK_MAPS);
>
> This should set the Class hierarchy resolver, patch dead code, and filter
> dead labels options from the original context.
Here the other options are irrelevant. The error report wraps already generated
incomplete code attribute into a fake class to be able to print details to the
error.
I think even the stack map generation suppression is irrelevant here, will
double check.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1213598996
Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v2]
On Thu, 1 Jun 2023 15:04:00 GMT, Chen Liang wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed benchmarks
>
> src/java.base/share/classes/java/lang/Module.java line 1593:
>
>> 1591: private Class loadModuleInfoClass(InputStream in) throws
>> IOException {
>> 1592: final String MODULE_INFO = "module-info";
>> 1593: var cc =
>> Classfile.of(Classfile.ConstantPoolSharingOption.DO_NOT_SHARE_CONSTANT_POOL);
>
> This `cc` can be stored in a static final field instead.
Yes, we can avoid repeated construction of default class hierarchy resolver
here.
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 55:
>
>> 53:
>> 54: static Classfile of() {
>> 55: return new ClassfileImpl();
>
> We can create a static final field in `ClassfileImpl` holding a default
> instance equivalent to that created by `new ClassfileImpl()` and have the
> `of()` factory return that instance instead.
Global static default context will hold global shared cached class hierarchy
resolver and that we want to avoid.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1213583268
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1213585474
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v28]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Integrating JDK-8308913 - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/9d111c26..f1acdb04 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=27 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=26-27 Stats: 26 lines in 1 file changed: 25 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded
On Thu, 1 Jun 2023 18:19:14 GMT, Mandy Chung wrote: > Convert `ClassForNameLeak` test to use `jdk.test.lib.util.ForceGC` that would > be more reliable in waiting for the class loader to be unloaded. @dougxc has verified that the updated test passes on GraalVM (thanks). - PR Comment: https://git.openjdk.org/jdk/pull/14269#issuecomment-1572619219
Re: RFR: 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded
On Thu, 1 Jun 2023 18:19:14 GMT, Mandy Chung wrote: > Convert `ClassForNameLeak` test to use `jdk.test.lib.util.ForceGC` that would > be more reliable in waiting for the class loader to be unloaded. Marked as reviewed by dnsimon (Committer). - PR Review: https://git.openjdk.org/jdk/pull/14269#pullrequestreview-1456119608
RFR: 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded
Convert `ClassForNameLeak` test to use `jdk.test.lib.util.ForceGC` that would be more reliable in waiting for the class loader to be unloaded. - Commit messages: - 8309241: ClassForNameLeak fails intermittently as the class loader hasn't been unloaded Changes: https://git.openjdk.org/jdk/pull/14269/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14269=00 Issue: https://bugs.openjdk.org/browse/JDK-8309241 Stats: 58 lines in 1 file changed: 15 ins; 29 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/14269.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14269/head:pull/14269 PR: https://git.openjdk.org/jdk/pull/14269
Re: RFR: 8308780: Fix the Java Integer types on Windows [v4]
On Thu, 1 Jun 2023 11:49:24 GMT, Julian Waters wrote: >> On Windows, the basic Java Integer types are defined as long and __int64 >> respectively. In particular, the former is rather problematic since it >> breaks compilation as the Visual C++ becomes stricter and more compliant >> with every release, which means the way Windows code treats long as a >> typedef for int is no longer correct, especially with -permissive- enabled. >> Instead of changing every piece of broken code to match the jint = long >> typedef, which is far too time consuming, we can instead change jint to an >> int (which is still the same 32 bit number type as long), as there are far >> fewer problems caused by this definition. It's better to get this over and >> done with sooner than later when a future version of Visual C++ finally >> starts to break on existing code > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Fix the code that is actually warning I'm not sure I understand the logic here. I would not want to move to using Java typedefs in places where the windows APIs specify the types they are expecting. If something comes in from a JNI down-call we should convert it to the type expected by Windows using the name expected by Windows. Also "compilation" isn't nearly good enough. How is this being tested ? - PR Comment: https://git.openjdk.org/jdk/pull/14125#issuecomment-1572518377
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 09:37:29 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 13 commits:
>
> - Revert test changes
> - Merge branch 'master' into JDK-8308804-uuid-buffers
> - Simplify clinit
> - Remove the properties
> - Swap lsb/msb
> - Fine-tune exceptions
> - Handle privileged properties
> - Use ByteArray to convert. Do version/variant preparations straight on
> locals. Move init out of optimistic lock section.
> - More touchups
> - Comment updates
> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
src/java.base/share/classes/java/util/UUID.java line 181:
> 179:
> 180: private static UUID fromRandom(long msb, long lsb) {
> 181: // set version to 3
The comment doesn't match the code. Is it version 4?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1213472422
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v27]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Integrate JDK-8308916 and JDK-8308831 - javax.model - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/850a2152..9d111c26 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=26 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=25-26 Stats: 313 lines in 6 files changed: 302 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v17]
On Thu, 1 Jun 2023 15:17:41 GMT, Chen Liang wrote: >> As John Rose has pointed out in this issue, the current j.l.r.Proxy based >> implementation of MethodHandleProxies.asInterface has a few issues: >> 1. Exposes too much information via Proxy supertype (and WrapperInstance >> interface) >> 2. Does not allow future expansion to support SAM[^1] abstract classes >> 3. Slow (in fact, very slow) >> >> This patch addresses all 3 problems: >> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method >> clashes >> 2. This patch obtains already generated classes from a ClassValue by the >> requested interface type; the ClassValue can later be updated to compute >> implementation generation for abstract classes as well. >> 3. This patch's faster than old implementation in general. >> >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.483 ±0.025 ns/op >> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 >> 0.264 ±0.006 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.773 ±0.040 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 16.754 ±0.411 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.609 ±1.598 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15 >> 0.424 ±0.024 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15 >> 1.936 ±0.008 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15 >> 1.766 ±0.014 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15 >> 0.414 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15 >> 0.271 ±0.006 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15 >> 0.263 ±0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15 >> 0.266 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15 >> 0.262 ±0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt 15 >> 0.264 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15 >> 18.000 ±0.181 ns/op >> MethodHandleProxiesAsIFInstanceCreat... > > Chen Liang 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 36 additional commits since > the last revision: > > - Some changes per Mandy's review. SecurityManager test fails in this patch > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'master' into explore/mhp-iface > - Merge branch 'master' into explore/mhp-iface > - stage, needs to fix is mh proxy instance check > - Merge branch 'master' into explore/mhp-iface > - Remove assertion, no longer true with teleport definition in MHP > - Fix tabs, and tests about modules and constructor access > - stupid faults > - Revert to shared-class implementation, found a way to deal with security > manager > - ... and 26 more: https://git.openjdk.org/jdk/compare/e6f1393e...56a282e4 Thanks for the update. The security permission check is done regardless of whether it passes the access check.So the generated code can't access internal API. For now, I think you can go back to spin bytecode for the check as done in Proxy. We will consider if it can be replaced by a public API in the future. - PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1572468328
Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently [v3]
On Thu, 1 Jun 2023 08:58:23 GMT, SUN Guoyun wrote:
>> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
>> TEST=gc/TestAllocHumongousFragment.java
>> error info:
>>
>> Caused by: java.lang.NullPointerException: Cannot invoke
>> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
>> at
>> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
>> ... 24 more
>>
>> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
>> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
>> (LocaleObjectCache uses SoftReferences, used by printf method called in
>> getRandomInstance(Utils.java:511)).
>>
>> Maybe we have to deal with the case where the getBaseLocale() return value
>> is null. the call stack is:
>>
>> at
>> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
>> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
>> at
>> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>>
>> in LocaleObjectCache.java:64
>>
>> 62 if (key == null || newVal == null) {
>>
>> 63 // subclass must return non-null key/value object
>>
>> 64 return null; // run here
>> 65 }
>
> SUN Guoyun has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale
> cache being cleared
Those SoftReference caches are introduced with this change:
https://bugs.openjdk.org/browse/JDK-8196869, and there is a stress test added
for it: test/jdk/java/util/Locale/SoftKeys.java
@sunny868 Would you be able to add a JMH test to make sure that your change
would not affect the startup time?
@cl4es would you take a look at this change?
Thanks!
-
PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1572362241
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v4]
On Fri, 26 May 2023 20:26:02 GMT, Jiangli Zhou wrote: > > Another possibility might be the user provided `BUILD_LDCXX` includes extra > > options in the testing build (?). If that's the case, we probably could > > define a separate `BUILD_LD_PARTIAL` with no added options. In our > > prototype on JDK 11, we define a separate one for partial linking. > > I tried building `static-libs-image` for linux-aarch64 on Ubuntu machine > using `devkit` to reproduce the failure. Also tried building for > `linux-ppc64le-server-release` target using `devkit` on Ubuntu. Both built > successfully with using `devkit`. I could not build a `devkit` for arm32 > (with `make TARGETS="arm-linux-gnueabihf" BASE_OS=Fedora BASE_OS_VERSION=17`, > also tried with BASE_OS_VERSION=17). > > @erikj79 Could you please help provide additional insight for the build > failure you found for arm32? `BUILD_LIBJVM_partial_link.cmdline` might help > shed some light. > There are no extra options. I suspect it's an issue with the older GCC version. Here is one failing command line: $ ( cd /home/erik/git/jdk/build/linux-arm32 ; /var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/arm-linux-gnueabihf-gcc -r --sysroot=/var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/arm-linux-gnueabihf/sysroot -o /home/erik/git/jdk/build/linux-arm32/support/native/java.rmi/librmi/static/librmi_relocatable.o /home/erik/git/jdk/build/linux-arm32/support/native/java.rmi/librmi/static/GC.o ) /var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/bin/ld: fatal error: cannot mix -r with dynamic object /var/tmp/jib-erik/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/lib/libgcc_s.so.1 > The partial linking was originally suggested by C++/Clang toolchain folks to > mitigate linker overhead that was observed during final executable link time. > For a static library containing any object file (`.o`) that was compiled with > ThinLTO (https://clang.llvm.org/docs/ThinLTO.html) enabled, linking an > executable using the static library without distributed ThinLTO could > experience more overhead and slow down linking. Solving the macosx `ar` > limitation is a side-effect/benefit of using partial linking. We probably > would want to include the partial linking even without the `ar` limitation. This seems to be Clang specific, so maybe only apply this to Clang and not GCC? Have you measured the difference in link time and concluded that this workaround is needed? If so, how big were the differences? Given that it prints a lot of warnings on mac, I would suggest leaving the partial linking out of this patch to get something in that is actually working. It was included in this patch because of the side effect it had with handling long path names. However, since looking closer at that issue, we were using a different workaround for Clang already and that same workaround should work fine for AR on mac as well. - PR Comment: https://git.openjdk.org/jdk/pull/14064#issuecomment-1572352386
Re: RFR: 8309196: Remove Thread.countStackFrames
On Thu, 1 Jun 2023 06:40:04 GMT, Alan Bateman wrote: > Thread.countStackFrames is/was an ill-defined method that dates from JDK 1.0 > for counting the stack frames of a suspended Thread. The method was > deprecated in JDK 1.2 (1998), deprecated for removal in Java 9, and > re-specified/degraded to throw UOE unconditionally in Java 14. Java 22 would > be a fine time to finally remove this method. > > Code that wants to count stack frames should be directed to use > j.l.StackWalker, or if is a a tool, then it can use JVM TI and other tool > APIs. > > A corpus analysis of 30M classes in 131k artifacts from Maven Central found > only 1 usages to this method. The 1 usage appears to be a class that just > wraps all methods, it doesn't actually make use of it. Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14257#pullrequestreview-1455848358
Re: RFR: 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread safe
On Mon, 20 Feb 2023 17:03:33 GMT, Peter Levart wrote: >> We don't fear calling the factory twice for benign races, as the distinct >> constructor factory instances are behaviorally the same. >> >> The true issue lies in the double getfield operations: Java memory model >> doesn't require the second read to happen-after a write reflected in the >> first read, so return this.genericInfo may return null while >> this.genericInfo == null evaluates to false, in case genericInfo is >> initialized lazily by another thread. See >> https://bugs.openjdk.org/browse/JDK-8261404 > > Hi @liach, > > I think @dholmes-ora is worried about the fields in the object being returned > by the getGenericInfo() method and similar. In above case this means fields > in class ConstructorRepository. > I checked it and the entire hierarchy based on > sun.reflect.generics.repository.AbstractRepository with subclasses including > ConstructorRepository is modeled such that all fields are either: > - volatile and lazily initialized; or > - final and initialized in constructor > > Such objects may be published via data race and still be seen consistent on > the accepting side. I don't have any issue with this version. Making the fields volatile is _currently_ unnecessary to ensure correctness -- thanks @plevart for double-checking that all fields in the hierarchy is either `volatile` or `final` -- but adding it is relatively benign (very minor performance cost to a likely-not-very-performance-sensitive code path), reduces fragility and might avoid extraneous allocations of under race conditions. - PR Comment: https://git.openjdk.org/jdk/pull/12643#issuecomment-1572275936
Integrated: 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread safe
On Sun, 19 Feb 2023 18:41:18 GMT, Chen Liang wrote: > 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not > thread safe This pull request has now been integrated. Changeset: be36096a Author:Chen Liang Committer: Claes Redestad URL: https://git.openjdk.org/jdk/commit/be36096a19bcfc12e789cdeaaa51d746567ac638 Stats: 16 lines in 4 files changed: 8 ins; 0 del; 8 mod 8302822: Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread safe Reviewed-by: stsypanov, redestad - PR: https://git.openjdk.org/jdk/pull/12643
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v4]
On Thu, 1 Jun 2023 15:28:33 GMT, Jorn Vernee wrote: >> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and >> `float` is promoted to `double`, when being passed as variadic argument (see >> e.g. >> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). >> This patch restricts the layouts that can be used as variadic layouts to >> what is allowed by the C specification. >> >> The fallback linker is also updated to use to correct function to link >> variadic calls (not doing this turned out not to be a problem so far, but it >> is problematic for instance on Mac/AArch64 when using the fallback linker). >> Adding the restriction on layouts for all linkers is also partly motivated >> by the fallback linker rejecting such unsupported variadic layouts already. >> >> I've added a small paragraph to the Linker javadoc as well that explains the >> restriction. Comments on that are welcome, but please explain. >> >> The tests are updated to no longer try to link variadic functions with the >> illegal layouts, and I've added some more negative tests to TestIllegalLink. >> >> Testing: >> - local testing on Windows/x64 >> - tier1-3 + jdk-tier5 (ongoing) >> - manual test run on mac/aarch64 with the fallback linker to verify the >> correctness of the fallback linker changes. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > move prototype-less name Thanks for the review! I've updated the CSR with the finalized version of the javadoc. Could you take a look at that as well? https://bugs.openjdk.org/browse/JDK-8309205 - PR Comment: https://git.openjdk.org/jdk/pull/14225#issuecomment-1572275551
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v3]
On Thu, 1 Jun 2023 15:16:42 GMT, Jorn Vernee wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rework javadoc
Looks good - I left an optional comment. feel free to ignore (or to deal with
that as part of some other PR).
src/java.base/share/classes/java/lang/foreign/Linker.java line 373:
> 371: * With an empty formal parameter list, such as: {@code void
> foo();}
> 372: *
> 373: * The latter is often called a prototype-less function as
> well. The arguments passed in place of the ellipsis,
Would it make sense to move the prototype-less name in the second bullet above?
-
PR Comment: https://git.openjdk.org/jdk/pull/14225#issuecomment-1572258397
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1213320545
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v3]
On Thu, 1 Jun 2023 15:19:48 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework javadoc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 373:
>
>> 371: * With an empty formal parameter list, such as: {@code void
>> foo();}
>> 372: *
>> 373: * The latter is often called a prototype-less function as
>> well. The arguments passed in place of the ellipsis,
>
> Would it make sense to move the prototype-less name in the second bullet
> above?
Yes, will do.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1213324562
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v4]
> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and > `float` is promoted to `double`, when being passed as variadic argument (see > e.g. > https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). > This patch restricts the layouts that can be used as variadic layouts to > what is allowed by the C specification. > > The fallback linker is also updated to use to correct function to link > variadic calls (not doing this turned out not to be a problem so far, but it > is problematic for instance on Mac/AArch64 when using the fallback linker). > Adding the restriction on layouts for all linkers is also partly motivated by > the fallback linker rejecting such unsupported variadic layouts already. > > I've added a small paragraph to the Linker javadoc as well that explains the > restriction. Comments on that are welcome, but please explain. > > The tests are updated to no longer try to link variadic functions with the > illegal layouts, and I've added some more negative tests to TestIllegalLink. > > Testing: > - local testing on Windows/x64 > - tier1-3 + jdk-tier5 (ongoing) > - manual test run on mac/aarch64 with the fallback linker to verify the > correctness of the fallback linker changes. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: move prototype-less name - Changes: - all: https://git.openjdk.org/jdk/pull/14225/files - new: https://git.openjdk.org/jdk/pull/14225/files/e806fae5..1f1e22eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14225=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14225=02-03 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14225.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14225/head:pull/14225 PR: https://git.openjdk.org/jdk/pull/14225
Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v2]
On Thu, 1 Jun 2023 15:20:27 GMT, Adam Sotona wrote:
>> Classfile context object and multi-state options have been discussed at
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html
>> This patch implements the proposed changes in Classfile API and fixes all
>> affected code across JDK sources and tests.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed benchmarks
In fact, I think we can probably mark Classfile as value-based. Then, whether
it's created from a factory at every call or if it's cached (my first 2 review
comments) no longer matters as much; the caching behavior will be handled by
individual option instances (such as class hierarchy caching in the class
hierarchy option's resolver)
src/java.base/share/classes/java/lang/Module.java line 1593:
> 1591: private Class loadModuleInfoClass(InputStream in) throws
> IOException {
> 1592: final String MODULE_INFO = "module-info";
> 1593: var cc =
> Classfile.of(Classfile.ConstantPoolSharingOption.DO_NOT_SHARE_CONSTANT_POOL);
This `cc` can be stored in a static final field instead.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 55:
> 53:
> 54: static Classfile of() {
> 55: return new ClassfileImpl();
We can create a static final field in `ClassfileImpl` holding a default
instance equivalent to that created by `new ClassfileImpl()` and have the
`of()` factory return that instance instead.
src/java.base/share/classes/jdk/internal/classfile/impl/StackMapGenerator.java
line 840:
> 838: try {
> 839: //clone SplitConstantPool with alternate context
> 840: var cc =
> Classfile.of(Classfile.StackMapsOption.DO_NOT_GENERATE_STACK_MAPS);
This should set the Class hierarchy resolver, patch dead code, and filter dead
labels options from the original context.
-
PR Comment: https://git.openjdk.org/jdk/pull/14180#issuecomment-1572260124
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1213300095
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1213302665
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1213314212
Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v2]
> Classfile context object and multi-state options have been discussed at > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html > This patch implements the proposed changes in Classfile API and fixes all > affected code across JDK sources and tests. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed benchmarks - Changes: - all: https://git.openjdk.org/jdk/pull/14180/files - new: https://git.openjdk.org/jdk/pull/14180/files/bcdf0f11..eae9bed3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14180=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14180=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14180.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14180/head:pull/14180 PR: https://git.openjdk.org/jdk/pull/14180
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Sun, 7 May 2023 13:34:54 GMT, Chen Liang wrote: >> As John Rose has pointed out in this issue, the current j.l.r.Proxy based >> implementation of MethodHandleProxies.asInterface has a few issues: >> 1. Exposes too much information via Proxy supertype (and WrapperInstance >> interface) >> 2. Does not allow future expansion to support SAM[^1] abstract classes >> 3. Slow (in fact, very slow) >> >> This patch addresses all 3 problems: >> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method >> clashes >> 2. This patch obtains already generated classes from a ClassValue by the >> requested interface type; the ClassValue can later be updated to compute >> implementation generation for abstract classes as well. >> 3. This patch's faster than old implementation in general. >> >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.483 ±0.025 ns/op >> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 >> 0.264 ±0.006 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.773 ±0.040 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 16.754 ±0.411 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.609 ±1.598 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15 >> 0.424 ±0.024 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15 >> 1.936 ±0.008 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15 >> 1.766 ±0.014 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15 >> 0.414 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15 >> 0.271 ±0.006 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15 >> 0.263 ±0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15 >> 0.266 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15 >> 0.262 ±0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt 15 >> 0.264 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15 >> 18.000 ±0.181 ns/op >> MethodHandleProxiesAsIFInstanceCreat... > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Remove assertion, no longer true with teleport definition in MHP About the security manager: the proposal to change SecurityManager to permit exporting the internal API packages would change the specification of `SecurityManger::checkPackageAccess`. Should I touch that in this patch and potentially require CSR review about SecurityManager changes, or should I go back to the original Proxy-like checks? - PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1572243080
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v17]
> As John Rose has pointed out in this issue, the current j.l.r.Proxy based > implementation of MethodHandleProxies.asInterface has a few issues: > 1. Exposes too much information via Proxy supertype (and WrapperInstance > interface) > 2. Does not allow future expansion to support SAM[^1] abstract classes > 3. Slow (in fact, very slow) > > This patch addresses all 3 problems: > 1. It updates the WrapperInstance methods to take an `Empty` to avoid method > clashes > 2. This patch obtains already generated classes from a ClassValue by the > requested interface type; the ClassValue can later be updated to compute > implementation generation for abstract classes as well. > 3. This patch's faster than old implementation in general. > > > Benchmark Mode Cnt > Score Error Units > MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 > 1.483 ±0.025 ns/op > MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 > 0.264 ±0.006 ns/op > MethodHandleProxiesAsIFInstance.testCall avgt 15 > 1.773 ±0.040 ns/op > MethodHandleProxiesAsIFInstance.testCreate avgt 15 >16.754 ±0.411 ns/op > MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >19.609 ±1.598 ns/op > MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15 > 0.424 ±0.024 ns/op > MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15 > 1.936 ±0.008 ns/op > MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15 > 1.766 ±0.014 ns/op > MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15 > 0.414 ±0.005 ns/op > MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15 > 0.271 ±0.006 ns/op > MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15 > 0.263 ±0.004 ns/op > MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15 > 0.266 ±0.005 ns/op > MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15 > 0.262 ±0.003 ns/op > MethodHandleProxiesAsIFInstanceCall.direct avgt 15 > 0.264 ±0.005 ns/op > MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15 >18.000 ±0.181 ns/op > MethodHandleProxiesAsIFInstanceCreate.createCallLambda ... Chen Liang 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 36 additional commits since the last revision: - Some changes per Mandy's review. SecurityManager test fails in this patch - Merge branch 'master' into explore/mhp-iface - Merge branch 'master' into explore/mhp-iface - Merge branch 'master' into explore/mhp-iface - stage, needs to fix is mh proxy instance check - Merge branch 'master' into explore/mhp-iface - Remove assertion, no longer true with teleport definition in MHP - Fix tabs, and tests about modules and constructor access - stupid faults - Revert to shared-class implementation, found a way to deal with security manager - ... and 26 more: https://git.openjdk.org/jdk/compare/7fb93a40...56a282e4 - Changes: - all: https://git.openjdk.org/jdk/pull/13197/files - new: https://git.openjdk.org/jdk/pull/13197/files/bd21e68e..56a282e4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13197=16 - incr: https://webrevs.openjdk.org/?repo=jdk=13197=15-16 Stats: 171579 lines in 3039 files changed: 128195 ins; 20981 del; 22403 mod Patch: https://git.openjdk.org/jdk/pull/13197.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13197/head:pull/13197 PR: https://git.openjdk.org/jdk/pull/13197
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 23:57:01 GMT, Jorn Vernee wrote: >> Also, should we say somewhere that, for prototype-less functions, >> `firstVariadicArg` should always have an index of 0 (e.g. any other value is >> illegal, trivially). It's not super important, just one of those little >> things that can reinforce understanding. > >> Perhaps if we used a bullet-list to define the two ways in which variadic >> function can be declared, the definition could stand out even more? > > Yes, I think this is a good idea. I'll do another pass. > >> Also, should we say somewhere that, for prototype-less functions, >> firstVariadicArg should always have an index of 0 > > I think this is also a good idea. It helps hammer the point home. Did another pass: https://github.com/openjdk/jdk/pull/14225/commits/e806fae5dc295726846d197dad59e6b0ba715ceb There are now 3 paragraphs in the javadoc: One that explains what a variadic function is (in C), one that explains how they are linked using specialized function descriptors, and finally one which explains the limitation on variadic argument layouts. Additionally I've switched the polarity of the check to be the same as it is in the doc, and aligned the exception message with the terminology in the doc. - PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r121331
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v3]
> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and > `float` is promoted to `double`, when being passed as variadic argument (see > e.g. > https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions). > This patch restricts the layouts that can be used as variadic layouts to > what is allowed by the C specification. > > The fallback linker is also updated to use to correct function to link > variadic calls (not doing this turned out not to be a problem so far, but it > is problematic for instance on Mac/AArch64 when using the fallback linker). > Adding the restriction on layouts for all linkers is also partly motivated by > the fallback linker rejecting such unsupported variadic layouts already. > > I've added a small paragraph to the Linker javadoc as well that explains the > restriction. Comments on that are welcome, but please explain. > > The tests are updated to no longer try to link variadic functions with the > illegal layouts, and I've added some more negative tests to TestIllegalLink. > > Testing: > - local testing on Windows/x64 > - tier1-3 + jdk-tier5 (ongoing) > - manual test run on mac/aarch64 with the fallback linker to verify the > correctness of the fallback linker changes. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Rework javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/14225/files - new: https://git.openjdk.org/jdk/pull/14225/files/9415e2a1..e806fae5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14225=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14225=01-02 Stats: 23 lines in 3 files changed: 10 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/14225.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14225/head:pull/14225 PR: https://git.openjdk.org/jdk/pull/14225
Re: RFR: 8308286 Fix clang warnings in linux code [v3]
On Wed, 31 May 2023 13:52:39 GMT, Weijun Wang wrote: >> Artem Semenov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update > > src/java.security.jgss/share/native/libj2gss/gssapi.h line 47: > >> 45: >> 46: // Condition was copied from >> 47: // >> Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/gssapi/gssapi.h > > In the current version of the file above, it's written as > > #if defined(__APPLE__) && (defined(__ppc__) ||... > > Is it better? done - PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1213297788
Re: RFR: 8308286 Fix clang warnings in linux code [v3]
On Thu, 1 Jun 2023 15:02:16 GMT, Artem Semenov wrote: >> src/java.security.jgss/share/native/libj2gss/gssapi.h line 47: >> >>> 45: >>> 46: // Condition was copied from >>> 47: // >>> Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/gssapi/gssapi.h >> >> In the current version of the file above, it's written as >> >> #if defined(__APPLE__) && (defined(__ppc__) ||... >> >> Is it better? > > done I didn't ask to revert the change. It's `s/TARGET_OS_MAC/defined(__APPLE__)/`. - PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1213300305
Re: RFR: 8309196: Remove Thread.countStackFrames
On Thu, 1 Jun 2023 06:40:04 GMT, Alan Bateman wrote: > Thread.countStackFrames is/was an ill-defined method that dates from JDK 1.0 > for counting the stack frames of a suspended Thread. The method was > deprecated in JDK 1.2 (1998), deprecated for removal in Java 9, and > re-specified/degraded to throw UOE unconditionally in Java 14. Java 22 would > be a fine time to finally remove this method. > > Code that wants to count stack frames should be directed to use > j.l.StackWalker, or if is a a tool, then it can use JVM TI and other tool > APIs. > > A corpus analysis of 30M classes in 131k artifacts from Maven Central found > only 1 usages to this method. The 1 usage appears to be a class that just > wraps all methods, it doesn't actually make use of it. Looks good. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14257#pullrequestreview-1455718528
Re: RFR: 8308286 Fix clang warnings in linux code [v4]
> When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. Artem Semenov has updated the pull request incrementally with one additional commit since the last revision: update - Changes: - all: https://git.openjdk.org/jdk/pull/14033/files - new: https://git.openjdk.org/jdk/pull/14033/files/d5b70cb2..b423bd4c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14033=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14033=02-03 Stats: 7 lines in 2 files changed: 1 ins; 4 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14033.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14033/head:pull/14033 PR: https://git.openjdk.org/jdk/pull/14033
RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s)
Classfile context object and multi-state options have been discussed at https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html This patch implements the proposed changes in Classfile API and fixes all affected code across JDK sources and tests. Please review. Thanks, Adam - Commit messages: - fixed snippets and added SnippetsTest - fixed javadoc - added Classfile context parameter to ClasRemapper::remapClass - records and methods implementations moved from Classfile to ClassfileImpl - added Classfile::buildTo override - implementation of Classfile::withOptions(Option... options) - added test for StackMapsOption.ALWAYS_GENERATE_STACK_MAPS - fixed tests - fixed options and jdk/classfile tests - fixed jdk/classfile tests - ... and 9 more: https://git.openjdk.org/jdk/compare/7d2a7ce2...bcdf0f11 Changes: https://git.openjdk.org/jdk/pull/14180/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14180=00 Issue: https://bugs.openjdk.org/browse/JDK-8308899 Stats: 1579 lines in 106 files changed: 478 ins; 195 del; 906 mod Patch: https://git.openjdk.org/jdk/pull/14180.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14180/head:pull/14180 PR: https://git.openjdk.org/jdk/pull/14180
Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s)
On Wed, 31 May 2023 05:42:58 GMT, Chen Liang wrote:
>> Classfile context object and multi-state options have been discussed at
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html
>> This patch implements the proposed changes in Classfile API and fixes all
>> affected code across JDK sources and tests.
>>
>> Please review.
>>
>> Thanks,
>> Adam
>
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 87:
>
>> 85: * @param r a function mapping attribute names to attribute mappers
>> 86: */
>> 87: record AttributeMapperOption(Function>
>> attributeMapper) implements Option {
>
> We might want to convert these options into interfaces and move the
> implementation records into ClassfileImpl, like Brian suggested about
> ClassHierarchyInfo.
Will fix it, thanks.
> src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 208:
>
>> 206: */
>> 207: default ClassModel parse(byte[] bytes) {
>> 208: return new ClassImpl(bytes, (ClassfileImpl)this);
>
> Should we implement this in ClassfileImpl to avoid the redundant cast?
yes, several methods should move to ClassfileImpl, thanks.
> src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java
> line 102:
>
>> 100: * @return re-mapped class file bytes
>> 101: */
>> 102: default byte[] remapClass(ClassModel clm) {
>
> This api should ask for a Classfile object instead. Thankfully, the new
> Classfile object actually allows us to find potentially bugs in our code (say
> if the remapped class refer to declared types outside of system class loader)
yes, I'll add the parameter, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1212084790
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1212094893
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r1212117128
Re: RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s)
On Fri, 26 May 2023 14:56:57 GMT, Adam Sotona wrote:
> Classfile context object and multi-state options have been discussed at
> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html
> This patch implements the proposed changes in Classfile API and fixes all
> affected code across JDK sources and tests.
>
> Please review.
>
> Thanks,
> Adam
A side comment about the API changes:
I think we should recommend users to store Classfile instances in static final
variables if sharing is reasonable, like in Module and BindingSpecializer; this
way, we can cache class hierarchy resolution information across builds.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 87:
> 85: * @param r a function mapping attribute names to attribute mappers
> 86: */
> 87: record AttributeMapperOption(Function>
> attributeMapper) implements Option {
We might want to convert these options into interfaces and move the
implementation records into ClassfileImpl, like Brian suggested about
ClassHierarchyInfo.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 208:
> 206: */
> 207: default ClassModel parse(byte[] bytes) {
> 208: return new ClassImpl(bytes, (ClassfileImpl)this);
Should we implement this in ClassfileImpl to avoid the redundant cast?
src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java
line 102:
> 100: * @return re-mapped class file bytes
> 101: */
> 102: default byte[] remapClass(ClassModel clm) {
This api should ask for a Classfile object instead. Thankfully, the new
Classfile object actually allows us to find potentially bugs in our code (say
if the remapped class refer to declared types outside of system class loader)
-
PR Review: https://git.openjdk.org/jdk/pull/14180#pullrequestreview-1452244549
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r123876
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r124779
PR Review Comment: https://git.openjdk.org/jdk/pull/14180#discussion_r126312
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview) [v4]
> This is the implementation of: > > - JEP 453: Structured Concurrency (Preview) > - JEP 446: Scoped Values (Preview) > > For the most part, this is just moving code and tests. StructuredTaskScope > moves to j.u.concurrent as a preview API, ScopedValue moves to j.lang as a > preview API, and module jdk.incubator.concurrent has been removed. The > significant API changes since incubator are: > > - StructuredTaskScope.fork returns Subtask instead of Future (JEP 453 has a > section on this) > - ScopedValue.where methods are replaced with runWhere, callWhere and getWhere Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Sync up from loom repo - Merge - Sync with loom repo, re-work ScopedValue class description - Sync up from loom repo - Remove csm.Threads - Merge - Test should not be in update for main line - Sync with loom repo - Sync up tests frmo loom repo - Sync up with loom repo - ... and 5 more: https://git.openjdk.org/jdk/compare/a46b5acc...cc902ce6 - Changes: https://git.openjdk.org/jdk/pull/13932/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13932=03 Stats: 9267 lines in 40 files changed: 4880 ins; 4325 del; 62 mod Patch: https://git.openjdk.org/jdk/pull/13932.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13932/head:pull/13932 PR: https://git.openjdk.org/jdk/pull/13932
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore
wrote:
>> As the FFM API evolved over time, some parts of the javadoc went out of
>> sync. Now that most of the API is stable, it is a good time to look again at
>> the javadoc as a whole, and bring some more consistency.
>>
>> While most of the changes in this PR are stylistic, I'd like to call out few
>> things which resulted in API changes:
>>
>> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified
>> requirement that its alignment parameter must be a power of two.
>>
>> * `MemoryLayout::sliceHandle` did not require the absence of dereference
>> paths in the provided layout path. While that is technically possible to
>> allow, currently the method is specified as returning a "slice"
>> corresponding to some "nested layout", so following pointers seems
>> incompatible with the spec. I have decided to disallow for now - we can
>> always compatibly enable it later, if required.
>>
>> * `MemorySegment::copy` - most of the overloads did not specify that
>> `UnsupportedOperationException` is thrown if the destination segment is
>> read-only.
>>
>> * In several places, an extra `@throws IllegalArgumentException` or `@throws
>> IllegalArgumentException` has been added to capture cases where element size
>> * index computation can overflow. This is true for all the element-wise
>> segment copy methods, for the indexed accessors in memory segment (e.g.
>> `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for
>> `SegmentAllocator::allocateArray(MemoryLayout, long)`.
>>
>> In all these cases (except for overflows), new tests have been added to
>> cover the additional API changes (a CSR will also be filed to cover these).
>>
>> The class with most changes is `MemoryLayout`. I realized that the javadoc
>> there was a bit sloppy around the definition of "layout paths". Now there
>> are several subsection in the class javadoc, which explain how different
>> kinds of paths can be used. I have introduced the notion of "open path
>> elements" to denote those path elements that are not fully specified, and
>> result in additional var handle coordinates to be added. There is also now a
>> definition of what it means for a layout path to be "well-formed", so that
>> all methods accepting a layout path can simply refer to it (this definition
>> is tricky to give inline in the javadoc of the various path-accepting
>> methods, as it depends on many factors).
>>
>> Another biggie change was in `MemorySegment` as now all dereference methods
>> state precisely their spatial checks requirements, as well also specifying
>> when the API can th...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix wrong link in layout well-formedness doc
src/java.base/share/classes/java/lang/foreign/AddressLayout.java line 116:
> 114: /**
> 115: * Returns an address layout with the same carrier, alignment
> constraint, name and order as this address layout,
> 116: * but with no target layout
Did you mean to drop the period from the end of the sentence?
src/java.base/share/classes/java/lang/foreign/Arena.java line 269:
> 267: * @throws IllegalStateException if this arena has already been
> {@linkplain #close() closed}.
> 268: * @throws WrongThreadException if this arena is confined, and this
> method is called from a thread {@code T}
> 269: * other than the arena's owner thread.
"thread T" hints that "T" will be used later, maybe it's not needed.
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 38:
> 36: /**
> 37: * A function descriptor models the signature of a foreign function. A
> function descriptor is made up of zero or more
> 38: * argument layouts and zero or one return layout. A function descriptor
> is used to create
You might want want to put a comma after "layouts" to avoid any ambiguity.
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 57:
> 55:
> 56: /**
> 57: * {@return the argument layouts of this function descriptor (as an
> immutable list)}.
I assume this should be "unmodifiable" rather than immutable here.
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 127:
> 125: /**
> 126: * Creates a function descriptor with the given argument layouts and
> no return layout. This is useful to model functions
> 127: * which return no values.
I think I would use "that return" rather than "which return" here.
src/java.base/share/classes/java/lang/foreign/Linker.java line 356:
> 354: * attach the segment to some existing {@linkplain Arena arena}, so that
> the lifetime of the region of memory
> 355: * backing the segment can be managed automatically, as for any other
> native segment created directly from Java code.
> 356: * Both these operations are accomplished using the
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 12:59:14 GMT, Brett Okken wrote: >> Aleksey Shipilev has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains 13 commits: >> >> - Revert test changes >> - Merge branch 'master' into JDK-8308804-uuid-buffers >> - Simplify clinit >> - Remove the properties >> - Swap lsb/msb >> - Fine-tune exceptions >> - Handle privileged properties >> - Use ByteArray to convert. Do version/variant preparations straight on >> locals. Move init out of optimistic lock section. >> - More touchups >> - Comment updates >> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a > > src/java.base/share/classes/java/util/UUID.java line 234: > >> 232: long msb = ByteArray.getLong(buf, 0); >> 233: long lsb = ByteArray.getLong(buf, 8); >> 234: return fromRandom(msb, lsb); > > is there any value in moving this outside of the critical section? I have tried it before, and it gives no benefit, but significantly complicates the code. This is the part under write lock, where we have spent a significant time refilling the buffer. The little improvement we might get here is drowning in those costs. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1213121408
Withdrawn: JDK-8308288: Fix xlc17 clang warnings in shared code
On Thu, 25 May 2023 09:14:14 GMT, JoKern65 wrote: > When using the new xlc17 compiler (based on a recent clang) to build OpenJDk > on AIX , we run into various "warnings as errors". > Some of those are in shared codebase and could be addressed by small > adjustments. > A lot of those changes are in hotspot, some might be somewhere else in the > OpenJDK C/C++ code. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14146
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v16]
On Thu, 1 Jun 2023 12:55:28 GMT, Alan Bateman wrote: >> Yes. There is some juggling going on to get the code to align with the >> requirement and the interpretation thereof. > >> Yes. There is some juggling going on to get the code to align with the >> requirement and the interpretation thereof. > > Is this stable now, specifically the "Selecting a main method" section and > the behavior change where an instance main takes precedence over an > "inherited" static main? Just checking before doing another pass over the > method finder. I think so - additional changes will be coming from Joe via https://github.com/openjdk/jdk/pull/14165 - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1213132645
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ Hi, As this PR is big and spans several components I split off the java.base, java.desktop and the sercivability/security issues into extra JBS issues. https://bugs.openjdk.org/browse/JDK-8309219 Fix xlc17 clang 1.5 warnings in java.base https://bugs.openjdk.org/browse/JDK-8309224 Fix xlc17 clang 1.5 warnings in java.desktop https://bugs.openjdk.org/browse/JDK-8309225 Fix xlc17 clang 1.5 warnings in security and servicability I’ll move the changes from this pull request into new pull requests. I will incorporate the requested changes right in the new PRs. I will reuse this issue 8308388 for the hotspot changes but come up with a new, smaller PR. @colleenp, I will move alloca.h to the globalDefinitions_xlc.hpp. @prrace, I will come up with an identical PR for the client files (java.desktop), but improve the comment as @kimbarrett proposed @mbaesken, I will use AIX and take up some of the other fixes you proposed. I guess we need to find a way to fix the issue with the malloc in globalDefinitions_xlc.hpp in the upcoming PR for hotspot. Thanks for your help so far! Hi, As this PR is big and spans several components I split off the java.base, java.desktop and the sercivability/security issues into extra JBS issues. https://bugs.openjdk.org/browse/JDK-8309219 Fix xlc17 clang 1.5 warnings in java.base https://bugs.openjdk.org/browse/JDK-8309224 Fix xlc17 clang 1.5 warnings in java.desktop https://bugs.openjdk.org/browse/JDK-8309225 Fix xlc17 clang 1.5 warnings in security and servicability I’ll move the changes from this pull request into new pull requests. I will incorporate the requested changes right in the new PRs. I will reuse this issue 8308388 for the hotspot changes but come up with a new, smaller PR. @colleenp, I will move alloca.h to the globalDefinitions_xlc.hpp. @prrace, I will come up with an identical PR for the client files (java.desktop), but improve the comment as @kimbarrett proposed @MBaesken, I will use AIX and take up some of the other fixes you proposed. I guess we need to find a way to fix the issue with the malloc in globalDefinitions_xlc.hpp in the upcoming PR for hotspot. Thanks for your help so far! - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1572023812 PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1572024628
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
On Thu, 1 Jun 2023 09:37:29 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 13 commits:
>
> - Revert test changes
> - Merge branch 'master' into JDK-8308804-uuid-buffers
> - Simplify clinit
> - Remove the properties
> - Swap lsb/msb
> - Fine-tune exceptions
> - Handle privileged properties
> - Use ByteArray to convert. Do version/variant preparations straight on
> locals. Move init out of optimistic lock section.
> - More touchups
> - Comment updates
> - ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
src/java.base/share/classes/java/util/UUID.java line 234:
> 232: long msb = ByteArray.getLong(buf, 0);
> 233: long lsb = ByteArray.getLong(buf, 8);
> 234: return fromRandom(msb, lsb);
is there any value in moving this outside of the critical section?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1213117598
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v16]
On Tue, 23 May 2023 18:09:03 GMT, Jim Laskey wrote: > Yes. There is some juggling going on to get the code to align with the > requirement and the interpretation thereof. Is this stable now, specifically the "Selecting a main method" section and the behavior change where an instance main takes precedence over an "inherited" static main? Just checking before doing another pass over the method finder. - PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1213112842
Re: RFR: 8303530: Redefine JAXP Configuration File [v15]
On Wed, 31 May 2023 21:58:44 GMT, Joe Wang wrote: >> Add a system property, jdk.xml.config.file, to return the path to a custom >> JAXP configuration file. The current configuration file, jaxp.properties, >> that the JDK supports will become the default configuration file. >> >> CSR: https://bugs.openjdk.org/browse/JDK-8303531 >> >> Tests: XML SQE and JCK tests passed. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > adjust javadoc I'm very happy where the spec changes have got to in this PR, esp. the module description and allowing a config file be specified with all configuration. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12985#pullrequestreview-1455379216
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v26]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Missing Preview feature enum - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/4bfb8020..850a2152 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=25 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=24-25 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
RFR: 8309196: Remove Thread.countStackFrames
Thread.countStackFrames is/was an ill-defined method that dates from JDK 1.0 for counting the stack frames of a suspended Thread. The method was deprecated in JDK 1.2 (1998), deprecated for removal in Java 9, and re-specified/degraded to throw UOE unconditionally in Java 14. Java 22 would be a fine time to finally remove this method. Code that wants to count stack frames should be directed to use j.l.StackWalker, or if is a a tool, then it can use JVM TI and other tool APIs. A corpus analysis of 30M classes in 131k artifacts from Maven Central found only 1 usages to this method. The 1 usage appears to be a class that just wraps all methods, it doesn't actually make use of it. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/14257/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14257=00 Issue: https://bugs.openjdk.org/browse/JDK-8309196 Stats: 20 lines in 2 files changed: 0 ins; 17 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14257.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14257/head:pull/14257 PR: https://git.openjdk.org/jdk/pull/14257
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v25]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Restrict access to unnamed class members when doing separate compilation. - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/d0189fc2..4bfb8020 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=24 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=23-24 Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Thu, 1 Jun 2023 12:01:15 GMT, Severin Gehwolf wrote: > It's easy to introduce a new change to this code which accidentally drags in > some of those (unwanted) dependencies again. I do not like the changes proposed here, they are all crying out for several round cleanups and modernization. One thing that would help is to split this up into a series of changes that could be evaluated, e.g. the use of regex may be a significant part of this so maybe start with that, report back, then work through the iterations to make it clean and maintainable. - PR Comment: https://git.openjdk.org/jdk/pull/14216#issuecomment-1571939629
Re: RFR: 8308780: Fix the Java Integer types on Windows [v4]
On Thu, 1 Jun 2023 11:49:24 GMT, Julian Waters wrote: >> On Windows, the basic Java Integer types are defined as long and __int64 >> respectively. In particular, the former is rather problematic since it >> breaks compilation as the Visual C++ becomes stricter and more compliant >> with every release, which means the way Windows code treats long as a >> typedef for int is no longer correct, especially with -permissive- enabled. >> Instead of changing every piece of broken code to match the jint = long >> typedef, which is far too time consuming, we can instead change jint to an >> int (which is still the same 32 bit number type as long), as there are far >> fewer problems caused by this definition. It's better to get this over and >> done with sooner than later when a future version of Visual C++ finally >> starts to break on existing code > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Fix the code that is actually warning While looking through the code for this I've come to realize that a staggering amount of code in the accessibility binaries specify longs where unsigned longs would be much more appropriate (see the one example in this PR for instance), wonder if this should also be fixed in the long term too - PR Comment: https://git.openjdk.org/jdk/pull/14125#issuecomment-1571912910
Integrated: 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem
On Thu, 25 May 2023 12:24:59 GMT, Adam Sotona wrote: > Fix of jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo returning correct > zero power of two. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: a6109bf1 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/a6109bf1ea2acbebd6a3517813c0b82fdba00c2f Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem Reviewed-by: jlahoda - PR: https://git.openjdk.org/jdk/pull/14148
Re: RFR: 8308780: Fix the Java Integer types on Windows [v4]
> On Windows, the basic Java Integer types are defined as long and __int64 > respectively. In particular, the former is rather problematic since it breaks > compilation as the Visual C++ becomes stricter and more compliant with every > release, which means the way Windows code treats long as a typedef for int is > no longer correct, especially with -permissive- enabled. Instead of changing > every piece of broken code to match the jint = long typedef, which is far too > time consuming, we can instead change jint to an int (which is still the same > 32 bit number type as long), as there are far fewer problems caused by this > definition. It's better to get this over and done with sooner than later when > a future version of Visual C++ finally starts to break on existing code Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Fix the code that is actually warning - Changes: - all: https://git.openjdk.org/jdk/pull/14125/files - new: https://git.openjdk.org/jdk/pull/14125/files/29b93688..5fa2d3eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14125=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14125=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14125.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14125/head:pull/14125 PR: https://git.openjdk.org/jdk/pull/14125
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
On Thu, 1 Jun 2023 10:55:26 GMT, Volker Simonis wrote: >>> > Sorry, but I don't understand this argument. If we do a short read we >>> > will work with corrupted ChildStuff and SpawnInfo >>> > structures. This can in the extreme case execute arbitrary code (e.g. if >>> > ChildStuff.argv is not fully read from the parent). You are >>> > basically saying it is better to work on corrupted data rather than >>> > reporting an error. >>> >>> No I am simply pointing out that this has changed more than just the issue >>> with close. And maybe a short-read does indicate data "corruption" and >>> maybe it should be a fatal error. But I don't know exactly how this might >>> manifest so perhaps there are benign short-reads that actually do happen. >>> Regardless it might be better to split this part out and focus on the close >>> issue here. >> >> Given the purpose and implementation of the `readFully` function, I don't >> see how it can return anything other than an error or the full requested >> read length. > > Thanks @RogerRiggs and @tstuefe for your help and patience with this PR. I've > now updated the [Release Note](https://bugs.openjdk.org/browse/JDK-8308297) > for this change to also include the error scenario described by @mlichtblau . Hey @simonis, thank you for taking the time to include our problem in the fix. I appreciate the detailed explanation of your fix! - PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1571841021
Integrated: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
On Fri, 12 May 2023 15:24:19 GMT, Volker Simonis wrote:
> Since JDK13, executing commands in a sub-process defaults to the so called
> `POSIX_SPAWN` launching mechanism (i.e.
> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
> using `posix_spawn(3)` to firstly start a tiny helper program called
> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
> command data from the parent Java process over a Unix pipe and finally
> executes (i.e. `execvp(3)`) the requested command.
>
> In cases where the parent process terminates abnormally before `jspawnhelper`
> has read all the expected data from the pipe, `jspawnhelper` will block
> indefinitely on the reading end of the pipe. This is especially harmful if
> the parent process had open sockets, because in that case, the forked
> `jspawnhelper` process will inherit them and keep all the corresponding ports
> open effectively preventing other processes to bind to them. Notice that this
> is not an abstract scenario. We've observed this regularly in production with
> services which couldn't be restarted after a crash after migrating to JDK 17.
>
> The fix of the issue is rather trivial. `jspawnhelper` has to close its
> writing end of the pipe which connects it with the parent Java process
> *before* starting to read from that pipe such that reads from the pipe can
> immediately return with EOF if the parent process terminates abnormally.
>
> Also did some cleanup:
> - in `jspawnhelper.c` correctly chek the result of `readFully()`
> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
> write the command data from the parent process to `jspawnhelper`
> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
> that's long gone.
This pull request has now been integrated.
Changeset: 39f6d807
Author:Volker Simonis
URL:
https://git.openjdk.org/jdk/commit/39f6d807dbbe0c9ecd72fe2f76bfbc3091b43c80
Stats: 389 lines in 5 files changed: 351 ins; 11 del; 27 mod
8307990: jspawnhelper must close its writing side of a pipe before reading from
it
Reviewed-by: stuefe, rriggs
-
PR: https://git.openjdk.org/jdk/pull/13956
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
On Fri, 19 May 2023 15:43:30 GMT, Roger Riggs wrote: >>> Sorry, but I don't understand this argument. If we do a short read we will >>> work with corrupted ChildStuff and SpawnInfo >>> structures. This can in the extreme case execute arbitrary code (e.g. if >>> ChildStuff.argv is not fully read from the parent). You are >>> basically saying it is better to work on corrupted data rather than >>> reporting an error. >> >> No I am simply pointing out that this has changed more than just the issue >> with close. And maybe a short-read does indicate data "corruption" and maybe >> it should be a fatal error. But I don't know exactly how this might manifest >> so perhaps there are benign short-reads that actually do happen. Regardless >> it might be better to split this part out and focus on the close issue here. > >> > Sorry, but I don't understand this argument. If we do a short read we will >> > work with corrupted ChildStuff and SpawnInfo >> > structures. This can in the extreme case execute arbitrary code (e.g. if >> > ChildStuff.argv is not fully read from the parent). You are >> > basically saying it is better to work on corrupted data rather than >> > reporting an error. >> >> No I am simply pointing out that this has changed more than just the issue >> with close. And maybe a short-read does indicate data "corruption" and maybe >> it should be a fatal error. But I don't know exactly how this might manifest >> so perhaps there are benign short-reads that actually do happen. Regardless >> it might be better to split this part out and focus on the close issue here. > > Given the purpose and implementation of the `readFully` function, I don't see > how it can return anything other than an error or the full requested read > length. Thanks @RogerRiggs and @tstuefe for your help and patience with this PR. I've now updated the [Release Note](https://bugs.openjdk.org/browse/JDK-8308297) for this change to also include the error scenario described by @mlichtblau . - PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1571822491
Re: RFR: 8308856: jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo math problem [v2]
On Mon, 29 May 2023 08:05:23 GMT, Adam Sotona wrote: >> Fix of jdk.internal.classfile.impl.EntryMap::nextPowerOfTwo returning >> correct zero power of two. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java > > Co-authored-by: Andrey Turbanov Looks OK to me. - Marked as reviewed by jlahoda (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14148#pullrequestreview-1455145211
Re: RFR: 8309191: Reduce JDK dependencies of cgroup support
On Tue, 30 May 2023 13:03:27 GMT, Aleksandar Pejović wrote:
> The current code for cgroup support in the JDK has large and expensive
> dependencies: it uses NIO, streams, and regular expressions. This leads to
> unnecessary class loading and slows down startup, especially when the code is
> executed early during an application startup. This is especially a problem
> for GraalVM, which executes this code during VM startup.
>
> This PR reduces the dependencies:
> - NIO is replaced with regular `java.io` for file access.
> - Streams are replaced with loops (a side effect of this is that files are
> read in full whereas previously they could be read up to a certain point,
> e.g., until a match is found).
> - Regular expressions are replaced with manual tokenization (and for usages
> of `String.split`, the "regex" is changed to single characters for which
> `String.split` has a fast-path implementation that avoids the regular
> expression engine).
I'm concerned about the hard-coding of delimiter values and the added
accidential complexity in order to avoid the Regex engine. Note that this test
fails due to the delimiter hard-coding:
jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java
This change seems hard to maintain. How would you ensure this won't regress?
src/java.base/linux/classes/jdk/internal/platform/CgroupInfo.java line 110:
> 108: */
> 109: static CgroupInfo fromCgroupsLine(String line) {
> 110: String[] tokens = line.split("\t");
With this change, we now hard-code the expected delimiter and, thus, depend on
what the kernel does. Do we have sufficient evidence this hasn't changed/won't
change in the future?
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
line 97:
> 95: Logger logger = System.getLogger("jdk.internal.platform");
> 96: logger.log(Level.DEBUG, msg);
> 97: }
This seems fine and uncontroversial. Suggested name change `log` over `warn`.
Perhaps apply this as a separate change?
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
line 347:
> 345: int separatorOrdinal = -1;
> 346: // loop over space-separated tokens
> 347: for (int tOrdinal = 1, tStart = 0, tEnd = line.indexOf(' ');
> tEnd != -1; tOrdinal++, tStart = tEnd + 1, tEnd = line.indexOf(' ', tStart)) {
AFAIK, this now also hard-codes the delimiter: A single space. If we really
want this custom parser, please add a unit test for it and extract it to a
separate class.
A hypothetical line like the following would confuse the parser, setting
ordinal to wrong values:
36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - cgroup /dev/root
rw,errors=continue
We'd have: `mountRoot = 98:0`, `mountPath = /mnt1`, `fsType = cgroup` since it
expects single space separated values. Seems fragile.
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java
line 109:
> 107: }
> 108:
> 109: public static long convertHierachicalLimitLine(String line) {
Pre-existing: typo `convertHierarchicalLimitLine`
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java
line 110:
> 108:
> 109: public static long convertHierachicalLimitLine(String line) {
> 110: String[] tokens = line.split(" ");
Again, assumes single space (` `) delimited entries in `memory.stat`. I'm not
sure we should hard-code this.
src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
line 144:
> 142: }
> 143: // $MAX $PERIOD
> 144: String[] tokens = cpuMaxRaw.split(" ");
This seems OK. According to
https://docs.kernel.org/admin-guide/cgroup-v2.html#format
src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
line 362:
> 360: return Long.valueOf(0);
> 361: }
> 362: String[] tokens = line.split(" ");
This seems OK. According to
https://docs.kernel.org/admin-guide/cgroup-v2.html#format
-
PR Review: https://git.openjdk.org/jdk/pull/14216#pullrequestreview-1454929344
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212816038
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212909205
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212877800
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212882937
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212891278
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212906805
PR Review Comment: https://git.openjdk.org/jdk/pull/14216#discussion_r1212907029
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v7]
> UUID is the very important class that is used to track identities of objects
> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
> of total CPU time, and is frequently a scalability bottleneck due to
> `SecureRandom` synchronization.
>
> The major issue with UUID code itself is that it reads from the single
> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
> is bashed with very small requests. This also has a chilling effect on other
> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>
> We can improve this by doing the bulk reads from the backing SecureRandom and
> possibly striping the reads across many instances of it.
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>
> # After
> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive scaling,
> ~1.5x
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative scaling
>
> # After
> BenchmarkMode Cnt Score Error Units
> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
> scaling, ~1.2x
>
>
> Note that there is still a scalability bottleneck in current default random
> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
> out the synchronization story there. The scalability fix in current default
> `SecureRandom` would be much more intrusive and risky, since it would change
> a core crypto class with unknown bug fanout.
>
> Using the bulk reads even when the underlying PRNG is heavily synchronized is
> still a win. A more scalable PRNG would benefit from this as well. This PR
> adds a system property to select the PRNG implementation, and there we can
> clearly see the benefit with more scalable PRNG sources:
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before, hacked `new SecureRandom()` to
> `SecureRandom.getInstance("SHA1PRNG")`
> UUIDRandomBench.single thrpt 15 3.661 ± 0.008 ops/us
> UUIDRandomBench...
Aleksey Shipilev has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 13 commits:
- Revert test changes
- Merge branch 'master' into JDK-8308804-uuid-buffers
- Simplify clinit
- Remove the properties
- Swap lsb/msb
- Fine-tune exceptions
- Handle privileged properties
- Use ByteArray to convert. Do version/variant preparations straight on
locals. Move init out of optimistic lock section.
- More touchups
- Comment updates
- ... and 3 more: https://git.openjdk.org/jdk/compare/4460429d...fd7eaa1a
-
Changes: https://git.openjdk.org/jdk/pull/14135/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=14135=06
Stats: 197 lines in 2 files changed: 186 ins; 8 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/14135.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14135/head:pull/14135
PR: https://git.openjdk.org/jdk/pull/14135
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v8]
On Tue, 30 May 2023 13:19:37 GMT, Volker Simonis wrote:
>> Since JDK13, executing commands in a sub-process defaults to the so called
>> `POSIX_SPAWN` launching mechanism (i.e.
>> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
>> using `posix_spawn(3)` to firstly start a tiny helper program called
>> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
>> command data from the parent Java process over a Unix pipe and finally
>> executes (i.e. `execvp(3)`) the requested command.
>>
>> In cases where the parent process terminates abnormally before
>> `jspawnhelper` has read all the expected data from the pipe, `jspawnhelper`
>> will block indefinitely on the reading end of the pipe. This is especially
>> harmful if the parent process had open sockets, because in that case, the
>> forked `jspawnhelper` process will inherit them and keep all the
>> corresponding ports open effectively preventing other processes to bind to
>> them. Notice that this is not an abstract scenario. We've observed this
>> regularly in production with services which couldn't be restarted after a
>> crash after migrating to JDK 17.
>>
>> The fix of the issue is rather trivial. `jspawnhelper` has to close its
>> writing end of the pipe which connects it with the parent Java process
>> *before* starting to read from that pipe such that reads from the pipe can
>> immediately return with EOF if the parent process terminates abnormally.
>>
>> Also did some cleanup:
>> - in `jspawnhelper.c` correctly chek the result of `readFully()`
>> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
>> write the command data from the parent process to `jspawnhelper`
>> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
>> that's long gone.
>
> Volker Simonis has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Enable JspawnhelperProtocol test on AIX as well because AIX also uses
> posix_spawn
I gave this a cursory eye, unfortunately i'm too snowed in atm for more. It
looks good to me, your reasoning makes sense. Very good job finding and fixing
this, and describing the issue.
About FD_Cloexec , thus can be done in a later change.
-
Marked as reviewed by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13956#pullrequestreview-1454968540
Re: RFR: 8308452: Extend internal Architecture enum with byte order and address size [v4]
On Wed, 31 May 2023 18:25:18 GMT, Roger Riggs wrote: >> The internal enum jdk.internal.util.Architecture does not provide >> information about the big or little endianness or the address size (64 or 32 >> bits). The endian-ness and address size are intrinsic to the architecture. >> >> The values of the enum are extended to separately identify the big endian >> and little-endian uses of the ISA. >> For example, `PPC64` and `PPC64LE` for the big and little-endian versions. >> The enum values directly reflect the build-time artifacts and resulting >> executables. >> >> This information about an architecture will make the enum more useful >> especially to identify a target platform in a cross-platform use case. A >> method is added to map well known aliases for the platforms to the >> Architecture enum. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove duplicate import Thanks for the updates! LGTM. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14063#pullrequestreview-1454964630
Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently [v3]
On Thu, 1 Jun 2023 08:58:23 GMT, SUN Guoyun wrote:
>> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
>> TEST=gc/TestAllocHumongousFragment.java
>> error info:
>>
>> Caused by: java.lang.NullPointerException: Cannot invoke
>> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
>> at
>> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
>> ... 24 more
>>
>> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
>> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
>> (LocaleObjectCache uses SoftReferences, used by printf method called in
>> getRandomInstance(Utils.java:511)).
>>
>> Maybe we have to deal with the case where the getBaseLocale() return value
>> is null. the call stack is:
>>
>> at
>> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
>> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
>> at
>> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
>> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>>
>> in LocaleObjectCache.java:64
>>
>> 62 if (key == null || newVal == null) {
>>
>> 63 // subclass must return non-null key/value object
>>
>> 64 return null; // run here
>> 65 }
>
> SUN Guoyun has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale
> cache being cleared
I've done basic testing jtreg tier1-3 and partial shenandoah testing with
vmoptions `-Xcomp -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=aggressive
-XX:+ShenandoahOOMDuringEvacALot`, the results are all OK.
-
PR Comment: https://git.openjdk.org/jdk/pull/14211#issuecomment-1571644464
Integrated: 8308803: Improve java/util/UUID/UUIDTest.java
On Wed, 24 May 2023 19:18:33 GMT, Aleksey Shipilev wrote: > UUID is very important class that is used to track identities of objects in > large scale systems. Yet, the coverage in JDK test is disappointing: it tests > only 100 of UUID instances per test, which is way too small to detect > collisions due to the bad randomness for example. > > I have some pending work to improve UUID performance, and tests should be > improved. > > The improved test still runs in less than 5 seconds on my laptop. This pull request has now been integrated. Changeset: 4460429d Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/4460429d7a50b9a7a99058ef4e5ae36fb30b956f Stats: 187 lines in 1 file changed: 107 ins; 0 del; 80 mod 8308803: Improve java/util/UUID/UUIDTest.java Reviewed-by: jpai, rriggs - PR: https://git.openjdk.org/jdk/pull/14134
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java [v3]
On Wed, 31 May 2023 14:53:09 GMT, Aleksey Shipilev wrote: >> UUID is very important class that is used to track identities of objects in >> large scale systems. Yet, the coverage in JDK test is disappointing: it >> tests only 100 of UUID instances per test, which is way too small to detect >> collisions due to the bad randomness for example. >> >> I have some pending work to improve UUID performance, and tests should be >> improved. >> >> The improved test still runs in less than 5 seconds on my laptop. > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Collect collisions and report them once at the end Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14134#issuecomment-1571630408
Re: RFR: 8289220: [Shenandoah] TestAllocObjectArrays fails intermittently [v3]
> command: make test CONF=fastdebug JTREG="VM_OPTIONS=-Xcomp"
> TEST=gc/TestAllocHumongousFragment.java
> error info:
>
> Caused by: java.lang.NullPointerException: Cannot invoke
> "sun.util.locale.BaseLocale.getVariant()" because "base" is null
> at java.base/java.util.Locale.forLanguageTag(Locale.java:1802)
> at
> java.base/sun.util.cldr.CLDRBaseLocaleDataMetaInfo.(CLDRBaseLocaleDataMetaInfo.java:41)
> ... 24 more
>
> Note that the test runs with -XX:ShenandoahGCHeuristics=aggressive
> -XX:+ShenandoahOOMDuringEvacALot and SoftReferences are involved
> (LocaleObjectCache uses SoftReferences, used by printf method called in
> getRandomInstance(Utils.java:511)).
>
> Maybe we have to deal with the case where the getBaseLocale() return value is
> null. the call stack is:
>
> at
> java.base/sun.util.locale.LocaleObjectCache.get(LocaleObjectCache.java:64)
> at java.base/sun.util.locale.BaseLocale.getInstance(BaseLocale.java:169)
> at
> java.base/sun.util.locale.InternalLocaleBuilder.getBaseLocale(InternalLocaleBuilder.java:524)
> at java.base/java.util.Locale.forLanguageTag(Locale.java:1874)
>
> in LocaleObjectCache.java:64
>
>62 if (key == null || newVal == null) {
>
>63 // subclass must return non-null key/value object
>
>64 return null; // run here
>65 }
SUN Guoyun has refreshed the contents of this pull request, and previous
commits have been removed. The incremental views will show differences compared
to the previous content of the PR. The pull request contains one new commit
since the last revision:
8289220: Locale.forLanguageTag throws NPE due to soft ref used in locale
cache being cleared
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14211/files
- new: https://git.openjdk.org/jdk/pull/14211/files/51883706..a09f1b6f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14211=02
- incr: https://webrevs.openjdk.org/?repo=jdk=14211=01-02
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/14211.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14211/head:pull/14211
PR: https://git.openjdk.org/jdk/pull/14211
Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]
On Thu, 1 Jun 2023 02:16:12 GMT, David Holmes wrote: >> Anyone willing to review this? > > @jerboaa I can't really review the tests themselves but will run through our > CI to see if they cause any problems. If not then they should be okay to add. Thanks @dholmes-ora for running them through your CI. - PR Comment: https://git.openjdk.org/jdk/pull/14090#issuecomment-1571584032
Re: RFR: 8308780: Fix the Java Integer types on Windows [v3]
> On Windows, the basic Java Integer types are defined as long and __int64 > respectively. In particular, the former is rather problematic since it breaks > compilation as the Visual C++ becomes stricter and more compliant with every > release, which means the way Windows code treats long as a typedef for int is > no longer correct, especially with -permissive- enabled. Instead of changing > every piece of broken code to match the jint = long typedef, which is far too > time consuming, we can instead change jint to an int (which is still the same > 32 bit number type as long), as there are far fewer problems caused by this > definition. It's better to get this over and done with sooner than later when > a future version of Visual C++ finally starts to break on existing code Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Nevermind - Changes: - all: https://git.openjdk.org/jdk/pull/14125/files - new: https://git.openjdk.org/jdk/pull/14125/files/628be6b2..29b93688 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14125=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14125=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14125.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14125/head:pull/14125 PR: https://git.openjdk.org/jdk/pull/14125
