Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 31, 2016, 21:16 UTC
Message-ID: <xmqqd1igrzhj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD3o89EzQLLsMtLWxBhKCG4RmbG6u+UrLwzyAAEyezv1cA@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 18 quoted lines

> On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> trailer.c currently splits lines while processing a buffer (and also
>> rejoins lines when needing to invoke ignore_non_trailer).
>>
>> Avoid such line splitting, except when generating the strings
>> corresponding to trailers (for ease of use by clients - a subsequent
>> patch will allow other components to obtain the layout of a trailer
>> block in a buffer, including the trailers themselves). The main purpose
>> of this is to make it easy to return pointers into the original buffer
>> (for a subsequent patch), but this also significantly reduces the number
>> of memory allocations required.
>>
>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>> ---
>>  trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 116 insertions(+), 99 deletions(-)
>
> IMHO it is telling that this needs 17 more lines.

Given that among which 13 are additional comments to explain what is going on, I think this is a surprisingly good outcome, relative to what the patch achieves (reducing a lot of split/rejoin operations and need for allocation associated with them that are all made unnecessary).

I agree with you that it is telling to see that this needs only 17 more lines (or 4, if you only count the true code additions) and clearly shows that strbuf_split() helper function was a not good match for the codepath touched by this patch. The helper function is very tempting in that with a simple single call it will give us tons of strbufs in an array, each of which are _MUCH_ more flexible than simple strings if we ever need to manipulate them by trimming, splicing and inserting etc. This patch shows us that if we do not need the flexibility, doing strbuf_split() as the first thing on the input and having to deal with an array of strbuf is an overall loss, I would think.

Although I admit that I carefully read only up to [2/4] I am fairly happy with this series. It finally fixes the second of the two issues I pointed out in the review of the original series (the other one was fixed in the series that this topic depends on), paying down technical debt.

Show 10 quoted lines

>> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
>>  {
>>         LIST_HEAD(head);
>>         LIST_HEAD(arg_head);
>> -       struct strbuf **lines;
>> +       struct strbuf sb = STRBUF_INIT;
>
> We often use "sb" as the name of strbuf variables, but I think at
> least here (and maybe in other places above) we could use something a
> bit more telling, like "input_buf" perhaps.

That sounds sensible.

>
>>         int trailer_end;
>>         FILE *outfile = stdout;
>>