Re: [PATCH 2/3] strbuf.h API users: don't hardcode 8192, use STRBUF_HINT_SIZE

Junio C Hamano [off-list ref] writes:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

Change a couple of users of strbuf_init() that pass a hint of 8192 to
pass STRBUF_HINT_SIZE instead.

Both of these hardcoded occurrences pre-date the use of the strbuf
API. See 5242bcbb638 (Use strbuf API in cache-tree.c, 2007-09-06) and
af6eb82262e (Use strbuf API in apply, blame, commit-tree and diff,
2007-09-06).

In both cases the exact choice of 8192 is rather arbitrary, e.g. for
commit buffers I think 1024 or 2048 would probably be a better
default (this commit message is getting this commit close to the
former, but I daresay it's already way above the average for git
commits).

Yes, they are arbitrary within the context of these callers.

I do not think using STRBUF_HINT_SIZE macro in them is the right
thing to do at all, as there is no reason to think that the best
value for the write chunk sizes in these codepath has any linkage to
the best value for the read chunk sizes used by strbuf_read() at
all.  When benchmarking reveals that the best default size for
strbuf_read() is 16k, you'd update STRBUF_HINT_SIZE to 16k, but how
do you tell that it also happens to be the best write buffer size
for the cache-tree writeout codepath (answer: you don't)?

Having said all that, I wouldn't be so opposed to an approach that

 - declares that we need only one "default I/O buffer size";

 - declares that the appropriate size for it is 8192;

 - #define DEFAULT_IO_SIZE 8192;

 - does something like your [PATCH 1/3], but not limited to strbuf
   API, and

 - covers also the writeout codepath of cache-tree, etc. that uses
   hardcoded I/O buffer size.

The biggest trouble I had with the posted patches, especially the
[PATCH 2/3], was that I am quite sure that you wouldn't have used
STRBUF_HINT_SIZE in the cache-tree writeout codepath or commit-tree
codepath if they didn't use strbuf as a convenient way to get an
elastic buffer.  The more relevant commonality across codepaths that
use 8192 is that the constant is used for sizing the I/O buffer, and
I got an impression that the 3-patch series posted did an incomplete
job of touching some that happen to use strbuf.

An approach that concentrated on the "right" commonality, i.e. we
have hardcoded magic constants for I/O buffer sizing, would have
covered copy.c, diff-delta.c, http-backend.c etc. that do not use
strbuf API where they have hardcoded 8k constants.

Thanks.