From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-67779-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 18170 invoked by alias); 18 Oct 2009 01:50:39 -0000
Received: (qmail 18106 invoked by uid 22791); 18 Oct 2009 01:50:37 -0000
X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 	tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS
X-Spam-Check-By: sourceware.org
Received: from mail-pz0-f192.google.com (HELO mail-pz0-f192.google.com) (209.85.222.192)     by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 18 Oct 2009 01:50:30 +0000
Received: by pzk30 with SMTP id 30so2728802pzk.24         for <gdb-patches@sourceware.org>; Sat, 17 Oct 2009 18:50:29 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.143.26.39 with SMTP id d39mr186768wfj.223.1255830629073; Sat,  	17 Oct 2009 18:50:29 -0700 (PDT)
In-Reply-To: <4AD761AD.40307@vmware.com>
References: <4AD761AD.40307@vmware.com>
From: Hui Zhu <teawater@gmail.com>
Date: Sun, 18 Oct 2009 01:50:00 -0000
Message-ID: <daef60380910171850m1de9d507v7d85b9360ad5526a@mail.gmail.com>
Subject: Re: [RFA] record.c - save some memory in record log.
To: Michael Snyder <msnyder@vmware.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
X-IsSubscribed: yes
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
X-SW-Source: 2009-10/txt/msg00402.txt.bz2

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 =3D (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). =A0Therefore, 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 =A0Michael Snyder =A0<msnyder@vmware.com>
>
> =A0 =A0 =A0 =A0* record.c (struct record_reg_entry): Replace ptr with uni=
on
> =A0 =A0 =A0 =A0of ptr and buf.
> =A0 =A0 =A0 =A0(struct record_mem_entry): Ditto.
> =A0 =A0 =A0 =A0(record_reg_alloc): Don't alloc ptr if reg will fit into b=
uf.
> =A0 =A0 =A0 =A0(record_mem_alloc): Ditto.
> =A0 =A0 =A0 =A0(record_reg_release): Don't free ptr if reg was stored in =
buf.
> =A0 =A0 =A0 =A0(record_mem_release): Ditto.
> =A0 =A0 =A0 =A0(record_get_loc): New function. =A0Return a pointer to whe=
re the
> =A0 =A0 =A0 =A0value (mem or reg) is to be stored.
> =A0 =A0 =A0 =A0(record_arch_list_add_reg): Call record_get_loc instead of=
 using ptr.
> =A0 =A0 =A0 =A0(record_arch_list_add_mem): Ditto.
> =A0 =A0 =A0 =A0(record_wait): Ditto.
>
> Index: record.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 record.c
> --- record.c =A0 =A015 Oct 2009 17:27:54 -0000 =A0 =A0 =A01.23
> +++ record.c =A0 =A015 Oct 2009 17:48:50 -0000
> @@ -43,12 +43,6 @@
> =A0 =A0Each struct record_entry is linked to "record_list" by "prev" and
> =A0 =A0"next" pointers. =A0*/
>
> -struct record_reg_entry
> -{
> - =A0int num;
> - =A0gdb_byte *val;
> -};
> -
> =A0struct record_mem_entry
> =A0{
> =A0 CORE_ADDR addr;
> @@ -56,7 +50,22 @@ struct record_mem_entry
> =A0 /* Set this flag if target memory for this entry
> =A0 =A0 =A0can no longer be accessed. =A0*/
> =A0 int mem_entry_not_accessible;
> - =A0gdb_byte *val;
> + =A0union
> + =A0{
> + =A0 =A0gdb_byte *ptr;
> + =A0 =A0gdb_byte buf[sizeof (gdb_byte *)];
> + =A0} u;
> +};
> +
> +struct record_reg_entry
> +{
> + =A0unsigned short num;
> + =A0unsigned short len;
> + =A0union
> + =A0{
> + =A0 =A0gdb_byte *ptr;
> + =A0 =A0gdb_byte buf[2 * sizeof (gdb_byte *)];
> + =A0} u;
> =A0};
>
> =A0struct record_end_entry
> @@ -143,7 +152,9 @@ record_reg_alloc (struct regcache *regca
> =A0 rec =3D (struct record_entry *) xcalloc (1, sizeof (struct record_ent=
ry));
> =A0 rec->type =3D record_reg;
> =A0 rec->u.reg.num =3D regnum;
> - =A0rec->u.reg.val =3D (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
> + =A0rec->u.reg.len =3D register_size (gdbarch, regnum);
> + =A0if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
> + =A0 =A0rec->u.reg.u.ptr =3D (gdb_byte *) xmalloc (MAX_REGISTER_SIZE);
>
> =A0 return rec;
> =A0}
> @@ -154,7 +165,8 @@ static inline void
> =A0record_reg_release (struct record_entry *rec)
> =A0{
> =A0 gdb_assert (rec->type =3D=3D record_reg);
> - =A0xfree (rec->u.reg.val);
> + =A0if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
> + =A0 =A0xfree (rec->u.reg.u.ptr);
> =A0 xfree (rec);
> =A0}
>
> @@ -169,7 +181,8 @@ record_mem_alloc (CORE_ADDR addr, int le
> =A0 rec->type =3D record_mem;
> =A0 rec->u.mem.addr =3D addr;
> =A0 rec->u.mem.len =3D len;
> - =A0rec->u.mem.val =3D (gdb_byte *) xmalloc (len);
> + =A0if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
> + =A0 =A0rec->u.mem.u.ptr =3D (gdb_byte *) xmalloc (len);
>
> =A0 return rec;
> =A0}
> @@ -180,7 +193,8 @@ static inline void
> =A0record_mem_release (struct record_entry *rec)
> =A0{
> =A0 gdb_assert (rec->type =3D=3D record_mem);
> - =A0xfree (rec->u.mem.val);
> + =A0if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
> + =A0 =A0xfree (rec->u.mem.u.ptr);
> =A0 xfree (rec);
> =A0}
>
> @@ -329,7 +343,29 @@ record_arch_list_add (struct record_entr
> =A0 =A0 }
> =A0}
>
> -/* Record the value of a register REGNUM to record_arch_list. =A0*/
> +/* Return the value storage location of a record entry. =A0*/
> +static inline gdb_byte *
> +record_get_loc (struct record_entry *rec)
> +{
> + =A0switch (rec->type) {
> + =A0case record_mem:
> + =A0 =A0if (rec->u.mem.len > sizeof (rec->u.mem.u.buf))
> + =A0 =A0 =A0return rec->u.mem.u.ptr;
> + =A0 =A0else
> + =A0 =A0 =A0return rec->u.mem.u.buf;
> + =A0case record_reg:
> + =A0 =A0if (rec->u.reg.len > sizeof (rec->u.reg.u.buf))
> + =A0 =A0 =A0return rec->u.reg.u.ptr;
> + =A0 =A0else
> + =A0 =A0 =A0return rec->u.reg.u.buf;
> + =A0case record_end:
> + =A0default:
> + =A0 =A0gdb_assert (0);
> + =A0 =A0return NULL;
> + =A0}
> +}
> +
> +/* Record the value of a register NUM to record_arch_list. =A0*/
>
> =A0int
> =A0record_arch_list_add_reg (struct regcache *regcache, int regnum)
> @@ -344,7 +380,7 @@ record_arch_list_add_reg (struct regcach
>
> =A0 rec =3D record_reg_alloc (regcache, regnum);
>
> - =A0regcache_raw_read (regcache, regnum, rec->u.reg.val);
> + =A0regcache_raw_read (regcache, regnum, record_get_loc (rec));
>
> =A0 record_arch_list_add (rec);
>
> @@ -370,7 +406,7 @@ record_arch_list_add_mem (CORE_ADDR addr
>
> =A0 rec =3D record_mem_alloc (addr, len);
>
> - =A0if (target_read_memory (addr, rec->u.mem.val, len))
> + =A0if (target_read_memory (addr, record_get_loc (rec), len))
> =A0 =A0 {
> =A0 =A0 =A0 if (record_debug)
> =A0 =A0 =A0 =A0fprintf_unfiltered (gdb_stdlog,
> @@ -857,8 +893,9 @@ record_wait (struct target_ops *ops,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0re=
cord_list->u.reg.num);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0regcache_cooked_read (regcache, record_list->u=
.reg.num, reg);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0regcache_cooked_write (regcache, record_list->=
u.reg.num,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
record_list->u.reg.val);
> - =A0 =A0 =A0 =A0 =A0 =A0 memcpy (record_list->u.reg.val, reg, MAX_REGIST=
ER_SIZE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
record_get_loc (record_list));
> + =A0 =A0 =A0 =A0 =A0 =A0 memcpy (record_get_loc (record_list), reg,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 record_list->u.reg.len);
> =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0else if (record_list->type =3D=3D record_mem)
> =A0 =A0 =A0 =A0 =A0 =A0{
> @@ -892,7 +929,7 @@ record_wait (struct target_ops *ops,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0{
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (target_write_memory (recor=
d_list->u.mem.addr,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0record_list->u.mem.val,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0record_get_loc (record_list),
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 record_list->u.mem.len))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0{
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (execution_directio=
n !=3D EXEC_REVERSE)
> @@ -907,7 +944,7 @@ record_wait (struct target_ops *ops,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0{
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 memcpy (record_list->u.=
mem.val, mem,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 memcpy (record_get_loc =
(record_list), mem,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0record=
_list->u.mem.len);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>
>