Re: [PATCH] Reduce the amount of WriteFile() syscalls when writing to buffered files

Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com> writes:

> In the meanwhile, apparently, there is an oversight in the core V1 patch
> (3-reduce-syscalls-for-buffered-writes-v1.patch.txt):
>
> If the buffer is not empty, and the caller issues a write with a chunk
> that slightly exceeds the buffer size, for example, 4100 bytes, it would
> result in flushing the buffer _and_ writing the remaining couple of bytes
> with WriteFile(). An appropriate thing to do here would be to flush the
> buffer, but put the few remaining bytes into the buffer, instead of writing
> them to disk and thus making an unnecessary syscall.
>
> With all this in mind, I will prepare the V2 version of the core patch.

I have attached the V2 version of the core patch.

To solve the aforementioned oversight in the V1 patch, I implemented the
optimization in a slightly different manner, by keeping the existing loop
but also handling a condition where we can write the remaining data with
a single WriteFile() call. Apart from this, the V2 patch also includes an
additional test, test_small_and_large_writes_buffered().

Speaking of the discussed idea of adjusting the strategy to avoid a memcpy()
with the write pattern like

    WriteFile(13)
    WriteFile(86267)
    ...

instead of

    WriteFile(4096)
    WriteFile(82185)
    ...

I preferred to keep the latter approach which keeps the minimum size of
the WriteFile() chunk equal to 4096, for the following reasons:

  - My benchmarks do not show that the alternative pattern that also avoids
    a memcpy() results in a quantifiable performance improvement.

  - The existing code had a property that all WriteFile() calls, except
    for maybe the last one, were performed in chunks with sizes that are
    never less than 4096. Although I can't reproduce this in my environment,
    it could be that introducing a possibility of writing in smaller chunks
    would result in the decreased performance with specific hardware, non-
    file handles or in situations when the OS write caching is disabled.
    Therefore, currently, I think that it would be better to keep this
    existing property.

  - Probably, implementing the first approach would result in a bit more
    complex code, as I think that it would require introducing an additional
    if-else code path.

The log message is included in the beginning of the patch file.

Thanks,
Evgeny Kotkov