Skip to content

refactor(format): inline custom-codec dispatch in row codecs#3716

Open
stevenschlansker wants to merge 1 commit into
apache:mainfrom
stevenschlansker:fory-row-codec-dispatch
Open

refactor(format): inline custom-codec dispatch in row codecs#3716
stevenschlansker wants to merge 1 commit into
apache:mainfrom
stevenschlansker:fory-row-codec-dispatch

Conversation

@stevenschlansker
Copy link
Copy Markdown
Contributor

@stevenschlansker stevenschlansker commented May 29, 2026

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, 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: 👍
  • 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: ✔️
  • provenance_license_confirmation: ✔️

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant