Aggregate memory_units_t in fetch to reduce cross-shard calls
#12092
Conversation
|
|
ballard26
force-pushed
the
memory-units-agg
branch
from
7cd6844 to
4ca51c1
Compare
ballard26
force-pushed
the
memory-units-agg
branch
from
4ca51c1 to
8a4af64
Compare
ballard26
changed the title
memory_units_t in fetch to reduce cross-shard calls
By aggregating memory_units_t from the same shard together we reduce cross shard function calls from linear to the number of fetched partitions to 1. From @StephanDollberg Don't submit empty and check sem counts instead of has_data. Co-authored-by: Stephan Dollberg <stephan@redpanda.com>
ballard26
force-pushed
the
memory-units-agg
branch
from
8a4af64 to
65c8ad6
Compare
| @@ -263,7 +275,7 @@ static ss::future<read_result> do_read_from_ntp( | |||
| ssx::semaphore& memory_sem, | |||
| ssx::semaphore& memory_fetch_sem) { | |||
| // control available memory | |||
| read_result::memory_units_t memory_units; | |||
| read_result::memory_units_t memory_units(memory_sem, memory_fetch_sem); | |||
There was a problem hiding this comment.
was this a latent bug?
There was a problem hiding this comment.
Yeah, if for some reason read_from_partition reads data even though skip_read is set a seg fault will occur when we try to adjust the semaphore units in memory_units_t. This doesn't happen today, but one never knows what the future holds.
This avoids this potential issue by setting the semaphore units to some reasonable default state.
|
Looks good to me. If this is still causing a burden after these optimizations, one avenue for investigation would be to see if we are in practice returning a small number of units for each fetch request, say 100s of KBs. In that case, we might reasonably say that each core can hold say 10 MB of free but not returned units from another core, and so buffer/batch the units until that number is hit which might coalesce a lot messages into 1. |
By aggregating memory_units_t from the same shard together we reduce cross shard function calls from linear to the number of fetched partitions to 1.
Backports Required
Release Notes