optimize: park merge send thread when idle to avoid 1ms polling CPU spin

[CuriousLinYu]

Fixes #6041

Problem

MergedSendRunnable polls every 1ms (MAX_MERGE_SEND_MILLS = 1) even when all baskets are empty. With no traffic, the thread wakes up 1000 times per second, each cycle acquiring mergeLock, iterating basketMap and going back to sleep. Issue #6041 reported this at ~30% CPU per thread (thread dump shows the rpcMergeMessageSend_*_1 threads in TIMED_WAITING), and it still reproduces on 2.x (RM client enables batch send by default: DEFAULT_ENABLE_RM_CLIENT_BATCH_SEND_REQUEST = true).

Solution

When basketMap is empty, the merge thread parks on mergeCondition.await() (no timeout) instead of a 1ms timed wait. Producers already offer to the basket and then signal (sendSyncRequest), so the parked thread is woken up as soon as a message arrives and drains the basket immediately.

About the removed 1ms timed wait:

• The offer-then-signal path has existed since the early implementation (the merge thread predates AbstractRpcRemotingClient; see the history of AbstractNettyRemotingClient.java, e.g. PR refactor TmRpcClient & RmClient for common use. #1105 and PR feature: add single send request for client. #1966) — sendSyncRequest always signals when the merge thread is not sending.
• funky-eyes pointed out the same idea in issue rpcMergeMessageSend_TMROLE_1、rpcMergeMessageSend_RMROLE_1的cpu使用率过高 #6041: the signal from sendSyncRequest is enough to wake the thread, so a timed wait is not required for correctness.
• The 1ms wait therefore acted as a fallback poll on top of the signal path; keeping it after the park adds a >=1ms floor to the first message after an idle period without buying meaningful batching (see measurements below).

Race safety

• The check-and-wait is atomic: isBasketEmpty() is evaluated and await() is entered while holding mergeLock.
• Producers offer to the basket before acquiring mergeLock and signalling (unchanged code), so a message can never sit in the basket with the merge thread parked indefinitely: either the producer's signal wakes the parked thread, or the producer sees isSending == true and the thread is already in the send loop which drains the basket on the next iteration.
• No lost wake-up: a producer can only signal after the thread has parked (Condition.await releases the lock atomically), and the empty-check is re-evaluated inside the while loop.

Measurements

Measured locally with an instrumented test client (basket-level offer/poll instrumentation, no network needed) across three variants. Thread CPU time via ThreadMXBean.getThreadCpuTime over a 10s idle window; batch sizes over 10 rounds of a 4x25 concurrent burst.

variant	idle CPU (10s window)	first-message drain latency	batch size (avg / max)
original (1ms poll)	31ms (0.312%), TIMED_WAITING	0.17 - 6.2ms	21.3 / 95
park + 1ms await	0ms (0.000%), WAITING	1.4 - 15.3ms	83.3 / 100
park + immediate drain (this PR)	0ms (0.000%), WAITING	0.09 - 0.21ms	50.0 / 100

The absolute idle-CPU ratio depends on the machine and CPU quota (issue #6041 measured ~30% in a constrained container). The relevant comparison is original > 0 vs this PR = 0. The "park + 1ms await" variant shows the >=1ms latency floor that this change removes, while batching does not regress vs the original.

Tests

• Added testMergedSendRunnableIdleWaitState: asserts the rpcMergeMessageSend thread is NOT in TIMED_WAITING after 300ms idle, then submits a request and verifies the basket is drained.
• Full NettyRemotingClientBehaviorTest: 69 tests, 0 failures.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.28%. Comparing base (2efc2aa) to head (2e39148).

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #8205      +/-   ##
============================================
+ Coverage     73.26%   73.28%   +0.01%     
  Complexity     1146     1146              
============================================
  Files          1153     1153              
  Lines         42348    42356       +8     
  Branches       5061     5064       +3     
============================================
+ Hits          31028    31040      +12     
+ Misses         8832     8830       -2     
+ Partials       2488     2486       -2     

Files with missing lines	Coverage Δ
...ta/core/rpc/netty/AbstractNettyRemotingClient.java	78.14% <100.00%> (-0.39%)	⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[funky-eyes]

With this approach, whenever a message arrives, the processing thread waits for up to another 1 ms. Why is this additional delay necessary?

With the implementation you’re currently using, consider the case where only a single message arrives: it wakes up the thread, but the thread then waits for another 1 ms before sending the message. This effectively adds about 1 ms to the request latency.

I don’t think an await of 1 ms alone should cause such high CPU usage. Could the high CPU utilization be related to Arthas being enabled? Given the performance of modern CPUs, 1,000 wake-ups per second that do essentially no work should not normally be enough to consume around 30% CPU.

[CuriousLinYu]

Thanks for the review — both points were spot on. I've removed the post-wakeup await(1ms) entirely: the thread now drains immediately after being signalled. Measured locally: first-message drain latency drops from ~1.4-15.3ms (with the 1ms await) to ~0.09-0.21ms, and batching does not regress — avg batch size is 50 vs 21.3 on the original under a 4x25 concurrent burst. The full measurement table is in the PR description.

On the 30% CPU number — fair point that Arthas sampling and container CPU quota matter. What the issue confirms is the thread dump: the rpcMergeMessageSend_*_1 threads sit in TIMED_WAITING while burning CPU. Locally (ThreadMXBean, 10s window) the original implementation consumes a steady 31ms of thread CPU time per 10s at idle; this PR takes it to 0. The absolute ratio is environment-dependent, but the idle wake-up cost is real and now gone.

[CuriousLinYu]

Hi @funky-eyes, thanks for the review — both points were addressed in 2e39148: the post-wakeup await(1ms) is gone, so the merge thread now drains immediately after being signalled (first-message drain latency drops from ~1.4-15.3ms to ~0.09-0.21ms, and batching does not regress — avg batch size 50 vs 21.3 on the original under a 4x25 burst). Details are in the reply above and in the PR description.

Two process-side items are still blocking the merge:

1. CI has not run on this PR at all — there are no check runs on the head commit and the combined status is pending. For a first-time contributor the workflow usually needs a maintainer to approve the run. Could you approve it when you get a chance?
2. The review thread above is still open (the code it refers to has since changed). If 2e39148 looks right to you, resolving it would clear the blocked merge state.

Happy to make any further adjustments.

[CuriousLinYu]

Pushed a merge with the latest 2.x (41694e140) to re-trigger CI — the fork's workflow runs need approval again.

On the previous spotless failure: I could not reproduce it locally. ./mvnw spotless:check passes on the full reactor (103 modules) under JDK 17, and core is clean (366 files) after clearing the spotless index. The failing run finished in 22s, while a neighbouring PR's spotless job took 37s and passed, so I suspect that run failed while resolving dependencies rather than on an actual violation.

Could someone approve the run so we can see a real result? If it comes back with a specific file, I'll fix it immediately.