stevenschlansker opened a new pull request, #3716: URL: https://github.com/apache/fory/pull/3716
## Why? Simplify codegen and runtime codepath, net -1 frame stack depth, set stage for future feature work ## What does this PR do? Replace CustomTypeEncoderRegistry$Gen<N>, a per-registration Janino-generated class whose static encode/decode/newCollection methods the row codec called into, with one static final CustomCodec / CustomCollectionFactory field per registered codec on the row codec class itself. Fields are initialized at class load from CustomTypeEncoderRegistry; encode/decode invoke the cached instance. Per-call cost is one indirect call; the Janino compile-and-define cycle on first registration is gone. CustomTypeEncoderRegistry collapses to two ConcurrentHashMap lookups plus a singleton handler view. findCodec / findCollectionFactory are public so a row codec generated under a child classloader can reach them. Key the collection-factory codegen cache on (container, element) rather than container alone, so two SortedSet-typed sibling fields with different element types dispatch to their own factories. ## AI Contribution Checklist - [ ] Substantial AI assistance was used in this PR: `yes` / `no` AI Usage Disclosure - substantial_ai_assistance: yes - scope: all - affected_files_or_subsystems: Java row format - ai_review: :+1: - ai_review_artifacts: > Nothing blocking. Net is a clear win: deletes ~165 lines of Janino-generated dispatch infrastructure, removes a class-generation step from cold-start registration, and folds in a real bug fix for sibling collection fields with a focused test. > LGTM with no requested changes. Cleanly deletes the per-registration Janino dispatch class in favor of static-final field caches on the row codec itself, and fixes the sibling-SortedSet cache-keying bug with a targeted test. fory-format suite is green (108/108, 4 pre-existing skips). > — Claude > Overall > Sound refactor and a meaningful simplification — a Janino compile/define cycle per registration is removed in favor of one monomorphic, JIT-friendly call site per codec field. The (beanType, fieldType) key matches at both codegen-time discovery and runtime lookup, and concurrency moves from coarse synchronized to ConcurrentHashMap cleanly. Residual risks are minor: small generated-class bloat if duplicate TypeRefs ever reach customEncode. > I followed the Apache Fory AI review policy: review was performed by a read-only subagent (no edits, commits, or pushes), with findings ordered by severity, citing exact file/line locations, and including the validation commands the author should run before merging. > — Claude (Opus 4.7) - human_verification: :heavy_check_mark: - provenance_license_confirmation: :heavy_check_mark: ## Does this PR introduce any user-facing change? Internal API change only ## Benchmark > - JMH numbers across both suites, n=15 per benchmark on each side: every measurement is within error bars of equal. No regression. > - Inlining graphs confirm: both versions fully inline the user's encode/decode body into the generated row codec, both call sites fully > monomorphic (5120/5120 type profile). Baseline has one extra frame (Gen1::encode trampoline); HEAD goes straight from toRow to > WrappedCodec::encode. Both collapse to the same machine code in steady state. > Verdict: no performance regression, and the inlining shape is what the static-final + monomorphic-INVOKEINTERFACE model predicts. The "static → virtual" concern doesn't materialize because the old static call was itself a thin static trampoline around an identical interface call. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
