-
Notifications
You must be signed in to change notification settings - Fork 343
feat(java): implement FinalFieldReplaceResolveSerializer for final fields with writeReplace/readResolve methods #2917
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
feat(java): implement FinalFieldReplaceResolveSerializer for final fields with writeReplace/readResolve methods #2917
Conversation
…elds with writeReplace/readResolve methods. This helps to avoid writing class names for final fields to the payload.
…ing descriptor context
… fields with writeReplace/readResolve methods
…t for final fields
|
@chaokunyang re-created my PR, addressed your comments in slack, please check |
java/fory-core/src/main/java/org/apache/fory/resolver/ClassResolver.java
Outdated
Show resolved
Hide resolved
java/fory-core/src/main/java/org/apache/fory/serializer/AbstractObjectSerializer.java
Outdated
Show resolved
Hide resolved
…dling in serializers. Add a cache for final fields.
java/fory-core/src/main/java/org/apache/fory/builder/BaseObjectCodecBuilder.java
Outdated
Show resolved
Hide resolved
java/fory-core/src/main/java/org/apache/fory/builder/BaseObjectCodecBuilder.java
Outdated
Show resolved
Hide resolved
| @Internal | ||
| @CodegenInvoke | ||
| @Override | ||
| public Serializer<?> getRawSerializerFinalField(Class<?> cls) { |
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 is only used in codegen, how baout using Invoke expression to create serializer directly?
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.
If so, most of changes in this file can be removed
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.
@chaokunyang which one is more preferable
// BaseObjectCodecBuilder
....
private static final TypeRef<?> FINAL_FIELD_SERIALIZER_TYPE =
TypeRef.of(FinalFieldReplaceResolveSerializer.class);
...
// in private Expression getOrCreateSerializer(Class<?> cls, boolean isField)
newSerializerExpr =
new Expression.NewInstance(FINAL_FIELD_SERIALIZER_TYPE, foryRef, fieldTypeExpr);or
// BaseObjectCodecBuilder
...
private static final TypeRef<?> FINAL_FIELD_SERIALIZER_TYPE =
TypeRef.of(FinalFieldReplaceResolveSerializer.class);
...
// in private Expression getOrCreateSerializer(Class<?> cls, boolean isField)
newSerializerExpr =
new Expression.StaticInvoke(
FinalFieldReplaceResolveSerializer.class,
"createSerializer",
FINAL_FIELD_SERIALIZER_TYPE,
foryRef,
fieldTypeExpr);
...
// FinalFieldReplaceResolveSerializer
...
@CodegenInvoke
public static FinalFieldReplaceResolveSerializer createSerializer(Fory fory, Class type) {
return new FinalFieldReplaceResolveSerializer(fory, 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.
I prefer first one
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.
done cf8b80d
| public class FinalFieldReplaceResolveSerializer extends ReplaceResolveSerializer { | ||
|
|
||
| public FinalFieldReplaceResolveSerializer(Fory fory, Class type) { | ||
| super(fory, type, 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.
super class will invoke :
public ReplaceResolveSerializer(Fory fory, Class type) {
super(fory, type);
refResolver = fory.getRefResolver();
classResolver = fory.getClassResolver();
// `setSerializer` before `newJDKMethodInfoCache` since it query classinfo from `classResolver`,
// which create serializer in turn.
// ReplaceResolveSerializer is used as data serializer for ImmutableList/Map,
// which serializer is already set.
classResolver.setSerializerIfAbsent(type, this);
classResolver.setSerializerIfAbsent(type, this); may set wrong serializer, could we add another protected contrcutore in parent class to pass setSerializer param and pass false 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.
|
Hi @mchernyakov , sorry for review late. I've reviewed this PR, most of parts are fine, I left some minor comments. We can merge it after those are resolved. And since it changes binary compatibility, we will include this PR in 0.14.0 instead 0.13.2 |
…tCodecBuilder.java
…ling in deserialization logic
…for non-final fields
|
|
||
| /** | ||
| * Serializer for class which 1) has jdk `writeReplace`/`readResolve` method defined, 2) is a final | ||
| * field of a class. //TODO do we ned to write the flag REPLACED_NEW_TYPE/REPLACED_SAME_TYPE even |
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.
waht does this TODO means?
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.
Could we also document that this serializer only works for consist mode, and add checks in FinalFieldReplaceResolveSerializer constructor.
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 in 446cc65 and removed the TODO
chaokunyang
left a comment
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.
LGTM
Why?
Based on the discussion #2786.
We can optimize the payload in cases where we have final fields in some Object.
re-created, closed PR #2904
What does this PR do?
Introduce
FinalFieldReplaceResolverwhich does not write the classname into the payload.Related issues
Does this PR introduce any user-facing change?
Benchmark