Re: fprintf_ln() is slow

From: Jeff King <peff@peff.net>
Date: Jun 27, 2019, 21:21 UTC
Message-ID: <20190627212154.GB20250@sigill.intra.peff.net>
In-Reply-To: <CACsJy8Cjn2z7TrH9HLQ94Ph7qPZ0fC01J=Lq7GcrhvaCQZaSmg@mail.gmail.com>

On Thu, Jun 27, 2019 at 04:27:07PM +0700, Duy Nguyen wrote:

Show 26 quoted lines

> >  int fprintf_ln(FILE *fp, const char *fmt, ...)
> >  {
> > +       char buf[1024];
> >         int ret;
> >         va_list ap;
> > +
> > +       /* Fast path: format it ourselves and dump it via fwrite. */
> > +       va_start(ap, fmt);
> > +       ret = vsnprintf(buf, sizeof(buf), fmt, ap);
> > +       va_end(ap);
> > +       if (ret < sizeof(buf)) {
> > +               buf[ret++] = '\n';
> > +               if (fwrite(buf, 1, ret, fp) != ret)
> > +                       return -1;
> > +               return ret;
> > +       }
> > +
> > +       /* Slow path: a normal fprintf/putc combo */
> >         va_start(ap, fmt);
> >         ret = vfprintf(fp, fmt, ap);
> >         va_end(ap);
> [...]
> 
> How about do all the formatting in strbuf and only fwrite last minute?
> A bit more overhead with malloc(), so I don't know if it's an
> improvement or not.

We could easily do that by replacing the vfprintf() slow path there with a strbuf, and it would be faster in the normal case for small outputs. The question is whether a malloc() or two write()s is worse.

Probably the malloc() costs less for sane-sized inputs, but would blow up in pathological cases as we ask for a lot of RAM.

I'm not sure it's worth caring that much about such a case. This is, after all, designed to print a single line. I suspect we could put BUG("woah, your line is really big") in the slow-path here and nobody would ever notice. ;)

I'm also not entirely convinced it's worth caring about this problem at all (whether the solution is buffering here, or just adding the "\n" back to the original strings). It's true that p1451 shows off a measurable improvement, but I think it's a silly, pathological case. I'd be surprised if anybody ever noticed the difference in the real world.

The biggest benefit I see to dealing with it is not performance, but that it makes our messages more likely to appear atomically in the output (if multiple entities are writing to stderr).

-Peff