Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
On Wed, 24 May 2023 02:29:09 GMT, Chen Liang wrote: >> This patch moves the parameters to an immutable list, to avoid allocations >> on `parameterList` as well. In addition, it fixes 8304932, the bug where if >> a caller keeps a reference to the array passed into `MethodTypeDesc.of`, the >> caller may mutate the Desc via the array and can create invalid >> MethodTypeDesc. >> >> The latest benchmark results are available here: >> https://github.com/openjdk/jdk/pull/13186#issuecomment-1560378822 >> >> This patch has minor performance gains on `ofDescriptor` factory, even >> compared to Adam's patch that optimized `ofDescriptor` in #12945. >> >> Benchmark of Oracle JDK 20: >> https://gist.github.com/683c6219e183cbc2b336224fc2c0d50a >> Benchmark of this patch: >> https://gist.github.com/22be9371a2370fb4a7b44f1684750ec4 >> Benchmark of [asotona's >> patch](https://github.com/openjdk/jdk/pull/12945/files#diff-ac8e413d3e13532a2b0d34a90253c6ddd7a4f04082f792b9d076e9b5a33f2078): >> https://gist.github.com/eb98579c3b51cafae481049a95a78f80 >> >> [sotona vs >> this](https://jmh.morethan.io/?gists=eb98579c3b51cafae481049a95a78f80,22be9371a2370fb4a7b44f1684750ec4); >> [20 vs >> this](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,22be9371a2370fb4a7b44f1684750ec4); >> [20 vs >> sotona](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,eb98579c3b51cafae481049a95a78f80), >> for reference > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > reuse immutable list to avoid array allocation Now updated to the latest MethodTypeDesc.of factories, this patch is ready for review and integration. Below is a few performance information about this patch. This patch: Benchmark (descString) Mode Cnt Score Error Units MethodTypeDescDescriptor.computeDescriptorString (Ljava/lang/Object;Ljava/lang/String;)I thrpt6 39518.106 ± 525.077 ops/ms MethodTypeDescDescriptor.computeDescriptorString ()V thrpt6 62312.500 ± 579.585 ops/ms MethodTypeDescDescriptor.computeDescriptorString ([IJLjava/lang/String;Z)Ljava/util/List; thrpt6 21856.065 ± 678.237 ops/ms MethodTypeDescDescriptor.computeDescriptorString ()[Ljava/lang/String; thrpt6 57457.483 ± 1416.646 ops/ms Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 21684.365 ± 941.586 ops/s RebuildMethodBodies.unshared thrpt4 15724.422 ± 153.420 ops/s master: Benchmark (descString) Mode Cnt Score Error Units MethodTypeDescDescriptorOld.computeDescriptorString (Ljava/lang/Object;Ljava/lang/String;)I thrpt6 6062.735 ± 61.852 ops/ms MethodTypeDescDescriptorOld.computeDescriptorString ()V thrpt6 8703.338 ± 205.429 ops/ms MethodTypeDescDescriptorOld.computeDescriptorString ([IJLjava/lang/String;Z)Ljava/util/List; thrpt6 5673.104 ± 62.810 ops/ms MethodTypeDescDescriptorOld.computeDescriptorString ()[Ljava/lang/String; thrpt6 8232.874 ± 161.060 ops/ms Benchmark Mode Cnt Score Error Units RebuildMethodBodies.sharedthrpt4 18317.288 ± 465.309 ops/s RebuildMethodBodies.unshared thrpt4 13744.541 ± 399.263 ops/s The Classfile API code generation and descriptor string computation speed up with this patch. Additional benchmarks for this patch: Benchmark (kind) Mode Cnt Score Error Units MethodTypeDescConstruct.ofArrayBench GENERIC thrpt6 203322.703 ± 5795.340 ops/ms MethodTypeDescConstruct.ofArrayBenchVOID thrpt6 397200.234 ± 7467.524 ops/ms MethodTypeDescConstruct.ofArrayBenchNO_PARAM thrpt6 398156.642 ± 7230.653 ops/ms MethodTypeDescConstruct.ofArrayBench ARBITRARY thrpt6 91751.039 ± 2451.052 ops/ms MethodTypeDescConstruct.ofDescriptorBenchGENERIC thrpt68184.264 ± 185.177 ops/ms MethodTypeDescConstruct.ofDescriptorBench VOID thrpt6 68775.908 ± 2471.949 ops/ms MethodTypeDescConstruct.ofDescriptorBench NO_PARAM thrpt6 28129.233 ± 454.477 ops/ms MethodTypeDescConstruct.ofDescriptorBench ARBITRARY thrpt67634.086 ± 333.202 ops/ms MethodTypeDescConstruct.ofListBench GENERIC thrpt6 311382.640 ± 15817.487 ops/ms MethodTypeDescConstruct.ofListBench VOID thrpt6 371300.447 ± 2310.545 ops/ms MethodTypeDescConstruct.ofListBench NO_PARAM thrpt6 367942.613 ± 10068.395 ops/ms MethodTypeDescConstruct.ofListBenchARBITRARY thrpt6 199825.985 ± 5963.658 ops/ms Shows that immutable
Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
> This patch moves the parameters to an immutable list, to avoid allocations on > `parameterList` as well. In addition, it fixes 8304932, the bug where if a > caller keeps a reference to the array passed into `MethodTypeDesc.of`, the > caller may mutate the Desc via the array and can create invalid > MethodTypeDesc. > > This patch has minor performance gains on `ofDescriptor` factory, even > compared to Adam's patch that optimized `ofDescriptor` in #12945. > > Benchmark of Oracle JDK 20: > https://gist.github.com/683c6219e183cbc2b336224fc2c0d50a > Benchmark of this patch: > https://gist.github.com/22be9371a2370fb4a7b44f1684750ec4 > Benchmark of [asotona's > patch](https://github.com/openjdk/jdk/pull/12945/files#diff-ac8e413d3e13532a2b0d34a90253c6ddd7a4f04082f792b9d076e9b5a33f2078): > https://gist.github.com/eb98579c3b51cafae481049a95a78f80 > > [sotona vs > this](https://jmh.morethan.io/?gists=eb98579c3b51cafae481049a95a78f80,22be9371a2370fb4a7b44f1684750ec4); > [20 vs > this](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,22be9371a2370fb4a7b44f1684750ec4); > [20 vs > sotona](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,eb98579c3b51cafae481049a95a78f80), > for reference Chen Liang has updated the pull request incrementally with one additional commit since the last revision: reuse immutable list to avoid array allocation - Changes: - all: https://git.openjdk.org/jdk/pull/13186/files - new: https://git.openjdk.org/jdk/pull/13186/files/6044b7ee..efc8018c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13186=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13186=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13186.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13186/head:pull/13186 PR: https://git.openjdk.org/jdk/pull/13186
Re: RFR: 8174722: Wrong behavior of DecimalFormat with RoundingMode.UP in special case
On Tue, 23 May 2023 23:30:11 GMT, Joe Darcy wrote: > Note there are some hazards here with respect to the numerical values of > double and the decimal string used to represent the double; in exact terms: > > ``` > jshell> new BigDecimal(1.0001) > $2 ==> 1.889865875957184471189975738525390625 > > jshell> new BigDecimal(0.0001) > $3 ==> 0.00014792173602385929598312941379845142364501953125 > ``` Thanks for bringing that to my attention Joe. I will re-evaluate how I test the changes then. - PR Comment: https://git.openjdk.org/jdk/pull/14110#issuecomment-1560295694
Re: RFR: 8174722: Wrong behavior of DecimalFormat with RoundingMode.UP in special case
On Tue, 23 May 2023 23:16:01 GMT, Justin Lu wrote:
> Please review this PR, which addresses a case where Decimal Format would
> violate certain RoundingMode contracts given the right pattern and double.
>
> For example,
>
>
> DecimalFormat df = new DecimalFormat();
> df.setRoundingMode(RoundingMode.UP);
> double small = 0.0001;
> double big = 1.0001;
> df.applyPattern("0.00");
> df.format(small); // returns 0.00, which violates UP
> df.format(big); // returns 1.01, which does not violate UP
>
>
> In this example `0.0001` becomes `0.00`, a decrease in magnitude. This
> violates the RoundingMode.UP contract as RoundingMode.UP states "Note that
> this rounding mode never decreases the magnitude of the calculated value."
>
> This edge case is a result of when values smaller than the absolute value of
> `.1` have more leading zeros between the decimal and the first non-zero digit
> (_0.0001 -> **3**_) than maximum fractional digits in the pattern (_0.00 ->
> **2**_).
>
> The change to Decimal Format ensures that during this case, the rounding mode
> is considered before truncation, which dictates if the formatted number
> should insert a 1 in the least significant digit location.
>
> The test validates the change by using data from larger counterparts. For
> example, If we are testing `0.0009`, we can format `1.0009` with the same
> pattern and mode that we use on `0.0009`, then compare the fractional
> portions to each other and ensure they are equal.
Note there are some hazards here with respect to the numerical values of double
and the decimal string used to represent the double; in exact terms:
jshell> new BigDecimal(1.0001)
$2 ==> 1.889865875957184471189975738525390625
jshell> new BigDecimal(0.0001)
$3 ==> 0.00014792173602385929598312941379845142364501953125
-
PR Comment: https://git.openjdk.org/jdk/pull/14110#issuecomment-1560254680
RFR: 8174722: Wrong behavior of DecimalFormat with RoundingMode.UP in special case
Please review this PR, which addresses a case where Decimal Format would
violate certain RoundingMode contracts given the right pattern and double.
For example,
DecimalFormat df = new DecimalFormat();
df.setRoundingMode(RoundingMode.UP);
double small = 0.0001;
double big = 1.0001;
df.applyPattern("0.00");
df.format(small); // returns 0.00, which violates UP
df.format(big); // returns 1.01, which does not violate UP
In this example `0.0001` becomes `0.00`, a decrease in magnitude. This violates
the RoundingMode.UP contract as RoundingMode.UP states "Note that this rounding
mode never decreases the magnitude of the calculated value."
This edge case is a result of when values smaller than the absolute value of
`.1` have more leading zeros between the decimal and the first non-zero digit
(_0.0001 -> **3**_) than maximum fractional digits in the pattern (_0.00 ->
**2**_).
The change to Decimal Format ensures that during this case, the rounding mode
is considered before truncation, which dictates if the formatted number should
insert a 1 in the least significant digit location.
The test validates the change by using data from larger counterparts. For
example, If we are testing `0.0009`, we can format `1.0009` with the same
pattern and mode that we use on `0.0009`, then compare the fractional portions
to each other and ensure they are equal.
-
Commit messages:
- Clarify test comment
- Missing period
- Add Impl
- Use RoundingMode.values() instead of hard coding a static array
- Add test
Changes: https://git.openjdk.org/jdk/pull/14110/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=14110=00
Issue: https://bugs.openjdk.org/browse/JDK-8174722
Stats: 164 lines in 2 files changed: 163 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14110.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14110/head:pull/14110
PR: https://git.openjdk.org/jdk/pull/14110
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Tue, 23 May 2023 20:32:10 GMT, Maurizio Cimadamore wrote: > Effectively, what you suggest amount at saying: we do have a JAVA_CHAR > layout, which is mostly there for Java interop. But a native linker (which > only cares about native interop) doesn't really care much about that. Does > that sound good? Yes. - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1560241481
RFR: 8290499: new File(parent, \"/\") breaks normalization – creates File with slash at the end
In `java.io.File`, change the constructors `File(File,String)` and `File(String,String)` so that they do not for typical cases return a `File` whose path has a trailing name separator. - Commit messages: - 8290499: new File(parent, \"/\") breaks normalization – creates File with slash at the end Changes: https://git.openjdk.org/jdk/pull/14109/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14109=00 Issue: https://bugs.openjdk.org/browse/JDK-8290499 Stats: 34 lines in 3 files changed: 28 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14109.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14109/head:pull/14109 PR: https://git.openjdk.org/jdk/pull/14109
Re: Classes used in method body are loaded lazily or eagerly depending on method return type
- Original Message - > From: "David Holmes" > To: "Raffaello Giulietti" , "core-libs-dev" > > Sent: Wednesday, May 24, 2023 12:23:24 AM > Subject: Re: Classes used in method body are loaded lazily or eagerly > depending on method return type > On 24/05/2023 12:50 am, Raffaello Giulietti wrote: >> I think the problem here is that you are deleting a class in a nest. >> >> During the verification of BaseClassReturner.getObject(), access control >> (JVMS, 5.4.4, second half) determines that the nest is broken, as >> ChildClass is not present anymore. > > Not sure access control gets involved at this stage of the verification > process. But in any case turning on logging does not show anything > related to nestmates happening between BaseClass and ChildClass. It > seems to just be the resolution of the return type during verification > of the method, that causes the loading of ChildClass and the subsequent > CNFE if it has been removed. > >> If you move ChildClass out of TestLoading so that it becomes a top-level >> class extending TestLoading.BaseClass, and if you adapt the line that >> initializes the local var classFileB to refer to the new location, the >> code will not throw, despite ChildClass being deleted. > > My simplified test shows it still throws when verifying BaseClassReturner. Nestmate checking is done lazily, so if you do not call a method/access a field of a nested class, the VM should not trigger a class loading. Moreover, if you test with Java 8 (nestmates were introduced in Java 11), it failed too. That's another clue that the error is not related to nestmates checking. > > > Cheers, > David regards, Rémi > >> >> Greetings >> Raffaello >> >> >> >> On 2023-05-23 13:20, Сергей Цыпанов wrote: >>> Hello, >>> >>> originally this question was asked here: >>> https://stackoverflow.com/q/76260269/12473843, >>> the code sample for reproduction will be put below or can be found via >>> the link >>> >>> The issue is about eager/lazy loading of a class depending on method >>> return type. >>> If one runs the code below with Java 11-19 it will fail with >>> NoClassDefFoundError (this is expected as delete class file for >>> ChildClass): >>> >>> java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass >>> at java.base/java.lang.Class.forName0(Native Method) >>> at java.base/java.lang.Class.forName(Class.java:390) >>> at java.base/java.lang.Class.forName(Class.java:381) >>> at org.example.TestLoading.loadMyClass(TestLoading.java:29) >>> at org.example.TestLoading.main(TestLoading.java:23) >>> Caused by: java.lang.ClassNotFoundException: >>> org.example.TestLoading$ChildClass >>> at >>> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) >>> at >>> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) >>> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521) >>> ... 5 more >>> >>> As of Java 20 chapter 12.4.1 of JLS states: >>> - >>> A class or interface T will be initialized immediately before the >>> first occurrence of any one of the following: >>> >>> - T is a class and an instance of T is created. >>> - a static method declared by T is invoked. >>> - a static field declared by T is assigned. >>> - a static field declared by T is used and the field is not a constant >>> variable (§4.12.4). >>> >>> When a class is initialized, its superclasses are initialized (if they >>> have not been previously initialized), as well as any superinterfaces >>> (§8.1.5) that declare any default methods (§9.4.3) (if they have not >>> been previously initialized). >>> Initialization of an interface does not, of itself, cause >>> initialization of any of its superinterfaces. >>> A reference to a static field (§8.3.1.1) causes initialization of only >>> the class or interface that actually declares it, even though it might >>> be referred to through the name of a subclass, a subinterface, or a >>> class that implements an interface. >>> Invocation of certain reflective methods in class Class and in package >>> java.lang.reflect also causes class or interface initialization. >>> A class or interface will not be initialized under any other >>> circumstance. >>> - >>> With the code snippet we see that calling >>> Class.forName(ObjectReturner.class.getName()) succeeds and >>> Class.forName(BaseClassReturner.class.getName()) fails even though >>> both declare returning an instance of ChildClass. >>> This failure is unexpected as in the code below we don't fulfill any >>> requirement for class loading as of JLS 12.4.1, but the JVM still >>> tries to load the class. >>> >>> I suspect it might be related to class file validation and/or >>> security, because when we run the code with -Xlog:class+init there's a >>> reference to LinkageError in loading log: >>>
Re: Classes used in method body are loaded lazily or eagerly depending on method return type
- Original Message -
> From: "Сергей Цыпанов"
> To: "core-libs-dev"
> Sent: Tuesday, May 23, 2023 1:20:44 PM
> Subject: Classes used in method body are loaded lazily or eagerly depending
> on method return type
> Hello,
Hello,
>
> originally this question was asked here:
> https://stackoverflow.com/q/76260269/12473843,
> the code sample for reproduction will be put below or can be found via the
> link
>
> The issue is about eager/lazy loading of a class depending on method return
> type.
It also depends on the content of the methods.
If you modify the code to return null, the NoClassDefFoundError disappears
public static class ObjectReturner {
public Object getObject() {
return null;
}
}
public static class BaseClassReturner {
public BaseClass getObject() {
return null;
}
}
which means that the NoClassDefFoundError comes from the bytecode verifier.
In your original ObjectReturner, the bytecode verifier needs to check if
ChildClass is an Object and there is a shortcircuit for that, so ChildClass
does not need to be loaded.
But in BaseClassReturner, the bytecode verifier needs to check if ChildClass is
a BaseClass, so it has to load ChildClass to know if the superclass is
BaseClass.
Another way to see that it's the verifier that triggers the class loading is to
use the option "-noverify"
java -noverify TestLoading
should works !
regards,
Rémi
> If one runs the code below with Java 11-19 it will fail with
> NoClassDefFoundError (this is expected as delete class file for ChildClass):
>
> java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass
> at java.base/java.lang.Class.forName0(Native Method)
> at java.base/java.lang.Class.forName(Class.java:390)
> at java.base/java.lang.Class.forName(Class.java:381)
> at org.example.TestLoading.loadMyClass(TestLoading.java:29)
> at org.example.TestLoading.main(TestLoading.java:23)
> Caused by: java.lang.ClassNotFoundException:
> org.example.TestLoading$ChildClass
> at
>
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
> at
>
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
> ... 5 more
>
> As of Java 20 chapter 12.4.1 of JLS states:
> -
> A class or interface T will be initialized immediately before the first
> occurrence of any one of the following:
>
> - T is a class and an instance of T is created.
> - a static method declared by T is invoked.
> - a static field declared by T is assigned.
> - a static field declared by T is used and the field is not a constant
> variable
> (§4.12.4).
>
> When a class is initialized, its superclasses are initialized (if they have
> not
> been previously initialized), as well as any superinterfaces (§8.1.5) that
> declare any default methods (§9.4.3) (if they have not been previously
> initialized).
> Initialization of an interface does not, of itself, cause initialization of
> any
> of its superinterfaces.
> A reference to a static field (§8.3.1.1) causes initialization of only the
> class
> or interface that actually declares it, even though it might be referred to
> through the name of a subclass, a subinterface, or a class that implements an
> interface.
> Invocation of certain reflective methods in class Class and in package
> java.lang.reflect also causes class or interface initialization.
> A class or interface will not be initialized under any other circumstance.
> -
> With the code snippet we see that calling
> Class.forName(ObjectReturner.class.getName()) succeeds and
> Class.forName(BaseClassReturner.class.getName()) fails even though both
> declare
> returning an instance of ChildClass.
> This failure is unexpected as in the code below we don't fulfill any
> requirement
> for class loading as of JLS 12.4.1, but the JVM still tries to load the class.
>
> I suspect it might be related to class file validation and/or security,
> because
> when we run the code with -Xlog:class+init there's a reference to LinkageError
> in loading log:
>
> loading: org.example.TestLoading$BaseClassReturner...
> [0.277s][info][class,init] Start class verification for:
> org.example.TestLoading$BaseClassReturner
> [0.277s][info][class,init] 771 Initializing
> 'java/lang/ReflectiveOperationException'(no method) (0x00084028)
> [0.277s][info][class,init] 772 Initializing
> 'java/lang/ClassNotFoundException'(no method) (0x00084288)
> [0.277s][info][class,init] 773 Initializing 'java/lang/LinkageError'(no
> method)
> (0x000844f8) <
> [0.277s][info][class,init] 774 Initializing
> 'java/lang/NoClassDefFoundError'(no
> method) (0x00084758)
> [0.277s][info][class,init]
Re: Classes used in method body are loaded lazily or eagerly depending on method return type
On 24/05/2023 12:50 am, Raffaello Giulietti wrote: I think the problem here is that you are deleting a class in a nest. During the verification of BaseClassReturner.getObject(), access control (JVMS, 5.4.4, second half) determines that the nest is broken, as ChildClass is not present anymore. Not sure access control gets involved at this stage of the verification process. But in any case turning on logging does not show anything related to nestmates happening between BaseClass and ChildClass. It seems to just be the resolution of the return type during verification of the method, that causes the loading of ChildClass and the subsequent CNFE if it has been removed. If you move ChildClass out of TestLoading so that it becomes a top-level class extending TestLoading.BaseClass, and if you adapt the line that initializes the local var classFileB to refer to the new location, the code will not throw, despite ChildClass being deleted. My simplified test shows it still throws when verifying BaseClassReturner. Cheers, David Greetings Raffaello On 2023-05-23 13:20, Сергей Цыпанов wrote: Hello, originally this question was asked here: https://stackoverflow.com/q/76260269/12473843, the code sample for reproduction will be put below or can be found via the link The issue is about eager/lazy loading of a class depending on method return type. If one runs the code below with Java 11-19 it will fail with NoClassDefFoundError (this is expected as delete class file for ChildClass): java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:390) at java.base/java.lang.Class.forName(Class.java:381) at org.example.TestLoading.loadMyClass(TestLoading.java:29) at org.example.TestLoading.main(TestLoading.java:23) Caused by: java.lang.ClassNotFoundException: org.example.TestLoading$ChildClass at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521) ... 5 more As of Java 20 chapter 12.4.1 of JLS states: - A class or interface T will be initialized immediately before the first occurrence of any one of the following: - T is a class and an instance of T is created. - a static method declared by T is invoked. - a static field declared by T is assigned. - a static field declared by T is used and the field is not a constant variable (§4.12.4). When a class is initialized, its superclasses are initialized (if they have not been previously initialized), as well as any superinterfaces (§8.1.5) that declare any default methods (§9.4.3) (if they have not been previously initialized). Initialization of an interface does not, of itself, cause initialization of any of its superinterfaces. A reference to a static field (§8.3.1.1) causes initialization of only the class or interface that actually declares it, even though it might be referred to through the name of a subclass, a subinterface, or a class that implements an interface. Invocation of certain reflective methods in class Class and in package java.lang.reflect also causes class or interface initialization. A class or interface will not be initialized under any other circumstance. - With the code snippet we see that calling Class.forName(ObjectReturner.class.getName()) succeeds and Class.forName(BaseClassReturner.class.getName()) fails even though both declare returning an instance of ChildClass. This failure is unexpected as in the code below we don't fulfill any requirement for class loading as of JLS 12.4.1, but the JVM still tries to load the class. I suspect it might be related to class file validation and/or security, because when we run the code with -Xlog:class+init there's a reference to LinkageError in loading log: loading: org.example.TestLoading$BaseClassReturner... [0.277s][info][class,init] Start class verification for: org.example.TestLoading$BaseClassReturner [0.277s][info][class,init] 771 Initializing 'java/lang/ReflectiveOperationException'(no method) (0x00084028) [0.277s][info][class,init] 772 Initializing 'java/lang/ClassNotFoundException'(no method) (0x00084288) [0.277s][info][class,init] 773 Initializing 'java/lang/LinkageError'(no method) (0x000844f8) < [0.277s][info][class,init] 774 Initializing 'java/lang/NoClassDefFoundError'(no method) (0x00084758) [0.277s][info][class,init] Verification for org.example.TestLoading$BaseClassReturner has exception pending 'java.lang.NoClassDefFoundError org/example/TestLoading$ChildClass' [0.277s][info][class,init] End class verification for:
Re: Classes used in method body are loaded lazily or eagerly depending on method return type
On 24/05/2023 5:04 am, Сергей Цыпанов wrote: Hi, Verification can require classes to be loaded to perform the verification - see JVMS 4.10 for all the details. sorry, I still don't get it completely. Here's the byte code for ObjectReturner.getObject(): // access flags 0x1 public getObject()Ljava/lang/Object; L0 LINENUMBER 43 L0 NEW org/example/TestLoading$ChildClass DUP INVOKESPECIAL org/example/TestLoading$ChildClass. ()V ARETURN L1 LOCALVARIABLE this Lorg/example/TestLoading$ObjectReturner; L0 L1 0 MAXSTACK = 2 MAXLOCALS = 1 and this one is for BaseClassReturner.getObject(): // access flags 0x1 public getObject()Lorg/example/TestLoading$BaseClass; L0 LINENUMBER 49 L0 NEW org/example/TestLoading$ChildClass DUP INVOKESPECIAL org/example/TestLoading$ChildClass. ()V ARETURN L1 LOCALVARIABLE this Lorg/example/TestLoading$BaseClassReturner; L0 L1 0 MAXSTACK = 2 MAXLOCALS = 1 Apart from type of 'this' the only difference is return type, so I've referenced JVMS 4.10 in the part where return type is described. There we have clause for areturn: "An areturn instruction is type safe iff the enclosing method has a declared return type, ReturnType, that is a reference type, and one can validly pop a type matching ReturnType off the incoming operand stack." and for return type: "If the method returns a reference type, only an areturn instruction may be used, and the type of the returned value must be assignment compatible with the return descriptor of the method (§4.3.3)" I guess the second one is a clue for to check whether returned value can be assigned to return type the VM should look into inheritance tree. Please correct me if idea is wrong. For the Object case the type-checking is always obviously trivially correct so it doesn't need to load the actual return type. So the seems a reasonable conclusion. If you take your test code and only have the BaseClassReturner and only load it then you see in the logging: loading: TestLoading$BaseClassReturner... [0.067s][info][class,init ] Start class verification for: TestLoading$BaseClassReturner [0.067s][info][verification] Verifying class TestLoading$BaseClassReturner with new format [0.067s][info][verification] Verifying method TestLoading$BaseClassReturner.()V [0.067s][info][verification] Verifying method TestLoading$BaseClassReturner.getObject()LTestLoading$BaseClass; [0.068s][info][class,load ] TestLoading$BaseClass source: file:/scratch/users/daholme/tests/ [0.068s][info][class,load ] TestLoading$ChildClass source: file:/scratch/users/daholme/tests/ [0.068s][info][class,init ] End class verification for: TestLoading$BaseClassReturner [0.068s][info][verification] End class verification for: TestLoading$BaseClassReturner So you can see the ChildClass being loaded. David - Regards, Sergey Hi, On 23/05/2023 9:20 pm, Сергей Цыпанов wrote: Hello, originally this question was asked here: https://stackoverflow.com/q/76260269/12473843, the code sample for reproduction will be put below or can be found via the link The issue is about eager/lazy loading of a class depending on method return type. If one runs the code below with Java 11-19 it will fail with NoClassDefFoundError (this is expected as delete class file for ChildClass): java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:390) at java.base/java.lang.Class.forName(Class.java:381) at org.example.TestLoading.loadMyClass(TestLoading.java:29) at org.example.TestLoading.main(TestLoading.java:23) Caused by: java.lang.ClassNotFoundException: org.example.TestLoading$ChildClass at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521) ... 5 more As of Java 20 chapter 12.4.1 of JLS states: - A class or interface T will be initialized immediately before the first occurrence of any one of the following: - T is a class and an instance of T is created. - a static method declared by T is invoked. - a static field declared by T is assigned. - a static field declared by T is used and the field is not a constant variable (§4.12.4). When a class is initialized, its superclasses are initialized (if they have not been previously initialized), as well as any superinterfaces (§8.1.5) that declare any default methods (§9.4.3) (if they have not been previously initialized). Initialization of an interface does not, of itself, cause initialization of any of its superinterfaces. A reference to a static field (§8.3.1.1) causes initialization of only the class
Integrated: 8306698: Add overloads to MethodTypeDesc::of
On Sat, 22 Apr 2023 16:31:31 GMT, Chen Liang wrote: > Please review this patch adding two new convenience methods that allows > easier access to MethodTypeDesc instances and its associated CSR as well. > This is a necessity to allow #13186 to reduce array copies in a few > scenarios; the implementation of the two methods will be updated there. > > Javadoc: > https://cr.openjdk.org/~liach/8306698/1/java.base/java/lang/constant/MethodTypeDesc.html This pull request has now been integrated. Changeset: 8ffa264c Author:Chen Liang Committer: Mandy Chung URL: https://git.openjdk.org/jdk/commit/8ffa264cf009ddb1af486831f12bc70f93d74cf5 Stats: 83 lines in 3 files changed: 45 ins; 30 del; 8 mod 8306698: Add overloads to MethodTypeDesc::of Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/13599
Re: RFR: JDK-8306584: Start of release updates for JDK 22
On Thu, 20 Apr 2023 20:28:18 GMT, Joe Darcy wrote: > Time to get JDK 22 underway... Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13567#pullrequestreview-1440582657
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Tue, 23 May 2023 20:32:10 GMT, Maurizio Cimadamore wrote: > Arguably C `unsigned short` could map to carriers Java `short` or Java > `char`, but i am inclined to say the user should cast between Java `short` to > `char` in such cases. (Another advantage of this is that, should we get proper unsigned carriers from Valhalla one day, native linkers could be updated to support those en masse - not just for `short`) - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1560118602
Re: RFR: JDK-8306584: Start of release updates for JDK 22
On Tue, 23 May 2023 20:38:34 GMT, Jonathan Gibbons wrote: >> Time to get JDK 22 underway... > > test/langtools/tools/javac/versions/Versions.java line 93: > >> 91: TWENTY(false,"64.0", "20", Versions::checksrc20), >> 92: TWENTY_ONE(false,"65.0", "21", Versions::checksrc20), >> 93: TWENTY_TWO(false,"66.0", "22", Versions::checksrc20); > > when do these get updated? Ah, when new non-preview language structures are added. With two new language features in JDK 21, looks like it is time for me to add another one; thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1202991806
Re: RFR: 8306698: Add overloads to MethodTypeDesc::of [v2]
On Sun, 23 Apr 2023 23:44:41 GMT, Chen Liang wrote: >> Please review this patch adding two new convenience methods that allows >> easier access to MethodTypeDesc instances and its associated CSR as well. >> This is a necessity to allow #13186 to reduce array copies in a few >> scenarios; the implementation of the two methods will be updated there. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306698/1/java.base/java/lang/constant/MethodTypeDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Update method type and specification, update tests Marked as reviewed by mchung (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13599#pullrequestreview-1440542755
Re: RFR: JDK-8306584: Start of release updates for JDK 22
On Thu, 20 Apr 2023 20:28:18 GMT, Joe Darcy wrote: > Time to get JDK 22 underway... Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13567#pullrequestreview-1440532444
Re: RFR: JDK-8306584: Start of release updates for JDK 22
On Thu, 20 Apr 2023 20:28:18 GMT, Joe Darcy wrote: > Time to get JDK 22 underway... test/langtools/tools/javac/versions/Versions.java line 26: > 24: /* > 25: * @test > 26: * @bug 4981566 5028634 5094412 6304984 7025786 7025789 8001112 8028545 > 8000961 8030610 8028546 8188870 8173382 8173382 8193290 8205619 8028563 > 8245147 8245586 8257453 8286035 8306586 At some point, this line should be wrapped test/langtools/tools/javac/versions/Versions.java line 93: > 91: TWENTY(false,"64.0", "20", Versions::checksrc20), > 92: TWENTY_ONE(false,"65.0", "21", Versions::checksrc20), > 93: TWENTY_TWO(false,"66.0", "22", Versions::checksrc20); when do these get updated? - PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1202974585 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1202976605
Re: RFR: JDK-8306584: Start of release updates for JDK 22
On Thu, 20 Apr 2023 20:28:18 GMT, Joe Darcy wrote: > Time to get JDK 22 underway... Typical start-of-JDK-next changes. As usual, the bulk of the PR is the symbol file information to support "javac --release 21". - PR Comment: https://git.openjdk.org/jdk/pull/13567#issuecomment-1560085911
RFR: JDK-8306584: Start of release updates for JDK 22
Time to get JDK 22 underway... - Commit messages: - Merge branch 'master' into JDK-8306584 - Merge branch 'master' into JDK-8306584 - Minor test fixes. - Merge branch 'master' into JDK-8306584 - Update symbol files to JDK 21 b23. - Merge branch 'JDK-8306584' of https://github.com/jddarcy/jdk into JDK-8306584 - Add symbol files. - Merge branch 'master' into JDK-8306584 - Problem list failing langtools test. - Merge branch 'master' into JDK-8306584 - ... and 9 more: https://git.openjdk.org/jdk/compare/8474e693...6047c2cb Changes: https://git.openjdk.org/jdk/pull/13567/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13567=00 Issue: https://bugs.openjdk.org/browse/JDK-8306584 Stats: 5650 lines in 69 files changed: 5596 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: 8308293: A linker should expose the layouts it supports [v4]
On Mon, 22 May 2023 09:34:53 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address further review comments
> On further reflection i think mapping C `unsigned short` only to Java `char`
> is a misleading. Although Java `char` is an integral type is not really
> intended to be used generally as an unsigned 16-bit integer, so arguably Java
> `char` is not as useful a carrier type for native interoperation as Java
> `short` might be even though it is signed. Thus i am inclined to remove that
> mapping.
>
> What if we say something to the effect of:
>
> > unless explicitly declared in the canonical layouts C's unsigned integral
> > types are mapped to the layouts associated with the required C's signed
> > integral types of the same bit sizes.
>
> ?
>
> Arguably C `unsigned short` could map to carriers Java `short` or Java
> `char`, but i am inclined to say the user should cast between Java `short` to
> `char` in such cases.
>
> FWIW i checked what the FFM API and jextract does today and it maps unsigned
> C types to signed Java types.
> On further reflection i think mapping C `unsigned short` only to Java `char`
> is a misleading. Although Java `char` is an integral type is not really
> intended to be used generally as an unsigned 16-bit integer, so arguably Java
> `char` is not as useful a carrier type for native interoperation as Java
> `short` might be even though it is signed. Thus i am inclined to remove that
> mapping.
>
> What if we say something to the effect of:
>
> > unless explicitly declared in the canonical layouts C's unsigned integral
> > types are mapped to the layouts associated with the required C's signed
> > integral types of the same bit sizes.
>
> ?
>
> Arguably C `unsigned short` could map to carriers Java `short` or Java
> `char`, but i am inclined to say the user should cast between Java `short` to
> `char` in such cases.
>
> FWIW i checked what the FFM API and jextract does today and it maps unsigned
> C types to signed Java types.
I tend to agree with your conclusion. And I confirm that we do not use "char"
anywhere in jextract. The only "problem" with that approach is that if we go
down that path, JAVA_CHAR is no longer a canonical type, so users cannot
mention it in function descriptors. Apart from requiring few test updates, I
don't see many other problems with it - if one really really wanted the result
of a native call to be converted to `char`, a MH adapter can be used.
Effectively, what you suggest amount at saying: we do have a JAVA_CHAR layout,
which is mostly there for Java interop. But a native linker (which only cares
about native interop) doesn't really care much about that. Does that sound good?
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1560086050
Re: RFR: 8306698: Add overloads to MethodTypeDesc::of [v2]
On Sun, 23 Apr 2023 23:44:41 GMT, Chen Liang wrote: >> Please review this patch adding two new convenience methods that allows >> easier access to MethodTypeDesc instances and its associated CSR as well. >> This is a necessity to allow #13186 to reduce array copies in a few >> scenarios; the implementation of the two methods will be updated there. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306698/1/java.base/java/lang/constant/MethodTypeDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Update method type and specification, update tests I plan to keep it. Migration to an immutable list to optimize `parameterList` while it doesn't affect the performance of other accessors. Meanwhile, the list passed by the user code to the factory can be shared safely if it's immutable, while we shouldn't trust the vararg array to the factory. - PR Comment: https://git.openjdk.org/jdk/pull/13599#issuecomment-1560082635
Re: RFR: 8306698: Add overloads to MethodTypeDesc::of [v2]
On Tue, 23 May 2023 20:03:35 GMT, Mandy Chung wrote: > yes, I'm back from vacation. > > This change looks fine. Can you update the copyright header to 2023? The copyright year was already updated to 2023 when covariant override for `resolveConstantDesc` was added. - PR Comment: https://git.openjdk.org/jdk/pull/13599#issuecomment-1560084308
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v36]
On Tue, 23 May 2023 15:21:02 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve comments about the Parameter Save Area.
Thanks a lot for all discussions, feedback and the reviews! This was really
helpful! I'm planning to integrate tomorrow.
-
PR Comment: https://git.openjdk.org/jdk/pull/12708#issuecomment-1560069523
Re: RFR: 8306698: Add overloads to MethodTypeDesc::of [v2]
On Sun, 23 Apr 2023 23:44:41 GMT, Chen Liang wrote: >> Please review this patch adding two new convenience methods that allows >> easier access to MethodTypeDesc instances and its associated CSR as well. >> This is a necessity to allow #13186 to reduce array copies in a few >> scenarios; the implementation of the two methods will be updated there. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306698/1/java.base/java/lang/constant/MethodTypeDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Update method type and specification, update tests What is the plan for https://git.openjdk.org/jdk/pull/13186? It seems that keeping `argTypes` in an array is okay. In any case, we can do the discussion in PR 13186 after this one is integrated. - PR Comment: https://git.openjdk.org/jdk/pull/13599#issuecomment-1560053685
Re: RFR: 8306698: Add overloads to MethodTypeDesc::of [v2]
On Sun, 23 Apr 2023 23:44:41 GMT, Chen Liang wrote: >> Please review this patch adding two new convenience methods that allows >> easier access to MethodTypeDesc instances and its associated CSR as well. >> This is a necessity to allow #13186 to reduce array copies in a few >> scenarios; the implementation of the two methods will be updated there. >> >> Javadoc: >> https://cr.openjdk.org/~liach/8306698/1/java.base/java/lang/constant/MethodTypeDesc.html > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Update method type and specification, update tests yes, I'm back from vacation. This change looks fine. Can you update the copyright header to 2023? - PR Comment: https://git.openjdk.org/jdk/pull/13599#issuecomment-1560051087
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v17]
> 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 33 commits: - Fix missing constructor error messages and handle inner class launching - Merge branch 'master' into 8306112 - Issue warning if traditional main not used. - Give subclass priority - Merge branch 'master' into 8306112 - Requested Changes #2 - Update VirtualParser.java - Merge branch 'master' into 8306112 - Refactor source code launcher - Typo - ... and 23 more: https://git.openjdk.org/jdk/compare/bddf4838...b55f82f8 - Changes: https://git.openjdk.org/jdk/pull/13689/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13689=16 Stats: 1235 lines in 28 files changed: 1071 ins; 32 del; 132 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: EnumSet doesn't implement SequencedSet
> It doesn't seem like the members of an EnumSet would be iterated frequently... We use EnumSet and EnumMap extensively, frequently iterating over them (the enum's natural order being critical). > ...usually treated as a bag of named booleans used as a set of boolean options. So for us it's... "ordered bag of named booleans with cheap lookup". But I agree our usage probably wouldn't benefit from sequenced/navigable functionality much (if at all). -- Aaron Scott-Boddendijk On Wed, May 24, 2023 at 5:51 AM Stuart Marks wrote: > The idea of EnumSet implementing SortedSet or NavigableSet has been around > for a long time; see > > https://bugs.openjdk.org/browse/JDK-6278287 > > But yes, EnumSet could also implement SequencedSet. Similarly, EnumMap > could implement SequencedMap, SortedMap, or NavigableMap. > > Implementing NavigableSet/Map is somewhat onerous, given that there's no > AbstractNavigableSet/Map, and there are a lot of methods and variations of > views to be implemented. > > I'm a bit skeptical about the usefulness of retrofitting EnumSet/Map this > way, though. My hunch is that EnumSet is usually treated as a bag of named > booleans used as a set of boolean options. The EnumSet is passed around as > an aggregate, with individual enum instances tested at specific points in > the code to adjust the logic. It doesn't seem like the members of an > EnumSet would be iterated frequently, or that it would be useful to have > specific subsets of an EnumSet (which is what NavigableSet would provide). > Maybe similar analysis applies to EnumMap. > > I could be convinced otherwise with use cases, though. > > s'marks > > > On 5/23/23 2:57 AM, Tagir Valeev wrote: > > Hello! > > Looks like this was overlooked. On the other hand, it could even implement > SortedSet or Navigable set, as it's essentially not just ordered but > sorted, according to the enum natural order. Same for EnumMap. > > With best regards, > Tagir Valeev > > On Tue, May 23, 2023, 11:46 Andrey Turbanov wrote: > >> Hello. >> I've noticed that EnumSet unfortunately doesn't implement the newly >> introduced interface SequencedSet. >> From the first look it fits into it. >> Is there a reason for that? Sorry if it was already discussed. >> >> Andrey Turbanov >> >
Re: RFR: 8308549: Classfile API should fail to generate over-sized Code attribute
On Tue, 23 May 2023 12:54:20 GMT, Adam Sotona wrote: > Classfile API allowed to generate Code attribute exceeding the 65k limit. No > exception has been thrown during class generation and the class failed > verification later during class loading. > This patch adds Code size limit check throwing IllegalArgumentException. > The patch also adds similar check for constant pool size limit to avoid > generation class file with corrupted constant pool. > Two new tests are added to check response on oversized Code attribute and > constant pool. > `VerifierImpl` is extended to check Code attribute size as a part of class > verification process. > > Please review. > > Thanks, > Adam This looks ok to me. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14100#pullrequestreview-1440411354
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview)
On Tue, 23 May 2023 18:23:21 GMT, Paul Sandoz wrote: > Oh, and i guess that has some performance implications in some cases? more so > on the set of instructions produced on ARM say rather than limiting C2 > optimizations? I think so. As far as I can remember, a release fence on x86 generates no code at all. > Given that the Supplier is likely to be the target of a lambda expression > which may also capture I was surprised there would be much of an increased > impact. (HotSpot may not strength reduce multiple fences in this case.) It may, or it may not. I don't really want the call without a checked exception to be more costly than a call with one. That seems a little weird, at least. > Can we update to add say "/* non-final */ to the field and update the class > doc to say the release fence inserted by HotSpot as a result of constructing > a class with final fields has performance implications <<"insert loose > quantification of those implications">> ? Sure, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1202870649
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview)
On Tue, 23 May 2023 14:57:26 GMT, Andrew Haley wrote: >> @theRealAph will want to comment on this as it is very performance >> sensitive. I think CallableAdapter.s is non-final to avoid the release fence. > > That's right. The problem is that we can never get rid of the release fence, > apparently even when the instance of the adapter is scalar replaced. I > imagine that'll get fixed one day, but this is internal JDK code. Oh, and i guess that has some performance implications in some cases? more so on the set of instructions produced on ARM say rather than limiting C2 optimizations? Given that the Supplier is likely to be the target of a lambda expression which may also capture I was surprised there would be much of an increased impact. (HotSpot may not strength reduce multiple fences in this case.) Can we update to add say "/* non-final */ to the field and update the class doc to say the release fence inserted by HotSpot as a result of constructing a class with final fields has performance implications <<"insert loose quantification of those implications">> ? - PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1202821873
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v16]
On Tue, 23 May 2023 18:02:16 GMT, Alan Bateman wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Give subclass priority
>
> src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 129:
>
>> 127: * @throws NoSuchMethodException when not preview and no method
>> found
>> 128: */
>> 129: public static Method findMainMethod(Class mainClass) throws
>> NoSuchMethodException {
>
> The latest iteration of changes removes the warning but the latest iteration
> of the JEP proposes a warning when an instance main or static no-args main is
> preferred over an inherited static main. Is it that the JEP is just running
> ahead a bit and the changes in the PR will catch up soon?
Yes. There is some juggling going on to get the code to align with the
requirement and the interpretation thereof.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1202804732
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v16]
On Thu, 18 May 2023 19:06:28 GMT, Jim Laskey wrote:
>> 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:
>
> Give subclass priority
src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 129:
> 127: * @throws NoSuchMethodException when not preview and no method found
> 128: */
> 129: public static Method findMainMethod(Class mainClass) throws
> NoSuchMethodException {
The latest iteration of changes removes the warning but the latest iteration of
the JEP proposes a warning when an instance main or static no-args main is
preferred over an inherited static main. Is it that the JEP is just running
ahead a bit and the changes in the PR will catch up soon?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1202796707
Re: EnumSet doesn't implement SequencedSet
The idea of EnumSet implementing SortedSet or NavigableSet has been around for a long time; see https://bugs.openjdk.org/browse/JDK-6278287 But yes, EnumSet could also implement SequencedSet. Similarly, EnumMap could implement SequencedMap, SortedMap, or NavigableMap. Implementing NavigableSet/Map is somewhat onerous, given that there's no AbstractNavigableSet/Map, and there are a lot of methods and variations of views to be implemented. I'm a bit skeptical about the usefulness of retrofitting EnumSet/Map this way, though. My hunch is that EnumSet is usually treated as a bag of named booleans used as a set of boolean options. The EnumSet is passed around as an aggregate, with individual enum instances tested at specific points in the code to adjust the logic. It doesn't seem like the members of an EnumSet would be iterated frequently, or that it would be useful to have specific subsets of an EnumSet (which is what NavigableSet would provide). Maybe similar analysis applies to EnumMap. I could be convinced otherwise with use cases, though. s'marks On 5/23/23 2:57 AM, Tagir Valeev wrote: Hello! Looks like this was overlooked. On the other hand, it could even implement SortedSet or Navigable set, as it's essentially not just ordered but sorted, according to the enum natural order. Same for EnumMap. With best regards, Tagir Valeev On Tue, May 23, 2023, 11:46 Andrey Turbanov wrote: Hello. I've noticed that EnumSet unfortunately doesn't implement the newly introduced interface SequencedSet. From the first look it fits into it. Is there a reason for that? Sorry if it was already discussed. Andrey Turbanov
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v36]
On Tue, 23 May 2023 15:21:02 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve comments about the Parameter Save Area.
This is really good to go now.
(maybe after a final round of tests)
Thanks a lot!
Richard.
-
Marked as reviewed by rrich (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/12708#pullrequestreview-1440198377
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
On Tue, 23 May 2023 12:26:52 GMT, Martin Doerr wrote: > I believe omitting the PSA is wrong for varargs, but we don't have this > information in the backend. It is clearly wrong. > So, I think we should simply not optimize it. Already agreed for this version. > Reserving 64 Byte stack space should be affordable for a downcall even if > it's not always needed. It is hardly ever needed to begin with. It's just not pretty to allocate the PSA for little test functions. It'll confuse people that compare the downcall stub to what the C compiler produces. They might even think we havn't understood the ABI ;) > The Java side could compute it, but there's no way to pass this information > to the backend. I'm not sure about that. First idea would be to keep the allocated `stack` in `nextStorage` if needed and pass it to the backend. > I've improved the comments. Please take a look. Great! Thanks :) - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1202652115
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v3]
On Tue, 23 May 2023 05:31:37 GMT, Justin Lu wrote: >> Please review this PR which updates the Scientific Notation section of >> Decimal Format. It aims to resolve >> [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as >> [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188). >> >> **Scientific Notation** in Decimal Format contains the definition for a >> scientific notation formatted number's mantissa as such: _The number of >> significant digits in the mantissa is the sum of the minimum integer and >> maximum fraction digits, and is unaffected by the maximum integer digits. >> For example, 12345 formatted with "##0.##E0" is "12.3E3"._ >> >> Both the definition and example are incorrect, as the actual result is >> "12.E345". >> >> The following example data show that scientific notation formatted numbers >> do not adhere to that definition. As, according to the definition, the >> following numbers should have 3 significant digits, but in reality, they >> have up to 5. >> >> 123 formatted by ##0.#E0 is 123E0 >> 1234 formatted by ##0.#E0 is 1.234E3 >> 12345 formatted by ##0.#E0 is 12.34E3 >> 123456 formatted by ##0.#E0 is 123.5E3 >> 1234567 formatted by ##0.#E0 is 1.235E6 >> 12345678 formatted by ##0.#E0 is 12.35E6 >> 123456789 formatted by ##0.#E0 is 123.5E6 >> >> >> The actual definition of the mantissa, as well as examples and further >> context are included in this change. In addition, a test has been added that >> tests various patterns to numbers and ensures the format follows the new >> definition's mathematical expression. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Review: replace counting with isDigit, use all caps static final vars Looks good - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14066#pullrequestreview-1440125960
Re: RFR: JDK-8282797: CompileCommand parsing errors should exit VM
On Tue, 2 May 2023 11:35:54 GMT, Tobias Holenstein wrote: > Currently, errors during compile command parsing just print an error but > don't exit the VM. As a result, issues go unnoticed. > > With this PR the behavior is changed to exit the VM when an error occurs. > > E.g. `java -XX:CompileCommand=compileonly,HashMap:: -version` will exit the > VM after a parsing occurred. > > CompileCommand: An error occurred during parsing > Error: Could not parse method pattern > Line: 'compileonly,HashMap::' > > Usage: '-XX:CompileCommand=,' - to set boolean option > to true > Usage: '-XX:CompileCommand=,,' > Use: '-XX:CompileCommand=help' for more information and to list all option. > > Error: Could not create the Java Virtual Machine. > Error: A fatal exception has occurred. Program will exit. > > > ### Updated Tests > Updated all tests to now expect an error code `1` for wrong `CompileCommand` Looks good. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13753#pullrequestreview-1440125001
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
On Tue, 23 May 2023 15:31:49 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test comment update Looks good - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14083#pullrequestreview-1440123393
Integrated: 8308016: Use snippets in java.io package
On Fri, 12 May 2023 16:17:38 GMT, Brian Burkhalter wrote:
> Replace `{@code ...}` patterns and the like with `{@snippet
> lang=java : ...}`.
This pull request has now been integrated.
Changeset: 710453c6
Author:Brian Burkhalter
URL:
https://git.openjdk.org/jdk/commit/710453c676712d021bf856dc601d965e4e270805
Stats: 159 lines in 20 files changed: 27 ins; 7 del; 125 mod
8308016: Use snippets in java.io package
Reviewed-by: rriggs
-
PR: https://git.openjdk.org/jdk/pull/13957
RFR: JDK-8282797: CompileCommand parsing errors should exit VM
Currently, errors during compile command parsing just print an error but don't exit the VM. As a result, issues go unnoticed. With this PR the behavior is changed to exit the VM when an error occurs. E.g. `java -XX:CompileCommand=compileonly,HashMap:: -version` will exit the VM after a parsing occurred. CompileCommand: An error occurred during parsing Error: Could not parse method pattern Line: 'compileonly,HashMap::' Usage: '-XX:CompileCommand=,' - to set boolean option to true Usage: '-XX:CompileCommand=,,' Use: '-XX:CompileCommand=help' for more information and to list all option. Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. ### Updated Tests Updated all tests to now expect an error code `1` for wrong `CompileCommand` - Commit messages: - update copyright - fix trailing whitespace - Update CheckCompileCommandOption.java - Update PrintIdealPhaseTest.java - update OptionTest.java: _id is not a valid intrinsics - update CompilerConfigFileWarning.java - adjust Scenario.java for failing CompileCommands - JDK-8282797: CompileCommand parsing errors should exit VM Changes: https://git.openjdk.org/jdk/pull/13753/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13753=00 Issue: https://bugs.openjdk.org/browse/JDK-8282797 Stats: 164 lines in 24 files changed: 46 ins; 7 del; 111 mod Patch: https://git.openjdk.org/jdk/pull/13753.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13753/head:pull/13753 PR: https://git.openjdk.org/jdk/pull/13753
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
On Tue, 23 May 2023 15:31:49 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test comment update Looks good to me, but it would be good to get this reviewed by someone with more knowledge of native character encoding. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14083#pullrequestreview-1439991999
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview)
On Mon, 22 May 2023 18:42:02 GMT, Alan Bateman wrote: >> src/java.base/share/classes/java/lang/ScopedValue.java line 399: >> >>> 397: var prevSnapshot = scopedValueBindings(); >>> 398: var newSnapshot = new Snapshot(this, prevSnapshot); >>> 399: return runWith(newSnapshot, new CallableAdapter(op)); >> >> Can we just do this instead? >> Suggestion: >> >> return runWith(newSnapshot, op::get); >> >> IIUC the current approach is to avoid the dynamic creation of a class via >> the invoke dynamic? I don't fully understand the comment about release >> fencing. > > @theRealAph will want to comment on this as it is very performance sensitive. > I think CallableAdapter.s is non-final to avoid the release fence. That's right. The problem is that we can never get rid of the release fence, apparently even when the instance of the adapter is scalar replaced. I imagine that'll get fixed one day, but this is internal JDK code. - PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1202492523
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview)
On Thu, 11 May 2023 13:08:55 GMT, Alan Bateman wrote:
> 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
src/java.base/share/classes/java/lang/ScopedValue.java line 252:
> 250: * bound (or rebound) to {@code v1}, and {@code k2} bound (or
> rebound) to {@code v2}.
> 251: * {@snippet lang=java :
> 252: * // @link substring="runWhere" target="#runWhere(ScopedValue,
> Object)" :
Is this correct?
src/java.base/share/classes/java/lang/ScopedValue.java line 399:
> 397: var prevSnapshot = scopedValueBindings();
> 398: var newSnapshot = new Snapshot(this, prevSnapshot);
> 399: return runWith(newSnapshot, new CallableAdapter(op));
Can we just do this instead?
Suggestion:
return runWith(newSnapshot, op::get);
IIUC the current approach is to avoid the dynamic creation of a class via the
invoke dynamic? I don't fully understand the comment about release fencing.
src/java.base/share/classes/java/lang/ScopedValue.java line 408:
> 406: // runtime bytecode generation nor any release fencing.
> 407: private static final class CallableAdapter implements
> Callable {
> 408: private Supplier s;
Suggestion:
private final Supplier s;
src/java.base/share/classes/java/lang/ScopedValue.java line 558:
> 556: * This method is implemented to be equivalent to:
> 557: * {@snippet lang=java :
> 558: * // @link substring="call" target="Carrier#call(Callable)" :
Suggestion:
* // @link substring="get" target="Carrier#get(Supplier)" :
?
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line
159:
> 157: * The example uses {@link Supplier#get()} to get the result of each
> subtask. Using
> 158: * {@code Supplier} instead of {@code Subtask} is preferred for common
> cases where the
> 159: * the object returned by fork is only used to get the result of a
> subtask that completed
Suggestion:
* {@code Supplier} instead of {@code Subtask} is preferred for common cases
where
* the object returned by fork is only used to get the result of a subtask that
completed
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line
1077:
> 1075: }
> 1076:
> 1077: throw new IllegalStateException("No completed subtasks");
I believe it may be possible to implement as the following if you so wish:
Suggestion:
return result(ExecutionException::new);
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line
1251:
> 1249: Throwable exception = firstException;
> 1250: if (exception != null)
> 1251: throw new ExecutionException(exception);
Suggestion:
throwIfFailed(ExecutionException::new);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1199509233
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1200863513
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1199502950
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1199508974
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1200910221
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1201014854
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1201028382
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
On Tue, 23 May 2023 15:18:36 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test comment update test/jdk/java/net/InetAddress/NullCharDriver.java line 29: > 27: * @modules java.base/java.net > 28: * @compile/module=java.base java/net/NullChar.java > 29: * @summary foo It would be good to have a better summary ;-) - PR Review Comment: https://git.openjdk.org/jdk/pull/14083#discussion_r1202541587
Re: RFR: 8306647: Implementation of Structured Concurrency (Preview)
On Sat, 20 May 2023 00:27:23 GMT, Paul Sandoz wrote:
>> 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
>
> src/java.base/share/classes/java/lang/ScopedValue.java line 252:
>
>> 250: * bound (or rebound) to {@code v1}, and {@code k2} bound (or
>> rebound) to {@code v2}.
>> 251: * {@snippet lang=java :
>> 252: * // @link substring="runWhere" target="#runWhere(ScopedValue,
>> Object)" :
>
> Is this correct?
Good catch, might have been the victim of search and replace, it should
continue to link to ScopedValue.where.
> src/java.base/share/classes/java/lang/ScopedValue.java line 399:
>
>> 397: var prevSnapshot = scopedValueBindings();
>> 398: var newSnapshot = new Snapshot(this, prevSnapshot);
>> 399: return runWith(newSnapshot, new CallableAdapter(op));
>
> Can we just do this instead?
> Suggestion:
>
> return runWith(newSnapshot, op::get);
>
> IIUC the current approach is to avoid the dynamic creation of a class via the
> invoke dynamic? I don't fully understand the comment about release fencing.
@theRealAph will want to comment on this as it is very performance sensitive. I
think CallableAdapter.s is non-final to avoid the release fence.
> src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java
> line 1077:
>
>> 1075: }
>> 1076:
>> 1077: throw new IllegalStateException("No completed subtasks");
>
> I believe it may be possible to implement as the following if you so wish:
> Suggestion:
>
> return result(ExecutionException::new);
Good, avoids duplication.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1199577982
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1200899135
PR Review Comment: https://git.openjdk.org/jdk/pull/13932#discussion_r1202097524
RFR: 8308645: Javadoc of FFM API needs to be refreshed
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 throw because of an overflow during offset computation. Finally, I've left most of the snippets alone, as they are being dealt with in https://github.com/openjdk/jdk/pull/14030 - Commit messages: - Merge branch 'master' into javadoc_fixes - Add overflow tests - Fix overflow @throw in MemorySegment::copy - Add overflow @throws clause on SegmentAllocator::allocateArray - More fixes to MemoryLayout - Finish ValueLayout - Merge branch 'master' into javadoc_fixes - Finish SymbolLookup - Finish SequenceLayout/StructLayout/UnionLayout - Finish SegmentAllocator - ... and 11 more: https://git.openjdk.org/jdk/compare/c0c4d771...afb7360e Changes: https://git.openjdk.org/jdk/pull/14098/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14098=00 Issue: https://bugs.openjdk.org/browse/JDK-8308645 Stats: 818 lines in 23 files changed: 269 ins; 116 del; 433 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: test comment update - Changes: - all: https://git.openjdk.org/jdk/pull/14083/files - new: https://git.openjdk.org/jdk/pull/14083/files/8cf24635..0acc456a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14083=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14083=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
RFR: 8306647: Implementation of Structured Concurrency (Preview)
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 - Commit messages: - Test should not be in update for main line - Sync with loom repo - Sync up tests frmo loom repo - Sync up with loom repo - Sync update API/impl/tests - Merge - Sync up with loom repo - Merge - Initial commit Changes: https://git.openjdk.org/jdk/pull/13932/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13932=00 Issue: https://bugs.openjdk.org/browse/JDK-8306647 Stats: 9389 lines in 42 files changed: 4995 ins; 4330 del; 64 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
On Tue, 23 May 2023 11:48:59 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 throw because of an ... Javadoc: https://cr.openjdk.org/~mcimadamore/jdk/8308645/javadoc/java.base/java/lang/foreign/package-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1559552992
Re: RFR: 8308549: Classfile API should fail to generate over-sized Code attribute
On Tue, 23 May 2023 12:54:20 GMT, Adam Sotona wrote: > Classfile API allowed to generate Code attribute exceeding the 65k limit. No > exception has been thrown during class generation and the class failed > verification later during class loading. > This patch adds Code size limit check throwing IllegalArgumentException. > The patch also adds similar check for constant pool size limit to avoid > generation class file with corrupted constant pool. > Two new tests are added to check response on oversized Code attribute and > constant pool. > `VerifierImpl` is extended to check Code attribute size as a part of class > verification process. > > Please review. > > Thanks, > Adam On a side note, does Classfile API reject methods with too many slots (locals) (MethodTypeDesc can represent parameter lists with over 255 slots) or stack (operand)? - PR Comment: https://git.openjdk.org/jdk/pull/14100#issuecomment-1559642335
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
On Tue, 23 May 2023 07:46:08 GMT, Richard Reingruber wrote: >> src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 163: >> >>> 161: // The Parameter Save Area needs to be at least 8 slots for ABIv1. >>> 162: // ABIv2 allows omitting it when all parameters can get passed in >>> registers. We currently don't optimize this. >>> 163: // For ABIv2, we only need (_input_registers.length() > 8) ? >>> _input_registers.length() : 0 >> >> The PSA is also needed if the parameter list is variable in length. Is the >> expression `(_input_registers.length() > 8) ? _input_registers.length() : 0` >> correct in that case too? >> Otherwise: `ABIv2 allows omitting it if the callee's prototype indicates >> that stack parameters are not expected. We currently don't optimize this.` > > Ok, I see now. This is not obvious though. There are a few layers of > abstraction at play which hide this. A comment is needed. Maybe like this: > ```c++ > // With ABIv1 a Parameter Save Area of at least 8 double words is > always needed. > // ABIv2 allows omitting it if the callee's prototype indicates > that stack parameters are not expected. > // We currently don't optimize this (see DowncallStubGenerator in > the backend). > if (reg == null) return stack; I believe omitting the PSA is wrong for varargs, but we don't have this information in the backend. So, I think we should simply not optimize it. Reserving 64 Byte stack space should be affordable for a downcall even if it's not always needed. The Java side could compute it, but there's no way to pass this information to the backend. I've improved the comments. Please take a look. - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1202235085
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v36]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
> Resolved after merging of
> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE). Update: T...
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Improve comments about the Parameter Save Area.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/b912155b..5a00d804
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=35
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=34-35
Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v5]
On Tue, 23 May 2023 14:21:06 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:
>
> Test: added better description, enabled for musl and improved wording in
> logs/exceptions
ok with latest doc and naming updates.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13956#pullrequestreview-1439795305
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]
On Tue, 23 May 2023 05:34:15 GMT, Thomas Stuefe wrote: >> Volker Simonis has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into JDK-8307990 >> - Address comments from tstuefe and RogerRiggs >> - REALLY adding the test :) >> - Added test case >> - 8307990: jspawnhelper must close its writing side of a pipe before >> reading from it > > test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 28: > >> 26: * @test >> 27: * @bug 8307990 >> 28: * @requires (os.family == "linux" & !vm.musl) > > Muslc supports posix_spawn. > > I tested your patch on Alpine, it works and the test works too. Please enable > for musl. Thanks, will do. > test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 92: > >> 90: } >> 91: } >> 92: > > Small nits, mainly to make this test easier to understand to the casual > reader: > - consistent naming: we have "simulateCrashInChild" and > "simulateCrashInParent", good, but then we have "childCode", which is > actually the code executed in the parent, right? > - simulateCrashInChild and simulateCrashInParent could be merged into one > function that does: > - spawn parent with env var > - read output, scan for `"posix_spawn:XXX"` > - parent must have exit code != 0 and should not have timed out > - if XXX is not 0, check grandchild pid (must not be a live process) This sounds reasonable. I've renamed `childCode()` to `parentCode()`, streamlined the wording for "parent" and "child" in the various log messages and exceptions and added a comment to the `main()` method which explains the working of the test and the meaning of "parent" and "child" in the output. I've not merged `simulateCrashInChild()` and `simulateCrashInParent()` because there are some subtle differences between the two (and now with the improved "parent"/"child" naming even more :) and I don't think that a merged version will improve the readability. - PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1202266007 PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1202365663
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v5]
> 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:
Test: added better description, enabled for musl and improved wording in
logs/exceptions
-
Changes:
- all: https://git.openjdk.org/jdk/pull/13956/files
- new: https://git.openjdk.org/jdk/pull/13956/files/6649b725..facbdcf3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=13956=04
- incr: https://webrevs.openjdk.org/?repo=jdk=13956=03-04
Stats: 28 lines in 1 file changed: 12 ins; 0 del; 16 mod
Patch: https://git.openjdk.org/jdk/pull/13956.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13956/head:pull/13956
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. @RogerRiggs , @tstuefe thanks for your reviews! - PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1559423017
RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters
This PR creates a new version of the JNI utility function JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which performs additional validation of the returned string, namely that it does not contain any embedded NULL characters. If any such characters are found the function returns NULL with an IAE pending. The change also switches usage in the networking native code to use the new function. This cautious approach was taken rather than changing the behavior of the existing function as each native code area needs to review the effect of making the switch. Otherwise, surprising behavior changes might occur (eg undocumented IAE being thrown to user code instead of some other exception). - Commit messages: - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - Merge branch 'master' into nullStrings - first impl Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14083=00 Issue: https://bugs.openjdk.org/browse/JDK-8300038 Stats: 183 lines in 9 files changed: 163 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
RFR: 8308549: Classfile API should fail to generate over-sized Code attribute
Classfile API allowed to generate Code attribute exceeding the 65k limit. No exception has been thrown during class generation and the class failed verification later during class loading. This patch adds Code size limit check throwing IllegalArgumentException. The patch also adds similar check for constant pool size limit to avoid generation class file with corrupted constant pool. Two new tests are added to check response on oversized Code attribute and constant pool. `VerifierImpl` is extended to check Code attribute size as a part of class verification process. Please review. Thanks, Adam - Commit messages: - 8308549: Classfile API should fail to generate over-sized Code attribute Changes: https://git.openjdk.org/jdk/pull/14100/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14100=00 Issue: https://bugs.openjdk.org/browse/JDK-8308549 Stats: 40 lines in 4 files changed: 35 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14100.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14100/head:pull/14100 PR: https://git.openjdk.org/jdk/pull/14100
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out [v3]
On Tue, 23 May 2023 09:02:27 GMT, Viktor Klang wrote: >> Doubling the max heap size since not all GCs might have compressed OOPs, so >> this change should make this test more robust. >> >> Cleared with @DougLea > > Viktor Klang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Removing OOMEInAQS from zgc problem list > - JDK-8298066: Increasing max heap size for OOMEinAQS since not all GCs > might have compressed oops Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14082#pullrequestreview-1439681598
Re: Classes used in method body are loaded lazily or eagerly depending on method return type
I think the problem here is that you are deleting a class in a nest.
During the verification of BaseClassReturner.getObject(), access control
(JVMS, 5.4.4, second half) determines that the nest is broken, as
ChildClass is not present anymore.
If you move ChildClass out of TestLoading so that it becomes a top-level
class extending TestLoading.BaseClass, and if you adapt the line that
initializes the local var classFileB to refer to the new location, the
code will not throw, despite ChildClass being deleted.
Greetings
Raffaello
On 2023-05-23 13:20, Сергей Цыпанов wrote:
Hello,
originally this question was asked here:
https://stackoverflow.com/q/76260269/12473843,
the code sample for reproduction will be put below or can be found via the link
The issue is about eager/lazy loading of a class depending on method return
type.
If one runs the code below with Java 11-19 it will fail with
NoClassDefFoundError (this is expected as delete class file for ChildClass):
java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:390)
at java.base/java.lang.Class.forName(Class.java:381)
at org.example.TestLoading.loadMyClass(TestLoading.java:29)
at org.example.TestLoading.main(TestLoading.java:23)
Caused by: java.lang.ClassNotFoundException: org.example.TestLoading$ChildClass
at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
... 5 more
As of Java 20 chapter 12.4.1 of JLS states:
-
A class or interface T will be initialized immediately before the first
occurrence of any one of the following:
- T is a class and an instance of T is created.
- a static method declared by T is invoked.
- a static field declared by T is assigned.
- a static field declared by T is used and the field is not a constant variable
(§4.12.4).
When a class is initialized, its superclasses are initialized (if they have not
been previously initialized), as well as any superinterfaces (§8.1.5) that
declare any default methods (§9.4.3) (if they have not been previously
initialized).
Initialization of an interface does not, of itself, cause initialization of any
of its superinterfaces.
A reference to a static field (§8.3.1.1) causes initialization of only the
class or interface that actually declares it, even though it might be referred
to through the name of a subclass, a subinterface, or a class that implements
an interface.
Invocation of certain reflective methods in class Class and in package
java.lang.reflect also causes class or interface initialization.
A class or interface will not be initialized under any other circumstance.
-
With the code snippet we see that calling
Class.forName(ObjectReturner.class.getName()) succeeds and
Class.forName(BaseClassReturner.class.getName()) fails even though both declare
returning an instance of ChildClass.
This failure is unexpected as in the code below we don't fulfill any
requirement for class loading as of JLS 12.4.1, but the JVM still tries to load
the class.
I suspect it might be related to class file validation and/or security, because
when we run the code with -Xlog:class+init there's a reference to LinkageError
in loading log:
loading: org.example.TestLoading$BaseClassReturner...
[0.277s][info][class,init] Start class verification for:
org.example.TestLoading$BaseClassReturner
[0.277s][info][class,init] 771 Initializing
'java/lang/ReflectiveOperationException'(no method) (0x00084028)
[0.277s][info][class,init] 772 Initializing
'java/lang/ClassNotFoundException'(no method) (0x00084288)
[0.277s][info][class,init] 773 Initializing 'java/lang/LinkageError'(no method)
(0x000844f8) <
[0.277s][info][class,init] 774 Initializing 'java/lang/NoClassDefFoundError'(no
method) (0x00084758)
[0.277s][info][class,init] Verification for
org.example.TestLoading$BaseClassReturner has exception pending
'java.lang.NoClassDefFoundError org/example/TestLoading$ChildClass'
[0.277s][info][class,init] End class verification for:
org.example.TestLoading$BaseClassReturner
So I've got three questions about this:
- Does class loading depend on method's return type?
- Which part of JLS/JVM spec describes eager class loading in this case?
- Could one point out the particular piece of the VM code responsible for class
loading in this case?
Regards,
Sergey Tsypanov
Code snippet for reproduction:
public class TestLoading {
public static void main(String[] args) throws Exception {
Re: Classes used in method body are loaded lazily or eagerly depending on method return type
Hi,
On 23/05/2023 9:20 pm, Сергей Цыпанов wrote:
Hello,
originally this question was asked here:
https://stackoverflow.com/q/76260269/12473843,
the code sample for reproduction will be put below or can be found via the link
The issue is about eager/lazy loading of a class depending on method return
type.
If one runs the code below with Java 11-19 it will fail with
NoClassDefFoundError (this is expected as delete class file for ChildClass):
java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:390)
at java.base/java.lang.Class.forName(Class.java:381)
at org.example.TestLoading.loadMyClass(TestLoading.java:29)
at org.example.TestLoading.main(TestLoading.java:23)
Caused by: java.lang.ClassNotFoundException: org.example.TestLoading$ChildClass
at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
... 5 more
As of Java 20 chapter 12.4.1 of JLS states:
-
A class or interface T will be initialized immediately before the first
occurrence of any one of the following:
- T is a class and an instance of T is created.
- a static method declared by T is invoked.
- a static field declared by T is assigned.
- a static field declared by T is used and the field is not a constant variable
(§4.12.4).
When a class is initialized, its superclasses are initialized (if they have not
been previously initialized), as well as any superinterfaces (§8.1.5) that
declare any default methods (§9.4.3) (if they have not been previously
initialized).
Initialization of an interface does not, of itself, cause initialization of any
of its superinterfaces.
A reference to a static field (§8.3.1.1) causes initialization of only the
class or interface that actually declares it, even though it might be referred
to through the name of a subclass, a subinterface, or a class that implements
an interface.
Invocation of certain reflective methods in class Class and in package
java.lang.reflect also causes class or interface initialization.
A class or interface will not be initialized under any other circumstance.
-
With the code snippet we see that calling
Class.forName(ObjectReturner.class.getName()) succeeds and
Class.forName(BaseClassReturner.class.getName()) fails even though both declare
returning an instance of ChildClass.
This failure is unexpected as in the code below we don't fulfill any
requirement for class loading as of JLS 12.4.1, but the JVM still tries to load
the class.
I suspect it might be related to class file validation and/or security, because
when we run the code with -Xlog:class+init there's a reference to LinkageError
in loading log:
loading: org.example.TestLoading$BaseClassReturner...
[0.277s][info][class,init] Start class verification for:
org.example.TestLoading$BaseClassReturner
[0.277s][info][class,init] 771 Initializing
'java/lang/ReflectiveOperationException'(no method) (0x00084028)
[0.277s][info][class,init] 772 Initializing
'java/lang/ClassNotFoundException'(no method) (0x00084288)
[0.277s][info][class,init] 773 Initializing 'java/lang/LinkageError'(no method)
(0x000844f8) <
[0.277s][info][class,init] 774 Initializing 'java/lang/NoClassDefFoundError'(no
method) (0x00084758)
[0.277s][info][class,init] Verification for
org.example.TestLoading$BaseClassReturner has exception pending
'java.lang.NoClassDefFoundError org/example/TestLoading$ChildClass'
[0.277s][info][class,init] End class verification for:
org.example.TestLoading$BaseClassReturner
So I've got three questions about this:
- Does class loading depend on method's return type?
- Which part of JLS/JVM spec describes eager class loading in this case?
- Could one point out the particular piece of the VM code responsible for class
loading in this case?
Verification can require classes to be loaded to perform the
verification - see JVMS 4.10 for all the details.
Note you seem to be confusing class loading with class initialization
above. The rules for initialization are very precise; the rules for
loading are far more lax.
Cheers,
David
-
Regards,
Sergey Tsypanov
Code snippet for reproduction:
public class TestLoading {
public static void main(String[] args) throws Exception {
Class.forName(BaseClass.class.getName());
URL classFileB = TestLoading.class.getResource(TestLoading.class.getSimpleName() +
"$ChildClass.class");
if (classFileB != null) {
if (!"file".equals(classFileB.getProtocol())) {
throw new
Classes used in method body are loaded lazily or eagerly depending on method return type
Hello,
originally this question was asked here:
https://stackoverflow.com/q/76260269/12473843,
the code sample for reproduction will be put below or can be found via the link
The issue is about eager/lazy loading of a class depending on method return
type.
If one runs the code below with Java 11-19 it will fail with
NoClassDefFoundError (this is expected as delete class file for ChildClass):
java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:390)
at java.base/java.lang.Class.forName(Class.java:381)
at org.example.TestLoading.loadMyClass(TestLoading.java:29)
at org.example.TestLoading.main(TestLoading.java:23)
Caused by: java.lang.ClassNotFoundException: org.example.TestLoading$ChildClass
at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
... 5 more
As of Java 20 chapter 12.4.1 of JLS states:
-
A class or interface T will be initialized immediately before the first
occurrence of any one of the following:
- T is a class and an instance of T is created.
- a static method declared by T is invoked.
- a static field declared by T is assigned.
- a static field declared by T is used and the field is not a constant variable
(§4.12.4).
When a class is initialized, its superclasses are initialized (if they have not
been previously initialized), as well as any superinterfaces (§8.1.5) that
declare any default methods (§9.4.3) (if they have not been previously
initialized).
Initialization of an interface does not, of itself, cause initialization of any
of its superinterfaces.
A reference to a static field (§8.3.1.1) causes initialization of only the
class or interface that actually declares it, even though it might be referred
to through the name of a subclass, a subinterface, or a class that implements
an interface.
Invocation of certain reflective methods in class Class and in package
java.lang.reflect also causes class or interface initialization.
A class or interface will not be initialized under any other circumstance.
-
With the code snippet we see that calling
Class.forName(ObjectReturner.class.getName()) succeeds and
Class.forName(BaseClassReturner.class.getName()) fails even though both declare
returning an instance of ChildClass.
This failure is unexpected as in the code below we don't fulfill any
requirement for class loading as of JLS 12.4.1, but the JVM still tries to load
the class.
I suspect it might be related to class file validation and/or security, because
when we run the code with -Xlog:class+init there's a reference to LinkageError
in loading log:
loading: org.example.TestLoading$BaseClassReturner...
[0.277s][info][class,init] Start class verification for:
org.example.TestLoading$BaseClassReturner
[0.277s][info][class,init] 771 Initializing
'java/lang/ReflectiveOperationException'(no method) (0x00084028)
[0.277s][info][class,init] 772 Initializing
'java/lang/ClassNotFoundException'(no method) (0x00084288)
[0.277s][info][class,init] 773 Initializing 'java/lang/LinkageError'(no method)
(0x000844f8) <
[0.277s][info][class,init] 774 Initializing 'java/lang/NoClassDefFoundError'(no
method) (0x00084758)
[0.277s][info][class,init] Verification for
org.example.TestLoading$BaseClassReturner has exception pending
'java.lang.NoClassDefFoundError org/example/TestLoading$ChildClass'
[0.277s][info][class,init] End class verification for:
org.example.TestLoading$BaseClassReturner
So I've got three questions about this:
- Does class loading depend on method's return type?
- Which part of JLS/JVM spec describes eager class loading in this case?
- Could one point out the particular piece of the VM code responsible for class
loading in this case?
Regards,
Sergey Tsypanov
Code snippet for reproduction:
public class TestLoading {
public static void main(String[] args) throws Exception {
Class.forName(BaseClass.class.getName());
URL classFileB =
TestLoading.class.getResource(TestLoading.class.getSimpleName() +
"$ChildClass.class");
if (classFileB != null) {
if (!"file".equals(classFileB.getProtocol())) {
throw new UnsupportedOperationException();
}
Path path = new File(classFileB.getPath()).toPath();
System.out.println("deleting: " + path);
Files.delete(path);
}
loadMyClass(ObjectReturner.class.getName());
loadMyClass(BaseClassReturner.class.getName());
}
private static void loadMyClass(String name) {
System.out.println("loading: " + name +
Re: EnumSet doesn't implement SequencedSet
Hello! Looks like this was overlooked. On the other hand, it could even implement SortedSet or Navigable set, as it's essentially not just ordered but sorted, according to the enum natural order. Same for EnumMap. With best regards, Tagir Valeev On Tue, May 23, 2023, 11:46 Andrey Turbanov wrote: > Hello. > I've noticed that EnumSet unfortunately doesn't implement the newly > introduced interface SequencedSet. > From the first look it fits into it. > Is there a reason for that? Sorry if it was already discussed. > > Andrey Turbanov >
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]
On Mon, 22 May 2023 10:22:16 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 with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into JDK-8307990
> - Address comments from tstuefe and RogerRiggs
> - REALLY adding the test :)
> - Added test case
> - 8307990: jspawnhelper must close its writing side of a pipe before reading
> from it
Looks good.
Please enable for muslc (see remark). Otherwise, all other remarks are nits and
I leave it up to you whether you accept them. I don't need another review.
src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 148:
> 146: r = sscanf (argv[argc-1], "%d:%d:%d", , , );
> 147: if (r == 3 && fcntl(fdinr, F_GETFD) != -1 && fcntl(fdinw, F_GETFD)
> != -1) {
> 148: fstat(fdinr, );
Preexisting, and possibly for another RFE:
- why don't we test fdout as well?
- we probably could make sure the output fds are valid for us (S_ISREG |
S_ISFIFO | (possibly?) S_ISSOCK)
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 28:
> 26: * @test
> 27: * @bug 8307990
> 28: * @requires (os.family == "linux" & !vm.musl)
Muslc supports posix_spawn.
I tested your patch on Alpine, it works and the test works too. Please enable
for musl.
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 92:
> 90: }
> 91: }
> 92:
Small nits, mainly to make this test easier to understand to the casual reader:
- consistent naming: we have "simulateCrashInChild" and
"simulateCrashInParent", good, but then we have "childCode", which is actually
the code executed in the parent, right?
- simulateCrashInChild and simulateCrashInParent could be merged into one
function that does:
- spawn parent with env var
- read output, scan for `"posix_spawn:XXX"`
- parent must have exit code != 0 and should not have timed out
- if XXX is not 0, check grandchild pid (must not be a live process)
-
Marked as reviewed by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13956#pullrequestreview-1438535260
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1201539615
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1201548057
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1201770607
Re: RFR: JDK-8027711: Unify wildcarding syntax for CompileCommand and CompileOnly [v2]
> At the moment `CompileCommand` and `CompileOnly` use different syntax for > matching methods. > > ### Old CompileOnly format > - matching a **method name** with **class name** and **package name**: > `-XX:CompileOnly=package/path/Class.method` > `-XX:CompileOnly=package/path/Class::method` > `-XX:CompileOnly=package.path.Class::method` > BUT NOT `-XX:CompileOnly=package.path.Class.method` > > - just matching a **single method name**: > `-XX:CompileOnly=.hashCode` > `-XX:CompileOnly=::hashCode` > BUT NOT `-XX:CompileOnly=hashCode` > > - Matching **all method names** in a **class name** with **package name** > `-XX:CompileOnly=java/lang/String` > BUT NOT `-XX:CompileOnly=java/lang/String.` > BUT NOT `-XX:CompileOnly=java.lang.String` > BUT NOT `-XX:CompileOnly=java.lang.String::` (This is actually a bug) > BUT NOT `-XX:CompileOnly=String` > BUT NOT `-XX:CompileOnly=String.` > BUT NOT `-XX:CompileOnly=String::` > > - Matching **all method names** in a **class name** with **NO package name** > `-XX:CompileOnly=String` > BUT NOT `-XX:CompileOnly=String.` > BUT NOT `-XX:CompileOnly=String::` > > - There is a bug when `CompileOnly` ends with `::` where the `CompileOnly` is > just ignored > e.g. `-XX:CompileOnly=String::` compiles as many methods as when omitting the > `-XX:CompileOnly=` command > > ### CompileCommand=compileonly format > `CompileCommand` allows two different forms for paths: > - `package/path/Class.method` > - `package.path.Class::method` > > In contrary to `CompileOnly` `CompileCommand` supports wildcard matching > using `*`. `*` can appear at the beginning and/or end of a > `package.path.Class` and `method` name. > > Valid forms: > `-XX:CompileCommand=compileonly,*.lang.*::*shCo*` > `-XX:CompileCommand=compileonly,*/lang/*.*shCo*` > `-XX:CompileCommand=compileonly,java.lang.String::*` > `-XX:CompileCommand=compileonly,*::hashCode` > `-XX:CompileCommand=compileonly,*ng.String::hashC*` > `-XX:CompileCommand=compileonly,*String::hash*` > > Invalid forms (Error: Embedded * not allowed): > `-XX:CompileCommand=compileonly,java.*.String::has*Code` > > ### Use CompileCommand syntax for CompileOnly > At the moment, in some cases it is not possible to just take pattern used > with `CompileOnly` and plug it into compile command file. Syntax used by > CompileOnly is also not very intuitive. > > `CompileOnly` is convenient because it's shorter to write and takes lists of > patterns, whereas `CompileCommand` only takes one pattern per command. > > With this PR `CompileOnly` becomes an alias for `CompileC... Tobias Holenstein has updated the pull request incrementally with three additional commits since the last revision: - Update Test8211698.java - Update src/hotspot/share/compiler/compilerOracle.cpp Co-authored-by: Christian Hagedorn - Update src/hotspot/share/compiler/compilerOracle.cpp Co-authored-by: Christian Hagedorn - Changes: - all: https://git.openjdk.org/jdk/pull/13802/files - new: https://git.openjdk.org/jdk/pull/13802/files/d6fca2b8..40b17296 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13802=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13802=00-01 Stats: 7 lines in 2 files changed: 5 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13802.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13802/head:pull/13802 PR: https://git.openjdk.org/jdk/pull/13802
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out [v3]
> Doubling the max heap size since not all GCs might have compressed OOPs, so > this change should make this test more robust. > > Cleared with @DougLea Viktor Klang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Removing OOMEInAQS from zgc problem list - JDK-8298066: Increasing max heap size for OOMEinAQS since not all GCs might have compressed oops - Changes: https://git.openjdk.org/jdk/pull/14082/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14082=02 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14082/head:pull/14082 PR: https://git.openjdk.org/jdk/pull/14082
Re: RFR: JDK-8027711: Unify wildcarding syntax for CompileCommand and CompileOnly [v2]
On Tue, 23 May 2023 08:01:07 GMT, Christian Hagedorn wrote: >> Tobias Holenstein has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Update Test8211698.java >> - Update src/hotspot/share/compiler/compilerOracle.cpp >> >>Co-authored-by: Christian Hagedorn >> - Update src/hotspot/share/compiler/compilerOracle.cpp >> >>Co-authored-by: Christian Hagedorn > > test/hotspot/jtreg/compiler/loopopts/Test8211698.java line 57: > >> 55: } >> 56: } >> 57: > > You can leave this empty line in at the end of the file. done - PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201860361
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out [v2]
On Tue, 23 May 2023 08:27:39 GMT, Viktor Klang wrote: >> Doubling the max heap size since not all GCs might have compressed OOPs, so >> this change should make this test more robust. >> >> Cleared with @DougLea > > Viktor Klang has refreshed the contents of this pull request, and previous > commits have been removed. Incremental views are not available. (Addressed conflict, hence the force-push) - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1558845566
Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]
> 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: > - [ ] GHA (still running) > - [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: - all: https://git.openjdk.org/jdk/pull/14090/files - new: https://git.openjdk.org/jdk/pull/14090/files/761f410e..8cca3671 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14090=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14090=00-01 Stats: 18326 lines in 687 files changed: 9112 ins; 4973 del; 4241 mod Patch: https://git.openjdk.org/jdk/pull/14090.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14090/head:pull/14090 PR: https://git.openjdk.org/jdk/pull/14090
EnumSet doesn't implement SequencedSet
Hello. I've noticed that EnumSet unfortunately doesn't implement the newly introduced interface SequencedSet. >From the first look it fits into it. Is there a reason for that? Sorry if it was already discussed. Andrey Turbanov
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]
On Mon, 22 May 2023 10:22:16 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 with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into JDK-8307990
> - Address comments from tstuefe and RogerRiggs
> - REALLY adding the test :)
> - Added test case
> - 8307990: jspawnhelper must close its writing side of a pipe before reading
> from it
> Hey, chiming on this conversation, because I believe it will fix an issue we
> encountered since, upgrading from JDK 11 to 17. In our application we spawn a
> lot of processes over time and noticed that more and more JVM threads get
> stuck in the native `ProcessImpl.forkAndExec`.
>
> Thread Dump Excerpt
> After some investigation we saw that we have `jspawnhelper` processes
> corresponding to the number of the stuck threads. `/proc/*/task/{id}/syscall`
> shows that all of these processes are currently reading from the pipe.
>
> Our theory is that the write calls from the parent thread were interrupted
> resulting in an incomplete write.
>
> https://github.com/openjdk/jdk/blob/69f508a2ac344eb61cef7be985348873b8265171/src/java.base/unix/native/libjava/ProcessImpl_md.c#L556-L559
>
>
> This would cause the `readFully` on the `jspawnhelper` side to wait for the
> remaining data.
> As the number of bytes written is not checked on the writer side it would
> continue and wait for the alive ping from the `jspawnhelper` which results in
> a deadlock.
> I would be interested in a reproducer e.g. using `gdb`, but to be honest I
> wouldn't know where to start.
> But regardless of whether this fixes our issue I think incomplete writes
> should be handled according to specification.
> I think the `restartableWrites` in this PR do that?
> We also mitigated by falling back to `VFORK` as was also mentioned in [this
> comment](https://github.com/openjdk/jdk/pull/13956#issuecomment-154042).
>
> Maybe it would be worth it to mention our bug situation in the release notes
> as well? Thank you and please let me know if I got something wrong or there
> is anything I could provide to help!
@mlichtblau thanks a lot for your report. I saw some similar reports from our
internal Maven builds (have to check though if they are exactly the same). Your
analysis of the problem seems sound and I hope the restartable writes will
indeed fix the problem.
I'll definitely add your example to the release notes and I'll try if I can
come up with a test that reproduces your issue.
-
PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1558794718
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out [v2]
On Tue, 23 May 2023 08:22:22 GMT, Viktor Klang wrote: >> Doubling the max heap size since not all GCs might have compressed OOPs, so >> this change should make this test more robust. >> >> Cleared with @DougLea > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Removing OOMEInAQS from zgc problem list Thanks for removing it from the exclude list. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14082#pullrequestreview-1438956387
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
On Tue, 23 May 2023 07:19:42 GMT, David Holmes wrote: >> @AlanBateman I wonder, perhaps it makes more sense, as you say, to clear the >> problem list together with this, and resurrect it in case it spuriously >> resurfaces. It's always a challenge to prove a negative. :) > > @viktorklang-ora if you think the test is fixed then you have to remove it > from the ProblemList files so that it actually gets tested again. @dholmes-ora done - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1558766951
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out [v2]
> Doubling the max heap size since not all GCs might have compressed OOPs, so > this change should make this test more robust. > > Cleared with @DougLea Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Removing OOMEInAQS from zgc problem list - Changes: - all: https://git.openjdk.org/jdk/pull/14082/files - new: https://git.openjdk.org/jdk/pull/14082/files/2145e6fb..9d2335d2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14082=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14082=00-01 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14082/head:pull/14082 PR: https://git.openjdk.org/jdk/pull/14082
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]
On Mon, 22 May 2023 10:22:16 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 with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into JDK-8307990
> - Address comments from tstuefe and RogerRiggs
> - REALLY adding the test :)
> - Added test case
> - 8307990: jspawnhelper must close its writing side of a pipe before reading
> from it
Hey, chiming on this conversation, because I believe it will fix an issue we
encountered since, upgrading from JDK 11 to 17.
In our application we spawn a lot of processes over time and noticed that more
and more JVM threads get stuck in the native `ProcessImpl.forkAndExec`.
Thread Dump Excerpt
Thread dump contains multiple threads in this state.
"redacted" #954 prio=5 os_prio=0 cpu=59718.38ms elapsed=286286.30s
tid=0x7f62f80201e0 nid=0x3c16 runnable [0x7f609ec56000]
java.lang.Thread.State: RUNNABLE
at java.lang.ProcessImpl.forkAndExec(java.base@17.0.7/Native Method)
at java.lang.ProcessImpl.(java.base@17.0.7/ProcessImpl.java:314)
at java.lang.ProcessImpl.start(java.base@17.0.7/ProcessImpl.java:244)
at
java.lang.ProcessBuilder.start(java.base@17.0.7/ProcessBuilder.java:1110)
at
java.lang.ProcessBuilder.start(java.base@17.0.7/ProcessBuilder.java:1073)
After some investigation we saw that we have `jspawnhelper` processes
corresponding to the number of the stuck threads.
`/proc/*/task/{id}/syscall` shows that all of these processes are currently
reading from the pipe.
Our theory is that the write calls from the parent thread were interrupted
resulting in an incomplete write.
https://github.com/openjdk/jdk/blob/69f508a2ac344eb61cef7be985348873b8265171/src/java.base/unix/native/libjava/ProcessImpl_md.c#L556-L559
This would cause the `readFully` on the `jspawnhelper` side to wait for the
remaining data.
As the number of bytes written is not checked on the writer side it would
continue and wait for the alive ping from the `jspawnhelper` which results in a
deadlock.
I would be interested in a reproducer e.g. using `gdb`, but to be honest I
wouldn't know where to start.
But regardless of whether this fixes our issue I think incomplete writes should
be handled according to specification.
I think the `restartableWrites` in this PR do that?
We also mitigated by falling back to `VFORK` as was also mentioned in [this
comment](https://github.com/openjdk/jdk/pull/13956#issuecomment-154042).
Maybe it would be worth it to mention our bug situation in the release notes as
well?
Thank you and please let me know if I got something wrong or there is anything
I could provide to help!
-
PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1558743318
Re: RFR: JDK-8027711: Unify wildcarding syntax for CompileCommand and CompileOnly
On Thu, 4 May 2023 13:36:22 GMT, Tobias Holenstein
wrote:
> At the moment `CompileCommand` and `CompileOnly` use different syntax for
> matching methods.
>
> ### Old CompileOnly format
> - matching a **method name** with **class name** and **package name**:
> `-XX:CompileOnly=package/path/Class.method`
> `-XX:CompileOnly=package/path/Class::method`
> `-XX:CompileOnly=package.path.Class::method`
> BUT NOT `-XX:CompileOnly=package.path.Class.method`
>
> - just matching a **single method name**:
> `-XX:CompileOnly=.hashCode`
> `-XX:CompileOnly=::hashCode`
> BUT NOT `-XX:CompileOnly=hashCode`
>
> - Matching **all method names** in a **class name** with **package name**
> `-XX:CompileOnly=java/lang/String`
> BUT NOT `-XX:CompileOnly=java/lang/String.`
> BUT NOT `-XX:CompileOnly=java.lang.String`
> BUT NOT `-XX:CompileOnly=java.lang.String::` (This is actually a bug)
> BUT NOT `-XX:CompileOnly=String`
> BUT NOT `-XX:CompileOnly=String.`
> BUT NOT `-XX:CompileOnly=String::`
>
> - Matching **all method names** in a **class name** with **NO package name**
> `-XX:CompileOnly=String`
> BUT NOT `-XX:CompileOnly=String.`
> BUT NOT `-XX:CompileOnly=String::`
>
> - There is a bug when `CompileOnly` ends with `::` where the `CompileOnly` is
> just ignored
> e.g. `-XX:CompileOnly=String::` compiles as many methods as when omitting the
> `-XX:CompileOnly=` command
>
> ### CompileCommand=compileonly format
> `CompileCommand` allows two different forms for paths:
> - `package/path/Class.method`
> - `package.path.Class::method`
>
> In contrary to `CompileOnly` `CompileCommand` supports wildcard matching
> using `*`. `*` can appear at the beginning and/or end of a
> `package.path.Class` and `method` name.
>
> Valid forms:
> `-XX:CompileCommand=compileonly,*.lang.*::*shCo*`
> `-XX:CompileCommand=compileonly,*/lang/*.*shCo*`
> `-XX:CompileCommand=compileonly,java.lang.String::*`
> `-XX:CompileCommand=compileonly,*::hashCode`
> `-XX:CompileCommand=compileonly,*ng.String::hashC*`
> `-XX:CompileCommand=compileonly,*String::hash*`
>
> Invalid forms (Error: Embedded * not allowed):
> `-XX:CompileCommand=compileonly,java.*.String::has*Code`
>
> ### Use CompileCommand syntax for CompileOnly
> At the moment, in some cases it is not possible to just take pattern used
> with `CompileOnly` and plug it into compile command file. Syntax used by
> CompileOnly is also not very intuitive.
>
> `CompileOnly` is convenient because it's shorter to write and takes lists of
> patterns, whereas `CompileCommand` only takes one pattern per command.
>
> With this PR `CompileOnly` becomes an alias for `CompileC...
Nice unification! I just have some small comments. Otherwise, looks good!
src/hotspot/share/compiler/compilerOracle.cpp line 1015:
> 1013:
> 1014: void CompilerOracle::parse_compile_only(char* line) {
> 1015: if (line[0] == '\0') return;
I suggest to use braces for single line ifs:
Suggestion:
if (line[0] == '\0') {
return;
}
src/hotspot/share/compiler/compilerOracle.cpp line 1017:
> 1015: if (line[0] == '\0') return;
> 1016: ResourceMark rm;
> 1017: char error_buf[1024] = {0};
Wouldn't it be sufficient to only initialize the first character with `\0`?
src/hotspot/share/compiler/compilerOracle.cpp line 1021:
> 1019: char* method_pattern;
> 1020: do {
> 1021: if (line[0] == '\0') break;
Suggestion:
if (line[0] == '\0') {
break;
}
test/hotspot/jtreg/compiler/loopopts/Test8211698.java line 57:
> 55: }
> 56: }
> 57:
You can leave this empty line in at the end of the file.
-
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13802#pullrequestreview-1438882451
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201705460
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201720761
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201705793
PR Review Comment: https://git.openjdk.org/jdk/pull/13802#discussion_r1201731777
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
On Mon, 22 May 2023 22:44:41 GMT, Martin Doerr wrote:
> Thanks for publishing our discussion, here. The unnecessary PSA affects other
> areas of hotspot much more than Panama. Yes, we should file an RFE. I think
> one for hotspot is sufficient as the downcall stub is part of it. I don't
> think it needs extra treatment.
That's fine. It should have a little list of areas to be revisited.
Adoc:
- Runtime calls by the interpreter, c1, and c2
- Interpreted and compiled JNI calls
- FFM API ("Panama") calls
- Runtime calls by continuation intrisics
- Runtime calls by GC barriers
Subtasks can be generated from that list.
-
PR Comment: https://git.openjdk.org/jdk/pull/12708#issuecomment-1558725262
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
On Tue, 23 May 2023 07:37:37 GMT, Richard Reingruber wrote: >> Martin Doerr has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Parameter Save Area is the correct name. > > src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 163: > >> 161: // The Parameter Save Area needs to be at least 8 slots for ABIv1. >> 162: // ABIv2 allows omitting it when all parameters can get passed in >> registers. We currently don't optimize this. >> 163: // For ABIv2, we only need (_input_registers.length() > 8) ? >> _input_registers.length() : 0 > > The PSA is also needed if the parameter list is variable in length. Is the > expression `(_input_registers.length() > 8) ? _input_registers.length() : 0` > correct in that case too? > Otherwise: `ABIv2 allows omitting it if the callee's prototype indicates that > stack parameters are not expected. We currently don't optimize this.` Ok, I see now. This is not obvious though. There are a few layers of abstraction at play which hide this. A comment is needed. Maybe like this: ```c++ // With ABIv1 a Parameter Save Area of at least 8 double words is always needed. // ABIv2 allows omitting it if the callee's prototype indicates that stack parameters are not expected. // We currently don't optimize this (see DowncallStubGenerator in the backend). if (reg == null) return stack; - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1201706335
Integrated: JDK-8308300: enhance exceptions in MappedMemoryUtils.c
On Fri, 19 May 2023 06:58:49 GMT, Matthias Baesken wrote: > MappedMemoryUtils.c can generate exceptions like those : > java.io.UncheckedIOException: java.io.IOException: Invalid argument >at > java.base/java.nio.MappedMemoryUtils.force(MappedMemoryUtils.java:105) >at java.base/java.nio.Buffer$2.force(Buffer.java:890) >at > java.base/jdk.internal.misc.ScopedMemoryAccess.forceInternal(ScopedMemoryAccess.java:317) >at > java.base/jdk.internal.misc.ScopedMemoryAccess.force(ScopedMemoryAccess.java:305) >at > java.base/jdk.internal.foreign.MappedMemorySegmentImpl.force(MappedMemorySegmentImpl.java:92) >at > TestByteBuffer.testMappedSegmentAsByteBuffer(TestByteBuffer.java:327) > > (we see this for example on AIX); there is some room for improvement, at > least the info should be added that msync failed and caused this exception. This pull request has now been integrated. Changeset: 69f508a2 Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/69f508a2ac344eb61cef7be985348873b8265171 Stats: 18 lines in 3 files changed: 13 ins; 0 del; 5 mod 8308300: enhance exceptions in MappedMemoryUtils.c Reviewed-by: alanb, clanger, bpb - PR: https://git.openjdk.org/jdk/pull/14054
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
On Mon, 22 May 2023 22:29:18 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Parameter Save Area is the correct name.
src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 163:
> 161: // The Parameter Save Area needs to be at least 8 slots for ABIv1.
> 162: // ABIv2 allows omitting it when all parameters can get passed in
> registers. We currently don't optimize this.
> 163: // For ABIv2, we only need (_input_registers.length() > 8) ?
> _input_registers.length() : 0
The PSA is also needed if the parameter list is variable in length. Is the
expression `(_input_registers.length() > 8) ? _input_registers.length() : 0`
correct in that case too?
Otherwise: `ABIv2 allows omitting it if the caller's prototype indicates that
stack parameters are not expected. We currently don't optimize this.`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1201693249
Re: RFR: 8308281: Java snippets in the FFM API need to be updated [v6]
> As the API has improved over the recent releases, not all `{@snippet ..}`
> sections have been kept in sync.
>
> This PR suggests all snippets used should be verified against real code that
> is placed in a new `snippet-files` folder and erroneous snippets are updated.
>
> In this PR, it is suggested duplicating code in the `Snippets.java` class and
> in the JavaDocs. The benefit of this is that code is directly visible in the
> code and not only in the generated javadoc.
>
> Another thing to think about is if there should be on single `Snippets.java`
> class or separate ones for each FFM class.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove last hybrid snippet
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14030/files
- new: https://git.openjdk.org/jdk/pull/14030/files/cb114ce5..06d04b24
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14030=05
- incr: https://webrevs.openjdk.org/?repo=jdk=14030=04-05
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14030.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14030/head:pull/14030
PR: https://git.openjdk.org/jdk/pull/14030
Integrated: 8308093: Disable language preview features use in JDK
On Mon, 22 May 2023 10:01:55 GMT, Adam Sotona wrote: > This patch disables temporary use of language preview features in JDK. > Temporary enabled language preview features (to allow Pattern Matching for > switch use in the Classfile API library) are no more necessary. > All redundant use of --enable-preview in the Classfile API tests are also > removed. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: c4408278 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/c4408278d1012746c91ba4c31068538850c68d30 Stats: 24 lines in 12 files changed: 0 ins; 22 del; 2 mod 8308093: Disable language preview features use in JDK Reviewed-by: liach, erikj, alanb, darcy - PR: https://git.openjdk.org/jdk/pull/14076
Re: RFR: 8308281: Java snippets in the FFM API need to be updated [v5]
> As the API has improved over the recent releases, not all `{@snippet ..}`
> sections have been kept in sync.
>
> This PR suggests all snippets used should be verified against real code that
> is placed in a new `snippet-files` folder and erroneous snippets are updated.
>
> In this PR, it is suggested duplicating code in the `Snippets.java` class and
> in the JavaDocs. The benefit of this is that code is directly visible in the
> code and not only in the generated javadoc.
>
> Another thing to think about is if there should be on single `Snippets.java`
> class or separate ones for each FFM class.
Per Minborg has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains seven commits:
- Merge master
- Remove hybrid snippets
- Use hybrid snippets for Arena
- Fix according to comments
- Reformat
- Remove file
- Add a snippet-files class with snippets
-
Changes: https://git.openjdk.org/jdk/pull/14030/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=14030=04
Stats: 750 lines in 7 files changed: 689 ins; 4 del; 57 mod
Patch: https://git.openjdk.org/jdk/pull/14030.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14030/head:pull/14030
PR: https://git.openjdk.org/jdk/pull/14030
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
On Mon, 22 May 2023 20:23:20 GMT, Viktor Klang wrote: >>> @AlanBateman My thinking was to integrate this and make sure that the >>> problem doesn't resurface before removing it from the problem list. Does >>> that make sense? 樂 >> >> No objection, it's just it would leave ProblemList-generational-zgc.txt >> listing a test against as issue marked as fixed. > > @AlanBateman I wonder, perhaps it makes more sense, as you say, to clear the > problem list together with this, and resurrect it in case it spuriously > resurfaces. It's always a challenge to prove a negative. :) @viktorklang-ora if you think the test is fixed then you have to remove it from the ProblemList files so that it actually gets tested again. - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1558679759
Integrated: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman wrote: > This is a test only change to the unit test for the ExecutorService returned > by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll > assume the threads started to execute the tasks do actually execute the task > code. The refresh in JEP 444 changed the implementation to use FutureTask, > and FutureTask checks the interrupt status before it executes the task code. > So some intermittent timeouts of the tests for interrupting invokeAll as > those tests were waiting for the task to complete. > > The main change is that the tests for interrupting invokeAll are changed to > interrupt when the main thread blocks in invokeAll. They are also changed to > check if the task started or not. The tests for interrupting invokeAny > already did this, but these are changed to use the same infrastructure to > avoid having two styles of tests in the same source file. This pull request has now been integrated. Changeset: fe8c689e Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/fe8c689eeea4fa19d4a8742f4ef1d8216f1394e6 Stats: 206 lines in 1 file changed: 87 ins; 57 del; 62 mod 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out Reviewed-by: dfuchs, jpai - PR: https://git.openjdk.org/jdk/pull/14072
Integrated: 8308235: ThreadContainer registry accumulates weak refs
On Thu, 18 May 2023 15:44:15 GMT, Alan Bateman wrote: > ThreadContainers is an internal class used to make thread pools and other > groupings of threads discoverable for observability. Some refactoring in 2021 > (in the loom repo, and before integration) accidentally changed the creation > of a weak reference so that it no longer associated with the reference queue. > The result is that stale refs aren't expunged from a CHM, leading to a memory > leak. The change to fix the issue is trivial. > > Tests for memory leaks can be problematic, often more trouble than they are > worth. I started with a test that polls the size of the internal CHM but > decided to ditch it. Instead, the test is simple. It just runs for a few > seconds creating ExecuorService implementations (including TPE, TPPE, and > FJP), unreferencing them without shutdown (so they don't terminate and > unregister). This is enough to causes OOME with product builds a small heap. This pull request has now been integrated. Changeset: ada416e6 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/ada416e66cbff6c8e631bf352acc0744c248740b Stats: 52 lines in 2 files changed: 50 ins; 0 del; 2 mod 8308235: ThreadContainer registry accumulates weak refs Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/14047
Re: RFR: 8308281: Java snippets in the FFM API need to be updated [v4]
> As the API has improved over the recent releases, not all `{@snippet ..}`
> sections have been kept in sync.
>
> This PR suggests all snippets used should be verified against real code that
> is placed in a new `snippet-files` folder and erroneous snippets are updated.
>
> In this PR, it is suggested duplicating code in the `Snippets.java` class and
> in the JavaDocs. The benefit of this is that code is directly visible in the
> code and not only in the generated javadoc.
>
> Another thing to think about is if there should be on single `Snippets.java`
> class or separate ones for each FFM class.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove hybrid snippets
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14030/files
- new: https://git.openjdk.org/jdk/pull/14030/files/df59d9fa..e99d9b33
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14030=03
- incr: https://webrevs.openjdk.org/?repo=jdk=14030=02-03
Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/14030.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14030/head:pull/14030
PR: https://git.openjdk.org/jdk/pull/14030
Withdrawn: 8307205: Downcall to clibrary printf fails on OS X AArch64
On Mon, 22 May 2023 15:21:37 GMT, Per Minborg wrote: > This PR adds a test that shows the documented way of calling `printf` works. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/14088
Re: RFR: 8307205: Downcall to clibrary printf fails on OS X AArch64
On Mon, 22 May 2023 15:21:37 GMT, Per Minborg wrote: > This PR adds a test that shows the documented way of calling `printf` works. I've closed the bug explicitly because it was not a bug. I am going to close this PR as the test is a bit redundant. - PR Comment: https://git.openjdk.org/jdk/pull/14088#issuecomment-1558619579
