-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Support for lambda class serialization #3792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for contributing to GraalVM, this PR is tracked by GR-33824 |
declaringClass = Class.forName("java.lang.invoke.SerializedLambda"); | ||
// Checkstyle: resume | ||
} catch (ClassNotFoundException e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use VMError.shouldNotReachHere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as suggested.
Method serializeLambdaMethod = type.getJavaClass().getDeclaredMethod("writeReplace"); | ||
RuntimeReflection.register(serializeLambdaMethod); | ||
} catch (NoSuchMethodException e) { | ||
VMError.shouldNotReachHere("You have to register class from which you want lambdas to be serialized."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use throw VMError.shouldNotReachHere
in front so IDEs don't get confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as suggested.
try { | ||
RuntimeReflection.register(serializationTargetClass.getDeclaredMethod("$deserializeLambda$", SerializedLambda.class)); | ||
} catch (NoSuchMethodException e) { | ||
VMError.shouldNotReachHere("Method named $deserializeLambda$ must exist in capturing class (" + serializationTargetClass.getName() + ") of lambdas that have writeReplace method."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the capturing class ... the writeReplace method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as suggested.
There is a segfault in one of the benchmarks when running with the agent:
|
@@ -878,6 +878,76 @@ private static JNIObjectHandle shouldIncludeMethod(JNIEnvironment jni, JNIObject | |||
return result; | |||
} | |||
|
|||
private static boolean registerCapturingClass(JNIEnvironment jni, Breakpoint bp, InterceptedState state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard for me to follow this code. I think a concise comment about what happens here would be nice. Alternatively, split the method in several calls that have a descriptive name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added as suggested.
* recursively. Call ObjectStreamClass.getClassDataLayout0() can get all of them. | ||
*/ | ||
|
||
JNIMethodId getClassDataLayout0MId = agent.handles().getJavaIoObjectStreamClassGetClassDataLayout0(jni, bp.clazz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a good candidate as there is almost the same segment in: com.oracle.svm.agent.BreakpointInterceptor#objectStreamClassConstructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New method created and called in all the places needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has been removed in the last commit. When we get to SerializedLambda#readResolve method, method on ObjectStreamClass has already been called for proper capturing class. So, we do not need to search recursively again in that case and this method is sufficient now. We only need this check here, not in other places in code.
@@ -85,7 +86,8 @@ public void registerWithTargetConstructorClass(ConfigurationCondition condition, | |||
} | |||
|
|||
@Override | |||
public void registerWithTargetConstructorClass(ConfigurationCondition condition, String className, String customTargetConstructorClassName) { | |||
public void registerWithTargetConstructorClass(ConfigurationCondition condition, String rawClassName, String customTargetConstructorClassName) { | |||
String className = rawClassName.contains("$$Lambda$") ? rawClassName.split(LambdaUtils.SPLIT_BY_LAMBDA)[0] + "$$Lambda$" : rawClassName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$$Lambda$" is a good candidate for LambdaUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to LambdaUtils
as suggested.
@@ -55,7 +56,14 @@ public SerializationConfigurationType(ConfigurationCondition condition, String q | |||
public void printJson(JsonWriter writer) throws IOException { | |||
writer.append('{').indent().newline(); | |||
ConfigurationConditionPrintable.printConditionAttribute(condition, writer); | |||
writer.quote(SerializationConfigurationParser.NAME_KEY).append(':').quote(qualifiedJavaName); | |||
|
|||
if (qualifiedJavaName.contains("$$Lambda$") && !qualifiedJavaName.contains("/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think abusing qualifiedJavaName is not necessary here. It can also lead to duplicates during writing. We need to put the new value properly in the model and then change the equality function to compare that as well.
Also, we need assertions that both can't be populated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New attribute isLambdaCapturingClass
added to distinct lambda capturing classes from classes that should be registered for serialization.
Assertion added as suggested.
@@ -58,9 +59,10 @@ public void parseAndRegister(Reader reader) throws IOException { | |||
} | |||
|
|||
private void parseSerializationDescriptorObject(Map<String, Object> data) { | |||
checkAttributes(data, "serialization descriptor object", Collections.singleton(NAME_KEY), Arrays.asList(CUSTOM_TARGET_CONSTRUCTOR_CLASS_KEY, CONDITIONAL_KEY)); | |||
checkMultipleOptionsAttributes(data, "serialization descriptor object", Collections.unmodifiableCollection(Arrays.asList(NAME_KEY, LAMBDA_CAPTURING_CLASS_KEY)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple
is not accurate. They are exclusive options so maybe checkExclusiveOptionsAttributes
.
This is also not overly general as it supports only a single option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to Arrays.asList(CUSTOM_TARGET_CONSTRUCTOR_CLASS_KEY, CONDITIONAL_KEY)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple
is not accurate. They are exclusive options so maybecheckExclusiveOptionsAttributes
.This is also not overly general as it supports only a single option.
Changed as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to
Arrays.asList(CUSTOM_TARGET_CONSTRUCTOR_CLASS_KEY, CONDITIONAL_KEY)
?
It's been removed by mistake.
if (declaringClass.getName().contains("$$Lambda$")) { | ||
try { | ||
// Checkstyle: stop | ||
declaringClass = Class.forName("java.lang.invoke.SerializedLambda"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we fetch this once in a final field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added final field serializedLambdaClass
and used in proper places.
Hello filozof50, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address strahinja_stanojevic -(at)- matf -(dot)- bg -(dot)- ac -(dot)- rs. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
b307da0
to
c2ce091
Compare
It's been fixed now. |
@@ -51,6 +51,8 @@ | |||
public final class LambdaUtils { | |||
private static final Pattern LAMBDA_PATTERN = Pattern.compile("\\$\\$Lambda\\$\\d+[/\\.][^/]+;"); | |||
private static final char[] HEX = "0123456789abcdef".toCharArray(); | |||
public static final String SPLIT_BY_LAMBDA = "\\$\\$Lambda\\$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be expressed as Pattern.quote(LAMBDA_FUNCTIONS_NAME_PATTERN)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LAMBDA_SPLIT_PATTERN would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be expressed as
Pattern.quote(LAMBDA_FUNCTIONS_NAME_PATTERN)
.
It cannot. LambdaUtils class is needed in runtime, so this should stay String in order to finish build process successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LAMBDA_SPLIT_PATTERN would be better.
Changed as suggested.
@@ -51,6 +51,8 @@ | |||
public final class LambdaUtils { | |||
private static final Pattern LAMBDA_PATTERN = Pattern.compile("\\$\\$Lambda\\$\\d+[/\\.][^/]+;"); | |||
private static final char[] HEX = "0123456789abcdef".toCharArray(); | |||
public static final String SPLIT_BY_LAMBDA = "\\$\\$Lambda\\$"; | |||
public static final String LAMBDA_FUNCTIONS_NAME_PATTERN = "$$Lambda$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LAMBDA_CLASS_NAME_PATTERN is maybe more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use the LAMBDA_PATTERN
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LAMBDA_CLASS_NAME_PATTERN is maybe more accurate.
Changed as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use the
LAMBDA_PATTERN
above?
I expects ;
at the end (not optional) which is not case in cases which are important for this PR. So, we need 2 new constants in LambdaUtils class.
@@ -51,6 +51,8 @@ | |||
public final class LambdaUtils { | |||
private static final Pattern LAMBDA_PATTERN = Pattern.compile("\\$\\$Lambda\\$\\d+[/\\.][^/]+;"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use this one for all we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the explanation is in the comment above.
checkRemainingAttributes(map, type, requiredAttrs, optionalAttrs, unseenRequired); | ||
} | ||
|
||
protected void warnOrFail(String message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to move these functions, it will just mess up the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order of the functions is not changed now (comparing to master branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not true, you moved warnOrFail
and checkAttributes
before checkRemainingAttributes
. Please move it back.
@@ -1000,8 +1029,10 @@ private static boolean objectStreamClassConstructor(JNIEnvironment jni, Breakpoi | |||
* additional ObjectStreamClass instances (usually the super classes) are created | |||
* recursively. Call ObjectStreamClass.getClassDataLayout0() can get all of them. | |||
*/ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
public void registerWithTargetConstructorClass(ConfigurationCondition condition, String className, String customTargetConstructorClassName) { | ||
serializations.add(createConfigurationType(condition, className, customTargetConstructorClassName)); | ||
public void registerWithTargetConstructorClass(ConfigurationCondition condition, String rawClassName, String customTargetConstructorClassName) { | ||
String className = rawClassName.contains(LambdaUtils.LAMBDA_FUNCTIONS_NAME_PATTERN) ? rawClassName.split(LambdaUtils.SPLIT_BY_LAMBDA)[0] + LambdaUtils.LAMBDA_FUNCTIONS_NAME_PATTERN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a need for appending $$Lambda$
here as you already have the boolean. I would keep the class name as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, fixed as suggested.
return serializations.contains(createConfigurationType(condition, serializationTargetClass, customTargetConstructorClass)); | ||
public boolean contains(ConfigurationCondition condition, String rawSerializationTargetClass, String customTargetConstructorClass) { | ||
String serializationTargetClass = rawSerializationTargetClass.contains(LambdaUtils.LAMBDA_FUNCTIONS_NAME_PATTERN) | ||
? rawSerializationTargetClass.split(LambdaUtils.SPLIT_BY_LAMBDA)[0] + LambdaUtils.LAMBDA_FUNCTIONS_NAME_PATTERN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as suggested.
c2ce091
to
baf5656
Compare
if (types.equals("types")) { | ||
printSerializationTypes(writer); | ||
} else { | ||
printLambdaCapturingSerializationTypes(writer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cleaner to pass in a Consumer<JsonWriter>
to call, or since printSerializationTypes
and printLambdaCapturingSerializationTypes
is essentially the same, you could pass them as a JsonPrintable
array or collection.
But I think you should actually be able to use JsonPrinter.printCollection
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used JsonPrintable
list to make an abstraction here.
} | ||
|
||
@Override | ||
public int compareTo(SerializationConfigurationLambdaCapturingType other) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should instead use an actual Comparator class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used as suggested.
@@ -89,10 +90,13 @@ public int compareTo(SerializationConfigurationType other) { | |||
if (compareName != 0) { | |||
return compareName; | |||
} | |||
int compareCondition = condition.compareTo(other.condition); | |||
|
|||
int compareCondition = condition.compareTo(condition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like it...
} | ||
} | ||
|
||
private void parseSerializationDescriptorObject(Map<String, Object> data, boolean lambdaCapturingType) { | ||
checkAttributes(data, "serialization descriptor object", Collections.singleton(NAME_KEY), Arrays.asList(CUSTOM_TARGET_CONSTRUCTOR_CLASS_KEY, CONDITIONAL_KEY)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not accept a target constructor for lambda-capturing types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed as suggested.
private void parseSerializationTypes(List<Object> listOfSerializationTypes) { | ||
for (Object serializationType : listOfSerializationTypes) { | ||
parseSerializationDescriptorObject(asMap(serializationType, "third level of document must be serialization descriptor objects"), false); | ||
} | ||
} | ||
|
||
private void parseLambdaCapturingTypes(List<Object> listOfLambdaCapturingTypes) { | ||
for (Object lambdaCapturingType : listOfLambdaCapturingTypes) { | ||
parseSerializationDescriptorObject(asMap(lambdaCapturingType, "third level of document must be serialization descriptor objects"), true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply pass a boolean
instead of having two methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used an additional argument as suggested.
private static final ByteArrayOutputStream byteArrayOutputStream; | ||
|
||
static { | ||
byteArrayOutputStream = new ByteArrayOutputStream(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this is necessary and you should be able to create the stream in the test method and pass it to the serialize
and deserialize
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stream passed as an argument.
private static final ByteArrayOutputStream byteArrayOutputStream; | ||
|
||
static { | ||
byteArrayOutputStream = new ByteArrayOutputStream(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this is necessary and you should be able to create the stream in the test method and pass it to the respective methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stream passed as an argument.
Function<Integer, String> fn = (Function<Integer, String>) objectInputStream.readObject(); | ||
return fn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cast when you return Object
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cast removed as suggested.
|
||
@AutomaticFeature | ||
public class SerializationFeature implements Feature { | ||
static final HashSet<Class<?>> capturingClasses = new HashSet<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be static
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to be, because it is being referenced from inner class SerializationBuilder
.
} | ||
return true; | ||
} | ||
|
||
private static boolean isSerializable(JNIEnvironment env, JNIObjectHandle serializeTargetClass) { | ||
JNIObjectHandle javaIoSerializable = agent.handles().findClass(env, "java/io/Serializable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's still not a lazy lookup.
* which is of type {@code DirectHotSpotObjectConstantImpl}. That class is protected and it is | ||
* not visible in the {@link SerializationFeature}. The only way to access fields and methods of | ||
* this class is through the reflection. There are 2 different ways of getting lambda from the | ||
* {@code ConstantNode#value}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to go through SnippetReflectionProvider.asObject
. You get the provider via GraalAccess.getOriginalProviders
that you already call elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SnippetReflectionProvider.asObject
used as suggested.
* 1. For capturing classes from the JDK {@code ConstantNode#value} of type | ||
* {@code DirectHotSpotObjectConstantImpl} has {@code DirectHotSpotObjectConstantImpl#object} of | ||
* type {@code DirectMethodHandle$Constructor} on Java 17 and in previous versions. Lambda class | ||
* can be extracted from {@code DirectMethodHandle$Constructor#initMethod} using the | ||
* {@code MemberName#getDeclaringClass()} or from {@code DirectMethodHandle#member} using the | ||
* {@code MemberName#getDeclaringClass()}. Since all of those classes are protected and are not | ||
* in the same package as the {@link SerializationFeature}, the only way to get the lambda class | ||
* is using the reflection. | ||
* | ||
* 2. For capturing classes that are user-defined classes, {@code ConstantNode#value} of type | ||
* {@code DirectHotSpotObjectConstantImpl} has {@code DirectHotSpotObjectConstantImpl#object} of | ||
* type {@code DirectMethodHandle$StaticAccessor} type. In that case, lambda class is stored in | ||
* the {@code DirectMethodHandle$StaticAccessor#staticBase}. Since | ||
* {@code DirectMethodHandle$StaticAccessor} is also protected class and cannot be imported in | ||
* the {@link SerializationFeature}, it is not possible to get values from this field in any | ||
* ways other than the reflection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem plausible that this is the only way. Instead, it should be possible to get the declaring class of a constructor method handle by calling type().returnType()
on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type.returnType()
does not return what we expected it would. But I used some other reflection providers to get all the relevant information and to get lambdas from registered capturing classes.
…zof50/graal into lambda-serialization-branch
…zof50/graal into lambda-serialization-branch
|
||
public SerializationConfigurationLambdaCapturingType(ConfigurationCondition condition, String qualifiedJavaName) { | ||
assert qualifiedJavaName.indexOf('/') == -1 : "Requires qualified Java name, not internal representation"; | ||
assert !qualifiedJavaName.startsWith("[") : "Requires Java source array syntax, for example java.lang.String[]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text doesn't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can an array be a capturing type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it doesn't make sense. It's been removed.
private final String qualifiedJavaName; | ||
|
||
public SerializationConfigurationLambdaCapturingType(ConfigurationCondition condition, String qualifiedJavaName) { | ||
assert qualifiedJavaName.indexOf('/') == -1 : "Requires qualified Java name, not internal representation"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The articles are missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added as suggested.
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added as suggested.
-H:+AllowVMInspection \ | ||
--features=com.oracle.svm.test.AbstractServiceLoaderTest$TestFeature,com.oracle.svm.test.NoProviderConstructorServiceLoaderTest$TestFeature \ | ||
--add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.containers=ALL-UNNAMED | ||
Args=--initialize-at-run-time=com.oracle.svm.test --initialize-at-build-time=com.oracle.svm.test.AbstractClassSerializationTest,com.oracle.svm.test.SerializationRegistrationTest,com.oracle.svm.test.LambdaClassSerializationTest,com.oracle.svm.test.LambdaClassDeserializationTest --features=com.oracle.svm.test.SerializationRegistrationTestFeature -H:+AllowVMInspection --features=com.oracle.svm.test.AbstractServiceLoaderTest$TestFeature,com.oracle.svm.test.NoProviderConstructorServiceLoaderTest$TestFeature --features=com.oracle.svm.test.SerializationRegistrationTestFeature -H:+AllowVMInspection --features=com.oracle.svm.test.AbstractServiceLoaderTest$TestFeature,com.oracle.svm.test.NoProviderConstructorServiceLoaderTest$TestFeature --add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.containers=ALL-UNNAMED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an unrelated change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually added 2 classes for build time initialization.
private static StructuredGraph createMethodGraph(ResolvedJavaMethod method, DebugContext debug) { | ||
StructuredGraph graph = new StructuredGraph.Builder(debug.getOptions(), debug).method(method).build(); | ||
try (DebugContext.Scope ignored = debug.scope("ParsingToMaterializeLambdas")) { | ||
GraphBuilderPhase lambdaParserPhase = new GraphBuilderPhase(buildLambdaParserConfig()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also put the GraphBuilderPhase
in the abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved as suggested.
It looks like this PR has been integrated as b455f23. Could someone please verify and close the PR if that's true? |
In order to serialize and deserialize lambdas, methods
serializeLambda
(from lambda instance) anddeserializeLambda
(from class that captures lambda) needs to be registered for the runtime reflection. This is done inSerializationFeature
class.We have to make distinction between classes that needs to be registered for serialization and capturing classes that captures lambdas,
so in this PR new attribute for serialization-config.json file is intorduced as following:
The agent has been changed accordingly.
We also introduced new format for serialization configuration files in native image.
Example of the new format in serilaization configuration file can be found in the following file:
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/doc-files/SerializationConfigurationFilesHelp.txt