Repository navigation
fix(concurrent): count the -Xmx headroom as available memory in MemoryLimitCalculator - #7809
Conversation
…yLimitCalculator MemoryLimitCalculator.maxAvailable() sampled only Runtime.freeMemory(), the free part of the currently committed heap, so MemorySafeLinkedBlockingQueue and the admission paths it backs (the shared HierarchicalWheelTimer queue and the work queues) rejected tasks whenever the committed heap had less free space than the configured floor, even though the JVM could still commit heap up to -Xmx. On small-Xms deployments the wheel timer queue (Constants.THE_256_MB) then silently dropped recurring tasks such as heartbeat registrations (issue repro: -Xms256m -Xmx768m, 156MiB free committed vs 668MiB headroom to -Xmx -> rejected). refresh() now derives the limit from maxMemory() - (totalMemory() - freeMemory()), which is identical to freeMemory() when the committed heap already equals -Xmx (pinned-heap deployments keep their exact behavior) and only admits more when the heap can actually grow. The apache/dubbo twin of this class still samples freeMemory(), so this intentionally improves on it. The MemorySafeLinkedBlockingQueue tests now use a maxMemory()+1 floor guarded by an assumption instead of the Integer.MAX_VALUE floor, which could no longer force rejection on heaps above 2GiB once the headroom is counted, and adds regression tests for the headroom accounting. Fixes apache#7803 Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
|
Hi maintainers — this is my first PR to the repo, so the Everything is verified locally in the meantime: the 9 Thanks! |
BobSong-dev
left a comment
There was a problem hiding this comment.
Thanks for the fix and the regression tests. The headroom calculation addresses the small-initial-heap case in my local verification. Could we make this assertion independent of the background refresh timing?
maxAvailable() is a cached sample refreshed every 50 ms, whereas runtime.totalMemory() is read live. With -Xms64m -Xmx256m, I initialized the calculator while about 170 MiB was used, released those objects, and triggered GC. The heap shrank to 64 MiB, so this comparison became 85 MiB >= 192 MiB and the new test method failed. It passed again after the cache refreshed; I reproduced this in five independent runs.
The formula itself is correct in this scenario. Could we test it against a controlled memory snapshot so a GC between sampling and assertion cannot make the regression test fail spuriously?
Aias00
left a comment
There was a problem hiding this comment.
Approved. The arithmetic is right: maxMemory - (totalMemory - freeMemory) is exactly freeMemory + (maxMemory - totalMemory), i.e. the free part of the committed heap plus the headroom the JVM can still commit up to -Xmx. The inline comment is also accurate that it degenerates to the old freeMemory() when the committed heap already equals -Xmx (-Xms == -Xmx), which is what makes the change safe for existing configurations.
The new testAdmissionCountsMemoryTheHeapCanStillCommit is the valuable part of this PR. Choosing floor = (free + headroom) / 2 places it strictly between the old and the new answer, so the test actually distinguishes the two implementations instead of passing trivially — that is the test the previous suite was missing, and it is why the old formula shipped.
Two notes, neither blocking:
-
This loosens the admission margin, deliberately.
MemoryLimitCalculator.maxAvailable()anddefaultLimit()are consumed byMemoryLimiterand the memory-safe queue, andmaxAvailableis now strictly larger than before. Callers that were conservatively rejecting work near-Xmxwill now admit it, which is the point of the fix, but it does mean the safety margin is now “everything up to-Xmx” rather than “what is already committed and free”.defaultLimit()keeps the 80% factor, so the effective ceiling moves from 80% of free-committed to 80% of free+headroom. Worth stating in the release notes, because operators tune pool sizes against the old number. -
The new assertion in
MemoryLimitCalculatorTestis close to tautological.maxAvailable() >= maxMemory - totalMemoryholds for any value that includes the headroom, and the oldfreeMemory()implementation would only fail it whenfreeMemory() < maxMemory - totalMemory. It is a directional guard rather than a real assertion; the queue-level test above is doing the actual work.
The assumeTrue guards and the switch from Integer.MAX_VALUE to maxMemory() + 1 in MemorySafeLinkedBlockingQueueTest are a good cleanup — the old “all memory reserved” fixtures were only deterministic on heaps below 2 GiB.
Note: CI has not reported on this branch yet — please confirm the workflow passes before merge.
Describe what this PR does and why we are doing this
MemoryLimitCalculator.maxAvailable()sampled onlyRuntime.freeMemory(), i.e. the free part of the currently committed heap. Queue admission built on it (MemorySafeLinkedBlockingQueue, which backsMemorySafeTaskQueue, the sharedHierarchicalWheelTimerqueue and the work queues) therefore rejected tasks whenever the committed heap held less free memory than the configured floor, even when the JVM could still commit heap up to-Xmx.The production-visible case from #7803: with
-Xms256m -Xmx768mthe wheel timer queue (built with theConstants.THE_256_MBfloor) silently dropped recurring tasks such as heartbeat registrations — 156MiB free committed vs 668MiB headroom to-Xmx,accepted=false, with the defaultDiscardPolicyswallowing the rejection.Change
refresh()now derives the limit frommaxMemory() - (totalMemory() - freeMemory()):-Xmx(-Xms == -Xmx, the usual pinned-heap / container deployment), the value is identical tofreeMemory(), so those deployments keep their exact behavior.-Xmx.Consumers that pick this up automatically:
MemorySafeLinkedBlockingQueue(+MemorySafeTaskQueue),MemorySafeLRUMap, and themaxWorkQueueMemorydefault inShenyuConfig. The apache/dubbo twin of this class still samplesfreeMemory(); this change intentionally improves on that, so the two are no longer byte-equivalent here.Tests
MemorySafeLinkedBlockingQueueTest.testAdmissionCountsMemoryTheHeapCanStillCommit: on a growable heap, a floor between the free committed memory and the-Xmxheadroom must admit. Verified to fail on the previous behavior.MemoryLimitCalculatorTest.testMaxAvailableCountsMemoryTheHeapCanStillCommit:maxAvailable()must cover the part of the heap the JVM can still commit.Integer.MAX_VALUEas a "guaranteed rejection" floor now use amaxMemory() + 1floor guarded byassumeTrue: the old floor could never force rejection once the headroom is counted on heaps above 2GiB, and was already environment-dependent before. They pass unchanged on the previous behavior.shenyu-commonmodule suite: 509 tests, 0 failures; the memory classes additionally verified under-Xms64m -Xmx256m(growable heap, all assumption-guarded tests execute).checkstyle:checkpasses.Fixes #7803
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true. (Ran./mvnw -pl shenyu-common -am testfor the affected module: 509 tests, 0 failures, pluscheckstyle:check; the change is confined toshenyu-common.)