You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fory.apache.org by "chaokunyang (via GitHub)" <gi...@apache.org> on 2026/08/01 18:03:01 UTC
[PR] feat(json): add GraalVM codegen support [fory]
chaokunyang opened a new pull request, #3907:
URL: https://github.com/apache/fory/pull/3907
<!--
**Thanks for contributing to Apache Fory™.**
**If this is your first time opening a PR on fory, you can refer to [CONTRIBUTING.md](https://github.com/apache/fory/blob/main/CONTRIBUTING.md).**
Contribution Checklist
- The **Apache Fory™** community has requirements on the naming of pr titles. You can also find instructions in [CONTRIBUTING.md](https://github.com/apache/fory/blob/main/CONTRIBUTING.md).
- Apache Fory™ has a strong focus on performance. If the PR you submit will have an impact on performance, please benchmark it first and provide the benchmark result here.
-->
## Why?
<!-- Describe the purpose of this PR. -->
## What does this PR do?
<!-- Describe the details of this PR. -->
## Related issues
<!--
Is there any related issue? If this PR closes them you say say fix/closes:
- #xxxx0
- #xxxx1
- Fixes #xxxx2
-->
## AI Contribution Checklist
<!-- Full requirements and disclosure template:
https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs -->
- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs) in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review` summary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described in `AI_POLICY.md`, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes.
<!-- If substantial AI assistance = `yes`, paste the completed checklist and disclosure block here, including the final ai_review summary and screenshot evidence or equivalent persisted links from both fresh reviewers described in AI_POLICY.md, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes. -->
## Does this PR introduce any user-facing change?
<!--
If any user-facing interface changes, please [open an issue](https://github.com/apache/fory/issues/new/choose) describing the need to do so and update the document if necessary.
Delete section if not applicable.
-->
- [ ] 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.
Delete section if not applicable.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(json): add GraalVM codegen support for json [fory]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang merged PR #3907:
URL: https://github.com/apache/fory/pull/3907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org
Re: [PR] feat(json): add GraalVM codegen support [fory]
Posted by "wenshao (via GitHub)" <gi...@apache.org>.
wenshao commented on code in PR #3907:
URL: https://github.com/apache/fory/pull/3907#discussion_r3696546519
##########
java/fory-json/src/main/java/org/apache/fory/json/codec/SqlJsonCodecs.java:
##########
@@ -110,12 +122,16 @@ public T readUtf8(Utf8JsonReader reader) {
return reader.tryReadNullToken() ? null : newSqlValue(reader.readLong());
}
+ @SuppressWarnings("unchecked")
private T newSqlValue(long millis) {
try {
- return constructor.newInstance(millis);
+ return constructorHandle == null
+ ? constructor.newInstance(millis)
+ : (T) constructorHandle.invoke(millis);
} catch (ReflectiveOperationException e) {
- throw new ForyJsonException(
- "Cannot create SQL JSON type " + constructor.getDeclaringClass(), e);
+ throw new ForyJsonException("Cannot create SQL JSON type " + type, e);
+ } catch (Throwable e) {
+ throw new ForyJsonException("Cannot create SQL JSON type " + type, e);
}
Review Comment:
**[Suggestion]** The `catch (ReflectiveOperationException e)` block is redundant — it is entirely subsumed by the subsequent `catch (Throwable e)` block, and both perform the identical wrapping. — Concrete cost: any future change to error handling (e.g., adding context, unwrapping `InvocationTargetException`) must be applied in two places or the blocks silently diverge.
```suggestion
} catch (Throwable e) {
throw new ForyJsonException("Cannot create SQL JSON type " + type, e);
}
```
_— qwen3-coder-plus via Qwen Code /review_
##########
java/fory-json/src/main/java/org/apache/fory/json/JsonGeneratedClassRegistry.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fory.json;
+
+import java.lang.reflect.Type;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
+import org.apache.fory.annotation.Internal;
+import org.apache.fory.json.resolver.JsonSharedRegistry.GeneratedClasses;
+import org.apache.fory.reflect.TypeRef;
+
+/** Frozen Native Image mapping from JSON configuration semantics to generated classes. */
+@Internal
+public final class JsonGeneratedClassRegistry {
+ private static Map<JsonCodegenKey, MutableConfiguration> pending = new HashMap<>();
+ private static Map<JsonCodegenKey, Configuration> configurations = Collections.emptyMap();
Review Comment:
**[Suggestion]** `configurations` is written under `synchronized` in `register()` but read without synchronization or `volatile` in the public `configuration()` method — a JMM data race on the reference. In the current GraalVM-only lifecycle the native-image heap-snapshot provides a happens-before edge, so the race does not manifest. However, `configuration()` is `@Internal public` and called from 12 unsynchronized sites in `JsonSharedRegistry`. If any future code path calls `configuration()` concurrently with `register()` without an external synchronization barrier, the reading thread can see the stale `Collections.emptyMap()` initial value. — Concrete cost: every `native*Class()` lookup returns `null`, silently falling back to interpreted codecs.
```suggestion
private static volatile Map<JsonCodegenKey, Configuration> configurations = Collections.emptyMap();
```
_— qwen3-coder-plus via Qwen Code /review_
##########
java/fory-json/src/main/java17/org/apache/fory/json/ForyJsonGraalVMFeature.java:
##########
@@ -114,17 +147,218 @@ public void duringAnalysis(DuringAnalysisAccess access) {
if (mixin != null) {
changed |= registerMixin(access, type, mixin.target());
}
+ if (type.getDeclaredAnnotation(ForyJsonProvider.class) != null) {
+ changed |= registerProvider(type);
+ }
if (type == ForyJson.class) {
registerBuiltInTypes(access);
changed = true;
}
}
}
+ changed |= generateConfigurations(access);
if (changed) {
access.requireAnalysisIteration();
}
}
+ private boolean registerProvider(Class<?> providerClass) {
+ if (!processedProviders.add(providerClass)) {
+ return false;
+ }
+ int modifiers = providerClass.getModifiers();
+ if (!Modifier.isPublic(modifiers)
+ || Modifier.isAbstract(modifiers)
+ || providerClass.isInterface()
+ || providerClass.isEnum()) {
+ throw providerFailure(
+ providerClass, "must be a public concrete class", null);
+ }
+ Constructor<?> constructor;
+ try {
+ constructor = providerClass.getConstructor();
+ } catch (NoSuchMethodException e) {
+ throw providerFailure(providerClass, "must have a public no-argument constructor", e);
+ }
+ Object provider;
+ try {
+ provider = constructor.newInstance();
+ } catch (ReflectiveOperationException | RuntimeException e) {
+ throw providerFailure(providerClass, "cannot be constructed", unwrap(e));
+ }
+ List<Method> methods = providerMethods(providerClass);
+ if (methods.isEmpty()) {
+ throw providerFailure(providerClass, "does not declare an effective ForyJson method", null);
+ }
+ boolean changed = false;
+ for (Method method : methods) {
+ if (Modifier.isStatic(method.getModifiers()) || method.getParameterCount() != 0) {
+ throw providerFailure(
+ providerClass,
+ "method must be a non-static zero-argument instance method: " + method,
+ null);
+ }
+ ForyJson json;
+ try {
+ json = (ForyJson) method.invoke(provider);
+ } catch (ReflectiveOperationException | RuntimeException e) {
+ throw providerFailure(
+ providerClass, "cannot invoke provider method " + method, unwrap(e));
+ }
+ if (json == null) {
+ throw providerFailure(providerClass, "provider method returned null: " + method, null);
+ }
+ JsonConfig config = json.config();
+ if (!config.codegenEnabled()) {
+ throw providerFailure(
+ providerClass, "provider method returned a codegen-disabled ForyJson: " + method, null);
+ }
+ JsonCodegenKey key = config.codegenKey();
+ if (!hostedConfigurations.containsKey(key)) {
+ hostedConfigurations.put(key, new HostedConfiguration(config));
+ changed = true;
+ }
+ }
+ return changed;
+ }
+
+ private static List<Method> providerMethods(Class<?> providerClass) {
+ Map<MethodSignature, Method> effective = new HashMap<>();
+ for (Method method : providerClass.getMethods()) {
+ if (method.isBridge()
+ || method.isSynthetic()
+ || method.getReturnType() != ForyJson.class) {
+ continue;
+ }
+ MethodSignature signature = new MethodSignature(method);
+ Method previous = effective.get(signature);
+ if (previous == null
+ || previous.getDeclaringClass().isAssignableFrom(method.getDeclaringClass())) {
+ effective.put(signature, method);
+ } else if (!method.getDeclaringClass().isAssignableFrom(previous.getDeclaringClass())
+ && method.getDeclaringClass().getName().compareTo(previous.getDeclaringClass().getName())
+ < 0) {
+ effective.put(signature, method);
+ }
+ }
+ ArrayList<Method> methods = new ArrayList<>(effective.values());
+ methods.sort(
+ Comparator.comparing(Method::getName)
+ .thenComparing(method -> method.getDeclaringClass().getName())
+ .thenComparing(Method::toGenericString));
+ return methods;
+ }
+
+ private boolean generateConfigurations(DuringAnalysisAccess access) {
Review Comment:
**[Suggestion]** Unused `DuringAnalysisAccess access` parameter — the method body never references `access`; it works entirely through instance fields (`hostedConfigurations`, `processedModels`, `reachableMixins`, `processedGenerations`). This is dead code left over from the removed `registerGeneratedCodec` path that used `access.findClassByName`. — Concrete cost: compiling with `-Xlint:unused` flags this as a warning, and a `-Werror` CI configuration would fail the build; the parameter also creates false coupling at the call site.
```suggestion
private boolean generateConfigurations() {
```
_— qwen3-coder-plus via Qwen Code /review_
##########
java/fory-json/src/main/java17/org/apache/fory/json/JsonCreatorCodegen.java:
##########
@@ -0,0 +1,442 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fory.json;
+
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.fory.platform.internal.DefineClass;
+import org.apache.fory.platform.internal._JDKAccess;
+
+/** Generates direct constructor invokers while Native Image analysis is still mutable. */
+final class JsonCreatorCodegen {
+ private static final int ACC_PUBLIC = 0x0001;
+ private static final int ACC_STATIC = 0x0008;
+ private static final int ACC_FINAL = 0x0010;
+ private static final int ACC_SUPER = 0x0020;
+ private static final int ACC_SYNTHETIC = 0x1000;
+
+ private JsonCreatorCodegen() {}
+
+ static Invokers create(Constructor<?> constructor) {
+ Class<?> ownerType = constructor.getDeclaringClass();
+ Class<?> invokerClass =
+ DefineClass.defineHiddenNestmate(ownerType, new ClassBytes(constructor).build());
+ try {
+ MethodHandles.Lookup lookup = _JDKAccess._trustedLookup(invokerClass);
+ MethodHandle creator =
+ lookup.findStatic(
+ invokerClass,
+ "invoke",
+ MethodType.methodType(Object.class, Object[].class));
+ Method creatorMethod = invokerClass.getDeclaredMethod("invoke", Object[].class);
+ if (constructor.getParameterCount() != 1
+ || constructor.getParameterTypes()[0] != String.class) {
+ return new Invokers(invokerClass, creator, creatorMethod, null, null);
+ }
+ MethodHandle stringCreator =
+ lookup.findStatic(
+ invokerClass,
+ "invoke",
+ MethodType.methodType(Object.class, String.class));
+ Method stringMethod = invokerClass.getDeclaredMethod("invoke", String.class);
+ return new Invokers(invokerClass, creator, creatorMethod, stringCreator, stringMethod);
+ } catch (ReflectiveOperationException cause) {
+ throw new IllegalStateException(
+ "Cannot resolve generated Fory JSON creator for " + constructor, cause);
+ }
+ }
+
+ static final class Invokers {
+ final Class<?> type;
+ final MethodHandle creator;
+ final Method creatorMethod;
+ final MethodHandle stringCreator;
+ final Method stringMethod;
+
+ private Invokers(
+ Class<?> type,
+ MethodHandle creator,
+ Method creatorMethod,
+ MethodHandle stringCreator,
+ Method stringMethod) {
+ this.type = type;
+ this.creator = creator;
+ this.creatorMethod = creatorMethod;
+ this.stringCreator = stringCreator;
+ this.stringMethod = stringMethod;
+ }
+ }
+
+ private static final class ClassBytes {
+ private static final String OBJECT = "java/lang/Object";
+ private static final String OBJECT_ARRAY_INVOKE = "([Ljava/lang/Object;)Ljava/lang/Object;";
+ private static final String STRING_INVOKE = "(Ljava/lang/String;)Ljava/lang/Object;";
+
+ private final Constructor<?> constructor;
+ private final Class<?>[] parameterTypes;
+ private final String owner;
+ private final String generatedName;
+ private final boolean stringCreator;
+ private final ConstantPool constants = new ConstantPool();
+
+ private ClassBytes(Constructor<?> constructor) {
+ this.constructor = constructor;
+ parameterTypes = constructor.getParameterTypes();
+ owner = internalName(constructor.getDeclaringClass());
+ generatedName = owner + "$$ForyJsonCreator";
+ stringCreator = parameterTypes.length == 1 && parameterTypes[0] == String.class;
+ }
+
+ private byte[] build() {
+ try {
+ int thisClass = constants.classInfo(generatedName);
+ int superClass = constants.classInfo(OBJECT);
+ MethodCode arrayInvoker = arrayInvoker();
+ MethodCode stringInvoker = stringCreator ? stringInvoker() : null;
+ arrayInvoker.register(constants);
+ if (stringCreator) {
+ stringInvoker.register(constants);
+ }
+
+ ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+ DataOutputStream output = new DataOutputStream(bytes);
+ output.writeInt(0xCAFEBABE);
+ output.writeShort(0);
+ output.writeShort(52);
+ constants.write(output);
+ output.writeShort(ACC_PUBLIC | ACC_FINAL | ACC_SUPER | ACC_SYNTHETIC);
+ output.writeShort(thisClass);
+ output.writeShort(superClass);
+ output.writeShort(0);
+ output.writeShort(0);
+ output.writeShort(stringCreator ? 2 : 1);
+ arrayInvoker.write(output, constants);
+ if (stringCreator) {
+ stringInvoker.write(output, constants);
+ }
+ output.writeShort(0);
+ output.flush();
+ return bytes.toByteArray();
+ } catch (IOException cause) {
+ throw new IllegalStateException("Cannot generate Fory JSON creator for " + constructor, cause);
+ }
+ }
+
+ private MethodCode arrayInvoker() throws IOException {
+ Code code = new Code(constants);
+ code.type(0xbb, owner);
+ code.op(0x59);
+ for (int i = 0; i < parameterTypes.length; i++) {
+ code.op(0x2a);
+ code.integer(i);
+ code.op(0x32);
+ code.convert(parameterTypes[i]);
+ }
+ code.method(0xb7, owner, "<init>", constructorDescriptor(parameterTypes));
+ code.op(0xb0);
+ return new MethodCode(
+ ACC_PUBLIC | ACC_STATIC,
+ "invoke",
+ OBJECT_ARRAY_INVOKE,
+ 4 + parameterTypes.length * 2,
+ 1,
+ code.bytes());
+ }
+
+ private MethodCode stringInvoker() throws IOException {
+ Code code = new Code(constants);
+ code.type(0xbb, owner);
+ code.op(0x59);
+ code.op(0x2a);
+ code.method(0xb7, owner, "<init>", constructorDescriptor(parameterTypes));
+ code.op(0xb0);
+ return new MethodCode(
+ ACC_PUBLIC | ACC_STATIC, "invoke", STRING_INVOKE, 3, 1, code.bytes());
+ }
+ }
+
+ private static final class MethodCode {
+ private final int access;
+ private final String name;
+ private final String descriptor;
+ private final int maxStack;
+ private final int maxLocals;
+ private final byte[] code;
+
+ private MethodCode(
+ int access, String name, String descriptor, int maxStack, int maxLocals, byte[] code) {
+ this.access = access;
+ this.name = name;
+ this.descriptor = descriptor;
+ this.maxStack = maxStack;
+ this.maxLocals = maxLocals;
+ this.code = code;
+ }
+
+ private void write(DataOutputStream output, ConstantPool constants) throws IOException {
+ output.writeShort(access);
+ output.writeShort(constants.utf8(name));
+ output.writeShort(constants.utf8(descriptor));
+ output.writeShort(1);
+ output.writeShort(constants.utf8("Code"));
+ output.writeInt(12 + code.length);
+ output.writeShort(maxStack);
+ output.writeShort(maxLocals);
+ output.writeInt(code.length);
+ output.write(code);
+ output.writeShort(0);
+ output.writeShort(0);
+ }
+
+ private void register(ConstantPool constants) {
+ constants.utf8(name);
+ constants.utf8(descriptor);
+ constants.utf8("Code");
+ }
+ }
+
+ private static final class Code {
+ private static final Map<Class<?>, Class<?>> BOX_TYPES = boxTypes();
+ private final ConstantPool constants;
+ private final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+ private final DataOutputStream output = new DataOutputStream(bytes);
+
+ private Code(ConstantPool constants) {
+ this.constants = constants;
+ }
+
+ private void op(int opcode) throws IOException {
+ output.writeByte(opcode);
+ }
+
+ private void type(int opcode, String type) throws IOException {
+ output.writeByte(opcode);
+ output.writeShort(constants.classInfo(type));
+ }
+
+ private void method(int opcode, String owner, String name, String descriptor)
+ throws IOException {
+ output.writeByte(opcode);
+ output.writeShort(constants.methodRef(owner, name, descriptor));
+ }
+
+ private void integer(int value) throws IOException {
+ if (value <= 5) {
+ output.writeByte(0x03 + value);
+ } else if (value <= Byte.MAX_VALUE) {
+ output.writeByte(0x10);
+ output.writeByte(value);
+ } else {
+ output.writeByte(0x11);
+ output.writeShort(value);
+ }
+ }
+
+ private void convert(Class<?> type) throws IOException {
+ if (!type.isPrimitive()) {
+ if (type != Object.class) {
+ this.type(0xc0, internalName(type));
+ }
+ return;
+ }
+ Class<?> boxType = BOX_TYPES.get(type);
+ this.type(0xc0, internalName(boxType));
Review Comment:
**[Suggestion]** `JsonCreatorCodegen` emits unboxing bytecode for 8 primitive types but only `int` is exercised through `@JsonCreator` constructors in the integration tests; `long`, `double`, `boolean`, `byte`, `short`, `char`, and `float` paths have no test coverage. — Concrete cost: if a future change to `convert()` or `boxTypes()` introduces a wrong box type or unboxing method for `long`, a `@JsonCreator` with a `long` parameter would produce a `VerifyError` or silent value truncation in the native image with no test to catch it.
Consider adding a `@JsonCreator` model to `ForyJsonExample` that exercises at least `long` and `double` constructor parameters.
_— qwen3-coder-plus via Qwen Code /review_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fory.apache.org
For additional commands, e-mail: commits-help@fory.apache.org