Re: [PATCH 4/6] strbuf: add an optimized 1-character strbuf_grow

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 6, 2015, 02:13 UTC
Message-ID: <CAPig+cTK5MJLfwETFfM1Zdw35M2geKKmA6Q9M0kHdMP9t8qLNg@mail.gmail.com>
In-Reply-To: <20150405011127.GD30127@peff.net>

On Sat, Apr 4, 2015 at 9:11 PM, Jeff King <peff@peff.net> wrote:

Show 31 quoted lines

> We have to call strbuf_grow anytime we are going to add data
> to a strbuf. In most cases, it's a noop (since we grow the
> buffer aggressively), and the cost of the function call and
> size check is dwarfed by the actual buffer operation.
>
> For a tight loop of single-character additions, though, this
> overhead is noticeable. Furthermore, the single-character
> case is much easier to check; since the "extra" parameter is
> 1, we can do it without worrying about overflow.
>
> This patch adds a simple inline function for checking
> single-character growth. For the growth case, it just calls
> into the regular strbuf_grow(). This is redundant, as
> strbuf_grow will check again whether we need to grow. But it
> keeps our inline code simple, and most calls will not need
> to grow, so it's OK to treat this as a rare "slow path".
>
> We apply the new function to strbuf_getwholeline. [...]
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/strbuf.c b/strbuf.c
> index af2bad4..2facd5f 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -445,7 +445,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
>         strbuf_reset(sb);
>         flockfile(fp);
>         while ((ch = getc_unlocked(fp)) != EOF) {
> -               strbuf_grow(sb, 1);
> +               strbuf_grow_ch(sb);

strbuf_grow_ch() seems overly special-case. What about instead taking advantage of inline strbuf_avail() to do something like this?

    if (!strbuf_avail())
        strbuf_grow(sb, 1);

(Minor tangent: The 1 is still slightly magical and potentially confusing for someone who doesn't know that the buffer is grown aggressively, so changing it to a larger number might make it more obvious to the casual reader that the buffer is in fact not being grown on every iteration.)

Show 25 quoted lines

>                 sb->buf[sb->len++] = ch;
>                 if (ch == term)
>                         break;
> diff --git a/strbuf.h b/strbuf.h
> index 1883494..ef41151 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -137,6 +137,15 @@ static inline size_t strbuf_avail(const struct strbuf *sb)
>   */
>  extern void strbuf_grow(struct strbuf *, size_t);
>
> +/*
> + * An optimized version of strbuf_grow() for a single character.
> + */
> +static inline void strbuf_grow_ch(struct strbuf *sb)
> +{
> +       if (!sb->alloc || sb->alloc - 1 <= sb->len)
> +               strbuf_grow(sb, 1);
> +}
> +
>  /**
>   * Set the length of the buffer to a given value. This function does *not*
>   * allocate new memory, so you should not perform a `strbuf_setlen()` to a
> --
> 2.4.0.rc0.363.gf9f328b