fix: avoid redundant Fury instance recreation to prevent FGC#1553
fix: avoid redundant Fury instance recreation to prevent FGC#1553sunhailin-Leo wants to merge 1 commit intomasterfrom
Conversation
Introduce boundClassLoaderHolder (ThreadLocal<ClassLoader>) to track the ClassLoader currently bound to the Fury instance per thread. Previously, every encode/decode call unconditionally invoked setClassLoader + clearClassLoader, causing ThreadLocalFury to discard and recreate Fury instances on every RPC call. Under high TPS (e.g. 20000 TPS), this produced thousands of Fury instances in the heap, each holding a large ClassResolver, leading to frequent Full GC. With this fix, setClassLoader/clearClassLoader are only called when the thread's ContextClassLoader actually changes (e.g. in SOFAArk multi-module environments). In typical single-ClassLoader deployments, the Fury instance is reused across calls on the same thread, keeping instance count proportional to thread count rather than TPS. Fixes #1424
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.java (1)
118-143: 🛠️ Refactor suggestion | 🟠 MajorSuggestion: Consider removing the finally blocks entirely.
Given the critical issue above, if the fix is applied, all three methods (
encodeand bothdecodeoverloads) can have their finally blocks removed entirely. TheclassLoaderSwitchedflag becomes unnecessary, simplifying the code and achieving the intended per-thread Fury reuse.Also applies to: 145-169, 171-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.java` around lines 118 - 143, Remove the redundant finally blocks and related classloader cleanup logic from FurySerializer.encode and both decode overloads: eliminate the classLoaderSwitched boolean, the switchClassLoaderIfNeeded(...) call, and the finally body that calls fury.clearClassLoader(contextClassLoader) and boundClassLoaderHolder.remove(); simplify the methods to rely on the corrected classloader handling elsewhere so they only perform the try/catch (preserving the existing exception handling that throws buildSerializeError/buildDeserializeError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.java`:
- Around line 138-141: The current logic in FurySerializer (use of
boundClassLoaderHolder.remove() in finally and the classLoaderSwitched flag)
clears the ThreadLocal on every call so Fury instances cannot be reused across
sequential calls; change the binding logic to only switch when the current
context ClassLoader actually differs from the bound one (compare
boundClassLoader vs contextClassLoader), do not call
boundClassLoaderHolder.remove() in the finally block, and remove the
classLoaderSwitched flag/related finally cleanup so the ClassLoader binding (and
associated Fury instance) persists per-thread; apply the same change to both
decode(...) overloads and ensure any call to
fury.clearClassLoader(contextClassLoader) is only invoked when you genuinely
unbind due to a ClassLoader change.
---
Outside diff comments:
In
`@codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.java`:
- Around line 118-143: Remove the redundant finally blocks and related
classloader cleanup logic from FurySerializer.encode and both decode overloads:
eliminate the classLoaderSwitched boolean, the switchClassLoaderIfNeeded(...)
call, and the finally body that calls fury.clearClassLoader(contextClassLoader)
and boundClassLoaderHolder.remove(); simplify the methods to rely on the
corrected classloader handling elsewhere so they only perform the try/catch
(preserving the existing exception handling that throws
buildSerializeError/buildDeserializeError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c06cf949-5e69-4964-a8c0-1bd6bae88dea
📒 Files selected for processing (1)
codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.java
| if (classLoaderSwitched) { | ||
| fury.clearClassLoader(contextClassLoader); | ||
| boundClassLoaderHolder.remove(); | ||
| } |
There was a problem hiding this comment.
Critical: Optimization ineffective for sequential calls due to remove() in finally.
The finally blocks call boundClassLoaderHolder.remove() whenever classLoaderSwitched is true. Since the first call on a thread always switches (from null to the context ClassLoader), every sequential call follows this pattern:
- Call 1:
boundClassLoader=null, context=CL1 → switch=true → finally removes ThreadLocal value - Call 2:
boundClassLoader=null(was removed) → switch=true → Fury instance recreated again
This defeats the PR's stated goal of reusing Fury instances per thread. The optimization only works for nested calls (e.g., inner encode() within outer encode()), not the common case of sequential RPC calls.
To achieve per-thread Fury reuse, the ClassLoader binding should persist across calls, only changing when the context ClassLoader actually differs.
🐛 Proposed fix: Don't clear in finally; only switch on actual ClassLoader change
`@Override`
public AbstractByteBuf encode(final Object object, final Map<String, String> context) throws SofaRpcException {
if (object == null) {
throw buildSerializeError("Unsupported null message!");
}
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
- boolean classLoaderSwitched = switchClassLoaderIfNeeded(contextClassLoader);
+ switchClassLoaderIfNeeded(contextClassLoader);
try {
// ... existing try block ...
} catch (Exception e) {
throw buildSerializeError(e.getMessage(), e);
- } finally {
- if (classLoaderSwitched) {
- fury.clearClassLoader(contextClassLoader);
- boundClassLoaderHolder.remove();
- }
}
+ // No finally cleanup - let the ClassLoader binding persist for reuse
}
private void switchClassLoaderIfNeeded(ClassLoader contextClassLoader) {
ClassLoader boundClassLoader = boundClassLoaderHolder.get();
if (boundClassLoader == contextClassLoader) {
- // ClassLoader unchanged: reuse the existing Fury instance on this thread
- return false;
+ return; // Same ClassLoader - Fury instance already bound correctly
+ }
+ // ClassLoader changed (or first call on this thread)
+ if (boundClassLoader != null) {
+ // Clear the previous ClassLoader binding before switching
+ fury.clearClassLoader(boundClassLoader);
}
fury.setClassLoader(contextClassLoader);
boundClassLoaderHolder.set(contextClassLoader);
- return true;
}Apply the same pattern to both decode() methods: remove the classLoaderSwitched flag, remove the finally blocks, and let the ClassLoader binding persist.
Also applies to: 164-167, 189-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.java`
around lines 138 - 141, The current logic in FurySerializer (use of
boundClassLoaderHolder.remove() in finally and the classLoaderSwitched flag)
clears the ThreadLocal on every call so Fury instances cannot be reused across
sequential calls; change the binding logic to only switch when the current
context ClassLoader actually differs from the bound one (compare
boundClassLoader vs contextClassLoader), do not call
boundClassLoaderHolder.remove() in the finally block, and remove the
classLoaderSwitched flag/related finally cleanup so the ClassLoader binding (and
associated Fury instance) persists per-thread; apply the same change to both
decode(...) overloads and ensure any call to
fury.clearClassLoader(contextClassLoader) is only invoked when you genuinely
unbind due to a ClassLoader change.
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce Full GC pressure in the Fury-based serializer by avoiding per-call thread-local Fury instance churn caused by unconditional setClassLoader / clearClassLoader calls, addressing issue #1424.
Changes:
- Added a per-thread
boundClassLoaderHolderto track the currently boundClassLoader. - Introduced
switchClassLoaderIfNeeded(...)and wired it intoencode/decodepaths to make ClassLoader switching conditional. - Adjusted
finallyblocks to conditionally callclearClassLoader.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (classLoaderSwitched) { | ||
| fury.clearClassLoader(contextClassLoader); | ||
| boundClassLoaderHolder.remove(); | ||
| } |
There was a problem hiding this comment.
Same issue as in encode: removing boundClassLoaderHolder in finally resets the per-thread bound ClassLoader every time, so subsequent calls will always consider the ClassLoader “switched” and re-run setClassLoader/clearClassLoader on every decode.
| if (classLoaderSwitched) { | ||
| fury.clearClassLoader(contextClassLoader); | ||
| boundClassLoaderHolder.remove(); | ||
| } |
There was a problem hiding this comment.
Same issue as the other methods: boundClassLoaderHolder.remove() in finally wipes the cached ClassLoader, so switchClassLoaderIfNeeded can’t ever observe “unchanged” across calls and will keep switching every time.
| ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader(); | ||
| boolean classLoaderSwitched = switchClassLoaderIfNeeded(contextClassLoader); | ||
| try { |
There was a problem hiding this comment.
There are existing unit tests for FurySerializer, but none exercise the new “switch ClassLoader only when needed” behavior. Please add tests that (1) call encode/decode multiple times with the same thread context ClassLoader and verify the switch path is not taken after the first call, and (2) change the thread context ClassLoader between calls and verify the switch path is taken.
| if (classLoaderSwitched) { | ||
| fury.clearClassLoader(contextClassLoader); | ||
| boundClassLoaderHolder.remove(); | ||
| } |
There was a problem hiding this comment.
boundClassLoaderHolder.remove() here defeats the tracking across calls: on the next invocation boundClassLoaderHolder.get() becomes null again, so switchClassLoaderIfNeeded(...) will always treat it as a switch and keep calling setClassLoader (and then clearClassLoader) every call. That makes the optimization ineffective and likely preserves the original Fury instance churn. Consider keeping the bound ClassLoader in the ThreadLocal across calls and only switching when the thread context ClassLoader actually changes (and avoid clearing/removing in the normal path).
What problem does this PR solve?
Fixes #1424
Root Cause
In the current implementation, every
encode/decodecall unconditionally invokes:ThreadLocalFury.clearClassLoader()discards the current thread-local Fury instance, causing a new Fury instance to be created on the next call. Since each Fury instance holds its ownClassResolver(a large heap object), this results in:Solution
Introduce
boundClassLoaderHolder(ThreadLocal<ClassLoader>) to track the ClassLoader currently bound to the Fury instance per thread.setClassLoader/clearClassLoaderare now only called when the thread'sContextClassLoaderactually changes (e.g. in SOFAArk multi-module environments). In typical single-ClassLoader deployments, the Fury instance is reused across all calls on the same thread.Impact
Changes
codec/codec-sofa-fury/src/main/java/com/alipay/sofa/rpc/codec/fury/FurySerializer.javaboundClassLoaderHolderfieldsetClassLoader/clearClassLoaderwith conditionalswitchClassLoaderIfNeededswitchClassLoaderIfNeededSummary by CodeRabbit