Re: Checkpointer write combining

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 is just 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 the current 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