Skip to content
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.

Sign up for GitHub

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

[Java] fix package access level class accessor jit #1210

Merged
merged 2 commits into from
Dec 5, 2023
Merged

[Java] fix package access level class accessor jit #1210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e616c53
fix package access level class accessor jit
chaokunyang Dec 5, 2023
35df38f
lint code
chaokunyang Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 49 additions & 31 deletions java/fury-core/src/main/java/io/fury/builder/AccessorHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package io.fury.builder;

import static io.fury.codegen.CodeGenerator.sourcePkgLevelAccessible;

import io.fury.codegen.CodeGenerator;
import io.fury.codegen.CodegenContext;
import io.fury.codegen.CompileUnit;
Expand Down Expand Up @@ -78,50 +80,53 @@ public static String genCode(Class<?> beanClass) {
// filter out super classes
Collection<Descriptor> descriptors = Descriptor.getAllDescriptorsMap(beanClass, false).values();
for (Descriptor descriptor : descriptors) {
if (!Modifier.isPrivate(descriptor.getModifiers())) {
{
String methodName = descriptor.getName();
String codeBody =
if (Modifier.isPrivate(descriptor.getModifiers())) {
continue;
}
boolean accessible = sourcePkgLevelAccessible(descriptor.getRawType());
{
// getter
String methodName = descriptor.getName();
String codeBody;
Class<?> returnType = accessible ? descriptor.getRawType() : Object.class;
if (isRecord) {
codeBody =
StringUtils.format(
"return ${obj}.${fieldName};",
"return ${obj}.${fieldName}();",
"obj",
OBJ_NAME,
"fieldName",
descriptor.getName());
Class<?> returnType = descriptor.getRawType();
ctx.addStaticMethod(methodName, codeBody, returnType, beanClass, OBJ_NAME);
}
{
String methodName = descriptor.getName();
String codeBody =
} else {
codeBody =
StringUtils.format(
"${obj}.${fieldName} = ${fieldValue};",
"return ${obj}.${fieldName};",
"obj",
OBJ_NAME,
"fieldName",
descriptor.getName(),
"fieldValue",
FIELD_VALUE);
ctx.addStaticMethod(
methodName,
codeBody,
void.class,
beanClass,
OBJ_NAME,
descriptor.getRawType(),
FIELD_VALUE);
descriptor.getName());
}
} else if (isRecord) {
ctx.addStaticMethod(methodName, codeBody, returnType, beanClass, OBJ_NAME);
}
if (accessible) {
String methodName = descriptor.getName();
String codeBody =
StringUtils.format(
"return ${obj}.${fieldName}();",
"${obj}.${fieldName} = ${fieldValue};",
"obj",
OBJ_NAME,
"fieldName",
descriptor.getName());
Class<?> returnType = descriptor.getRawType();
ctx.addStaticMethod(methodName, codeBody, returnType, beanClass, OBJ_NAME);
descriptor.getName(),
"fieldValue",
FIELD_VALUE);
ctx.addStaticMethod(
methodName,
codeBody,
void.class,
beanClass,
OBJ_NAME,
descriptor.getRawType(),
FIELD_VALUE);
}
// getter/setter may lose some inner state of an object, so we set them to null to avoid
// creating getter/setter accessor.
Expand Down Expand Up @@ -194,26 +199,39 @@ public static Class<?> getAccessorClass(Class<?> beanClass) {
}
}

/** Should be invoked only when {@link #defineAccessor} returns true. */
public static Class<?> getAccessorClass(Field field) {
Class<?> beanClass = field.getDeclaringClass();
return getAccessorClass(beanClass);
}

/** Should be invoked only when {@link #defineAccessor} returns true. */
public static Class<?> getAccessorClass(Method method) {
Class<?> beanClass = method.getDeclaringClass();
return getAccessorClass(beanClass);
}

public static boolean defineAccessor(Field field) {
if (ReflectionUtils.isPrivate(field.getType())) {
Class<?> beanClass = field.getDeclaringClass();
return defineAccessorClass(beanClass);
}

public static boolean defineAccessor(Method method) {
Class<?> beanClass = method.getDeclaringClass();
return defineAccessorClass(beanClass);
}

public static boolean defineSetter(Field field) {
if (ReflectionUtils.isPrivate(field.getType()) || !sourcePkgLevelAccessible(field.getType())) {
return false;
}
Class<?> beanClass = field.getDeclaringClass();
return defineAccessorClass(beanClass);
}

public static boolean defineAccessor(Method method) {
if (ReflectionUtils.isPrivate(method.getReturnType())) {
public static boolean defineSetter(Method method) {
if (ReflectionUtils.isPrivate(method.getReturnType())
|| !sourcePkgLevelAccessible(method.getReturnType())) {
return false;
}
Class<?> beanClass = method.getDeclaringClass();
Expand Down
3 changes: 1 addition & 2 deletions java/fury-core/src/main/java/io/fury/builder/BaseObjectCodecBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package io.fury.builder;

import static io.fury.codegen.CodeGenerator.getPackage;
import static io.fury.codegen.CodeGenerator.sourceAccessible;
import static io.fury.codegen.Expression.Invoke.inlineInvoke;
import static io.fury.codegen.Expression.Reference.fieldRef;
import static io.fury.codegen.ExpressionOptimizer.invokeGenerated;
Expand Down Expand Up @@ -530,7 +529,7 @@ protected Expression getOrCreateSerializer(Class<?> cls) {
}

protected Expression getClassExpr(Class<?> cls) {
if (sourceAccessible(cls)) {
if (sourcePublicAccessible(cls)) {
return Literal.ofClass(cls);
} else {
return new StaticInvoke(
Expand Down
34 changes: 19 additions & 15 deletions java/fury-core/src/main/java/io/fury/builder/CodecBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package io.fury.builder;

import static io.fury.codegen.CodeGenerator.sourceAccessible;
import static io.fury.codegen.Expression.Invoke.inlineInvoke;
import static io.fury.type.TypeUtils.CLASS_TYPE;
import static io.fury.type.TypeUtils.OBJECT_ARRAY_TYPE;
Expand Down Expand Up @@ -122,6 +121,10 @@ public CodecBuilder(CodegenContext ctx, TypeToken<?> beanType) {
/** Returns an expression that serialize java bean of type {@link CodecBuilder#beanClass}. */
public abstract Expression buildEncodeExpression();

protected boolean sourcePublicAccessible(Class<?> cls) {
return ctx.sourcePublicAccessible(cls);
}

protected Expression tryInlineCast(Expression expression, TypeToken<?> targetType) {
return tryCastIfPublic(expression, targetType, true);
}
Expand All @@ -138,7 +141,7 @@ protected Expression tryCastIfPublic(
return expression;
}
if (inline) {
if (sourceAccessible(rawType)) {
if (sourcePublicAccessible(rawType)) {
return new Cast(expression, targetType);
} else {
return new Cast(expression, ReflectionUtils.getPublicSuperType(TypeToken.of(rawType)));
Expand All @@ -150,7 +153,8 @@ protected Expression tryCastIfPublic(
protected Expression tryCastIfPublic(
Expression expression, TypeToken<?> targetType, String valuePrefix) {
Class<?> rawType = getRawType(targetType);
if (sourceAccessible(rawType) && !expression.type().wrap().isSubtypeOf(targetType.wrap())) {
if (sourcePublicAccessible(rawType)
&& !expression.type().wrap().isSubtypeOf(targetType.wrap())) {
return new Cast(expression, targetType, valuePrefix);
}
return expression;
Expand Down Expand Up @@ -185,16 +189,16 @@ protected Expression buildDefaultComponentsArray() {
protected Expression getFieldValue(Expression inputBeanExpr, Descriptor descriptor) {
TypeToken<?> fieldType = descriptor.getTypeToken();
Class<?> rawType = descriptor.getRawType();
if (!sourceAccessible(rawType)) {
fieldType = OBJECT_TYPE;
}
String fieldName = descriptor.getName();
if (isRecord) {
return getRecordFieldValue(inputBeanExpr, descriptor);
}
if (duplicatedFields.contains(fieldName) || !Modifier.isPublic(beanClass.getModifiers())) {
return unsafeAccessField(inputBeanExpr, beanClass, descriptor);
}
if (!sourcePublicAccessible(rawType)) {
fieldType = OBJECT_TYPE;
}
// public field or non-private non-java field access field directly.
if (Modifier.isPublic(descriptor.getModifiers())) {
return new Expression.FieldValue(inputBeanExpr, fieldName, fieldType, fieldNullable, false);
Expand Down Expand Up @@ -230,6 +234,9 @@ protected Expression getFieldValue(Expression inputBeanExpr, Descriptor descript

private Expression getRecordFieldValue(Expression inputBeanExpr, Descriptor descriptor) {
TypeToken<?> fieldType = descriptor.getTypeToken();
if (!sourcePublicAccessible(descriptor.getRawType())) {
fieldType = OBJECT_TYPE;
}
String fieldName = descriptor.getName();
if (Modifier.isPublic(beanClass.getModifiers())) {
Preconditions.checkNotNull(descriptor.getReadMethod());
Expand All @@ -256,9 +263,7 @@ private Expression getRecordFieldValue(Expression inputBeanExpr, Descriptor desc
}
if (!fieldType.isPrimitive()) {
Expression v = inlineInvoke(ref, methodInfo.f1, OBJECT_TYPE, fieldNullable, inputBeanExpr);
TypeToken<?> publicSuperType =
ReflectionUtils.getPublicSuperType(descriptor.getTypeToken());
return new Cast(v, publicSuperType, fieldName);
return tryCastIfPublic(v, descriptor.getTypeToken(), fieldName);
} else {
return new Invoke(ref, methodInfo.f1, fieldType, fieldNullable, inputBeanExpr);
}
Expand Down Expand Up @@ -296,8 +301,7 @@ private Expression unsafeAccessField(
fieldNullable,
inputObject,
fieldOffsetExpr);
TypeToken<?> publicSuperType = ReflectionUtils.getPublicSuperType(descriptor.getTypeToken());
return tryCastIfPublic(getObj, publicSuperType, fieldName);
return tryCastIfPublic(getObj, descriptor.getTypeToken(), fieldName);
}
}

Expand Down Expand Up @@ -337,7 +341,7 @@ protected Expression setFieldValue(Expression bean, Descriptor d, Expression val
if (value instanceof Inlineable) {
((Inlineable) value).inline();
}
if (duplicatedFields.contains(fieldName) || !sourceAccessible(beanClass)) {
if (duplicatedFields.contains(fieldName) || !sourcePublicAccessible(beanClass)) {
return unsafeSetField(bean, d, value);
}
if (!Modifier.isFinal(d.getModifiers()) && Modifier.isPublic(d.getModifiers())) {
Expand All @@ -346,7 +350,7 @@ protected Expression setFieldValue(Expression bean, Descriptor d, Expression val
return new Invoke(bean, d.getWriteMethod().getName(), value);
} else {
if (!Modifier.isFinal(d.getModifiers()) && !Modifier.isPrivate(d.getModifiers())) {
if (AccessorHelper.defineAccessor(d.getField())) {
if (AccessorHelper.defineSetter(d.getField())) {
Class<?> accessorClass = AccessorHelper.getAccessorClass(d.getField());
if (!value.type().equals(d.getTypeToken())) {
value = new Cast(value, d.getTypeToken());
Expand All @@ -356,7 +360,7 @@ protected Expression setFieldValue(Expression bean, Descriptor d, Expression val
}
}
if (d.getWriteMethod() != null && !Modifier.isPrivate(d.getWriteMethod().getModifiers())) {
if (AccessorHelper.defineAccessor(d.getWriteMethod())) {
if (AccessorHelper.defineSetter(d.getWriteMethod())) {
Class<?> accessorClass = AccessorHelper.getAccessorClass(d.getWriteMethod());
if (!value.type().equals(d.getTypeToken())) {
value = new Cast(value, d.getTypeToken());
Expand Down Expand Up @@ -452,7 +456,7 @@ private Reference getOrCreateField(
/** Returns an Expression that create a new java object of type {@link CodecBuilder#beanClass}. */
protected Expression newBean() {
// TODO allow default access-level class.
if (sourceAccessible(beanClass)) {
if (sourcePublicAccessible(beanClass)) {
return new Expression.NewInstance(beanType);
} else {
return new StaticInvoke(Platform.class, "newInstance", OBJECT_TYPE, beanClassExpr());
Expand Down
17 changes: 14 additions & 3 deletions java/fury-core/src/main/java/io/fury/codegen/CodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,12 +479,23 @@ static StringBuilder stripIfHasLastNewline(StringBuilder sb) {
return sb;
}

/** Returns true if class is accessible from source. */
public static boolean sourceAccessible(Class<?> clz) {
/** Returns true if class is public accessible from source. */
public static boolean sourcePublicAccessible(Class<?> clz) {
if (clz.isPrimitive()) {
return true;
}
if (!ReflectionUtils.isPublic(clz) || clz.getCanonicalName() == null) {
if (!ReflectionUtils.isPublic(clz)) {
return false;
}
return sourcePkgLevelAccessible(clz);
}

/** Returns true if class is package level accessible from source. */
public static boolean sourcePkgLevelAccessible(Class<?> clz) {
if (clz.isPrimitive()) {
return true;
}
if (clz.getCanonicalName() == null) {
return false;
}
// Scala may produce class name like: xxx.SomePackageObject.package$SomeClass
Expand Down
16 changes: 15 additions & 1 deletion java/fury-core/src/main/java/io/fury/codegen/CodegenContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ public String namePrefix(Class<?> clz) {
* return canonical name otherwise.
*/
public String type(Class<?> clz) {
if (!CodeGenerator.sourceAccessible(clz)) {
if (!sourcePkgLevelAccessible(clz)) {
return "Object";
}
if (clz.isArray()) {
Expand Down Expand Up @@ -675,4 +675,18 @@ public String optimizeMethodCode(String code) {
return code;
}
}

private final Map<Class<?>, Boolean> sourcePublicAccessibleCache = new HashMap<>();
private final Map<Class<?>, Boolean> sourcePkgLevelAccessibleCache = new HashMap<>();

/** Returns true if class is public accessible from source. */
public boolean sourcePublicAccessible(Class<?> clz) {
return sourcePublicAccessibleCache.computeIfAbsent(clz, CodeGenerator::sourcePublicAccessible);
}

/** Returns true if class is package level accessible from source. */
public boolean sourcePkgLevelAccessible(Class<?> clz) {
return sourcePkgLevelAccessibleCache.computeIfAbsent(
clz, CodeGenerator::sourcePkgLevelAccessible);
}
}
9 changes: 9 additions & 0 deletions java/fury-core/src/test/java/io/fury/FuryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.testng.Assert.assertThrows;
import static org.testng.Assert.assertTrue;

import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import io.fury.annotation.Ignore;
Expand Down Expand Up @@ -610,4 +611,12 @@ class A {
Assert.assertTrue(e.getMessage().contains("reference"));
}
}

@Test
public void testPkgAccessLevelParentClass() {
Fury fury = Fury.builder().withRefTracking(true).requireClassRegistration(false).build();
HashBasedTable<Object, Object, Object> table = HashBasedTable.create(2, 4);
table.put("r", "c", 100);
serDeCheckSerializer(fury, table, "Codec");
}
}
18 changes: 17 additions & 1 deletion java/fury-core/src/test/java/io/fury/builder/AccessorHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.testng.Assert.assertTrue;

import io.fury.TestUtils;
import io.fury.builder.pkg.AccessLevelClass;
import io.fury.test.bean.Foo;
import io.fury.test.bean.Struct;
import io.fury.type.Descriptor;
Expand All @@ -45,7 +46,8 @@ public static class A {

@Test
public void genCode() {
AccessorHelper.genCode(A.class);
System.out.println(AccessorHelper.genCode(A.class));
;
}

@Test
Expand All @@ -65,6 +67,20 @@ public void defineAccessorClass() throws Exception {
assertSame(AccessorHelper.getAccessorClass(A.class), accessorClass);
}

@Data
static class PkgAccessAccessorClass {
protected String f1;
String f2;
}

@Test
public void definePkgAccessAccessorClass() {
assertTrue(AccessorHelper.defineAccessorClass(PkgAccessAccessorClass.class));
assertTrue(AccessorHelper.defineAccessorClass(AccessLevelClass.class));
Method[] methods = AccessorHelper.getAccessorClass(AccessLevelClass.class).getDeclaredMethods();
assertEquals(methods.length, 4);
}

@Test
public void defineAccessorClassInDefaultPackage() {
Class<?> testAccessorClass = Struct.createStructClass("TestAccessorDefaultPackage", 1);
Expand Down
Loading