Skip to content

Commit

Permalink
fix(java): fix abstract collection elems same type serialization (#1641)
Browse files Browse the repository at this point in the history
## What does this PR do?
This PR fix inlineable expression codegen and reduce class load cost in
generated code.

With those changes, it fixed abstract collection elems same type
serialization in #1640

## Related issues
Closes #1640 


## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/incubator-fury/issues/new/choose)
describing the need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?


## Benchmark

<!--
When the PR has an impact on performance (if you don't know whether the
PR will have an impact on performance, you can submit the PR first, and
if it will have impact on performance, the code reviewer will explain
it), be sure to attach a benchmark data here.
-->
  • Loading branch information
chaokunyang authored May 20, 2024
1 parent 262c578 commit cf13c99
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@
import org.apache.fury.codegen.Expression.Literal;
import org.apache.fury.codegen.Expression.Reference;
import org.apache.fury.codegen.Expression.Return;
import org.apache.fury.codegen.Expression.StaticInvoke;
import org.apache.fury.codegen.ExpressionUtils;
import org.apache.fury.codegen.ExpressionVisitor.ExprHolder;
import org.apache.fury.collection.Tuple2;
Expand Down Expand Up @@ -521,12 +520,7 @@ protected Expression getClassExpr(Class<?> cls) {
if (sourcePublicAccessible(cls)) {
return Literal.ofClass(cls);
} else {
return new StaticInvoke(
ReflectionUtils.class,
"loadClass",
CLASS_TYPE,
beanClassExpr(),
Literal.ofString(cls.getName()));
return staticClassFieldExpr(cls, "__class__" + cls.getName().replace(".", "_"));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,16 +519,22 @@ protected Expression beanClassExpr() {
}

protected Expression staticBeanClassExpr() {
if (sourcePublicAccessible(beanClass)) {
return Literal.ofClass(beanClass);
}
return staticClassFieldExpr(beanClass, "__class__");
}

protected Expression staticClassFieldExpr(Class<?> cls, String fieldName) {
Preconditions.checkArgument(
!sourcePublicAccessible(cls), "Public class %s should use class literal instead", cls);
return getOrCreateField(
true,
Class.class,
"__class__",
fieldName,
() ->
new StaticInvoke(
ReflectionUtils.class,
"loadClass",
CLASS_TYPE,
Literal.ofString(beanClass.getName()))
ReflectionUtils.class, "loadClass", CLASS_TYPE, Literal.ofString(cls.getName()))
.inline());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,15 @@ default ExprCode genCode(CodegenContext ctx) {
} else {
ExprCode genCode = doGenCode(ctx);
ctx.exprState.put(this, new ExprState(new ExprCode(genCode.isNull(), genCode.value())));
if (this instanceof Inlineable) {
if (!((Inlineable) this).inlineCall) {
Preconditions.checkArgument(
StringUtils.isNotBlank(genCode.code()),
"Expression %s has empty code %s",
this,
genCode);
}
}
return genCode;
}
}
Expand Down Expand Up @@ -136,7 +145,7 @@ public Inlineable inline(boolean inlineCall) {
}

/** An expression that have a value as the result of the evaluation. */
abstract class ValueExpression implements Expression {
abstract class ValueExpression extends Inlineable {
// set to others to get a more context-dependent variable name.
public String valuePrefix = "value";

Expand Down Expand Up @@ -781,6 +790,16 @@ public ExprCode doGenCode(CodegenContext ctx) {
}
}

@Override
public Inlineable inline(boolean inlineCall) {
if (!inlineCall) {
if (targetObject instanceof Inlineable) {
((Inlineable) targetObject).inlineCall = false;
}
}
return super.inline(inlineCall);
}

@Override
public boolean nullable() {
return targetObject.nullable();
Expand Down Expand Up @@ -1805,7 +1824,6 @@ public String toString() {
}

class BinaryOperator extends ValueExpression {
private final boolean inline;
private final String operator;
private final TypeRef<?> type;
private Expression left;
Expand All @@ -1821,7 +1839,7 @@ public BinaryOperator(boolean inline, String operator, Expression left, Expressi

protected BinaryOperator(
boolean inline, String operator, Expression left, Expression right, TypeRef<?> t) {
this.inline = inline;
this.inlineCall = inline;
this.operator = operator;
this.left = left;
this.right = right;
Expand Down Expand Up @@ -1871,7 +1889,7 @@ public ExprCode doGenCode(CodegenContext ctx) {
}
}

if (inline) {
if (inlineCall) {
String value = String.format("(%s)", arith);
String code = StringUtils.isBlank(codeBuilder) ? null : codeBuilder.toString();
return new ExprCode(code, FalseLiteral, Code.variable(getRawType(type), value));
Expand Down Expand Up @@ -2025,7 +2043,7 @@ public While(BinaryOperator predicate, Expression action, Expression[] cutPoints
this.predicate = predicate;
this.action = action;
this.cutPoints = cutPoints;
Preconditions.checkArgument(predicate.inline, predicate);
Preconditions.checkArgument(predicate.inlineCall, predicate);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -617,4 +617,31 @@ public void testCollectionNullElements(boolean refTracking) {
Fury f = Fury.builder().withLanguage(Language.JAVA).withRefTracking(refTracking).build();
serDeCheck(f, data);
}

@Data
abstract static class Foo {
private int f1;
}

static class Foo1 extends Foo {}

@Data
static class CollectionAbstractTest {
private List<Foo> fooList;
}

@Test(dataProvider = "enableCodegen")
public void testAbstractCollectionElementsSerialization(boolean enableCodegen) {
Fury fury = Fury.builder().withCodegen(enableCodegen).requireClassRegistration(false).build();
{
CollectionAbstractTest test = new CollectionAbstractTest();
test.fooList = new ArrayList<>(ImmutableList.of(new Foo1(), new Foo1()));
serDeCheck(fury, test);
}
{
CollectionAbstractTest test = new CollectionAbstractTest();
test.fooList = new ArrayList<>(ofArrayList(new Foo1(), new Foo1()));
serDeCheck(fury, test);
}
}
}

0 comments on commit cf13c99

Please sign in to comment.