Re: RFR: 8262889: Compiler implementation for Record Patterns [v5]
On Tue, 10 May 2022 09:57:48 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing (non-)exhaustiveness for enum. looks good to me, great job! - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Mon, 9 May 2022 14:37:35 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'type-pattern-third' into patterns-record-deconstruction3
> - Using Type.isRaw instead of checking the AST structure.
> - Exhaustiveness should accept supertypes of the specified type.
> - Renaming the features from deconstruction pattern to record pattern.
> - Fixing guards after record patterns.
> - Raw types are not allowed in record patterns.
> - Reflecting review feedback.
> - 8262889: Compiler implementation for Record Patterns
I've noticed that this code:
class Test {
String e(E e) {
return switch (e) {
case A -> "42";
};
}
enum E {
A, B;
}
}
fails with:
Test.java:3: error: the switch expression does not cover all possible input
values
return switch (e) {
^
1 error
before this change but gets accepted with no error message after it
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Mon, 9 May 2022 14:37:35 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains eight commits: > > - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 > - Using Type.isRaw instead of checking the AST structure. > - Exhaustiveness should accept supertypes of the specified type. > - Renaming the features from deconstruction pattern to record pattern. > - Fixing guards after record patterns. > - Raw types are not allowed in record patterns. > - Reflecting review feedback. > - 8262889: Compiler implementation for Record Patterns src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 822: > 820: > nestedComponentPatterns); > 821: > 822: //for each of the symbols covered by the starting component, > find all deconstruction patterns this comment should probably read: find all `record` patterns? - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Fri, 6 May 2022 17:40:25 GMT, Jan Lahoda wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4217: >> >>> 4215: } >>> 4216: ListBuffer outBindings = new ListBuffer<>(); >>> 4217: List recordTypes = expectedRecordTypes; >> >> nit: probably a matter of style but why not reusing the already declared >> `expectedRecordTypes` declaring a new variable seems unnecessary > > Please note the full `expectedRecordTypes` are used for error reporting, but > the reference to `List` in `recordTypes` is overwritten in the loop (at the > time of an error report, it may not longer point to the full expected types, > and hence cannot be used for error reporting). Ok I see, thanks - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:09:24 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review feedback. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4217: > 4215: } > 4216: ListBuffer outBindings = new ListBuffer<>(); > 4217: List recordTypes = expectedRecordTypes; nit: probably a matter of style but why not reusing the already declared `expectedRecordTypes` declaring a new variable seems unnecessary - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 15:21:49 GMT, Maurizio Cimadamore
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 64:
>
>> 62: public enum Feature {
>> 63: SWITCH_PATTERN_MATCHING,
>> 64: DECONSTRUCTION_PATTERNS,
>
> The spec and JEP talks about record patterns - I think we should follow the
> correct name here
yup, I agree
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v5]
On Tue, 19 Apr 2022 18:47:16 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup - more total -> unconditional pattern renaming. I don't have any other comments, looks good - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v5]
On Tue, 19 Apr 2022 18:47:16 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup - more total -> unconditional pattern renaming. src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java line 2: > 1: /* > 2: * Copyright (c) 1999, 20222, Oracle and/or its affiliates. All rights > reserved. typo: `20222` instead of `2022` - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v5]
On Tue, 19 Apr 2022 18:47:16 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup - more total -> unconditional pattern renaming. nit: just ran langtools tests as part of my routine and these seem to be failing, CheckExamples.java due to: test/langtools/tools/javac/diags/examples/FeatureTotalPatternsInInstanceof.java and test/langtools/tools/javac/diags/examples/FeatureTotalPatternsInInstanceof.java and this one too: test/langtools/tools/javac/patterns/InstanceofTotalPattern.java (seems to be a golden file issue) - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Tue, 12 Apr 2022 13:18:14 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup. the patch doesn't apply cleanly on the master repo probably need to rebase? - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Tue, 12 Apr 2022 13:18:14 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1779: > 1777: //binding pattern > 1778: attribExpr(pat, switchEnv); > 1779: var primary = TreeInfo.primaryPatternType(pat); general comment: the handleSwitch method is getting more and more complex, please consider refactoring it, probably splitting it, for example different subrutines handling different case kinds. Of course this probably should be done as a separate effort. - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Thu, 14 Apr 2022 08:46:50 GMT, Aggelos Biboudis
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Cleanup.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java line
> 1377:
>
>> 1375: }
>> 1376: JCExpression guard = ((JCPattern) cse).guard;
>> 1377: if (guard != null && guard.type.hasTag(BOOLEAN)) {
>
> In the spec we see the following:
>
>> A pattern case element is said to be unrefined if either (i) it has no
>> associated when expression, or (ii) it has an associated when expression
>> that is a constant expression
>> ([15.29](https://docs.oracle.com/javase/specs/jls/se18/html/jls-15.html#jls-15.29))
>> with value true. A case element is unrefined if it is not a pattern case
>> element or it is an unrefined pattern case element.
>
> However, in the definition of `unconditional` I don't see the part for the
> constant expression.
>
> I wonder if the spec should be updated or the code at this point
the code below checks that the guard is boolean constant `true`, this is the
way the compiler can do that, a constant can be retrieved using the
`constValue` method
-
PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Tue, 12 Apr 2022 13:18:14 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup. src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java line 242: > 240: PATTERN_SWITCH(JDK17, Fragments.FeaturePatternSwitch, > DiagKind.PLURAL), > 241: REDUNDANT_STRICTFP(JDK17), > 242: TOTAL_PATTERN_IN_INSTACEOF(JDK17, > Fragments.FeatureTotalPatternsInInstanceof, DiagKind.PLURAL), shouldn't be JDK19? - PR: https://git.openjdk.java.net/jdk/pull/8182
Integrated: 8284361: Updating ASM to 9.3 for JDK 19
On Thu, 7 Apr 2022 03:33:02 GMT, Vicente Romero wrote: > We recently updated our ASM version to 9.2 and just this week version 9.3 was > announced with support for JDK19 so it makes sense to update to this last > version. > > Thanks in advance for the reviews, > Vicente This pull request has now been integrated. Changeset: 1bd8975c Author:Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/1bd8975cafade1234f653ab994cb7c6f0a82590f Stats: 438 lines in 57 files changed: 73 ins; 70 del; 295 mod 8284361: Updating ASM to 9.3 for JDK 19 Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/8135
Re: RFR: 8284361: Updating ASM to 9.3 for JDK 19
On Fri, 8 Apr 2022 16:44:15 GMT, Mandy Chung wrote: >> We recently updated our ASM version to 9.2 and just this week version 9.3 >> was announced with support for JDK19 so it makes sense to update to this >> last version. >> >> Thanks in advance for the reviews, >> Vicente > > Looks okay. Most of the changes is in javadoc and rename `var` to > `varIndex`. The change in ASMifier looks good. thanks @mlchung for the review - PR: https://git.openjdk.java.net/jdk/pull/8135
RFR: 8284361: Updating ASM to 9.3 for JDK 19
We recently updated our ASM version to 9.2 and just this week version 9.3 was announced with support for JDK19 so it makes sense to update to this last version. Thanks in advance for the reviews, Vicente - Commit messages: - updating test ValidateJarWithSealedAndRecord - adaptations after automatic conversion - 8284361: Updating ASM to 9.3 for JDK 19 Changes: https://git.openjdk.java.net/jdk/pull/8135/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8135=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284361 Stats: 438 lines in 57 files changed: 73 ins; 70 del; 295 mod Patch: https://git.openjdk.java.net/jdk/pull/8135.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8135/head:pull/8135 PR: https://git.openjdk.java.net/jdk/pull/8135
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19 [v3]
On Wed, 30 Mar 2022 02:56:41 GMT, Vicente Romero wrote: >> Please review this PR which is updating the ASM included in the JDK to ASM >> 9.2. This version should be included in JDK19. There are basically two >> commits here, one that was automatically generated and that mostly changes >> file headers etc and another one, smaller, that make changes to the code to >> adapt it to our needs, JDK version etc. In this second commit one can see >> that two classes that were removed by the automatically generated change >> have been copied back: >> >> jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter >> jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter >> >> This is because package: `jdk.jfr.internal.instrument` needs them. >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > fixing files missing new line at the end anymore comments on this one? Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19 [v3]
> Please review this PR which is updating the ASM included in the JDK to ASM > 9.2. This version should be included in JDK19. There are basically two > commits here, one that was automatically generated and that mostly changes > file headers etc and another one, smaller, that make changes to the code to > adapt it to our needs, JDK version etc. In this second commit one can see > that two classes that were removed by the automatically generated change have > been copied back: > > jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter > jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter > > This is because package: `jdk.jfr.internal.instrument` needs them. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: fixing files missing new line at the end - Changes: - all: https://git.openjdk.java.net/jdk/pull/8000/files - new: https://git.openjdk.java.net/jdk/pull/8000/files/41ae618e..f5e0d931 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8000=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8000=01-02 Stats: 252 lines in 129 files changed: 125 ins; 0 del; 127 mod Patch: https://git.openjdk.java.net/jdk/pull/8000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8000/head:pull/8000 PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 17:08:12 GMT, Lance Andersen wrote: > With this fix, I believe > [JDK-8282446](https://bugs.openjdk.java.net/browse/JDK-8282446) should also > be addressed. Thanks for mentioning this. I have uploaded another commit [41ae618e3a5ce696e3400a8654d98801226d7c1b](https://github.com/openjdk/jdk/pull/8000/commits/41ae618e3a5ce696e3400a8654d98801226d7c1b), which addresses this issue. - PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19 [v2]
> Please review this PR which is updating the ASM included in the JDK to ASM > 9.2. This version should be included in JDK19. There are basically two > commits here, one that was automatically generated and that mostly changes > file headers etc and another one, smaller, that make changes to the code to > adapt it to our needs, JDK version etc. In this second commit one can see > that two classes that were removed by the automatically generated change have > been copied back: > > jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter > jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter > > This is because package: `jdk.jfr.internal.instrument` needs them. > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8000/files - new: https://git.openjdk.java.net/jdk/pull/8000/files/2e468674..41ae618e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8000=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8000=00-01 Stats: 83 lines in 2 files changed: 82 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8000/head:pull/8000 PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 17:50:25 GMT, Rémi Forax wrote: > I suppose that you are raising commons.RemappingMethodAdapter and > commons.RemappingAnnotationAdapter from the dead because you want to fix the > code in jdk.jfr.internal.instrument later ? correct, I would prefer the team maintaining jdk.jfr.internal.instrument to do that update after this PR has been pushed > > Otherwise it's a little dangerous because that code as started to drift, the > new remapper has new code not supported by the old remapper. BTW, @deprecated > on ASM source is equivalent to. @deprecated + forRemoval in the JDK. > > The support of Java 19 is fine, the same code will be included in ASM soon. - PR: https://git.openjdk.java.net/jdk/pull/8000
RFR: 8282508: Updating ASM to 9.2 for JDK 19
Please review this PR which is updating the ASM included in the JDK to ASM 9.2. This version should be included in JDK19. TIA - Commit messages: - additional adaptations after the automatic conversion - 8282508: Updating ASM to 9.2 for JDK 19 Changes: https://git.openjdk.java.net/jdk/pull/8000/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8000=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282508 Stats: 1762 lines in 136 files changed: 544 ins; 765 del; 453 mod Patch: https://git.openjdk.java.net/jdk/pull/8000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8000/head:pull/8000 PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255 [v2]
On Mon, 14 Mar 2022 21:27:29 GMT, Joe Darcy wrote: >> Improving the exception messages for out-of-supported-range array types. >> >> I'll update copyrights before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. lgtm - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7812
Re: RFR: JDK-8283075: Bad IllegalArgumentException message when calling ClassDesc.arrayType(int) which results in a rank > 255
On Mon, 14 Mar 2022 19:56:17 GMT, Joe Darcy wrote:
> Improving the exception messages for out-of-supported-range array types.
>
> I'll update copyrights before pushing.
src/java.base/share/classes/java/lang/constant/ClassDesc.java line 186:
> 184: if (rank <= 0 || netRank >
> ConstantUtils.MAX_ARRAY_TYPE_DESC_DIMENSIONS)
> 185: throw new IllegalArgumentException("rank: " + netRank +
> 186:" exceeds maximum
> supported dimension of " +
if the `rank < 0` it won't be exceeding the maximum supported dimensions so
this message only applies to the second condition
-
PR: https://git.openjdk.java.net/jdk/pull/7812
Integrated: 8213905: reflection not working for type annotations applied to exception types in the inner class constructor
On Thu, 16 Dec 2021 17:44:04 GMT, Vicente Romero wrote: > Hi, > > Please review this change that is fixing a bug in reflection in particular in > `sun.reflect.annotation.TypeAnnotationParser::buildAnnotatedTypes` the > current code is assuming that for inner class constructors, there are only > type annotations on parameters, but it is also invoked to extract type > annotations applied to exception types for example. This bug affects the > behavior of method: > `java.lang.reflect.Executable::getAnnotatedExceptionTypes` which is not > behaving according to its specification. Given that this fix affects the > behavior of a method belonging to our API a CSR has been filed too. Please > review it at [JDK-8278926](https://bugs.openjdk.java.net/browse/JDK-8278926). > > TIA This pull request has now been integrated. Changeset: 2eab86b5 Author:Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/2eab86b513a9e4566b3f5989f899ae44280d3834 Stats: 16 lines in 2 files changed: 11 ins; 1 del; 4 mod 8213905: reflection not working for type annotations applied to exception types in the inner class constructor Reviewed-by: jlahoda - PR: https://git.openjdk.java.net/jdk/pull/6869
Re: RFR: 8213905: reflection not working for type annotations applied to exception types in the inner class constructor
On Fri, 21 Jan 2022 12:41:37 GMT, Jan Lahoda wrote: > To me, looks good. thanks - PR: https://git.openjdk.java.net/jdk/pull/6869
Re: RFR: 8213905: reflection not working for type annotations applied to exception types in the inner class constructor
On Thu, 16 Dec 2021 18:01:54 GMT, Joe Darcy wrote: > Hi Vicente. > > Please file a CSR for the behavioral change. sure, thanks - PR: https://git.openjdk.java.net/jdk/pull/6869
RFR: 8213905: reflection not working for type annotations applied to exception types in the inner class constructor
Hi, Please review this change that is fixing a bug in reflection in particular in `sun.reflect.annotation.TypeAnnotationParser::buildAnnotatedTypes` the current code is assuming that for inner class constructors it is always working on type annotations on parameters, but it is also invoked to extract type annotations applied to exception types for example. TIA - Commit messages: - 8213905: Reflection, TYPE_USE annotation on THROWS on inner class constructor, Java 9+ Changes: https://git.openjdk.java.net/jdk/pull/6869/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6869=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8213905 Stats: 16 lines in 2 files changed: 11 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6869.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6869/head:pull/6869 PR: https://git.openjdk.java.net/jdk/pull/6869
Re: RFR: 8261847: performance of java.lang.Record::toString should be improved [v6]
On Tue, 23 Nov 2021 15:09:53 GMT, Claes Redestad wrote: > Thanks for adding the comment and fixing typos. sure, thanks to you - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v6]
On Tue, 23 Nov 2021 15:05:47 GMT, Vicente Romero wrote: >> Please review this PR which aims to optimize the implementation of the >> `toString` method we provide for records. A benchmark comparing the >> implementation we are providing for records with lombok found out that >> lombok is much faster mainly because our implementation uses >> `String::format`. This fix is basically delegating on >> StringConcatFactory::makeConcatWithConstants which is faster. >> >> TIA >> >> This is the result of the benchmark comparing records to lombok with vanilla >> JDK: >> >> Benchmark Mode CntScoreError Units >> MyBenchmark.base avgt40.849 ± 0.111 ns/op >> MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op >> MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op >> MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op >> MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op >><-- Before >> MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op >> MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op >> >> >> after this patch: >> >> Benchmark Mode Cnt Score Error Units >> MyBenchmark.base avgt4 0.680 ± 0.185 ns/op >> MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op >> MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op >> MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op >> MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op >><--- After >> MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op >> MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding import at benchmark this ship is sailing! thanks everyone for the feedback and interest! - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v6]
> Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt 4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: adding import at benchmark - Changes: - all: https://git.openjdk.java.net/jdk/pull/6403/files - new: https://git.openjdk.java.net/jdk/pull/6403/files/52d04ecc..0b68deb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6403=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6403=04-05 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v5]
> Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt 4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: fixing typos and adding a comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/6403/files - new: https://git.openjdk.java.net/jdk/pull/6403/files/2d3240e1..52d04ecc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6403=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6403=03-04 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]
On Mon, 22 Nov 2021 15:53:53 GMT, Claes Redestad wrote: > FWIW I did a few experiments trying to move the chunking to `SCF`. Most made > things worse, but this is getting close: > https://github.com/openjdk/jdk/compare/master...cl4es:scf_split?expand=1 > > The threshold for when the JIT fails to optimize seem to be pushed up a bit > and I get a 4x gains when concatenating 100 Strings (though it takes a good > while for the microbenchmark to stabilize). It also fails to make much of a > difference when looking at 200 arguments (maybe 1.4x win). The experiment I > have done so far are crude and aggregates the results into a String builder > with no size estimation, so it adds a but of unnecessary allocation that > could be improved. I think this needs way more work to be a good enhancement > and that splitting up outside is going to remain better for now. > > A "hidden" drawback with chunking is that you're likely going to be > allocating more. > > I also re-realized that it'll be pretty hard (or impossible) to get around > having some MH slot limit: even though we chunk it up internally, the MH we > return must have a type that matches the type given to the bootstrap method, > and any operations on the tree that require some temporary arguments we need > to pass around will require some argument slots. The current strategy doesn't > use too many temporaries, so the real limit might be around 250, but it makes > sense to give some room for alternative implementations that use more > temporaries if that makes things more efficient. Thanks for sharing this and for doing the experiment! - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
On Sat, 20 Nov 2021 12:10:40 GMT, Jim Laskey wrote: >> @stuart-marks yes, a general purpose splitting logic moved into the >> `StringConcatFactory` would be able to get rid of the arbitrary 200 slot >> limit (we would hit a harder but less arbitrary limit at around 253 instead). >> >> @JimLaskey I don't see why it wouldn't work generally from the point of view >> of the `StringConcatFactory`: Vicente's code operates on a `MethodHandle[]` >> with getters as inputs to the `SCF` bootstrap method, whereas `SCF` would >> deal with arguments directly (retrieved from an `Object[]`). I think the >> code changes from the patch here after moving the logic into `SCF` should be >> pretty minimal and straightforward: if I'm not missing anything we'd only >> conceptually be replacing the `filterArguments` on line 313 with an >> `insertArguments`. > > @cl4es I was thinking of the more general case 200 < N. You were in the 200 < > N < 253 case. I think it would be better to pass in a chunk size (<= 200) and > an array of ptype and get an array of method handles each taking a chunk size > of arguments plus result of prior call. This is more like what javac does. @JimLaskey I'm generating several recipes one per each invocation of StringConcatFactory::makeConcatWithConstants I think that generating only one recipe is possible only if the number of `getters` we are dealing with is less than the maximum number of slots we want to split the `getters` into - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]
On Sun, 21 Nov 2021 00:29:46 GMT, Vicente Romero wrote: >> Please review this PR which aims to optimize the implementation of the >> `toString` method we provide for records. A benchmark comparing the >> implementation we are providing for records with lombok found out that >> lombok is much faster mainly because our implementation uses >> `String::format`. This fix is basically delegating on >> StringConcatFactory::makeConcatWithConstants which is faster. >> >> TIA >> >> This is the result of the benchmark comparing records to lombok with vanilla >> JDK: >> >> Benchmark Mode CntScoreError Units >> MyBenchmark.base avgt40.849 ± 0.111 ns/op >> MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op >> MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op >> MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op >> MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op >><-- Before >> MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op >> MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op >> >> >> after this patch: >> >> Benchmark Mode Cnt Score Error Units >> MyBenchmark.base avgt4 0.680 ± 0.185 ns/op >> MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op >> MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op >> MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op >> MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op >><--- After >> MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op >> MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > setting max split size to 20 hi guys, can I have a thumbs up for this one? I will continue doing additional research for the `hashCode` and `equals` methods as a follow up but would like to close this one as the performance for the `Record::toString` was the worst compared to lombok. TIA - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v4]
> Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt 4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: setting max split size to 20 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6403/files - new: https://git.openjdk.java.net/jdk/pull/6403/files/4698f98e..2d3240e1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6403=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6403=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
On Fri, 19 Nov 2021 05:07:23 GMT, Vicente Romero wrote: >> Please review this PR which aims to optimize the implementation of the >> `toString` method we provide for records. A benchmark comparing the >> implementation we are providing for records with lombok found out that >> lombok is much faster mainly because our implementation uses >> `String::format`. This fix is basically delegating on >> StringConcatFactory::makeConcatWithConstants which is faster. >> >> TIA >> >> This is the result of the benchmark comparing records to lombok with vanilla >> JDK: >> >> Benchmark Mode CntScoreError Units >> MyBenchmark.base avgt40.849 ± 0.111 ns/op >> MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op >> MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op >> MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op >> MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op >><-- Before >> MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op >> MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op >> >> >> after this patch: >> >> Benchmark Mode Cnt Score Error Units >> MyBenchmark.base avgt4 0.680 ± 0.185 ns/op >> MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op >> MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op >> MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op >> MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op >><--- After >> MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op >> MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding the benchmark Thanks guys for the discussion so far. I wanted to add some graphs I generated. I modified the code at ObjectMethods to split the getters in slot sizes from 10 to 200 in increments of 10. Then I executed the benchmark with the given slot size for records and lombok classes with `int` and `String` components. In every case the record and the lombok class had all ints or all String components / fields. I collected data for 1, 10, 100 and 254 components / fields. See below the graphical representation of the data I got. Some comments below analyzing the data for records: ### 1 component As expected the slot size doesn't matter much for this case as we are dealing with only one component:  ### 10 components Here the slot size doesn't make a big difference as expected. There is a tight range for the case when all the components are ints. For the string case the range is also very tight modulo a couple of data points but we could safely say that the numbers are pretty good for slots sizes from 10-80:  ### 100 components Here we start seeing the number of slots playing a more important role. See how the time grows in a quasi logarithmic function for 100 string components once the slot size is greater than 70 for the all integers case we get pretty good numbers up to slot size 60:  ### 254 components The results for 254 integer components show a lot of variation. The results are good for slot sizes 10, 20 and then for most sizes from 70 to 170, not for 150. Whereas for the all strings case we have good numbers up to slot size 60 and it peaks up from there:  So these data seems to confirm that slot sizes from 10-30 is where we want to be. I will update the patch to use 20 slots to split the getters. Thanks again for the discussion! - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
On Fri, 19 Nov 2021 05:28:10 GMT, Guoxiong Li wrote: > FYI: this patch also seems to solve > [JDK-8265747](https://bugs.openjdk.java.net/browse/JDK-8265747). yep, although I prefer to keep [JDK-8265747](https://bugs.openjdk.java.net/browse/JDK-8265747) because it is also referring to the hashCode method, and so far I have been focusing on the `toString` only which is the one for which we have the worst performance right now. Thanks for the heads-up - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
On Fri, 19 Nov 2021 05:07:23 GMT, Vicente Romero wrote: >> Please review this PR which aims to optimize the implementation of the >> `toString` method we provide for records. A benchmark comparing the >> implementation we are providing for records with lombok found out that >> lombok is much faster mainly because our implementation uses >> `String::format`. This fix is basically delegating on >> StringConcatFactory::makeConcatWithConstants which is faster. >> >> TIA >> >> This is the result of the benchmark comparing records to lombok with vanilla >> JDK: >> >> Benchmark Mode CntScoreError Units >> MyBenchmark.base avgt40.849 ± 0.111 ns/op >> MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op >> MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op >> MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op >> MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op >><-- Before >> MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op >> MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op >> >> >> after this patch: >> >> Benchmark Mode Cnt Score Error Units >> MyBenchmark.base avgt4 0.680 ± 0.185 ns/op >> MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op >> MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op >> MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op >> MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op >><--- After >> MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op >> MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding the benchmark I have done some additional experiments as suggested by Claes, thanks, for different number of record components. In all cases all the components are of type `int` so they occupy only one slot. Here are some numbers. I tried with 0, 1, 10, 100 and 254 record components. Benchmark Mode Cnt Score Error Units MyBenchmark.base avgt4 0.775 ± 0.552 ns/op MyBenchmark.record0_toString avgt4 4.973 ± 2.800 ns/op MyBenchmark.record1_toString avgt4 16.026 ± 5.100 ns/op MyBenchmark.record10_toString avgt4 81.412 ± 3.365 ns/op MyBenchmark.record100_toString avgt4 12269.500 ± 179.784 ns/op MyBenchmark.record254_toString avgt4 51191.953 ± 11679.762 ns/op MyBenchmark.valueClass0_toString avgt4 5.134 ± 2.372 ns/op MyBenchmark.valueClass1_toString avgt4 23.321 ± 9.234 ns/op MyBenchmark.valueClass10_toString avgt4 94.048 ± 7.017 ns/op MyBenchmark.valueClass100_toString avgt4 9253.282 ± 4843.738 ns/op MyBenchmark.valueClass254_toString avgt4 31963.158 ± 24050.499 ns/op Then also after a suggestion from Claes I modified the maximum number of slots I would be chopping the arguments into, I gave a try with 20 slots and got these numbers: Benchmark Mode Cnt Score Error Units MyBenchmark.record0_toString avgt4 5.009 ± 3.454 ns/op MyBenchmark.record1_toString avgt4 14.207 ±10.551 ns/op MyBenchmark.record10_toString avgt4 81.018 ± 7.320 ns/op MyBenchmark.record100_toString avgt4 2862.641 ± 1233.862 ns/op MyBenchmark.record254_toString avgt4 23002.280 ± 97103.923 ns/op MyBenchmark.valueClass0_toString avgt4 4.967 ± 3.947 ns/op MyBenchmark.valueClass1_toString avgt4 23.756 ± 8.499 ns/op MyBenchmark.valueClass10_toString avgt4 87.691 ± 7.956 ns/op MyBenchmark.valueClass100_toString avgt4 9539.272 ± 9461.516 ns/op MyBenchmark.valueClass254_toString avgt4 28323.478 ± 11932.474 ns/op It seems like the execution is way faster now. - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v3]
> Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt 4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: adding the benchmark - Changes: - all: https://git.openjdk.java.net/jdk/pull/6403/files - new: https://git.openjdk.java.net/jdk/pull/6403/files/2a2856cd..4698f98e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6403=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6403=01-02 Stats: 918 lines in 2 files changed: 156 ins; 684 del; 78 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v2]
On Wed, 17 Nov 2021 22:00:30 GMT, Vicente Romero wrote: >> Please review this PR which aims to optimize the implementation of the >> `toString` method we provide for records. A benchmark comparing the >> implementation we are providing for records with lombok found out that >> lombok is much faster mainly because our implementation uses >> `String::format`. This fix is basically delegating on >> StringConcatFactory::makeConcatWithConstants which is faster. >> >> TIA >> >> This is the result of the benchmark comparing records to lombok with vanilla >> JDK: >> >> Benchmark Mode CntScoreError Units >> MyBenchmark.base avgt40.849 ± 0.111 ns/op >> MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op >> MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op >> MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op >> MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op >><-- Before >> MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op >> MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op >> >> >> after this patch: >> >> Benchmark Mode Cnt Score Error Units >> MyBenchmark.base avgt4 0.680 ± 0.185 ns/op >> MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op >> MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op >> MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op >> MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op >><--- After >> MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op >> MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > addressing review comments I have uploaded another iteration, some numbers for when the number of record components is pushed to the maximum (254 components): JDK18 vanilla: Before: Benchmark Mode Cnt Score Error Units MyBenchmark.base avgt4 0.670 ±0.078 ns/op MyBenchmark.equals_record avgt4 9092.649 ± 400.200 ns/op MyBenchmark.equals_value avgt4107.184 ± 15.993 ns/op MyBenchmark.record_hash_code avgt4 6284.711 ± 1985.212 ns/op MyBenchmark.record_to_string avgt4 69614.364 ± 6310.593 ns/op MyBenchmark.value_class_to_string avgt4 25910.041 ± 7822.274 ns/op MyBenchmark.value_hash_codeavgt4315.969 ± 36.545 ns/op With this PR: Benchmark Mode Cnt Score Error Units MyBenchmark.base avgt4 0.658 ±0.053 ns/op MyBenchmark.equals_record avgt4 8222.407 ± 308.440 ns/op MyBenchmark.equals_value avgt4102.627 ± 56.522 ns/op MyBenchmark.record_hash_code avgt4 6030.478 ± 243.390 ns/op MyBenchmark.record_to_string avgt4 46504.151 ± 3597.182 ns/op MyBenchmark.value_class_to_string avgt4 26196.565 ± 8154.037 ns/op MyBenchmark.value_hash_codeavgt4313.918 ±4.577 ns/op some observations, we are doing better than before but still worst than lombok. I don't think having to create a more complicated MH tree is the key, our numbers are still worst even if there is no need to create too much levels in the MH tree like for 100 record components occupying a spot each. Another observation is that even though our slowest implementation is the one for the `toString` method, in absolute terms, lombok is faster in all cases. So more work could be necessary for the rest of the methods if deemed worthy. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved [v2]
> Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt 4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/6403/files - new: https://git.openjdk.java.net/jdk/pull/6403/files/0ccb37ab..2a2856cd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6403=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6403=00-01 Stats: 965 lines in 2 files changed: 932 ins; 13 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved
On Tue, 16 Nov 2021 05:24:38 GMT, Vicente Romero wrote: > Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op thanks all for the comments! I will be uploading a new iteration soon - PR: https://git.openjdk.java.net/jdk/pull/6403
RFR: 8261847: performace of java.lang.Record::toString should be improved
Please review this PR which aims to optimize the implementation of the `toString` method we provide for records. A benchmark comparing the implementation we are providing for records with lombok found out that lombok is much faster mainly because our implementation uses `String::format`. This fix is basically delegating on StringConcatFactory::makeConcatWithConstants which is faster. TIA This is the result of the benchmark comparing records to lombok with vanilla JDK: Benchmark Mode CntScoreError Units MyBenchmark.base avgt40.849 ± 0.111 ns/op MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op <-- Before MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op after this patch: Benchmark Mode Cnt Score Error Units MyBenchmark.base avgt4 0.680 ± 0.185 ns/op MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op <--- After MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op - Commit messages: - 8261847: Suboptimal java.lang.record's methods generation Changes: https://git.openjdk.java.net/jdk/pull/6403/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6403=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261847 Stats: 56 lines in 1 file changed: 20 ins; 15 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Integrated: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
On Mon, 23 Aug 2021 18:08:02 GMT, Vicente Romero wrote: > Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) This pull request has now been integrated. Changeset: 0609421d Author:Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/0609421d4b824c5642ca75d525bad3edd72cd23a Stats: 32 lines in 2 files changed: 18 ins; 0 del; 14 mod 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null Reviewed-by: mchung, chegar - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v4]
> Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Merge branch 'master' into JDK-8272347 - adding pattern matching - clarifying that the names parameter is ignored in some cases - addressing review comments - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null - Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/102cd1aa..e0a7f5b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5226=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5226=02-03 Stats: 6008 lines in 269 files changed: 4342 ins; 755 del; 911 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v3]
On Mon, 30 Aug 2021 01:45:49 GMT, Mandy Chung wrote: >> Vicente Romero has updated the pull request incrementally with one >> additional commit since the last revision: >> >> clarifying that the names parameter is ignored in some cases > > src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 345: > >> 343: Arrays.stream(getters).forEach(Objects::requireNonNull); >> 344: MethodType methodType; >> 345: if (type instanceof MethodType) > > Since you are modifying this file, do you mind taking Jesper's suggestion [1] > posted in another PR to use pattern matching. > > Suggestion: > > if (type instanceof MethodType mt) > methodType = mt; > > > [1] https://github.com/openjdk/valhalla/pull/528#discussion_r688100918 sure I will do this before pushing, thanks - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v3]
> Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: clarifying that the names parameter is ignored in some cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/89086ca1..102cd1aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5226=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5226=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On 8/25/21 4:45 PM, Mandy Chung wrote: On 8/25/21 12:08 PM, Vicente Romero wrote: On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung wrote: Hi Mandy, I have changed the implementation of the method to explicitly require all arguments but lookup to be non-null as suggested by Brian. I have also covered, I think, all the missing test cases in test `ObjectMethodsTest`, thanks for your comments. I think you meant requiring `names` to be non-null but `lookup` may still be null. It's okay to change the spec to require `names` to be non-null. Probably better to mention in `@param names` that `names` is ignored when the `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null. What I meant is to require it to be non-null but the spec should also mention `names` parameter is ignored if the method name is `equals` and `hashCode`. Mandy I see, I have updated the PR, thanks for your comments - PR:https://git.openjdk.java.net/jdk/pull/5226 Vicente
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Wed, 25 Aug 2021 02:17:12 GMT, Mandy Chung wrote: >> Hi Mandy, I have changed the implementation of the method to explicitly >> require all arguments but lookup to be non-null as suggested by Brian. I >> have also covered, I think, all the missing test cases in test >> `ObjectMethodsTest`, thanks for your comments. > > I think you meant requiring `names` to be non-null but `lookup` may still be > null. It's okay to change the spec to require `names` to be non-null. > > Probably better to mention in `@param names` that `names` is ignored when the > `methodName` is `equals` or `hashCode`. no I think I prefer to force names to be non-null all the time, that way we offer a more consistent interface to users. They don't have to remember what case was for which names can be null. - PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
On Mon, 23 Aug 2021 23:13:58 GMT, Mandy Chung wrote:
>> Vicente Romero has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressing review comments
>
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 327:
>
>> 325: * @throws IllegalArgumentException if the bootstrap arguments are
>> invalid
>> 326: * or inconsistent
>> 327: * @throws NullPointerException if any argument but {@code lookup}
>> is {@code null}
>
> `names` may be null if the {@code methodName} is {@code "equals"} or {@code
> "hashCode"}.This should be captured here.
Hi Mandy, I have changed the implementation of the method to explicitly require
all arguments but lookup to be non-null as suggested by Brian. I have also
covered, I think, all the missing test cases in test `ObjectMethodsTest`,
thanks for your comments.
-
PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null [v2]
> Please review this simple PR along with the associated CSR. The PR is > basically adding a line the the specification of method > `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a > NPE will be thrown. > > TIA > > link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5226/files - new: https://git.openjdk.java.net/jdk/pull/5226/files/b7489b41..89086ca1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5226=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5226=00-01 Stats: 30 lines in 2 files changed: 17 ins; 1 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
Re: RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
On 8/23/21 4:07 PM, Brian Goetz wrote: Actually, it will not NPE if `names` is null and you have selected equals/hashCode as the name. Might be better to do requiresNonNull() up front for all the arguments, just to make such analysis simpler: requireNonNull(methodName); requireNonNull(type); requireNonNull(recordClass); requireNonNull(names); requireNonNull(getters); will do, thanks, Vicente On 8/23/2021 4:04 PM, Brian Goetz wrote: +1 On 8/23/2021 2:22 PM, Vicente Romero wrote: Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown. TIA link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) - Commit messages: - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null Changes: https://git.openjdk.java.net/jdk/pull/5226/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5226=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272347 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
RFR: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
Please review this simple PR along with the associated CSR. The PR is basically adding a line the the specification of method `java.lang.runtime.ObjectMethods::bootstrap` stating under what conditions a NPE will be thrown. TIA link to the [CSR](https://bugs.openjdk.java.net/browse/JDK-8272852) - Commit messages: - 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null Changes: https://git.openjdk.java.net/jdk/pull/5226/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5226=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272347 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5226.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5226/head:pull/5226 PR: https://git.openjdk.java.net/jdk/pull/5226
[jdk17] Integrated: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
On Sat, 10 Jul 2021 19:03:36 GMT, Vicente Romero wrote: > Please review this PR that is fixing a mismatch between the implementation > for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its > implementation. I made a mistake while working on a recent CSR > [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the > API but mistakenly thought that the implementation was in sync with the spec. > This is why this change is also including a unit test of the API for > `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` > which is covered in test `IndyDescTest` in the same test suite. Also this > change needs a CSR as while fixing the implementation of method `::withArgs` > I realized that the API of the varargs overloaded version of method `::of` > needed some rewording too as it is invoking the same private constructor > `::withArgs` is invoking. So it didn't make sense for the API of one method > to be more restrictive than the other. Please review also the accompanying > CSR. > > Thanks, > Vicente This pull request has now been integrated. Changeset: 8583aab3 Author:Vicente Romero URL: https://git.openjdk.java.net/jdk17/commit/8583aab374c3c2ad94c88e7f649d81ce5f319a5f Stats: 197 lines in 2 files changed: 195 ins; 0 del; 2 mod 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE Reviewed-by: jvernee, mchung - PR: https://git.openjdk.java.net/jdk17/pull/242
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE [v3]
On Mon, 12 Jul 2021 18:06:30 GMT, Vicente Romero wrote: >> Please review this PR that is fixing a mismatch between the implementation >> for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its >> implementation. I made a mistake while working on a recent CSR >> [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed >> the API but mistakenly thought that the implementation was in sync with the >> spec. This is why this change is also including a unit test of the API for >> `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` >> which is covered in test `IndyDescTest` in the same test suite. Also this >> change needs a CSR as while fixing the implementation of method `::withArgs` >> I realized that the API of the varargs overloaded version of method `::of` >> needed some rewording too as it is invoking the same private constructor >> `::withArgs` is invoking. So it didn't make sense for the API of one method >> to be more restrictive than the other. Please review also the accompanying >> CSR. >> >> Thanks, >> Vicente > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > review comments thanks for the reviews! - PR: https://git.openjdk.java.net/jdk17/pull/242
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE [v3]
> Please review this PR that is fixing a mismatch between the implementation > for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its > implementation. I made a mistake while working on a recent CSR > [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the > API but mistakenly thought that the implementation was in sync with the spec. > This is why this change is also including a unit test of the API for > `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` > which is covered in test `IndyDescTest` in the same test suite. Also this > change needs a CSR as while fixing the implementation of method `::withArgs` > I realized that the API of the varargs overloaded version of method `::of` > needed some rewording too as it is invoking the same private constructor > `::withArgs` is invoking. So it didn't make sense for the API of one method > to be more restrictive than the other. Please review also the accompanying > CSR. > > Thanks, > Vicente Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/242/files - new: https://git.openjdk.java.net/jdk17/pull/242/files/f7fddc99..6702a42f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=242=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=242=01-02 Stats: 13 lines in 1 file changed: 0 ins; 3 del; 10 mod Patch: https://git.openjdk.java.net/jdk17/pull/242.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/242/head:pull/242 PR: https://git.openjdk.java.net/jdk17/pull/242
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE [v2]
> Please review this PR that is fixing a mismatch between the implementation > for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its > implementation. I made a mistake while working on a recent CSR > [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the > API but mistakenly thought that the implementation was in sync with the spec. > This is why this change is also including a unit test of the API for > `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc` > which is covered in test `IndyDescTest` in the same test suite. Also this > change needs a CSR as while fixing the implementation of method `::withArgs` > I realized that the API of the varargs overloaded version of method `::of` > needed some rewording too as it is invoking the same private constructor > `::withArgs` is invoking. So it didn't make sense for the API of one method > to be more restrictive than the other. Please review also the accompanying > CSR. > > Thanks, > Vicente Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/242/files - new: https://git.openjdk.java.net/jdk17/pull/242/files/602a85c4..f7fddc99 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=242=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=242=00-01 Stats: 42 lines in 1 file changed: 8 ins; 20 del; 14 mod Patch: https://git.openjdk.java.net/jdk17/pull/242.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/242/head:pull/242 PR: https://git.openjdk.java.net/jdk17/pull/242
Re: [jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
On Mon, 12 Jul 2021 17:11:01 GMT, Mandy Chung wrote:
>> Please review this PR that is fixing a mismatch between the implementation
>> for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its
>> implementation. I made a mistake while working on a recent CSR
>> [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed
>> the API but mistakenly thought that the implementation was in sync with the
>> spec. This is why this change is also including a unit test of the API for
>> `java.lang.constant.DynamicCallSiteDesc` modulo method `resolveCallSiteDesc`
>> which is covered in test `IndyDescTest` in the same test suite. Also this
>> change needs a CSR as while fixing the implementation of method `::withArgs`
>> I realized that the API of the varargs overloaded version of method `::of`
>> needed some rewording too as it is invoking the same private constructor
>> `::withArgs` is invoking. So it didn't make sense for the API of one method
>> to be more restrictive than the other. Please review also the accompanying
>> CSR.
>>
>> Thanks,
>> Vicente
>
> test/jdk/java/lang/constant/DynamicCallSiteDescTest.java line 60:
>
>> 58: "bootstrap",
>> 59: ClassDesc.of("java.lang.invoke.CallSite")
>> 60: ),
>
> A minor suggestion: you can have the return `DirectMethodHandleDesc` in a
> local variable to be used in all `DynamicCallSiteDesc::of` calls that would
> avoid copy-n-paste of same `ConstantDescs::ofCallsiteBootstrap` call.
will do
-
PR: https://git.openjdk.java.net/jdk17/pull/242
[jdk17] RFR: 8270025: DynamicCallSiteDesc::withArgs doesn't throw NPE
Please review this PR that is fixing a mismatch between the implementation for method `java.lang.constant.DynamicCallSiteDesc::withArgs` and its implementation. I made a mistake while working on a recent CSR [JDK-8224985](https://bugs.openjdk.java.net/browse/JDK-8224985) and fixed the API but mistakenly thought that the implementation was in sync with the spec. This is why this change is also including a unit test of the API for `java.lang.constant.DynamicCallSiteDesc` module for method `resolveCallSiteDesc` which is covered in test `IndyDescTest` in the same test suite. Also this change needs a CSR while fixing the implementation of method `::withArgs` I realized that the API of the varargs overloaded version of method `::of` needed some rewording too as it is invoking the same private constructor `::withArgs` is invoking. So it didn't make sense for the API of one method to be more restrictive than the other. Please review also the accompanying CSR. Thanks, Vicente - Commit messages: - 8270025: DynamicCallSiteDesc.withArgs doesn't throw NPE Changes: https://git.openjdk.java.net/jdk17/pull/242/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=242=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270025 Stats: 212 lines in 2 files changed: 210 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk17/pull/242.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/242/head:pull/242 PR: https://git.openjdk.java.net/jdk17/pull/242
Integrated: 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES
On Wed, 23 Jun 2021 20:57:24 GMT, Vicente Romero wrote: > Enum constant: jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES was > not removed when Sealed Classes were made final because the build was failing > without it. Now that the feature is final we should be able to safely removed > it. This is the intention of this patch. > > TIA, > Vicente This pull request has now been integrated. Changeset: 01c29d8f Author:Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/01c29d8f2c865009c0d5379ba2e2cd4d3015f018 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES Reviewed-by: jlahoda - PR: https://git.openjdk.java.net/jdk/pull/4578
RFR: 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES
Enum constant: jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES was not removed when Sealed Classes were made final because the build was failing without it. Now that the feature is final we should be able to safely removed it. This is the intention of this patch. TIA, Vicente - Commit messages: - 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES Changes: https://git.openjdk.java.net/jdk/pull/4578/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4578=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266407 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4578/head:pull/4578 PR: https://git.openjdk.java.net/jdk/pull/4578
[jdk17] Integrated: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
On Fri, 11 Jun 2021 15:01:20 GMT, Vicente Romero wrote: > This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) > which was intended to openjdk/jdk. > > Please review this PR which is syncing the implementation of > `DirectMethodHandleDesc.Kind.valueOf(int)` and > `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading > of the method's spec is that if the value of the refKind parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of true for its second argument, currently it is invoked with false > regardless of the value of the refKind parameter > > TIA This pull request has now been integrated. Changeset: 6b14c8a1 Author:Vicente Romero URL: https://git.openjdk.java.net/jdk17/commit/6b14c8a1e5316b6c8584e93ee7a94d9eaec676cf Stats: 36 lines in 2 files changed: 23 ins; 8 del; 5 mod 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Tue, 22 Jun 2021 00:45:36 GMT, Mandy Chung wrote:
>> Vicente Romero has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> updating after review comments
>
> test/jdk/java/lang/constant/MethodHandleDescTest.java line 379:
>
>> 377: for (int refKind : isInterfaceIgnored) {
>> 378: assertEquals(Kind.valueOf(refKind, false),
>> Kind.valueOf(refKind, true));
>> 379: }
>
> Can you also add the test cases to verify `Kind::valueOf` with
> `REF_invokeStatic` and `REF_invokeSpecial`?
please see them below, thanks
-
PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v4]
> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) > which was intended to openjdk/jdk. > > Please review this PR which is syncing the implementation of > `DirectMethodHandleDesc.Kind.valueOf(int)` and > `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading > of the method's spec is that if the value of the refKind parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of true for its second argument, currently it is invoked with false > regardless of the value of the refKind parameter > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/29/files - new: https://git.openjdk.java.net/jdk17/pull/29/files/be1a932c..d36a5528 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=29=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=29=02-03 Stats: 9 lines in 1 file changed: 8 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/29.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29 PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v3]
> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) > which was intended to openjdk/jdk. > > Please review this PR which is syncing the implementation of > `DirectMethodHandleDesc.Kind.valueOf(int)` and > `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading > of the method's spec is that if the value of the refKind parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of true for its second argument, currently it is invoked with false > regardless of the value of the refKind parameter > > TIA Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' into JDK-8267421 - updating after review comments - 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling - Changes: - all: https://git.openjdk.java.net/jdk17/pull/29/files - new: https://git.openjdk.java.net/jdk17/pull/29/files/8ed4cdb3..be1a932c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=29=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=29=01-02 Stats: 16865 lines in 275 files changed: 10155 ins; 6060 del; 650 mod Patch: https://git.openjdk.java.net/jdk17/pull/29.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29 PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Fri, 11 Jun 2021 18:17:10 GMT, Vicente Romero wrote: >> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) >> which was intended to openjdk/jdk. >> >> Please review this PR which is syncing the implementation of >> `DirectMethodHandleDesc.Kind.valueOf(int)` and >> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My >> reading of the method's spec is that if the value of the refKind parameter >> is: MethodHandleInfo.REF_invokeInterface, then >> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a >> value of true for its second argument, currently it is invoked with false >> regardless of the value of the refKind parameter >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > updating after review comments @mlchung I have uploaded a new commit, thanks for your comments - PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) > which was intended to openjdk/jdk. > > Please review this PR which is syncing the implementation of > `DirectMethodHandleDesc.Kind.valueOf(int)` and > `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading > of the method's spec is that if the value of the refKind parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of true for its second argument, currently it is invoked with false > regardless of the value of the refKind parameter > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: updating after review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/29/files - new: https://git.openjdk.java.net/jdk17/pull/29/files/b6b3f87c..8ed4cdb3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=29=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=29=00-01 Stats: 12 lines in 1 file changed: 0 ins; 9 del; 3 mod Patch: https://git.openjdk.java.net/jdk17/pull/29.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29 PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
On Fri, 11 Jun 2021 15:15:12 GMT, Vicente Romero wrote:
>> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416)
>> which was intended to openjdk/jdk.
>>
>> Please review this PR which is syncing the implementation of
>> `DirectMethodHandleDesc.Kind.valueOf(int)` and
>> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My
>> reading of the method's spec is that if the value of the refKind parameter
>> is: MethodHandleInfo.REF_invokeInterface, then
>> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a
>> value of true for its second argument, currently it is invoked with false
>> regardless of the value of the refKind parameter
>>
>> TIA
>
> src/java.base/share/classes/java/lang/constant/DirectMethodHandleDesc.java
> line 143:
>
>> 141: }
>> 142: if (kind.refKind == refKind &&
>> 143: (refKind != REF_invokeStatic || refKind !=
>> REF_invokeSpecial || kind.isInterface == isInterface)){
>
> @mlchung 13 hours ago Member
>
> It reads to me that the static initializer tries to set up the table such
> that valueOf(refKind, isInterface) should find the proper kind to return
> except this:
>
> // There is not a specific Kind for interfaces
> if (kind == VIRTUAL) kind = INTERFACE_VIRTUAL;
>
> This changes the entry for REF_invokeVirtual to kind INTERFACE_VIRTUAL. Do
> you know why? If this entry is set to VIRTUAL, then each refKind has two
> entries in the table corresponding to the correct result for valueOf.
this is the table that is being generated right now:
Table has 20 entries
0: null
1: null
2: Kind: [name: GETTER, refKind: 1, isInterface: false]
3: Kind: [name: GETTER, refKind: 1, isInterface: false]
4: Kind: [name: STATIC_GETTER, refKind: 2, isInterface: false]
5: Kind: [name: STATIC_GETTER, refKind: 2, isInterface: false]
6: Kind: [name: SETTER, refKind: 3, isInterface: false]
7: Kind: [name: SETTER, refKind: 3, isInterface: false]
8: Kind: [name: STATIC_SETTER, refKind: 4, isInterface: false]
9: Kind: [name: STATIC_SETTER, refKind: 4, isInterface: false]
10: Kind: [name: VIRTUAL, refKind: 5, isInterface: false]
11: Kind: [name: INTERFACE_VIRTUAL, refKind: 9, isInterface: true]
12: Kind: [name: STATIC, refKind: 6, isInterface: false]
13: Kind: [name: INTERFACE_STATIC, refKind: 6, isInterface: true]
14: Kind: [name: SPECIAL, refKind: 7, isInterface: false]
15: Kind: [name: INTERFACE_SPECIAL, refKind: 7, isInterface: true]
16: Kind: [name: CONSTRUCTOR, refKind: 8, isInterface: false]
17: Kind: [name: CONSTRUCTOR, refKind: 8, isInterface: false]
18: Kind: [name: INTERFACE_VIRTUAL, refKind: 9, isInterface: true]
19: Kind: [name: INTERFACE_VIRTUAL, refKind: 9, isInterface: true]
-
PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
On Fri, 11 Jun 2021 15:01:20 GMT, Vicente Romero wrote:
> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416)
> which was intended to openjdk/jdk.
>
> Please review this PR which is syncing the implementation of
> `DirectMethodHandleDesc.Kind.valueOf(int)` and
> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading
> of the method's spec is that if the value of the refKind parameter is:
> MethodHandleInfo.REF_invokeInterface, then
> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a
> value of true for its second argument, currently it is invoked with false
> regardless of the value of the refKind parameter
>
> TIA
src/java.base/share/classes/java/lang/constant/DirectMethodHandleDesc.java line
143:
> 141: }
> 142: if (kind.refKind == refKind &&
> 143: (refKind != REF_invokeStatic || refKind !=
> REF_invokeSpecial || kind.isInterface == isInterface)){
@mlchung 13 hours ago Member
It reads to me that the static initializer tries to set up the table such that
valueOf(refKind, isInterface) should find the proper kind to return except this:
// There is not a specific Kind for interfaces
if (kind == VIRTUAL) kind = INTERFACE_VIRTUAL;
This changes the entry for REF_invokeVirtual to kind INTERFACE_VIRTUAL. Do you
know why? If this entry is set to VIRTUAL, then each refKind has two entries in
the table corresponding to the correct result for valueOf.
test/jdk/java/lang/constant/MethodHandleDescTest.java line 364:
> 362: public void testKind() {
> 363: for (Kind k : Kind.values()) {
> 364: assertEquals(Kind.valueOf(k.refKind),
> Kind.valueOf(k.refKind, k.refKind == MethodHandleInfo.REF_invokeInterface));
@mlchung mlchung 2 days ago Member
Looks like the test does not verify the cases specified by valueOf(int refKind,
boolean isInterface).
i.e. For most values of refKind, there is an exact match regardless of the
value of isInterface except REF_invokeStatic and REF_invokeSpecial.
Do you mind adding those cases?
@vicente-romero-oracle vicente-romero-oracle 22 hours ago •
hum, the implementation for valueOf(int refKind, boolean isInterface) is
incorrect, the behavior does depends on the value of isInterface for example:
Kind.valueOf(1, false) returns GETTER while Kind.valueOf(1, true) fails with
java.lang.IllegalArgumentException will fix the implementation of valueOf(int
refKind, boolean isInterface) for it to match its spec
-
PR: https://git.openjdk.java.net/jdk17/pull/29
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Thu, 10 Jun 2021 23:26:21 GMT, Vicente Romero wrote: >> Please review this PR which is just syncing the implementation of >> DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the >> method's spec is that if the value of the `refKind` parameter is: >> MethodHandleInfo.REF_invokeInterface, then >> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a >> value of `true` for its second argument, currently it is invoked with >> `false` regardless of the value of the `refKind` parameter >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > addressing review changes I will be withdrawing this one to open a new [PR-29](https://github.com/openjdk/jdk17/pull/29) intended to jdk17, please let's follow the discussion there - PR: https://git.openjdk.java.net/jdk/pull/4416
Withdrawn: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
On Tue, 8 Jun 2021 16:46:36 GMT, Vicente Romero wrote: > Please review this PR which is just syncing the implementation of > DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the > method's spec is that if the value of the `refKind` parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of `true` for its second argument, currently it is invoked with `false` > regardless of the value of the `refKind` parameter > > TIA This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4416
[jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) which was intended to openjdk/jdk. Please review this PR which is syncing the implementation of `DirectMethodHandleDesc.Kind.valueOf(int)` and `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading of the method's spec is that if the value of the refKind parameter is: MethodHandleInfo.REF_invokeInterface, then DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a value of true for its second argument, currently it is invoked with false regardless of the value of the refKind parameter TIA - Commit messages: - 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling Changes: https://git.openjdk.java.net/jdk17/pull/29/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=29=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267421 Stats: 21 lines in 2 files changed: 17 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk17/pull/29.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29 PR: https://git.openjdk.java.net/jdk17/pull/29
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Wed, 9 Jun 2021 17:22:30 GMT, Mandy Chung wrote:
>> Vicente Romero has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressing review changes
>
> test/jdk/java/lang/constant/MethodHandleDescTest.java line 362:
>
>> 360: public void testKind() {
>> 361: for (Kind k : Kind.values()) {
>> 362: assertEquals(Kind.valueOf(k.refKind),
>> Kind.valueOf(k.refKind, k.refKind == MethodHandleInfo.REF_invokeInterface));
>
> Looks like the test does not verify the cases specified by `valueOf(int
> refKind, boolean isInterface)`.
> i.e. For most values of refKind, there is an exact match regardless of the
> value of isInterface except `REF_invokeStatic` and `REF_invokeSpecial`.
>
> Do you mind adding those cases?
@mlchung I have updated the PR with another commit, thanks for your comments
-
PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
> Please review this PR which is just syncing the implementation of > DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the > method's spec is that if the value of the `refKind` parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of `true` for its second argument, currently it is invoked with `false` > regardless of the value of the `refKind` parameter > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/4416/files - new: https://git.openjdk.java.net/jdk/pull/4416/files/ea47769d..a5e1c8e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4416=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4416=00-01 Stats: 19 lines in 2 files changed: 16 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4416/head:pull/4416 PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
On Wed, 9 Jun 2021 17:22:30 GMT, Mandy Chung wrote: > Looks like the test does not verify the cases specified by `valueOf(int > refKind, boolean isInterface)`. > i.e. For most values of refKind, there is an exact match regardless of the > value of isInterface except `REF_invokeStatic` and `REF_invokeSpecial`. > > Do you mind adding those cases? hum, the spec for `valueOf(int refKind, boolean isInterface)` is incorrect, the behavior does depends on the value of `isInterface` for example: `Kind.valueOf(1, false)` returns GETTER while `Kind.valueOf(1, true)` fails with `java.lang.IllegalArgumentException` will fix the implementation of `valueOf(int refKind, boolean isInterface)` for it to match its spec - PR: https://git.openjdk.java.net/jdk/pull/4416
RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling
Please review this PR which is just syncing the implementation of DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the method's spec is that if the value of the `refKind` parameter is: MethodHandleInfo.REF_invokeInterface, then DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a value of `true` for its second argument, currently it is invoked with `false` regardless of the value of the `refKind` parameter TIA - Commit messages: - 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling Changes: https://git.openjdk.java.net/jdk/pull/4416/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4416=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267421 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4416/head:pull/4416 PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 58:
> 56: void statement(SealedTypeChangesIntf obj) {
> 57: switch (obj) {
> 58: case A a -> System.err.println(1);
what about having tests with a case that matches the sealed class?
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
> 69: }
> 70:
> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
just for completeness shouldn't we have a test with sealed, non-abstract
classes?
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Integrated: 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded
On Mon, 17 May 2021 16:53:24 GMT, Vicente Romero wrote: > This is a very small patch that is just rewording the spec for > DynamicCallSiteDesc::withArgs. Basically adding that NPE can also be thrown > if the content of the argument is `null` > > TIA for the review This pull request has now been integrated. Changeset: f04db5fb Author: Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/f04db5fbd77892e94a325942542815bbb24cddea Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded Reviewed-by: jlahoda - PR: https://git.openjdk.java.net/jdk/pull/4067
Re: RFR: 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded
On Mon, 17 May 2021 16:53:24 GMT, Vicente Romero wrote: > This is a very small patch that is just rewording the spec for > DynamicCallSiteDesc::withArgs. Basically adding that NPE can also be thrown > if the content of the argument is `null` > > TIA for the review any reviewers for this simple PR? - PR: https://git.openjdk.java.net/jdk/pull/4067
Re: RFR: 8265130: Make ConstantDesc class hierarchy sealed [v4]
On Fri, 21 May 2021 08:53:51 GMT, Gavin Bierman wrote: >> Hi all, >> >> Please review this patch to make the ConstantDesc hierarchy `sealed`, as was >> promised in its Javadoc, now that sealed classes are finalising in JDK 17. >> >> Thanks, >> Gavin > > Gavin Bierman has updated the pull request incrementally with one additional > commit since the last revision: > > Reordering class modifiers looks good to me - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4135
Integrated: 8260517: implement Sealed Classes as a standard feature in Java
On Fri, 16 Apr 2021 01:08:57 GMT, Vicente Romero wrote: > Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. This pull request has now been integrated. Changeset: 0fa9223f Author:Vicente Romero URL: https://git.openjdk.java.net/jdk/commit/0fa9223f34bc33635079763362f42f0a5c53759b Stats: 444 lines in 54 files changed: 51 ins; 275 del; 118 mod 8260517: implement Sealed Classes as a standard feature in Java Co-authored-by: Harold Seigel Co-authored-by: Vicente Romero Reviewed-by: dholmes, mcimadamore, jlahoda - PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v8]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - restoring jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES, it is needed by the build - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - updating comment after review feedback - removing javax.lang.model changes - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - fixing failing regression tests - ... and 2 more: https://git.openjdk.java.net/jdk/compare/cc4f43fb...d220bc20 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/0208dcf0..d220bc20 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=06-07 Stats: 40144 lines in 1123 files changed: 19391 ins; 13080 del; 7673 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded
On Mon, 17 May 2021 16:53:24 GMT, Vicente Romero wrote: > This is a very small patch that is just rewording the spec for > DynamicCallSiteDesc::withArgs. Basically adding that NPE can also be thrown > if the content of the argument is `null` > > TIA for the review ping - PR: https://git.openjdk.java.net/jdk/pull/4067
RFR: 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded
This is a very small patch that is just rewording the spec for DynamicCallSiteDesc::withArgs. Basically adding that NPE can also be thrown if the content of the argument is `null` TIA for the review - Commit messages: - 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded Changes: https://git.openjdk.java.net/jdk/pull/4067/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4067=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8224158 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4067.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4067/head:pull/4067 PR: https://git.openjdk.java.net/jdk/pull/4067
Re: RFR: 8264561: javap get NegativeArraySizeException on bad instruction
On Mon, 17 May 2021 13:11:56 GMT, Adam Sotona wrote: > Actual javap implementation reacts on corrupted TABLESWITCH or LOOKUPSWITCH > bytecode instructions resulting to negative array allocation with > NegativeArraySizeException, which is reported to user with stack trace and as > serious internal error. > > The fix in c.s.t.classfile.Instruction is checking for negative array size of > corrupted TABLESWITCH or LOOKUPSWITCH bytecode and throwing > j.l.IllegalStateException instead of the NegativeArraySizeException. > > Another part of the fix in c.s.t.javap.CodeWriter is catching > j.l.IllegalStateException and reporting it as error in the analyzed bytecode, > instead of passing it up and causing serious internal javap error. lgtm - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4061
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v7]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision: - Merge branch 'master' into JDK-8260517 - restoring jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES, it is needed by the build - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - updating comment after review feedback - removing javax.lang.model changes - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - fixing failing regression tests - JVM related changes - ... and 1 more: https://git.openjdk.java.net/jdk/compare/10336a26...0208dcf0 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/304fd76a..0208dcf0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=05-06 Stats: 18908 lines in 391 files changed: 9887 ins; 4814 del; 4207 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v6]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: restoring jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES, it is needed by the build - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/2744f615..304fd76a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=04-05 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v5]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - updating comment after review feedback - removing javax.lang.model changes - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - fixing failing regression tests - JVM related changes - 8260517: Compiler implementation for Sealed Classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/43e9cb5f..2744f615 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=03-04 Stats: 506473 lines in 3844 files changed: 20558 ins; 483521 del; 2394 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v4]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' into JDK-8260517 - updating comment after review feedback - removing javax.lang.model changes - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - fixing failing regression tests - JVM related changes - 8260517: Compiler implementation for Sealed Classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/8ebe56fd..43e9cb5f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=02-03 Stats: 40790 lines in 1391 files changed: 8730 ins; 27464 del; 4596 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v3]
On Wed, 21 Apr 2021 14:42:39 GMT, Maurizio Cimadamore wrote: > Compiler changes look good (I have not checked SymbolGenerator). > > Why were some tests removed? thanks for the review. The removed tests were already covered in langtools regression tests, so I only removed duplicated tests - PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v3]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: updating comment after review feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/5aef5108..8ebe56fd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v2]
On Fri, 16 Apr 2021 02:10:05 GMT, David Holmes wrote:
> Hi Vicente,
>
> Hotspot and hotspot tests all look fine. One query: why was this test removed?
>
> test/hotspot/jtreg/runtime/sealedClasses/AbstractSealedTest.java
>
> is that functionality tested elsewhere? (The other deleted test seemed
> obviously trivial.)
>
> Thanks,
> David
Hi David, thanks for your comments, yes regarding `test
test/hotspot/jtreg/runtime/sealedClasses/AbstractSealedTest.java`, it was
removed because the functionality is tested in
`test/langtools/tools/javac/sealed/SealedCompilationTests.java`
> src/hotspot/share/classfile/classFileParser.cpp line 3916:
>
>> 3914: record_attribute_start = cfs->current();
>> 3915: record_attribute_length = attribute_length;
>> 3916: } else if (_major_version >= JAVA_17_VERSION) {
>
> Can you update the comment at L3932 to say JAVA_17_VERSION please.
sure
-
PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260517: implement Sealed Classes as a standard feature in Java [v2]
> Please review this PR that intents to make sealed classes a final feature in > Java. This PR contains compiler and VM changes. In line with similar PRs, > which has made preview features final, this one is mostly removing preview > related comments from APIs plus options in test cases. Please also review the > related [CSR](https://bugs.openjdk.java.net/browse/JDK-8265090) > > Thanks > > note: this PR is related to > [PR-3528](https://github.com/openjdk/jdk/pull/3528) and must be integrated > after it. Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: removing javax.lang.model changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3526/files - new: https://git.openjdk.java.net/jdk/pull/3526/files/6e2a99c6..5aef5108 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3526=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3526=00-01 Stats: 9 lines in 2 files changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
RFR: 8260517: implement Sealed Classes as a standard feature
Please review this PR that intents to make sealed classes a final feature in Java. This PR contains compiler and VM changes. In line with similar PRs, which has made preview features final, this one is mostly removing preview related comments from APIs plus options in test cases. Thanks - Commit messages: - Merge branch 'master' into JDK-8260517 - Merge branch 'master' into JDK-8260517 - fixing failing regression tests - JVM related changes - 8260517: Compiler implementation for Sealed Classes Changes: https://git.openjdk.java.net/jdk/pull/3526/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3526=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260517 Stats: 450 lines in 56 files changed: 48 ins; 282 del; 120 mod Patch: https://git.openjdk.java.net/jdk/pull/3526.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3526/head:pull/3526 PR: https://git.openjdk.java.net/jdk/pull/3526
Re: RFR: 8260403: javap should be more robust in the face of invalid class files
On Thu, 25 Feb 2021 13:01:30 GMT, Adam Sotona wrote: > Please review javap fix to handle java.lang.IllegalStateException for classes > with invalid Signature attribute. > New test (T8260403) parsing class with invalid Signature attribute (as > described in the bug) is included. > Fix wraps java.lang.IllegalStateException, reports "Error: Invalid value for > Signature attribute: " and continues. > > Thanks, > Adam lgtm! - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2724
RFR: 8260959: remove RECORDS from PreviewFeature.Feature enum
Please review this simple fix that is removing the RECORDS enum constant from the PreviewFeature.Feature enum, now that RECORDS are final in 16 this constant can be safely removed. Thanks, Vicente - Commit messages: - 8260959: remove RECORDS from PreviewFeature.Feature enum Changes: https://git.openjdk.java.net/jdk/pull/2360/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2360=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260959 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2360.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2360/head:pull/2360 PR: https://git.openjdk.java.net/jdk/pull/2360
Re: [jdk16] RFR: 8256693: getAnnotatedReceiverType parameterizes types too eagerly [v2]
On Thu, 17 Dec 2020 10:07:13 GMT, Joel Borggrén-Franck wrote: >> The fix for JDK-8256693 too often produces a ParameterizedType as the result >> of getAnnotatedReceiverType().getType() . A ParameterizedType is necessary >> only when this type or any of its transitive owner types has type >> parameters, but should be avoided if this isn't the case. >> >> This implementation recursively creates a chain of ParameterizedTypes >> starting from the outermost type that has type parameters. >> >> See here for the now closed JDK 17 pr: >> https://github.com/openjdk/jdk/pull/1414 > > Joel Borggrén-Franck has updated the pull request incrementally with one > additional commit since the last revision: > > Use constant for expected cases Marked as reviewed by vromero (Reviewer). - PR: https://git.openjdk.java.net/jdk16/pull/33
Re: [jdk16] RFR: 8256693: getAnnotatedReceiverType parameterizes types too eagerly
On Wed, 16 Dec 2020 19:57:32 GMT, Joel Borggrén-Franck wrote: > Yes, not great, but at least it isn't brittle when running the test, only > when changing it. I'd like to take a separate pass over the tests for 17 if > possible. what about declaring a static final field for that value instead of using a literal? - PR: https://git.openjdk.java.net/jdk16/pull/33
Re: [jdk16] RFR: 8256693: getAnnotatedReceiverType parameterizes types too eagerly
On Wed, 16 Dec 2020 09:41:47 GMT, Joel Borggrén-Franck
wrote:
> The fix for JDK-8256693 too often produces a ParameterizedType as the result
> of getAnnotatedReceiverType().getType() . A ParameterizedType is necessary
> only when this type or any of its transitive owner types has type parameters,
> but should be avoided if this isn't the case.
>
> This implementation recursively creates a chain of ParameterizedTypes
> starting from the outermost type that has type parameters.
>
> See here for the now closed JDK 17 pr:
> https://github.com/openjdk/jdk/pull/1414
Changes requested by vromero (Reviewer).
test/jdk/java/lang/annotation/typeAnnotations/GetAnnotatedReceiverType.java
line 181:
> 179: if (failures != 0)
> 180: throw new RuntimeException("Test failed, see log for
> details");
> 181: else if (tests != 25)
this looks a bit brittle, isn't it better to count the number of tests failing
and issue an error if that number is > 0?
-
PR: https://git.openjdk.java.net/jdk16/pull/33
