Thanks for continuing to review! I've revised the patches to
incorporate all of your feedback except for where I mention below.
There were failures in CI due to issues with max batch size, so
attached v8 also seeks to fix those.
- Melanie
On Thu, Oct 16, 2025 at 12:25 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> 3 - 0003
> ```
> +/*
> + * Return the next buffer in the ring or InvalidBuffer if the current sweep is
> + * over.
> + */
> +Buffer
> +StrategySweepNextBuffer(BufferAccessStrategy strategy, int *sweep_cursor)
> +{
> + if (++(*sweep_cursor) >= strategy->nbuffers)
> + *sweep_cursor = 0;
> +
> + return strategy->buffers[*sweep_cursor];
> +}
> ```
>
> Feels the function comment is a bit confusing, because the function code doesn’t really perform sweep, the function
isjust a getter. InvalidBuffer just implies the current sweep is over.
>
> Maybe rephrase to something like: “Return the next buffer in the range. If InvalidBuffer is returned, that implies
thecurrent sweep is done."
Yes, actually I think having these helpers mention the sweep is more
confusing than anything else. I've revised them to be named more
generically and updated the comments accordingly.
> 5 - 0004
> ```
> +uint32
> +StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
> +{
> + uint32 max_possible_buffer_limit;
> + uint32 max_write_batch_size;
> + int strategy_pin_limit;
> +
> + max_write_batch_size = io_combine_limit;
> +
> + strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
> + max_possible_buffer_limit = GetPinLimit();
> +
> + max_write_batch_size = Min(strategy_pin_limit, max_write_batch_size);
> + max_write_batch_size = Min(max_possible_buffer_limit, max_write_batch_size);
> + max_write_batch_size = Max(1, max_write_batch_size);
> + max_write_batch_size = Min(max_write_batch_size, io_combine_limit);
> + Assert(max_write_batch_size < MAX_IO_COMBINE_LIMIT);
> + return max_write_batch_size;
> +}
> ```
>
> This implementation is hard to understand. I tried to simplify it:
> ```
> uint32
> StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
> {
> int strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
> uint32 max_write_batch_size = Min(GetPinLimit(), (uint32)strategy_pin_limit);
>
> /* Clamp to io_combine_limit and enforce minimum of 1 */
> if (max_write_batch_size > io_combine_limit)
> max_write_batch_size = io_combine_limit;
> if (max_write_batch_size == 0)
> max_write_batch_size = 1;
>
> Assert(max_write_batch_size < MAX_IO_COMBINE_LIMIT);
> return max_write_batch_size;
> }
> ```
I agree that the implementation was hard to understand. I've not quite
gone with your version but I have rewritten it like this:
uint32
StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
{
uint32 max_write_batch_size = Min(io_combine_limit,
MAX_IO_COMBINE_LIMIT);
int strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
uint32 max_possible_buffer_limit = GetPinLimit();
/* Identify the minimum of the above */
max_write_batch_size = Min(strategy_pin_limit, max_write_batch_size);
max_write_batch_size = Min(max_possible_buffer_limit, max_write_batch_size);
/* Must allow at least 1 IO for forward progress */
max_write_batch_size = Max(1, max_write_batch_size);
return max_write_batch_size;
}
Is this better?
- Melanie
