Re: RFR: 8246774: Record Classes (final) implementation [v8]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. - Changes: - all: https://git.openjdk.java.net/jdk/pull/290/files - new: https://git.openjdk.java.net/jdk/pull/290/files/482fedec..76e3d278 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=290=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=290=06-07 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v7]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge branch 'JDK-8246774' of https://github.com/vicente-romero-oracle/jdk into JDK-8246774 - adding missing changes to some tests - Merge branch 'master' into JDK-8246774 - modifiying @since from 14 to 16 - Merge pull request #1 from ChrisHegarty/record-serial-tests Remove preview args from JDK tests - Remove preview args from ObjectMethodsTest - Remove preview args from record serialization tests - removing the javax.lang.model related code to be moved to a separate bug - 8246774: Record Classes (final) implementation Co-authored-by: Vicente Romero Co-authored-by: Harold Seigel Co-authored-by: Jonathan Gibbons Co-authored-by: Brian Goetz Co-authored-by: Maurizio Cimadamore Co-authored-by: Joe Darcy Co-authored-by: Chris Hegarty Co-authored-by: Jan Lahoda - adding missing changes to some tests - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d5be8294...482fedec - Changes: https://git.openjdk.java.net/jdk/pull/290/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=290=06 Stats: 535 lines in 109 files changed: 32 ins; 278 del; 225 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v6]
On Fri, 25 Sep 2020 00:41:47 GMT, Vicente Romero wrote: >> Co-authored-by: Vicente Romero >> Co-authored-by: Harold Seigel >> Co-authored-by: Jonathan Gibbons >> Co-authored-by: Brian Goetz >> Co-authored-by: Maurizio Cimadamore >> Co-authored-by: Joe Darcy >> Co-authored-by: Chris Hegarty >> Co-authored-by: Jan Lahoda > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > adding missing changes to some tests Marked as reviewed by jlahoda (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v3]
On Fri, 25 Sep 2020 15:47:30 GMT, Peter Levart wrote: >> [CSR: Record Classes](https://bugs.openjdk.java.net/browse/JDK-8253605) > > Hi @vicente-romero-oracle , note that besides tests, there is also a JMH > benchmark that measures the performance of > records deserialization (org.openjdk.bench.java.io.RecordDeserialization) > which forced us to modify the build procedure > for all benchmarks to include --enable-preview option in JDK 15 and backports > (see > https://bugs.openjdk.java.net/browse/JDK-8248135). If you undo this change in > JDK 16 then also the problem described in > https://bugs.openjdk.java.net/browse/JDK-8250669 and > https://bugs.openjdk.java.net/browse/JDK-8248429 will disapear. > After that, perhaps undoing the same for JDK 15 and backports together with > removing the benchmark is also possible to > resolve the issues in older releases as most developement will probably > happen in JDK 16 then and so the need for > performance testing will mostly be needed in there. We still have to figure > out how to enable having benchmarks for > preview features as in the future there most probably will be a need for that. Langtools code looks good to me. We probably can also remove the RECORDS entry from PreviewFeature.Feature. - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v3]
On Fri, 25 Sep 2020 02:38:01 GMT, Vicente Romero wrote: >> I have modified the `@since`: 14 -> 16 > > [CSR: Record Classes](https://bugs.openjdk.java.net/browse/JDK-8253605) Hi @vicente-romero-oracle , note that besides tests, there is also a JMH benchmark that measures the performance of records deserialization which forced us to modify the build procedure for all benchmarks to include --enable-preview option in JDK 15 and backports (see https://bugs.openjdk.java.net/browse/JDK-8248135). If you undo this change in JDK 16 then also the problem described in https://bugs.openjdk.java.net/browse/JDK-8250669 and https://bugs.openjdk.java.net/browse/JDK-8248429 will disapear. After that, perhaps undoing the same for JDK 15 and backports together with removing the benchmark is also possible to resolve the issues in older releases as most developement will probably happen in JDK 16 then and so the need for performance testing will mostly be needed in there. We still have to figure out how to enable having benchmarks for preview features as in the future the sure will be a need for that. - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v3]
On Thu, 24 Sep 2020 15:45:22 GMT, Vicente Romero wrote: >> The classfile parser changes look good to me. > > I have modified the `@since`: 14 -> 16 [CSR: Record Classes](https://bugs.openjdk.java.net/browse/JDK-8253605) - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v6]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: adding missing changes to some tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/290/files - new: https://git.openjdk.java.net/jdk/pull/290/files/89f7cc54..915b67e0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=290=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=290=04-05 Stats: 71 lines in 5 files changed: 9 ins; 11 del; 51 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v5]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge branch 'master' into JDK-8246774 - modifiying @since from 14 to 16 - Merge pull request #1 from ChrisHegarty/record-serial-tests Remove preview args from JDK tests - Remove preview args from ObjectMethodsTest - Remove preview args from record serialization tests - removing the javax.lang.model related code to be moved to a separate bug - 8246774: Record Classes (final) implementation Co-authored-by: Vicente Romero Co-authored-by: Harold Seigel Co-authored-by: Jonathan Gibbons Co-authored-by: Brian Goetz Co-authored-by: Maurizio Cimadamore Co-authored-by: Joe Darcy Co-authored-by: Chris Hegarty Co-authored-by: Jan Lahoda - Changes: https://git.openjdk.java.net/jdk/pull/290/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=290=04 Stats: 464 lines in 104 files changed: 23 ins; 267 del; 174 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v4]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: modifiying @since from 14 to 16 - Changes: - all: https://git.openjdk.java.net/jdk/pull/290/files - new: https://git.openjdk.java.net/jdk/pull/290/files/26b80775..514b0a80 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=290=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=290=02-03 Stats: 8 lines in 7 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v3]
On Thu, 24 Sep 2020 12:23:13 GMT, Coleen Phillimore wrote: >> Vicente Romero has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Merge pull request #1 from ChrisHegarty/record-serial-tests >> >>Remove preview args from JDK tests >> - Remove preview args from ObjectMethodsTest >> - Remove preview args from record serialization tests > > The classfile parser changes look good to me. I have modified the `@since`: 14 -> 16 - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v3]
On Wed, 23 Sep 2020 03:34:29 GMT, Vicente Romero wrote: >> Co-authored-by: Vicente Romero >> Co-authored-by: Harold Seigel >> Co-authored-by: Jonathan Gibbons >> Co-authored-by: Brian Goetz >> Co-authored-by: Maurizio Cimadamore >> Co-authored-by: Joe Darcy >> Co-authored-by: Chris Hegarty >> Co-authored-by: Jan Lahoda > > Vicente Romero has updated the pull request incrementally with three > additional commits since the last revision: > > - Merge pull request #1 from ChrisHegarty/record-serial-tests > >Remove preview args from JDK tests > - Remove preview args from ObjectMethodsTest > - Remove preview args from record serialization tests The classfile parser changes look good to me. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
On Thu, 24 Sep 2020 00:23:13 GMT, Mandy Chung wrote: >> @vicente-romero-oracle I noticed that we can also remove the preview args >> from the record serialization tests and >> ObjectMethodsTest. I opened a PR against the branch in your fork. You should >> be able to just merge in the changes. See >> https://github.com/vicente-romero-oracle/jdk/pull/1 > > What is the policy of `@since` release value when a preview API becomes > final.I would expect `@since` should be > updated from 14 to 16 because 16 is the Java SE release these APIs are added?? Thanks Alex. @vicente-romero-oracle `@since` needs to be changed. - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
On 9/23/2020 5:26 PM, Mandy Chung wrote: What is the policy of `@since` release value when a preview API becomes final.I would expect `@since` should be updated from 14 to 16 because 16 is the Java SE release these APIs are added?? Yes. Per http://openjdk.java.net/jeps/12#Specifications-of-Preview-Features : In particular, all elements of a preview API must have the following tags: ... An @since tag that indicates the release when [the API element] was first added. *If the API element is eventually made final and permanent in Java SE $N, then the @since tag must be changed to indicate the $N release (the element's history prior to $N is not of long-term interest).* Alex
Re: RFR: 8246774: Record Classes (final) implementation
On Tue, 22 Sep 2020 09:49:12 GMT, Chris Hegarty wrote: >> note: I have removed from the original patch the code related to >> javax.lang.model, I will publish them in a separate PR > > @vicente-romero-oracle I noticed that we can also remove the preview args > from the record serialization tests and > ObjectMethodsTest. I opened a PR against the branch in your fork. You should > be able to just merge in the changes. See > https://github.com/vicente-romero-oracle/jdk/pull/1 What is the policy of `@since` release value when a preview API becomes final. I would expect `@since` should be updated from 14 to 16 because 16 is the Java SE release these APIs are added?? - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v3]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has updated the pull request incrementally with three additional commits since the last revision: - Merge pull request #1 from ChrisHegarty/record-serial-tests Remove preview args from JDK tests - Remove preview args from ObjectMethodsTest - Remove preview args from record serialization tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/290/files - new: https://git.openjdk.java.net/jdk/pull/290/files/543e5054..26b80775 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=290=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=290=01-02 Stats: 95 lines in 21 files changed: 0 ins; 35 del; 60 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
good catch Chris, thanks for the patch, Vicente On 9/22/20 5:51 AM, Chris Hegarty wrote: On Mon, 21 Sep 2020 23:21:18 GMT, Vicente Romero wrote: Hi Vicente, Please file a separate subtask for the javax.lang.model changes. This helps with the JSR 269 MR paperwork. Thanks, -Joe note: I have removed from the original patch the code related to javax.lang.model, I will publish them in a separate PR @vicente-romero-oracle I noticed that we can also remove the preview args from the record serialization tests and ObjectMethodsTest. I opened a PR against the branch in your fork. You should be able to just merge in the changes. See https://github.com/vicente-romero-oracle/jdk/pull/1 - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
On Mon, 21 Sep 2020 23:21:18 GMT, Vicente Romero wrote: >> Hi Vicente, >> Please file a separate subtask for the javax.lang.model changes. This helps >> with the JSR 269 MR paperwork. >> Thanks, >> -Joe > > note: I have removed from the original patch the code related to > javax.lang.model, I will publish them in a separate PR @vicente-romero-oracle I noticed that we can also remove the preview args from the record serialization tests and ObjectMethodsTest. I opened a PR against the branch in your fork. You should be able to just merge in the changes. See https://github.com/vicente-romero-oracle/jdk/pull/1 - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation [v2]
> Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: removing the javax.lang.model related code to be moved to a separate bug - Changes: - all: https://git.openjdk.java.net/jdk/pull/290/files - new: https://git.openjdk.java.net/jdk/pull/290/files/9eedb3ab..543e5054 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=290=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=290=00-01 Stats: 134 lines in 12 files changed: 130 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290 PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
On Mon, 21 Sep 2020 21:53:05 GMT, Joe Darcy wrote: >> Please review the fix for [1]. The intention of this patch is to make >> records final removing the need to >> use --enable-preview in order to be able to include a record declaration in >> a source or for the VM to execute code >> compiled from record classes, Thanks >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8246774 > > Hi Vicente, > Please file a separate subtask for the javax.lang.model changes. This helps > with the JSR 269 MR paperwork. > Thanks, > -Joe note: I have removed from the original patch the code related to javax.lang.model, I will publish them in a separate PR - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
On Mon, 21 Sep 2020 21:36:39 GMT, Vicente Romero wrote: >> Co-authored-by: Vicente Romero >> Co-authored-by: Harold Seigel >> Co-authored-by: Jonathan Gibbons >> Co-authored-by: Brian Goetz >> Co-authored-by: Maurizio Cimadamore >> Co-authored-by: Joe Darcy >> Co-authored-by: Chris Hegarty >> Co-authored-by: Jan Lahoda > > Please review the fix for [1]. The intention of this patch is to make records > final removing the need to > use --enable-preview in order to be able to include a record declaration in a > source or for the VM to execute code > compiled from record classes, Thanks > > [1] https://bugs.openjdk.java.net/browse/JDK-8246774 Hi Vicente, Please file a separate subtask for the javax.lang.model changes. This helps with the JSR 269 MR paperwork. Thanks, -Joe - PR: https://git.openjdk.java.net/jdk/pull/290
Re: RFR: 8246774: Record Classes (final) implementation
On Mon, 21 Sep 2020 21:30:51 GMT, Vicente Romero wrote: > Co-authored-by: Vicente Romero > Co-authored-by: Harold Seigel > Co-authored-by: Jonathan Gibbons > Co-authored-by: Brian Goetz > Co-authored-by: Maurizio Cimadamore > Co-authored-by: Joe Darcy > Co-authored-by: Chris Hegarty > Co-authored-by: Jan Lahoda Please review the fix for [1]. The intention of this patch is to make records final removing the need to use --enable-preview in order to be able to include a record declaration in a source or for the VM to execute code compiled from record classes, Thanks [1] https://bugs.openjdk.java.net/browse/JDK-8246774 - PR: https://git.openjdk.java.net/jdk/pull/290
