Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v4]
On Tue, 22 Sep 2020 18:24:02 GMT, Gilles Duboscq wrote: > Thanks @mlchung. Do I need a second review? No need. You can integrate once you run the regression tests (I usually run tier1-tier3). - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v4]
On Tue, 22 Sep 2020 18:17:49 GMT, Mandy Chung wrote: >> Gilles Duboscq has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental >> views will show differences compared to the previous content of the PR. The >> pull request contains six new commits since >> the last revision: >> - Move LambdaEagerInitTest to test/jdk/java/lang/invoke/lambda >> - Include capturing case test, use jdk.test.lib.Assert >> - Remove disableEagerInitialization concerns from BridgeMethod.java >> - Remove extra field test from LambdaTest6 >> - Wrap long lines >> - Add dedicated test in the jdk tests > > Looks good. Thanks for the update. Thanks @mlchung. Do I need a second review? - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v4]
On Tue, 22 Sep 2020 18:03:37 GMT, Gilles Duboscq wrote: >> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced >> the >> jdk.internal.lambda.disableEagerInitialization system property to be able to >> disable eager initialization of lambda >> classes. This was necessary to prevent side effects of class initializers >> triggered by such initialization in the >> context of the GraalVM native image tool. However, the change as it is >> implemented means that the behaviour of >> non-capturing lambdas depends on the value of `disableEagerInitialization`: >> when it is false (the default) such lambdas >> are actually a singleton while when it is true, a fresh instance is returned >> every time. Programs should definitely >> _not_ rely on reference equality since the Java spec does not guarantee it. >> However, in order to separate concern and >> ease debugging such bad programs, `disableEagerInitialization` shouldn't >> influence the singleton vs. fresh instance >> behaviour of lambdas in either direction. > > Gilles Duboscq has refreshed the contents of this pull request, and previous > commits have been removed. The incremental > views will show differences compared to the previous content of the PR. Looks good. Thanks for the update. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v4]
> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the > jdk.internal.lambda.disableEagerInitialization system property to be able to > disable eager initialization of lambda > classes. This was necessary to prevent side effects of class initializers > triggered by such initialization in the > context of the GraalVM native image tool. However, the change as it is > implemented means that the behaviour of > non-capturing lambdas depends on the value of `disableEagerInitialization`: > when it is false (the default) such lambdas > are actually a singleton while when it is true, a fresh instance is returned > every time. Programs should definitely > _not_ rely on reference equality since the Java spec does not guarantee it. > However, in order to separate concern and > ease debugging such bad programs, `disableEagerInitialization` shouldn't > influence the singleton vs. fresh instance > behaviour of lambdas in either direction. Gilles Duboscq has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains six new commits since the last revision: - Move LambdaEagerInitTest to test/jdk/java/lang/invoke/lambda - Include capturing case test, use jdk.test.lib.Assert - Remove disableEagerInitialization concerns from BridgeMethod.java - Remove extra field test from LambdaTest6 - Wrap long lines - Add dedicated test in the jdk tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/93/files - new: https://git.openjdk.java.net/jdk/pull/93/files/625feb94..5525f217 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=93=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=93=02-03 Stats: 164 lines in 2 files changed: 91 ins; 73 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/93.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/93/head:pull/93 PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v3]
On Wed, 9 Sep 2020 16:41:22 GMT, Mandy Chung wrote: >> Gilles Duboscq has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove disableEagerInitialization concerns from BridgeMethod.java > > Looks good. I agree with Jan's suggestion that it's good to move the test > to test/jdk/java/lang/invoke/lambda which > is a better home for it. Thanks for updating BridgeMethod.java. I expected that the new LambdaEagerInitTest.java will be updated to verify the capturing lambda case that does not have the static `LAMBDA_INSTANCE$` field. BTW, this new regression test should be moved to `test/jdk/java/lang/invoke/lambda/` instead of `test/jdk/java/lang/invoke`. The test uses `assert`. Note that java assertion is not enabled by default. So regression tests should do an explicit check and throw runtime exception when the test fails.You can also use the JDK test library `jdk.test.lib.Asserts` or make this a testng test to use TestNG Asserts API. I updated and improved the test for your reference (send you separately). - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v3]
On Thu, 10 Sep 2020 16:34:02 GMT, Mandy Chung wrote: >> Gilles Duboscq has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove disableEagerInitialization concerns from BridgeMethod.java > > test/langtools/tools/javac/lambda/lambdaExpression/LambdaTest6.java line 33: > >> 31: * @compile LambdaTest6.java >> 32: * @run main LambdaTest6 >> 33: */ > > This line was added by JDK-8232806 > (https://hg.openjdk.java.net/jdk/jdk/rev/27c2d2a4b695). > I assume you want to move the test case for JDK-8232806 to > test/jdk/java/lang/invoke? If so, > BridgeMethod.java should be looked at too. I have removed all the `disableEagerInitialization` tests from `BridgeMethod.java`. It is now restored to its pre-JDK-8232806 state. - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v3]
> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the > jdk.internal.lambda.disableEagerInitialization system property to be able to > disable eager initialization of lambda > classes. This was necessary to prevent side effects of class initializers > triggered by such initialization in the > context of the GraalVM native image tool. However, the change as it is > implemented means that the behaviour of > non-capturing lambdas depends on the value of `disableEagerInitialization`: > when it is false (the default) such lambdas > are actually a singleton while when it is true, a fresh instance is returned > every time. Programs should definitely > _not_ rely on reference equality since the Java spec does not guarantee it. > However, in order to separate concern and > ease debugging such bad programs, `disableEagerInitialization` shouldn't > influence the singleton vs. fresh instance > behaviour of lambdas in either direction. Gilles Duboscq has updated the pull request incrementally with one additional commit since the last revision: Remove disableEagerInitialization concerns from BridgeMethod.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/93/files - new: https://git.openjdk.java.net/jdk/pull/93/files/422cd01d..625feb94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=93=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=93=01-02 Stats: 32 lines in 1 file changed: 0 ins; 22 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/93.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/93/head:pull/93 PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v2]
On Thu, 10 Sep 2020 15:23:14 GMT, Gilles Duboscq wrote: >> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced >> the >> jdk.internal.lambda.disableEagerInitialization system property to be able to >> disable eager initialization of lambda >> classes. This was necessary to prevent side effects of class initializers >> triggered by such initialization in the >> context of the GraalVM native image tool. However, the change as it is >> implemented means that the behaviour of >> non-capturing lambdas depends on the value of `disableEagerInitialization`: >> when it is false (the default) such lambdas >> are actually a singleton while when it is true, a fresh instance is returned >> every time. Programs should definitely >> _not_ rely on reference equality since the Java spec does not guarantee it. >> However, in order to separate concern and >> ease debugging such bad programs, `disableEagerInitialization` shouldn't >> influence the singleton vs. fresh instance >> behaviour of lambdas in either direction. > > Gilles Duboscq has updated the pull request incrementally with three > additional commits since the last revision: > > - Remove extra field test from LambdaTest6 > - Wrap long lines > - Add deciated test in the jdk tests test/langtools/tools/javac/lambda/lambdaExpression/LambdaTest6.java line 33: > 31: * @compile LambdaTest6.java > 32: * @run main LambdaTest6 > 33: */ This line was added by JDK-8232806 (https://hg.openjdk.java.net/jdk/jdk/rev/27c2d2a4b695). I assume you want to move the test case for JDK-8232806 to test/jdk/java/lang/invoke? If so, BridgeMethod.java should be looked at too. - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v2]
On Thu, 10 Sep 2020 11:50:04 GMT, Gilles Duboscq wrote: >> It's a good suggestion as `disableEagerInitialization` support is not part >> of javac. > > OK makes sense. I guess it's still good to clean the test comments of the old > `disableEagerInitialization` references? I have created a new test under `test/jdk/java/lang/invoke/lambda` and cleaned up `LambdaTest6.java` - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v2]
> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the > jdk.internal.lambda.disableEagerInitialization system property to be able to > disable eager initialization of lambda > classes. This was necessary to prevent side effects of class initializers > triggered by such initialization in the > context of the GraalVM native image tool. However, the change as it is > implemented means that the behaviour of > non-capturing lambdas depends on the value of `disableEagerInitialization`: > when it is false (the default) such lambdas > are actually a singleton while when it is true, a fresh instance is returned > every time. Programs should definitely > _not_ rely on reference equality since the Java spec does not guarantee it. > However, in order to separate concern and > ease debugging such bad programs, `disableEagerInitialization` shouldn't > influence the singleton vs. fresh instance > behaviour of lambdas in either direction. Gilles Duboscq has updated the pull request incrementally with three additional commits since the last revision: - Remove extra field test from LambdaTest6 - Wrap long lines - Add deciated test in the jdk tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/93/files - new: https://git.openjdk.java.net/jdk/pull/93/files/979186b8..422cd01d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=93=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=93=00-01 Stats: 111 lines in 3 files changed: 76 ins; 32 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/93.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/93/head:pull/93 PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode [v2]
On Wed, 9 Sep 2020 16:36:43 GMT, Mandy Chung wrote:
>> Gilles Duboscq has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Remove extra field test from LambdaTest6
>> - Wrap long lines
>> - Add deciated test in the jdk tests
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
> line 215:
>
>> 213: if (disableEagerInitialization) {
>> 214: try {
>> 215: return new
>> ConstantCallSite(caller.findStaticGetter(innerClass, LAMBDA_INSTANCE_FIELD,
>> invokedType.returnType()));
>
> Nit: it'd be good to wrap this long line. There are a couple long lines in
> this patch.
I have wrapped some lines that were longer than the typical line in this file.
Let me know if the wrapping looks good to you.
-
PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
On Wed, 9 Sep 2020 16:39:25 GMT, Mandy Chung wrote: >> test/langtools/tools/javac/lambda/lambdaExpression/LambdaTest6.java line 29: >> >>> 27: * @summary Add lambda tests >>> 28: * Test bridge methods for certain SAM conversions >>> 29: * Test the set of generate fields >> >> I would suggest to consider having the test under >> test/jdk/(java/lang/invoke/lambda), not under >> test/langtools/tools/javac. > > It's a good suggestion as `disableEagerInitialization` support is not part of > javac. OK makes sense. I guess it's still good to clean the test comments of the old `disableEagerInitialization` references? - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
On Wed, 9 Sep 2020 08:18:11 GMT, Gilles Duboscq wrote: > [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the > jdk.internal.lambda.disableEagerInitialization system property to be able to > disable eager initialization of lambda > classes. This was necessary to prevent side effects of class initializers > triggered by such initialization in the > context of the GraalVM native image tool. However, the change as it is > implemented means that the behaviour of > non-capturing lambdas depends on the value of `disableEagerInitialization`: > when it is false (the default) such lambdas > are actually a singleton while when it is true, a fresh instance is returned > every time. Programs should definitely > _not_ rely on reference equality since the Java spec does not guarantee it. > However, in order to separate concern and > ease debugging such bad programs, `disableEagerInitialization` shouldn't > influence the singleton vs. fresh instance > behaviour of lambdas in either direction. Looks good. I agree with Jan's suggestion that it's good to move the test to test/jdk/java/lang/invoke/lambda which is a better home for it. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
On Wed, 9 Sep 2020 12:19:04 GMT, Jan Lahoda wrote: >> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced >> the >> jdk.internal.lambda.disableEagerInitialization system property to be able to >> disable eager initialization of lambda >> classes. This was necessary to prevent side effects of class initializers >> triggered by such initialization in the >> context of the GraalVM native image tool. However, the change as it is >> implemented means that the behaviour of >> non-capturing lambdas depends on the value of `disableEagerInitialization`: >> when it is false (the default) such lambdas >> are actually a singleton while when it is true, a fresh instance is returned >> every time. Programs should definitely >> _not_ rely on reference equality since the Java spec does not guarantee it. >> However, in order to separate concern and >> ease debugging such bad programs, `disableEagerInitialization` shouldn't >> influence the singleton vs. fresh instance >> behaviour of lambdas in either direction. > > test/langtools/tools/javac/lambda/lambdaExpression/LambdaTest6.java line 29: > >> 27: * @summary Add lambda tests >> 28: * Test bridge methods for certain SAM conversions >> 29: * Test the set of generate fields > > I would suggest to consider having the test under > test/jdk/(java/lang/invoke/lambda), not under > test/langtools/tools/javac. It's a good suggestion as `disableEagerInitialization` support is not part of javac. - PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
On Wed, 9 Sep 2020 08:18:11 GMT, Gilles Duboscq wrote:
> [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the
> jdk.internal.lambda.disableEagerInitialization system property to be able to
> disable eager initialization of lambda
> classes. This was necessary to prevent side effects of class initializers
> triggered by such initialization in the
> context of the GraalVM native image tool. However, the change as it is
> implemented means that the behaviour of
> non-capturing lambdas depends on the value of `disableEagerInitialization`:
> when it is false (the default) such lambdas
> are actually a singleton while when it is true, a fresh instance is returned
> every time. Programs should definitely
> _not_ rely on reference equality since the Java spec does not guarantee it.
> However, in order to separate concern and
> ease debugging such bad programs, `disableEagerInitialization` shouldn't
> influence the singleton vs. fresh instance
> behaviour of lambdas in either direction.
src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
line 215:
> 213: if (disableEagerInitialization) {
> 214: try {
> 215: return new
> ConstantCallSite(caller.findStaticGetter(innerClass, LAMBDA_INSTANCE_FIELD,
> invokedType.returnType()));
Nit: it'd be good to wrap this long line. There are a couple long lines in
this patch.
-
PR: https://git.openjdk.java.net/jdk/pull/93
Re: RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
On Wed, 9 Sep 2020 08:18:11 GMT, Gilles Duboscq wrote: > [JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the > jdk.internal.lambda.disableEagerInitialization system property to be able to > disable eager initialization of lambda > classes. This was necessary to prevent side effects of class initializers > triggered by such initialization in the > context of the GraalVM native image tool. However, the change as it is > implemented means that the behaviour of > non-capturing lambdas depends on the value of `disableEagerInitialization`: > when it is false (the default) such lambdas > are actually a singleton while when it is true, a fresh instance is returned > every time. Programs should definitely > _not_ rely on reference equality since the Java spec does not guarantee it. > However, in order to separate concern and > ease debugging such bad programs, `disableEagerInitialization` shouldn't > influence the singleton vs. fresh instance > behaviour of lambdas in either direction. test/langtools/tools/javac/lambda/lambdaExpression/LambdaTest6.java line 29: > 27: * @summary Add lambda tests > 28: * Test bridge methods for certain SAM conversions > 29: * Test the set of generate fields I would suggest to consider having the test under test/jdk/(java/lang/invoke/lambda), not under test/langtools/tools/javac. - PR: https://git.openjdk.java.net/jdk/pull/93
RFR: 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode
[JDK-8232806](https://bugs.openjdk.java.net/browse/JDK-8232806) introduced the jdk.internal.lambda.disableEagerInitialization system property to be able to disable eager initialization of lambda classes. This was necessary to prevent side effects of class initializers triggered by such initialization in the context of the GraalVM native image tool. However, the change as it is implemented means that the behaviour of non-capturing lambdas depends on the value of `disableEagerInitialization`: when it is false (the default) such lambdas are actually a singleton while when it is true, a fresh instance is returned every time. Programs should definitely _not_ rely on reference equality since the Java spec does not guarantee it. However, in order to separate concern and ease debugging such bad programs, `disableEagerInitialization` shouldn't influence the singleton vs. fresh instance behaviour of lambdas in either direction. - Commit messages: - 8242451: ensure semantics of non-capturing lambdas are preserved independent of execution mode Changes: https://git.openjdk.java.net/jdk/pull/93/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=93=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8242451 Stats: 97 lines in 2 files changed: 73 ins; 5 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/93.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/93/head:pull/93 PR: https://git.openjdk.java.net/jdk/pull/93
