Add support for Records in JDK 14 #766

FrauBoes · 2020-09-03T16:35:32Z

Here is the initial version of a record serializer. A few things to note:

The serializer uses method handles instead of core reflection, i.e to invoke the record's constructor
The serial form of the record is a stream of the record components in the same order as in the class file. I see FieldSerialiser orders by name, this could be done here as well.
I kept plenty of debugging output in RecordSerializerTest, this can be cleaned up before integration.
RecordSerializerTest was added to the default serializers. Do we needs to add a test case to SerializerCompatTest.TestData?
Additional config was added to the pom files to only compile and run the new test if JDK 14+ is available. In this case all tests are compiled and run with JDK 14+ --enable-preview. I am not very familiar with Maven and ended up with this solution after asking a few questions on their mailing list. There might well be a better way of handling test compilation and test runs.
The only test that acts up when run with JDK 14 is UnsafeByteBufferInputOutputTest

[INFO] Running com.esotericsoftware.kryo.io.UnsafeByteBufferInputOutputTest Streams with preallocated direct memory are not supported on this JVM java.lang.UnsupportedOperationException: No direct ByteBuffer constructor is available. at com.esotericsoftware.kryo.unsafe.UnsafeUtil.newDirectBuffer(UnsafeUtil.java:125) at com.esotericsoftware.kryo.io.UnsafeByteBufferInputOutputTest.testByteBufferOutputWithPreallocatedMemory(UnsafeByteBufferInputOutputTest.java:43)

I'm sure there are plenty of things that can be improved. Any suggestions welcome!

theigl · 2020-09-04T09:44:26Z

@FrauBoes: This is great! Thanks again for taking the time to implement this.

The serializer uses method handles instead of core reflection, i.e to invoke the record's constructor

That's perfectly fine with me. This is just an implementation detail and can be changed later on if necessary.

The serial form of the record is a stream of the record components in the same order as in the class file. I see FieldSerialiser orders by name, this could be done here as well.

FieldSerializer orders by name because the order of fields returned by getDeclaredFields is not guaranteed to be the same across JDK implementations. Is the specification for getRecordComponents more strict? Are they always be returned in the same order?

I kept plenty of debugging output in RecordSerializerTest, this can be cleaned up before integration.

👍

RecordSerializerTest was added to the default serializers. Do we needs to add a test case to SerializerCompatTest.TestData

Yes, we should add a TestDataJava14 to SerializationCompatTestData and register it in SerializerCompatTest. The problem is that we cannot depend on the record class directly in the test data because it has to be compilable with JDK11. Is it possible to create a record reflectively? Alternatively, we can move the test data to the JDK14 folder and only load it when running on JDK 14+.

We can probably move to a JDK11 target and compile with JDK14+ in Kryo 6. This will make the test setup much simpler and we can get rid of all the reflection currently required for RecordSerializer.

Additional config was added to the pom files to only compile and run the new test if JDK 14+ is available. In this case all tests are compiled and run with JDK 14+ --enable-preview. I am not very familiar with Maven and ended up with this solution after asking a few questions on their mailing list. There might well be a better way of handling test compilation and test runs.

This looks good to me. It is basically the same approach we currently use to run JDK11 tests while still targeting JDK8.

The only test that acts up when run with JDK 14 is UnsafeByteBufferInputOutputTest

That can be fixed. We just need to use a different constructor in UnsafeUtil:

directByteBufferConstructor = buffer.getClass().getDeclaredConstructor(long.class, int.class);

instead of:

directByteBufferConstructor = buffer.getClass().getDeclaredConstructor(long.class, int.class, Object.class);

I have some other minor comments that I will add as a PR review.

pom.xml

src/com/esotericsoftware/kryo/Kryo.java

src/com/esotericsoftware/kryo/serializers/RecordSerializer.java

theigl · 2020-09-04T10:29:40Z

@magro: Could you take a look at this as well?

The main decision we have to make is if we want to include support for records in Kryo 5 or not. If we set the baseline for Kryo 6 to JDK11 and compile using JDK14+, the implementation of the serializer would be significantly simpler.

theigl · 2020-09-04T11:05:43Z

Jackson chose a slightly different (and simpler) approach for separating tests for different JDK versions: FasterXML/jackson-databind#2714

They use a different root folder for tests that depend on a certain JDK version:

<source>src/test-jdk14/java</source>

I think we should adopt the same approach.

ChrisHegarty · 2020-09-04T15:48:16Z

Is the specification for getRecordComponents more strict? Are they always be returned in the same order?

@theigl Yes, Class::getRecordComponents is specified to return the record components in the same order. More specifically, from Class.java:

Returns an array of RecordComponent objects representing all the record
components of this record class, or null if this class is not a record class.

The components are returned in the same order that they are declared in
the record header. The array is empty if this record class has no
components. If the class is not a record class, that is isRecord() returns
false, then this method returns null. Conversely, if isRecord() returns true,
then this method returns a non-null value.

FrauBoes · 2020-09-04T16:06:06Z

One advantage of ordering record components by name is that it facilitates duck typing of different record type versions. It's not required but provides more flexibility.

theigl · 2020-09-07T11:14:52Z

@ChrisHegarty: Thanks for clarifying this!

One advantage of ordering record components by name is that it facilitates duck typing of different record type versions. It's not required but provides more flexibility.

@FrauBoes: Either way is fine for me as long as we don't need to break serialization compatibility later on.

What I don't quite understand is this: How would you invoke the constructor with the right order of arguments if they are sorted by name?

ChrisHegarty · 2020-09-07T16:35:46Z

src/com/esotericsoftware/kryo/unsafe/UnsafeUtil.java

@@ -124,7 +125,7 @@ public static ByteBuffer newDirectBuffer (long address, int size) {
 		if (directByteBufferConstructor == null)
 			throw new UnsupportedOperationException("No direct ByteBuffer constructor is available.");
 		try {
-			return directByteBufferConstructor.newInstance(address, size, null);
+			return directByteBufferConstructor.newInstance(address, size);
 		} catch (Exception ex) {
 			throw new KryoException("Error creating a ByteBuffer at address: " + address, ex);


This change is unrelated to records, but is clearly required to successfully run on JDK 14+, since the non-Public constructors of DirectByteBuffer has been refactored / changed.

FrauBoes · 2020-09-07T16:58:18Z

@theigl Thanks for your comments so far.

Yes, we should add a TestDataJava14 to SerializationCompatTestData and register it in SerializerCompatTest. The problem is that we cannot depend on the record class directly in the test data because it has to be compilable with JDK11. Is it possible to create a record reflectively? Alternatively, we can move the test data to the JDK14 folder and only load it when running on JDK 14+.

That's indeed not that straight-forward. I have a version working that uses an in-memory compiler and a custom classloader to instantiate a record at runtime. It's a lot of utility code for a single test case so I would suggest to put a stripped down copy of SerializationCompatTest/TestData that only covers the record test case into the dedicated jdk14 test directory.

Jackson chose a slightly different (and simpler) approach for separating tests for different JDK versions: FasterXML/jackson-databind#2714

They use a different root folder for tests that depend on a certain JDK version:

<source>src/test-jdk14/java</source>

I think we should adopt the same approach.

This is definitely an option. It's based on the fact that in this location maven ignores the test-jdk14 directory during default-compile and default-testCompile, but it does require an additional plugin build-helper-maven-plugin to add these test sources. I did make a change to the poms and moved all the configuration into the root pom, which is neater. Also, if we do add a jdk14 version of SerializationCompatTest, it would make sense to keep it in the test directory to have access to KryoTestCase etc.

What I don't quite understand is this: How would you invoke the constructor with the right order of arguments if they are sorted by name?

The idea would be to order by name before serialization and expect the same order at deserialization. The order can be stored as part of the RecordComponent helper class in the RecordSerializer.
While this adds a bit more code for the ordering, it supports record type versioning as mentioned. It is also in line with regular types, which means future type transitions from regular type to record type are facilitated.

theigl · 2020-09-08T16:01:06Z

That's indeed not that straight-forward. I have a version working that uses an in-memory compiler and a custom classloader to instantiate a record at runtime. It's a lot of utility code for a single test case so I would suggest to put a stripped down copy of SerializationCompatTest/TestData that only covers the record test case into the dedicated jdk14 test directory.

Let's go for the simplest approach! I think it should be possible to put only the TestData into the jdk14 folder and add it to the test datas in SerializationCompatTest via Class.forName for JDK >= 14. If that doesn't work, we could create a temporary, stripped down version of SerializationCompatTest as you suggested.

This is definitely an option. It's based on the fact that in this location maven ignores the test-jdk14 directory during default-compile and default-testCompile, but it does require an additional plugin build-helper-maven-plugin to add these test sources. I did make a change to the poms and moved all the configuration into the root pom, which is neater. Also, if we do add a jdk14 version of SerializationCompatTest, it would make sense to keep it in the test directory to have access to KryoTestCase etc.

Your current solution looks good to me! Everything is in one place and we do not need any another plugin.

The idea would be to order by name before serialization and expect the same order at deserialization. The order can be stored as part of the RecordComponent helper class in the RecordSerializer.
While this adds a bit more code for the ordering, it supports record type versioning as mentioned. It is also in line with regular types, which means future type transitions from regular type to record type are facilitated.

That sounds reasonable. Let's go for the ordered implementation if the code for ordering does not greatly increase the complexity of the serializer.

FrauBoes · 2020-09-09T17:04:07Z

Added TestDataJava14 and ordering to RecordSerializer. Let me know if there is anything else that needs attention.

FrauBoes · 2020-09-16T11:00:56Z

@theigl I tried to build with the IntelliJ Maven integration and it works fine with JDK14. What I do:

jdk14ge profile is auto-selected (see screenshot)
mvn clean
mvn install

If the profile is not selected, compilation with JDK14 fails. Could that be the issue? I'm still happy to make the change, but want to understand the issue.

theigl · 2020-09-16T11:31:45Z

@FrauBoes: The problem is not the IntelliJ Maven Integration. That can be made to work. I was referring to opening any test in IntelliJ and simply running it. IntelliJ then tries to compile all classes and fails:

This is because IntelliJ infers the language level from Maven when importing the project. In the Jackson setup, they override the default language level for the JDK14 profile and force IntelliJ to use JDK14. This, and upgrading the Maven shade plugin allowed me to run individual tests in IntelliJ as described above.

If we now switch the project SDK to JDK11 and try to run a test from within IntelliJ, it fails. This is because IntelliJ does not understand Maven excludes and tries to compile RecordSerializerTest.

To summarize:

The goal is that any user on JDK11 - 15 should be able to checkout the project, build it with Maven, and run individual unit tests directly in IntelliJ. Our current approach fails in the last requirement.

FrauBoes · 2020-09-18T08:23:46Z

@theigl Gotcha, now I understand the requirements. I'll look into making the change. Thanks!

FrauBoes · 2020-09-18T11:51:06Z

test/com/esotericsoftware/kryo/SerializationCompatTest.java

-		int[] versions = new int[] {parseInt(strVersions[0]), parseInt(strVersions[1])};
-		JAVA_VERSION = versions[0] > 1 ? versions[0] : versions[1];
+		if (strVersions.length == 1) {
+			JAVA_VERSION = parseInt(strVersions[0]);


Unrelated change to better support version strings

FrauBoes · 2020-09-18T12:03:41Z

Made the change we discussed. I'm able to build with Maven and run individual tests in IntelliJ with JDK 11, 14 and 15. The only caveat is that jdk14 tests are not compiled automatically when the project SDK is set to JDK14+, one has to build with Maven first. I'm not sure if that's related to my specific IDE build configurations.
Let me know if it works as expected on your end. Thanks!

theigl · 2020-09-22T09:57:16Z

Made the change we discussed. I'm able to build with Maven and run individual tests in IntelliJ with JDK 11, 14 and 15. The only caveat is that jdk14 tests are not compiled automatically when the project SDK is set to JDK14+, one has to build with Maven first. I'm not sure if that's related to my specific IDE build configurations.

@FrauBoes: This works great! I had the same issue with the jdk14 tests but I don't think this is that much of a problem.

The only thing that bothers me now is that we use a different approach for jdk11. I think we should consolidate this and use a custom test-jdk11 folder with a jdk11ge profile instead of the current profile that excludes the test for older JDKs. WDYT?

Please also remove the println statements from the tests now that we are almost finished with the PR.

theigl · 2020-09-22T10:20:33Z

If you can, please import code format and import order from the eclipse folder using IntelliJ's "Eclipse Code Formatter" plugin and apply it to the Java files you touched.

FrauBoes · 2020-09-23T09:46:17Z

Agreed, I moved the jdk11 tests to the root and added a jdk11ge profile similar to our approach for jdk14. Debugging statements are removed and the imports are now in line with the rest of the project.

theigl · 2020-09-25T10:58:00Z

Agreed, I moved the jdk11 tests to the root and added a jdk11ge profile similar to our approach for jdk14. Debugging statements are removed and the imports are now in line with the rest of the project.

@FrauBoes: Thanks a lot! I think this PR turned out great! 👍

There is only one last thing: The copyright headers. Nathan and I don't feel comfortable with this. We would greatly prefer it if you add yourself or Oracle as @author instead.

FrauBoes · 2020-09-28T08:34:59Z

Thanks for reviewing the contribution, @theigl, it definitely made for a better result. With regards to the copyright header, let me look into this and get back to you.

theigl · 2020-11-23T15:05:05Z

@FrauBoes: Did you get any feedback on the copyright headers?

Kryo 5.0.0 has been released and is quite stable, so we could apply this PR now.

FrauBoes · 2020-11-23T15:55:25Z

@theigl We are still on it. I'll get back to you as soon as I know more.

FrauBoes · 2021-01-29T17:08:42Z

Hi @theigl, sorry for the delay. We’re still working through getting the approvals necessary to contribute. Would it be possible for you to modify the last sentence in the CONTRIBUTING.md from

“Whatever content You Contribute will be provided under the Project License(s).”

to something like the following, which we feel will make the connection clearer between the contribution, license, and copyright:

“Whatever content You Contribute will be under the copyright and license listed in LICENSE.md”

?

theigl · 2021-02-04T09:01:10Z

@FrauBoes: Thanks for the update!

I'll ask @NathanSweet and get back to you.

theigl · 2021-02-04T09:27:40Z

@FrauBoes: The suggested change would be OK for us.

FrauBoes · 2021-03-09T14:18:16Z

Hi @theigl, we got the green light and can move ahead with the contribution \o/. I've added @author tags to the relevant files and updated the CONTRIBUTING.md. Does the PR look good to you?

theigl · 2021-03-12T18:04:14Z

@FrauBoes: Thanks a lot for the updated PR!

I just released the current master as Kryo 5.0.4 and will review (and most likely merge) your PR in the next couple of days.

FrauBoes · 2021-03-15T10:10:04Z

@theigl That's great news. Very happy to see Kryo support records! Thanks again for your input and engagement on the PR, as said before it made for a better result. If anything else comes up along the way, let me know.

theigl · 2021-03-18T10:46:37Z

Merged! 🎉

I will create a release (or RC) with your changes in the next couple of days.

theigl · 2021-03-26T20:27:00Z

@FrauBoes: I realized today that the MethodHandles used for the RecordSerializer will most likely break our (already brittle) support for Android API <26.

I think we should switch to core reflection for the time being. Is this something you could help with? Sorry that I didn't think about this before merging your PR!

FrauBoes · 2021-03-29T09:42:13Z

@theigl I can help with that. I'll create a new PR in the next few days.

Julia Boes added 8 commits August 26, 2020 11:03

add test and serializer skeleton and update pom

370aad5

Merge branch 'master' into records

7dae878

add record serializer and test

57822d2

minimal commit to reproduce pom config

054adcf

fix test compilation

e46c198

fix compile and test run configuration

f167b17

add record serializer and test and update pom

59d7532

fix lower bound for compiler profile

f97e3b3

theigl mentioned this pull request Sep 4, 2020

Java 14 records : how to deal with them? #735

Closed

theigl requested changes Sep 4, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

src/com/esotericsoftware/kryo/Kryo.java Outdated Show resolved Hide resolved

src/com/esotericsoftware/kryo/serializers/RecordSerializer.java Outdated Show resolved Hide resolved

jerrinot mentioned this pull request Sep 7, 2020

Support records in SQL query engine hazelcast/hazelcast#17283

Open

Julia Boes added 4 commits September 7, 2020 15:16

adjust config and small clean ups

2a7c2bf

fix indentation

2d1497c

merge with upstream

48809c9

change to 2-arg constructor to work with JDK14

da5a8c2

ChrisHegarty reviewed Sep 7, 2020

View reviewed changes

Julia Boes added 5 commits September 9, 2020 14:39

Merge branch 'records' of github.com:oracle/kryo into records

b3ac7de

add test data for JDK14 and add ordering by name to serializer

7641fca

remove child pom config (again)

5ad3f11

remove .ser files and cleanup RecordSerializer

260ccca

fix typo and empty line

04dbbe8

move jdk14 tests and add with build-helper-maven-plugin

c7be874

FrauBoes commented Sep 18, 2020

View reviewed changes

consolidate jdk11 and jdk14 tests, cleanup imports and debugging output

9d7ff95

Julia Boes added 4 commits March 9, 2021 09:47

merge in master

34fbe7d

add author tags

e5e6422

update ser test/resources and serializer count

18df838

fix serializier count

de170a7

FrauBoes changed the title ~~Records~~ Add support for Records in JDK 14 Mar 9, 2021

theigl approved these changes Mar 12, 2021

View reviewed changes

theigl merged commit 15067d5 into EsotericSoftware:master Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Records in JDK 14 #766

Add support for Records in JDK 14 #766

FrauBoes commented Sep 3, 2020

theigl commented Sep 4, 2020

theigl commented Sep 4, 2020 •

edited

Loading

theigl commented Sep 4, 2020

ChrisHegarty commented Sep 4, 2020

FrauBoes commented Sep 4, 2020

theigl commented Sep 7, 2020

ChrisHegarty Sep 7, 2020

FrauBoes commented Sep 7, 2020

theigl commented Sep 8, 2020 •

edited

Loading

FrauBoes commented Sep 9, 2020

FrauBoes commented Sep 16, 2020

theigl commented Sep 16, 2020 •

edited

Loading

FrauBoes commented Sep 18, 2020

FrauBoes Sep 18, 2020

FrauBoes commented Sep 18, 2020 •

edited

Loading

theigl commented Sep 22, 2020

theigl commented Sep 22, 2020

FrauBoes commented Sep 23, 2020

theigl commented Sep 25, 2020

FrauBoes commented Sep 28, 2020

theigl commented Nov 23, 2020

FrauBoes commented Nov 23, 2020

FrauBoes commented Jan 29, 2021

theigl commented Feb 4, 2021

theigl commented Feb 4, 2021

FrauBoes commented Mar 9, 2021

theigl commented Mar 12, 2021

FrauBoes commented Mar 15, 2021

theigl commented Mar 18, 2021

theigl commented Mar 26, 2021

FrauBoes commented Mar 29, 2021

Add support for Records in JDK 14 #766

Add support for Records in JDK 14 #766

Conversation

FrauBoes commented Sep 3, 2020

theigl commented Sep 4, 2020

theigl commented Sep 4, 2020 • edited Loading

theigl commented Sep 4, 2020

ChrisHegarty commented Sep 4, 2020

FrauBoes commented Sep 4, 2020

theigl commented Sep 7, 2020

ChrisHegarty Sep 7, 2020

Choose a reason for hiding this comment

FrauBoes commented Sep 7, 2020

theigl commented Sep 8, 2020 • edited Loading

FrauBoes commented Sep 9, 2020

FrauBoes commented Sep 16, 2020

theigl commented Sep 16, 2020 • edited Loading

FrauBoes commented Sep 18, 2020

FrauBoes Sep 18, 2020

Choose a reason for hiding this comment

FrauBoes commented Sep 18, 2020 • edited Loading

theigl commented Sep 22, 2020

theigl commented Sep 22, 2020

FrauBoes commented Sep 23, 2020

theigl commented Sep 25, 2020

FrauBoes commented Sep 28, 2020

theigl commented Nov 23, 2020

FrauBoes commented Nov 23, 2020

FrauBoes commented Jan 29, 2021

theigl commented Feb 4, 2021

theigl commented Feb 4, 2021

FrauBoes commented Mar 9, 2021

theigl commented Mar 12, 2021

FrauBoes commented Mar 15, 2021

theigl commented Mar 18, 2021

theigl commented Mar 26, 2021

FrauBoes commented Mar 29, 2021

theigl commented Sep 4, 2020 •

edited

Loading

theigl commented Sep 8, 2020 •

edited

Loading

theigl commented Sep 16, 2020 •

edited

Loading

FrauBoes commented Sep 18, 2020 •

edited

Loading