[#2196] Improve queue consumer starvation under low configuration#2197
[#2196] Improve queue consumer starvation under low configuration#2197mattrpav wants to merge 2 commits into
Conversation
mattrpav
changed the title
mattrpav
force-pushed
the
amq-gh-2196-queue-consumer-starvation
branch
from
b733033 to
607f112
Compare
mattrpav
force-pushed
the
amq-gh-2196-queue-consumer-starvation
branch
from
607f112 to
a5b8ef9
Compare
cshannon
left a comment
cshannon
left a comment
There was a problem hiding this comment.
I only took a quick look so far but I don't really understand why you don't just set lazyDispatch to true? The point of that flag is to do exactly this where it will page in what consumers can handle.
| if (isLazyDispatch() && !force) { | ||
| // Only page in the minimum number of messages which can be | ||
| // dispatched immediately. | ||
| toPageIn = Math.min(toPageIn, getConsumerMessageCountBeforeFull()); |
There was a problem hiding this comment.
getConsumerMessageCountBeforeFull() is an expensive operation and has to iterate, it should be called one time and not 3 times and result saved
There was a problem hiding this comment.
I think also dispatchPendingList trigger maxPageSize each cycle (related).
| // dispatched immediately. | ||
| toPageIn = Math.min(toPageIn, getConsumerMessageCountBeforeFull()); | ||
| } | ||
| if (pagedInPendingSize >= maxPageSize && !force) { |
There was a problem hiding this comment.
Shouldn't this be an if/else so you aren't doing the same thing twice?
LazyDispatch vs this change comes down to when force = true. I think you are right that lazy dispatch would handle the normal consumer, since it looks for force=false scenarios. I'll do a pass through cursor scenarios. iirc the force=true scenarios were the management use cases. edit: lazyDispatch does not provide relief. The issue is this must also evaluate to true: When the pageSize is super low and the pagedInPendingSize is larger, this will evaluate false. That being said, I'm not convinced taking the penalty to calculate if consumers have space makes sense to put on everyone's queue, every time for this one scenario. I'm leaning towards the best approach is to not starve the queue with a small maxPageSize. |
…figuration Test demonstrates that stalled low-priority network consumers with saturated prefetch throttle a normal-priority app consumer's throughput by 5-20x. Root cause: Queue.doPageInForDispatch() uses dispatchPendingList.size() against maxPageSize, so messages stuck behind full network consumers block page-in for all consumers. The test assertion (slowdownFactor <= 3.0) is expected to FAIL against the current code, proving the bug exists. The subsequent fix commit will make it pass.
…l prefetch Queue.doPageInForDispatch() used pagedInPendingSize < maxPageSize as the sole gate for paging in messages from the store. When low-priority network consumers had saturated prefetch (not acking), their undispatched messages accumulated in dispatchPendingList, counting against maxPageSize and blocking page-in for all consumers — including higher-priority app consumers with available prefetch capacity. Two changes: 1. When pagedInPendingSize >= maxPageSize, cap toPageIn to what consumers can actually accept (getConsumerMessageCountBeforeFull) 2. Allow page-in when any consumer has available prefetch capacity, even if pagedInPendingSize >= maxPageSize
mattrpav
force-pushed
the
amq-gh-2196-queue-consumer-starvation
branch
from
a5b8ef9 to
2b73dd1
Compare
|
I also have concerns that this could page in a lot more into memory and cause a huge memory spike if you are using count before full. The default prefetch is 32k and obviously that should be tuned but it could cause a large increase in paged in messages if there are a lot of consumers even with a prefetch not that high |
This might be fine the more i think about it (after all we want to keep prefetch full), i just want to take a closer look as it has been a while since i've looked at all the logic with paging stuff in |
Note: queuePrefetch default is 1,000, but yeah the overall concern is valid. Its an odd scenario and it looks like it trips up on the somewhat side-ways logic. memoryUsage: has space The conditional comparing current pending count vs pageSize is a context switch in the comparison logic. Smells like an apples-to-oranges type deal rotating around counts vs space in memory usage. |
3 participants
Run the test before the fix and observe:
Test run after fix:
edit: Tests passed, except for assembly on an address-in-use bind error. Re-running tests to get a green checkmark