[Bug libc/13328] New: proposal of small optimization in tr_freehook() for mtrace functionality

["marcukr at op dot pl" <sourceware-bugzilla at sourceware dot org>]

http://sourceware.org/bugzilla/show_bug.cgi?id=13328

             Bug #: 13328
           Summary: proposal of small optimization in tr_freehook() for
                    mtrace functionality
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: minor
          Priority: P2
         Component: libc
        AssignedTo: drepper.fsp@gmail.com
        ReportedBy: marcukr@op.pl
    Classification: Unclassified


Hi,
I've started to use mtrace functionality to get the summary of
allocations/deallocations done in my system. First I've got into the deadlock
which I found is solved (Bug #6420) in latest libc. Thanks that I had
opportunity to look into libc sources.

Now I would like to propose small optimization in tr_freehook() function.
IMO the hooks should do their job as fast as possible, on the beginning
they block on lock, then call the original malloc/realloc/free...
and unblock the lock when not needed anymore.
I case of tr_freehook() it does the allocation & deallocation twice, but
most of the time they don't need to be called.

The code from latest glibc is like following:

static void tr_freehook (__ptr_t, const __ptr_t) __THROW;
static void
tr_freehook (ptr, caller)
     __ptr_t ptr;
     const __ptr_t caller;
{
  if (ptr == NULL)
    return;

  Dl_info mem;
  Dl_info *info = lock_and_info (caller, &mem);
  tr_where (caller, info);
  /* Be sure to print it first.  */
  fprintf (mallstream, "- %p\n", ptr);

  __libc_lock_unlock (lock);     <==== the unlock
  if (ptr == mallwatch)          <==== I didn't know about mallwatch till
looking into mtrace.c source, therefore in many cases it is just NULL
    tr_break ();
  __libc_lock_lock (lock);       <==== the lock again

  __free_hook = tr_old_free_hook;
  if (tr_old_free_hook != NULL)
    (*tr_old_free_hook) (ptr, caller);
  else
    free (ptr);
  __free_hook = tr_freehook;
  __libc_lock_unlock (lock);
}


My proposal is to do call "internal" __libc_lock_unlock(lock) &
__libc_lock_lock(lock) only inside if (ptr == mallwatch), therefore the code
will look like following:

static void tr_freehook (__ptr_t, const __ptr_t) __THROW;
.....
{
  if (ptr == NULL)
    return;

  Dl_info mem;
  Dl_info *info = lock_and_info (caller, &mem);
  tr_where (caller, info);
  /* Be sure to print it first.  */
  fprintf (mallstream, "- %p\n", ptr);
  if (ptr == mallwatch)
  {
    __libc_lock_unlock (lock);
    tr_break ();
    __libc_lock_lock (lock);
  }
  __free_hook = tr_old_free_hook;
  if (tr_old_free_hook != NULL)
    (*tr_old_free_hook) (ptr, caller);
.......


The advantages:
if ptr != mallwatch the lock is locked/unlocked only once -> the hook will be
executed faster
if ptr == mallwatch & gdb breakpoint is set on tr_break() debugger will stop
with lock unlocked (as before the potential optimization change)

Regards
Mariusz

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.