fix(java): fix codegen config hash when registering serializers by sagiruthvik · Pull Request #3122 · apache/fory

sagiruthvik · 2026-01-11T16:08:50Z

Why?

What does this PR do?

Related issues

Closes #3116

Does this PR introduce any user-facing change?

Does this PR introduce any public API change?
Does this PR introduce any binary protocol compatibility change?

Benchmark

chaokunyang · 2026-01-11T16:50:46Z

java/fory-core/src/main/java/org/apache/fory/Fory.java

  private static final boolean isLittleEndian = ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN;
  private static final byte BITMAP = isLittleEndian ? isLittleEndianFlag : 0;
  private static final short MAGIC_NUMBER = 0x62D4;
+  private static final AtomicInteger runtimeHashCounter = new AtomicInteger(0);


we don't need this, we could update configHash instance field direclty when invoke registerSerializer everytime. Andu compute new hash based hash of full classname of registered Serializer.

Hi @chaokunyang
I think the intent was to keep with having the generated class suffix be a relatively small integer.
If going for a full hash of the registered serializers, then perhaps we implement hashCode() for Fory to include Config.hashCode()?

The suffix don't use hash, it use hash as key to store a counter for suffix.
And we should not impl hash code for Fory. Maybe we should not name this method as hash either

Yes. That's why we were following that pattern, but moved it to Fory to cover it being not just the initial config.
Now the incrementAndGet in the ctor and when modifying means that e.g. Fory#1 will get 1, and Fory#2 will get 2, but then Fory#2 gets a serializer added so it becomes 3.

However, does this cover pooled Fory? Does that create multiple instances, so should still use the same trick, so we should use the Map in Fory but include the serializer hashes for computeIfAbsent

Pooled Fory will apply register callback, which will invoke registerSerializer API on every pooled Fory instance. All fory instance will compute to same hash value

Yes. So we cannot do what has been added on line 145. This approach is wrong.

You are agreeing with me.

we could update configHash instance field direclty when invoke registerSerializer everytime. And compute new hash based hash of full classname of registered Serializer

This will work, why don't you use this approach？

Fory will never be invoked By multiple threads, we never need to use any concurrent data structure here. Please remove AtomicInteger here.

I've updated the implementation to compute the hash based on registered serializers instead of using a counter. Now configHash is calculated from config.hashCode() plus the hash of all registered serializer class names. This way, pooled Fury instances with the same serializers will share the same hash, while instances with different serializers get different hashes.

chaokunyang · 2026-01-16T17:53:55Z

java/fory-core/src/main/java/org/apache/fory/Fory.java

  private final IdentityMap<Object, Object> originToCopyMap;
  private int classDefEndOffset;
+  private int configHash;
+  private final Set<String> registeredSerializers = new HashSet<>();


Please delete this, we don't need to store registered serializer

chaokunyang · 2026-01-16T17:54:27Z

java/fory-core/src/main/java/org/apache/fory/Fory.java

+    return configHash;
+  }
+
+  private void invalidateCodegenCache() {


Please also delete this method

chaokunyang · 2026-01-16T17:56:52Z

java/fory-core/src/main/java/org/apache/fory/Fory.java

+    registeredSerializers.add(type.getName() + ":" + serializerClass.getName());
+    invalidateCodegenCache();


Suggested change

registeredSerializers.add(type.getName() + ":" + serializerClass.getName());

invalidateCodegenCache();

this.configHash = Objects.hashCode(configHash, type, serializerClass);

And org.apache.fory.Fory#registerSerializer(java.lang.Class<?>, org.apache.fory.serializer.Serializer<?>) also needs updates

chaokunyang

LGTM

…ges accordingly

…registration and update test

…ounter

nealeu · 2026-01-19T17:11:45Z

Rebased to solve merge conflict. It should now squash nicely

nealeu · 2026-01-19T17:14:44Z

If you want, you could change "when register serializers" in the title to "when registering serializers" for better English in the release notes :-)

chaokunyang · 2026-01-20T16:05:49Z

@nealeu @sagiruthvik Could you fix ci failure errors? Otherwise, we can't merge this PR

nealeu · 2026-01-28T16:11:56Z

Sorry. Bit of a push on here. Ping us if we don't pick it up next week

chaokunyang reviewed Jan 11, 2026

View reviewed changes

sagiruthvik force-pushed the fix-codegen-config-hash branch from f19a744 to 23d71c7 Compare January 16, 2026 16:57

sagiruthvik marked this pull request as ready for review January 16, 2026 17:12

sagiruthvik requested review from chaokunyang and nealeu January 16, 2026 17:13

chaokunyang changed the title ~~Fix codegen config hash~~ fix(java): fix codegen config hash when register serializers Jan 16, 2026

chaokunyang reviewed Jan 16, 2026

View reviewed changes

sagiruthvik force-pushed the fix-codegen-config-hash branch from 23d71c7 to d5b1adc Compare January 17, 2026 14:43

sagiruthvik requested a review from chaokunyang January 17, 2026 14:44

chaokunyang approved these changes Jan 17, 2026

View reviewed changes

sagiruthvik added 6 commits January 19, 2026 17:07

test: Added a test to check codegen cache isolation

5c16a6e

refactor: Move getConfigHash() from Config to Fory and update the usa…

cd12c56

…ges accordingly

fix: Fix codegen cache collision by invalidating cache on serializer …

c2cee05

…registration and update test

fix: Fix configHash to use content-based hashing instead of counter

8dd815f

refactor: update getConfigHash() usages

906b27d

refactor: compute configHash inline using Objects.hash() instead of c…

dfbbaed

…ounter

nealeu force-pushed the fix-codegen-config-hash branch from d5b1adc to dfbbaed Compare January 19, 2026 17:10

chaokunyang changed the title ~~fix(java): fix codegen config hash when register serializers~~ fix(java): fix codegen config hash when registering serializers Jan 20, 2026

		registeredSerializers.add(type.getName() + ":" + serializerClass.getName());
		invalidateCodegenCache();

	registeredSerializers.add(type.getName() + ":" + serializerClass.getName());
	invalidateCodegenCache();
	this.configHash = Objects.hashCode(configHash, type, serializerClass);

Conversation

sagiruthvik commented Jan 11, 2026 • edited by chaokunyang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

What does this PR do?

Related issues

Does this PR introduce any user-facing change?

Benchmark

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chaokunyang left a comment

Choose a reason for hiding this comment

Uh oh!

nealeu commented Jan 19, 2026

Uh oh!

nealeu commented Jan 19, 2026

Uh oh!

chaokunyang commented Jan 20, 2026

Uh oh!

nealeu commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sagiruthvik commented Jan 11, 2026 •

edited by chaokunyang

Loading