Fix collection performance anti-patterns and ChannelContext logic bugs#1535
Open
Fix collection performance anti-patterns and ChannelContext logic bugs#1535
Conversation
…ndant synchronizedMap wrapping, eliminate double map lookup, add ChannelContext tests Co-authored-by: sunhailin-Leo <17564655+sunhailin-Leo@users.noreply.github.com>
…update tests Co-authored-by: sunhailin-Leo <17564655+sunhailin-Leo@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Identify and fix performance or logic issues
Fix collection performance anti-patterns and ChannelContext logic bugs
Mar 3, 2026
There was a problem hiding this comment.
Pull request overview
This PR audits several core modules to fix collection performance anti-patterns and a logic bug in ChannelContext.
Changes:
isEmpty()replacements: Replacessize() == 0withisEmpty()across 6 files (AbstractHttpServer,LocalRegistry,BatchExecutorQueue,RpcConfigs,BeanIdMatchFilter,AbstractLoadBalancer) for idiomatic and potentially more performant checks.RouterChainsimplification: Removes the redundantCollections.synchronizedMap()wrapper aroundConcurrentHashMapforPROVIDER_AUTO_ACTIVESandCONSUMER_AUTO_ACTIVES(safe since iterations over these maps are followed by a sort-by-order step).ChannelContextbug fix: CorrectsinvalidateHeadCache's parameter type fromBytetoShort(matching theTwoWayMap<Short, String>backing), eliminates thecontainsKey+getdouble-lookup, and adds a newChannelContextTestcovering 12 cases.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
AbstractHttpServer.java |
size() == 0 → isEmpty() in unRegisterProcessor |
LocalRegistry.java |
size() == 0 → isEmpty() in unSubscribe |
BatchExecutorQueue.java |
size() == 0 → isEmpty() on snapshot (a LinkedList) in run() |
RpcConfigs.java |
size() == 0 → isEmpty() in unSubscribe |
RouterChain.java |
Removes Collections.synchronizedMap() double-wrapping around ConcurrentHashMap |
BeanIdMatchFilter.java |
size() == 0 → isEmpty() in isMatch |
AbstractLoadBalancer.java |
size() == 0 → isEmpty() in select |
ChannelContext.java |
Fixes invalidateHeadCache parameter type Byte → Short and eliminates containsKey+get double-lookup |
ChannelContextTest.java |
New 12-case unit test class covering cache operations and ChannelContext accessors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of core modules for performance and correctness issues.
Performance fixes
size() == 0→isEmpty()across 6 files —isEmpty()is O(1) for all collections;size()is O(n) forConcurrentLinkedQueue(used inBatchExecutorQueue)Collections.synchronizedMap(new ConcurrentHashMap<>())inRouterChain— double-wrapping negates ConcurrentHashMap's striped locking with a single coarse mutexcontainsKey()+get()double-lookup inChannelContext.invalidateHeadCache()— singleget()with null checkLogic bug fix
ChannelContext.invalidateHeadCache(Byte key, ...)→Short key— the backingTwoWayMap<Short, String>usesShortkeys, soBytelookups viaMap.get(Object)never matched, making the method silently no-op:Tests
ChannelContextTest(12 cases) covering cache put/get, invalidation (success, mismatch, missing key, null cache), ref index allocation, and property accessors. Previously had zero coverage.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
repository.jboss.org/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -classpath /home/REDACTED/work/sofa-rpc/sofa-rpc/.mvn/wrapper/maven-wrapper.jar -Dmaven.home=/home/REDACTED/work/sofa-rpc -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/sofa-rpc/sofa-rpc org.apache.maven.wrapper.MavenWrapperMain -f pom.xml -B -V -e -Dfindbugs.skip -Dcheckstyle.skip -Dpmd.skip=true -Dspotbugs.skip -Denforcer.skip -Dmaven.javadoc.skip -DskipTests -Dmaven.test.skip.exec -Dlicense.skip=true(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.