Serialization feature support #15380
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
in case, that |
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
791048e
to
baebcd8
Compare
|
Both problems (with From my POV it has to be decided whether we would like to use this path (which contains duplication of code from GraalVM - but in fact it is only one simple if...) |
|
@gsmet, @stuartwdouglas I'm not sure whether this is desired solution. It works, There are be some things to improve, more tests to add., ... |
stuartwdouglas
requested changes
Mar 16, 2021
core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAutoFeatureStep.java
Outdated
Show resolved
Hide resolved
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
baebcd8
to
79133cf
Compare
|
@stuartwdouglas I moved almost all code, which was generated for serialization, into a new method and method is called from class registrations (if is required). I also added generated code for |
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
79133cf
to
b3192c5
Compare
stuartwdouglas
approved these changes
Mar 21, 2021
stuartwdouglas
requested changes
Mar 21, 2021
...ment/src/main/java/io/quarkus/deployment/builditem/nativeimage/ReflectiveClassBuildItem.java
Outdated
Show resolved
Hide resolved
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
b3192c5
to
2a05cbe
Compare
|
Error in the build does not seem to be linked to the current changes.
|
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
2a05cbe
to
20f6056
Compare
stuartwdouglas
approved these changes
Mar 22, 2021
JiriOndrusek
force-pushed
the
serialization-POC
branch
2 times, most recently
from
dd439a1
to
bc3be0a
Compare
|
PR is ready to be merged. |
@JiriOndrusek thanks for this. Just one question, is it safe to assume that if serialization is not detected this PR doesn't break compatibility with GraalVM 20.3? |
|
@zakkak yes. Only if a reflective class with flag
|
machi1990
reviewed
Mar 31, 2021
machi1990
reviewed
Mar 31, 2021
core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageAutoFeatureStep.java
Outdated
Show resolved
Hide resolved
machi1990
reviewed
Mar 31, 2021
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
bc3be0a
to
2f412f1
Compare
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
2f412f1
to
493e338
Compare
Test Failures⚙️ jvm-linux-jdk11📦 integration-tests/kafka# Tests: 10
+ Success: 8
- Failures: 0
- Errors: 2
! Skipped: 0❌
|
|
Test failures |
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
7cc7a08
to
be8d88d
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs
Test Failures⚙️ JVM Tests - JDK 11 #📦 extensions/smallrye-reactive-messaging/deployment# Tests: 49
+ Success: 48
- Failures: 0
- Errors: 1
! Skipped: 0❌
|
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
be8d88d
to
a12b56c
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building a12b56c
|
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
a12b56c
to
2f77711
Compare
|
PR is ready to be merged. |
|
Is this good to merge, @gsmet? This blocks a couple of issues in Camel Quarkus. |
Closed
|
Is there any hope to get this in Quarkus 2.0.0? |
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
2f77711
to
af62061
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building af62061
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/grpc-streaming✖ |
|
Failed test |
JiriOndrusek
force-pushed
the
serialization-POC
branch
from
af62061
to
c82f416
Compare
|
@gsmet did you have any more concerns about this one? |
|
Giving up hoping to get this in 2.0.0, now hoping for 2.1.0 ;) |
|
Are we good to go with this one? It looks like it... |
|
If there are any issues we can fix them later, nobody seems to have any more concerns so I don't see any reason to hold it. |
|
There is a small issue/missing feature which would make the use of serialization easier. Currently all classes used by serialization has to be registered, even the primitive ones. It would be nice to make registration for serialization of some types like String, Boolean, ..... simpler. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #14530
GraalVM contains support for serialization oracle/graal#2730
This PR tries to add programmatical approach for extensions. Main idea is to provide generated registration of serialization classes using existing BuildItems.
Description
serializationfield is added intoReflectiveClassBuildItem,ReflectiveHierarchyBuildItemandRegisterForReflectionannotation.com.oracle.svm.reflect.serialize.SerializationSupport.addConstructorAccessorand reflection is registered viacom.oracle.svm.reflect.serialize.SerializationSupport.addReflectionsPossible optimizations and features for better experience
com.oracle.svm.reflect.serialize.SerializationSupport.addReflections. This could be avoided.Externalizableinterface. Similar classes could be skipped for serialization registration.ReferenceSerializationBuiltItem, which register allserializablereferences of the target class. It would make registration easier, there will be no requirement to register for examplejava.lang.String, which is typically part of the serialized classes.Problems and TODOsI hit a problem with several types during its serialization. I'm not sure what is happening. But it has to be investigated and fixed.Ifjava.util.concurrent.atomic.AtomicBooleanis registered with this feature, I see following error (registration via config works):Ifjava.lang.Stringis registered with this feature, I see following error (registration via config works):Alternative approaches
It could be feasible to gather all classes registered for serialization, generate serialization-config.xml and attach it to build args. This approach won't duplicate any code from GraalVM.
JUnit test is present.
Edited - some problems were solved. See comments.