Re: [patch] Cleanup: don't call free(NULL) unnecessarily

[OndÅej BÃlka <neleai at seznam dot cz>]

On Mon, Mar 11, 2013 at 05:02:39PM -0400, Carlos O'Donell wrote:
> On 03/11/2013 12:55 PM, Roland McGrath wrote:
> > Since free (NULL) is thoroughly well-specified as a no-op, it never
> > qualifies as a "cleanup" to add code to avoid it.  If anything, it's an
> > optimization.  Like all claims of optimization, it needs exploration of
> > the trade-offs involved and justification for the change.
> 
> The use of the word "cleanup" depends on the scope with which you consider
> the statement in question. In this case we have logically identical code
> already checking for free (NULL) and avoiding it.

A rationale was following:
On 03/05/2013 02:18 PM, Paul Pluzhnikov wrote:
> Greetings,
>
> This patch eliminates calls to free(NULL) from within vfprintf.
>
> This is desirable because e.g. fprintf(stderr, ...) is often called from
> signal handlers, and while technically illegal, this tends to work.
>
> Glibc free(NULL) also works in signal context, but end user could supply
> his own malloc/free, which may not.
Which is definitely not optimization. 

On Mon, Mar 11, 2013 at 05:02:39PM -0400, Carlos O'Donell wrote:
> 
> Though one might argue we should remove all free (NULL) checks to reduce
> I-cache pressure for the fast-path.
> 
See below.

> > It was past policy never to add tests before free calls.  The rationale
> > for that is that free (NULL) is cheap enough as a no-op and the cases
> > where it would actually be called are not in hot paths.  
> 
> What I need to know as a maintainer is the probability of free (NULL)
> being called in the "average use of the library" because this is
> the weighting that I will use to determine if the trade off of between
> two sequences of code.
> 
> How do I determine that?
> 
> Do I stick a systemtap probe into vfprint, run the testsuite and look
> at the results?
> 
> I'd like a deterministic way of figuring this out.
> 
> I doubt any human can look at the glibc code base and easily point out
> what is or is not a hot-path in some of the more complex functions.
>
In this case it is quite easy to figure out. Corresponding fragments
follow malloca pattern: 

  /* Allocate memory for all three argument arrays.  */
    if (__libc_use_alloca (nargs * bytes_per_arg))
        args_value = alloca (nargs * bytes_per_arg);
    else
      {
        args_value = args_malloced = malloc (nargs * bytes_per_arg);
        if (args_value == NULL)
          {
            done = -1;
            goto all_done;
          }
      }

and

            size_t needed = ((size_t) width + 32) * sizeof (CHAR_T);
            if (__libc_use_alloca (needed))
              workend = (CHAR_T *) alloca (needed) + width + 32;
            else
              {
                workstart = (CHAR_T *) malloc (needed);
                if (workstart == NULL)
                  {
                    done = -1;
                    goto all_done;
                  }
                workend = workstart + width + 32;
              }

Unless printf overestimates sizes by order of magnitude probability that
it allocates more than 65535 is practicaly zero.

Testing null is profitable here, test on x64 is 4 bytes long. Even with
pessimistic estimate of 1 cycle/byte lost you would need free (NULL) take 4 bytes.

When handoptimized this saves typicaly 4 bytes (test ptr;jmp vs mov
ptr,rsi; call free) as I can move call to cache line that is not
accessed.

> > If they're not
> > hot paths, then there is no justification for an optimization change.
> > In fact, adding any instructions at all is should always be presumed to
> > be a pessimization due to general I-cache pressure.  So adding a test
> > must be justified on a specific performance-analysis basis as an
> > optimization, and weighed against the maintenance burden of making the
> > source code larger and more complex.
> 
> That makes sense in general.
> 
> Cheers,
> Carlos.