Re: [RFA] record.c - save some memory in record log.

[Hui Zhu <teawater@gmail.com>]

Thanks Michael, This patch is very cool.

+struct record_reg_entry
+{
+  unsigned short num;
+  unsigned short len;
+  union
+  {
+    gdb_byte *ptr;
+    gdb_byte buf[2 * sizeof (gdb_byte *)];

Why this part use "2 * sizeof (gdb_byte *)"?

+    rec->u.reg.u.ptr = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
I suggest change MAX_REGISTER_SIZE to rec->u.reg.len.


Thanks,
Hui

On Fri, Oct 16, 2009 at 01:53, Michael Snyder <msnyder@vmware.com> wrote:
> This change saves approximately 25 percent of memory used for
> the record log in process record (my empirical measurement).
>
> It's based on the observation that the objects that we save
> (registers and small memory writes) are frequently the size of
> a pointer (or smaller).  Therefore, instead of allocating both
> a pointer and some malloc memory to hold them, they can simply
> be saved in the space used for the pointer itself.
>
> Saves of larger objects (bigger memory saves, or unusually
> large registers) will still fall back to the old method.
>
> This should also represent a huge saving in heap fragmentation,
> since the vast majority of the mallocs would have been 4 bytes
> or 8 bytes.
>
>
> 2009-10-15  Michael Snyder  <msnyder@vmware.com>
>
>        * record.c (struct record_reg_entry): Replace ptr with union
>        of ptr and buf.
>        (struct record_mem_entry): Ditto.
>        (record_reg_alloc): Don't alloc ptr if reg will fit into buf.
>        (record_mem_alloc): Ditto.
>        (record_reg_release): Don't free ptr if reg was stored in buf.
>        (record_mem_release): Ditto.
>        (record_get_loc): New function.  Return a pointer to where the
>        value (mem or reg) is to be stored.
>        (record_arch_list_add_reg): Call record_get_loc instead of using ptr.
>        (record_arch_list_add_mem): Ditto.
>        (record_wait): Ditto.
>
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 record.c
> --- record.c    15 Oct 2009 17:27:54 -0000      1.23
> +++ record.c    15 Oct 2009 17:48:50 -0000
> @@ -43,12 +43,6 @@
>    Each struct record_entry is linked to "record_list" by "prev" and
>    "next" pointers.  */
>
> -struct record_reg_entry
> -{
> -  int num;
> -  gdb_byte *val;
> -};
> -
>  struct record_mem_entry
>  {
>   CORE_ADDR addr;
> @@ -56,7 +50,22 @@ struct record_mem_entry
>   /* Set this flag if target memory for this entry
>      can no longer be accessed.  */
>   int mem_entry_not_accessible;
> -  gdb_byte *val;
> +  union
> +  {
> +    gdb_byte *ptr;
> +    gdb_byte buf[sizeof (gdb_byte *)];
> +  } u;
> +};
> +
> +struct record_reg_entry
> +{
> +  unsigned short num;
> +  unsigned short len;
> +  union
> +  {
> +    gdb_byte *ptr;
> +    gdb_byte buf[2 * sizeof (gdb_byte *)];
> +  } u;
>  };
>
>  struct record_end_entry
> @@ -143,7 +152,9 @@ record_reg_alloc (struct regcache *regca
>   rec = (struct record_entry *) xcalloc (1, sizeof (struct record_entry));
>   rec->type = record_reg;
>   rec->u.reg.num = regnum;
> -  rec->u.reg.val = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
> +  rec->u.reg.len = register_size (gdbarch, regnum);
> +  if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
> +    rec->u.reg.u.ptr = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
>
>   return rec;
>  }
> @@ -154,7 +165,8 @@ static inline void
>  record_reg_release (struct record_entry *rec)
>  {
>   gdb_assert (rec->type == record_reg);
> -  xfree (rec->u.reg.val);
> +  if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
> +    xfree (rec->u.reg.u.ptr);
>   xfree (rec);
>  }
>
> @@ -169,7 +181,8 @@ record_mem_alloc (CORE_ADDR addr, int le
>   rec->type = record_mem;
>   rec->u.mem.addr = addr;
>   rec->u.mem.len = len;
> -  rec->u.mem.val = (gdb_byte *) xmalloc (len);
> +  if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
> +    rec->u.mem.u.ptr = (gdb_byte *) xmalloc (len);
>
>   return rec;
>  }
> @@ -180,7 +193,8 @@ static inline void
>  record_mem_release (struct record_entry *rec)
>  {
>   gdb_assert (rec->type == record_mem);
> -  xfree (rec->u.mem.val);
> +  if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
> +    xfree (rec->u.mem.u.ptr);
>   xfree (rec);
>  }
>
> @@ -329,7 +343,29 @@ record_arch_list_add (struct record_entr
>     }
>  }
>
> -/* Record the value of a register REGNUM to record_arch_list.  */
> +/* Return the value storage location of a record entry.  */
> +static inline gdb_byte *
> +record_get_loc (struct record_entry *rec)
> +{
> +  switch (rec->type) {
> +  case record_mem:
> +    if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
> +      return rec->u.mem.u.ptr;
> +    else
> +      return rec->u.mem.u.buf;
> +  case record_reg:
> +    if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
> +      return rec->u.reg.u.ptr;
> +    else
> +      return rec->u.reg.u.buf;
> +  case record_end:
> +  default:
> +    gdb_assert (0);
> +    return NULL;
> +  }
> +}
> +
> +/* Record the value of a register NUM to record_arch_list.  */
>
>  int
>  record_arch_list_add_reg (struct regcache *regcache, int regnum)
> @@ -344,7 +380,7 @@ record_arch_list_add_reg (struct regcach
>
>   rec = record_reg_alloc (regcache, regnum);
>
> -  regcache_raw_read (regcache, regnum, rec->u.reg.val);
> +  regcache_raw_read (regcache, regnum, record_get_loc (rec));
>
>   record_arch_list_add (rec);
>
> @@ -370,7 +406,7 @@ record_arch_list_add_mem (CORE_ADDR addr
>
>   rec = record_mem_alloc (addr, len);
>
> -  if (target_read_memory (addr, rec->u.mem.val, len))
> +  if (target_read_memory (addr, record_get_loc (rec), len))
>     {
>       if (record_debug)
>        fprintf_unfiltered (gdb_stdlog,
> @@ -857,8 +893,9 @@ record_wait (struct target_ops *ops,
>                                    record_list->u.reg.num);
>              regcache_cooked_read (regcache, record_list->u.reg.num, reg);
>              regcache_cooked_write (regcache, record_list->u.reg.num,
> -                                    record_list->u.reg.val);
> -             memcpy (record_list->u.reg.val, reg, MAX_REGISTER_SIZE);
> +                                    record_get_loc (record_list));
> +             memcpy (record_get_loc (record_list), reg,
> +                     record_list->u.reg.len);
>            }
>          else if (record_list->type == record_mem)
>            {
> @@ -892,7 +929,7 @@ record_wait (struct target_ops *ops,
>                  else
>                    {
>                      if (target_write_memory (record_list->u.mem.addr,
> -                                              record_list->u.mem.val,
> +                                              record_get_loc (record_list),
>                                               record_list->u.mem.len))
>                        {
>                          if (execution_direction != EXEC_REVERSE)
> @@ -907,7 +944,7 @@ record_wait (struct target_ops *ops,
>                        }
>                      else
>                        {
> -                         memcpy (record_list->u.mem.val, mem,
> +                         memcpy (record_get_loc (record_list), mem,
>                                  record_list->u.mem.len);
>                        }
>                    }
>
>