feat(json): add GraalVM codegen support - #3907
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3-coder-plus via Qwen Code /review (v0.21.2)
| } 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); | ||
| } |
There was a problem hiding this 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.
| } 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); | |
| } | |
| } catch (Throwable e) { | |
| throw new ForyJsonException("Cannot create SQL JSON type " + type, e); | |
| } |
— qwen3-coder-plus via Qwen Code /review
| 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)); |
There was a problem hiding this 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
| return methods; | ||
| } | ||
|
|
||
| private boolean generateConfigurations(DuringAnalysisAccess access) { |
There was a problem hiding this 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.
| private boolean generateConfigurations(DuringAnalysisAccess access) { | |
| private boolean generateConfigurations() { |
— qwen3-coder-plus via Qwen Code /review
| @Internal | ||
| public final class JsonGeneratedClassRegistry { | ||
| private static Map<JsonCodegenKey, MutableConfiguration> pending = new HashMap<>(); | ||
| private static Map<JsonCodegenKey, Configuration> configurations = Collections.emptyMap(); |
There was a problem hiding this 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.
| private static Map<JsonCodegenKey, Configuration> configurations = Collections.emptyMap(); | |
| private static volatile Map<JsonCodegenKey, Configuration> configurations = Collections.emptyMap(); |
— qwen3-coder-plus via Qwen Code /review
3 participants
Why?
What does this PR do?
Related issues
AI Contribution Checklist
yes/noyes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described inAI_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?
Benchmark