[PATCH] Improve performance of large restore commands

[PATCH] Improve performance of large restore commands

[Anton Blanchard]

I noticed a large (100MB) restore took hours to complete. The problem
is target_xfer_partial repeatedly mallocs and memcpys the entire
100MB buffer only to find a small portion of it is actually written.

We already cap reads to 4K, so do that for writes. The testcase that
originally took hours now takes 50 seconds.

--

2013-07-25  Anton Blanchard  <anton@samba.org>

	* target.c (target_write_with_progress): Cap write to 4K

Index: b/gdb/target.c
===================================================================
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2287,9 +2287,11 @@ target_write_with_progress (struct targe
 
   while (xfered < len)
     {
+      /* Cap the write to 4K */
+      int to_transfer = min(4096, len - xfered);
       LONGEST xfer = target_write_partial (ops, object, annex,
 					   (gdb_byte *) buf + xfered,
-					   offset + xfered, len - xfered);
+					   offset + xfered, to_transfer);
 
       if (xfer == 0)
 	return xfered;

Re: [PATCH] Improve performance of large restore commands

[Luis Machado]

On 07/25/2013 09:08 AM, Anton Blanchard wrote:
>
> I noticed a large (100MB) restore took hours to complete. The problem
> is target_xfer_partial repeatedly mallocs and memcpys the entire
> 100MB buffer only to find a small portion of it is actually written.
>
> We already cap reads to 4K, so do that for writes. The testcase that
> originally took hours now takes 50 seconds.
>
> --
>
> 2013-07-25  Anton Blanchard  <anton@samba.org>
>
> 	* target.c (target_write_with_progress): Cap write to 4K
>
> Index: b/gdb/target.c
> ===================================================================
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2287,9 +2287,11 @@ target_write_with_progress (struct targe
>
>     while (xfered < len)
>       {
> +      /* Cap the write to 4K */
> +      int to_transfer = min(4096, len - xfered);
>         LONGEST xfer = target_write_partial (ops, object, annex,
>   					   (gdb_byte *) buf + xfered,
> -					   offset + xfered, len - xfered);
> +					   offset + xfered, to_transfer);
>
>         if (xfer == 0)
>   	return xfered;
>
>

Looks good to me.

Did you consider extending the comment a little to explain what will 
happen if we don't cap the write to 4K?

Thanks,
Luis

Re: [PATCH] Improve performance of large restore commands

[Pedro Alves]

On 07/25/2013 01:08 PM, Anton Blanchard wrote:
> 
> I noticed a large (100MB) restore took hours to complete. The problem
> is target_xfer_partial repeatedly mallocs and memcpys the entire
> 100MB buffer only to find a small portion of it is actually written.

I think you meant memory_xfer_partial, in the breakpoint shadow
handling, right?  I'd prefer pushing the capping close to the offending
malloc/memcpy.

We could conceivably change that shadowing algorithm to not malloc
at all.  E.g., say, with a memory block like

  |------B------|
start          end

with B being the address where a breakpoint is supposed to be planted,
write block [start,B), then a write for the breakpoint instruction
at B, then another block write for (B,e).

Or, we could throttle the requested window width up/down depending
on the buffer size returned at each partial transfer.

I'm not actually suggesting doing this, only explaining why I'd
rather put the cap close to the problem it solves.  target_write_partial
is used for other targets objects too, not just memory.

> We already cap reads to 4K

Where exactly?  In the target backend, perhaps?  I'm not finding a
cap at the target.c level.

> --
> 
> 2013-07-25  Anton Blanchard  <anton@samba.org>
> 
> 	* target.c (target_write_with_progress): Cap write to 4K

Period at end of sentence.

> 
> Index: b/gdb/target.c
> ===================================================================
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2287,9 +2287,11 @@ target_write_with_progress (struct targe
>  
>    while (xfered < len)
>      {
> +      /* Cap the write to 4K */
> +      int to_transfer = min(4096, len - xfered);
>        LONGEST xfer = target_write_partial (ops, object, annex,

Empty line after last declaration.  Missing space before parens.

>  					   (gdb_byte *) buf + xfered,
> -					   offset + xfered, len - xfered);
> +					   offset + xfered, to_transfer);
>  
>        if (xfer == 0)
>  	return xfered;
> 

-- 
Pedro Alves

Re: [PATCH] Improve performance of large restore commands

[Anton Blanchard]

Hi Pedro,

Thanks for the review.

> > I noticed a large (100MB) restore took hours to complete. The
> > problem is target_xfer_partial repeatedly mallocs and memcpys the
> > entire 100MB buffer only to find a small portion of it is actually
> > written.
> 
> I think you meant memory_xfer_partial, in the breakpoint shadow
> handling, right?  I'd prefer pushing the capping close to the
> offending malloc/memcpy.

I wrote the summary after looking at the perf profile data. It is
indeed in memory_xfer_partial and to confirm I reran with a gdb
built with -fno-inline:

    memcpy |
                     --- memcpy
                         target_xfer_partial
                         target_write_with_progress
                         target_write_memory
                         restore_command

I've moved the check closer and also added a better comment as Luis
suggested. Updated patch below.

> We could conceivably change that shadowing algorithm to not malloc
> at all.  E.g., say, with a memory block like
> 
>   |------B------|
> start          end
> 
> with B being the address where a breakpoint is supposed to be planted,
> write block [start,B), then a write for the breakpoint instruction
> at B, then another block write for (B,e).
> 
> Or, we could throttle the requested window width up/down depending
> on the buffer size returned at each partial transfer.
> 
> I'm not actually suggesting doing this, only explaining why I'd
> rather put the cap close to the problem it solves.
> target_write_partial is used for other targets objects too, not just
> memory.

Yes, that definitely makes sense.

> > We already cap reads to 4K
> 
> Where exactly?  In the target backend, perhaps?  I'm not finding a
> cap at the target.c level.

I've removed this comment. I was looking at target_fileio_read_alloc_1
but it actually starts at 4K and doubles in size as it grows.

Anton
--

[PATCH] Improve performance of large restore commands

I noticed a large (100MB) restore took hours to complete. The problem
is memory_xfer_partial repeatedly mallocs and memcpys the entire
100MB buffer for breakpoint shadow handling only to find a small portion
of it is actually written.

The testcase that originally took hours now takes 50 seconds.

--

2013-07-29  Anton Blanchard  <anton@samba.org>

	* target.c (memory_xfer_partial): Cap write to 4K

Index: b/gdb/target.c
===================================================================
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1669,6 +1669,13 @@ memory_xfer_partial (struct target_ops *
       void *buf;
       struct cleanup *old_chain;
 
+      /* A large write request is likely to be partially satisfied
+	 by memory_xfer_partial_1. We will continually malloc
+	 and free a copy of the entire write request for breakpoint
+	 shadow handling even though we only end up writing a small
+	 subset of it. Cap writes to 4K to mitigate this.  */
+      len = min(4096, len);
+
       buf = xmalloc (len);
       old_chain = make_cleanup (xfree, buf);
       memcpy (buf, writebuf, len);

Re: [PATCH] Improve performance of large restore commands

[Pedro Alves]

On 07/29/2013 06:44 AM, Anton Blanchard wrote:

> 2013-07-29  Anton Blanchard  <anton@samba.org>
> 
> 	* target.c (memory_xfer_partial): Cap write to 4K

Period at end of sentence.  Might as well write KB :-).

> 
> Index: b/gdb/target.c
> ===================================================================
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1669,6 +1669,13 @@ memory_xfer_partial (struct target_ops *
>        void *buf;
>        struct cleanup *old_chain;
>  
> +      /* A large write request is likely to be partially satisfied
> +	 by memory_xfer_partial_1. We will continually malloc
> +	 and free a copy of the entire write request for breakpoint
> +	 shadow handling even though we only end up writing a small
> +	 subset of it. Cap writes to 4K to mitigate this.  */

Double space after all periods, not just the last.

> +      len = min(4096, len);

Space before parens:

     len = min (4096, len);

Otherwise OK.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Improve performance of large restore commands

[Anton Blanchard]

Hi Pedro,

Thanks for the comments!

Anton
--

I noticed a large (100MB) restore took hours to complete. The problem
is memory_xfer_partial repeatedly mallocs and memcpys the entire
100MB buffer for breakpoint shadow handling only to find a small portion
of it is actually written.

The testcase that originally took hours now takes 50 seconds.

--

2013-07-29  Anton Blanchard  <anton@samba.org>

	* target.c (memory_xfer_partial): Cap write to 4kB.

Index: b/gdb/target.c
===================================================================
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1669,6 +1669,13 @@ memory_xfer_partial (struct target_ops *
       void *buf;
       struct cleanup *old_chain;
 
+      /* A large write request is likely to be partially satisfied
+	 by memory_xfer_partial_1.  We will continually malloc
+	 and free a copy of the entire write request for breakpoint
+	 shadow handling even though we only end up writing a small
+	 subset of it.  Cap writes to 4kB to mitigate this.  */
+      len = min (4096, len);
+
       buf = xmalloc (len);
       old_chain = make_cleanup (xfree, buf);
       memcpy (buf, writebuf, len);

Re: [PATCH] Improve performance of large restore commands

[Pedro Alves]

Thanks.

On 07/30/2013 12:24 AM, Anton Blanchard wrote:
>On 07/29/2013 03:13 PM, Pedro Alves wrote:
>>> > 	* target.c (memory_xfer_partial): Cap write to 4K
>> Period at end of sentence.  Might as well write KB :-).

FAOD, this is still OK, but,

> 2013-07-29  Anton Blanchard  <anton@samba.org>
> 
> 	* target.c (memory_xfer_partial): Cap write to 4kB.
...
> +	 subset of it.  Cap writes to 4kB to mitigate this.  */
...

do write upper K, not k.  Lowercase k usually indicates decimal
10^3.  (see e.g.,
http://en.wikipedia.org/wiki/Template:Bit_and_byte_prefixes).

-- 
Pedro Alves

Re: [PATCH] Improve performance of large restore commands

[Anton Blanchard]

Hi Pedro,

> > 	* target.c (memory_xfer_partial): Cap write to 4kB.
> ...
> > +	 subset of it.  Cap writes to 4kB to mitigate this.  */
> ...
> 
> do write upper K, not k.  Lowercase k usually indicates decimal
> 10^3.  (see e.g.,
> http://en.wikipedia.org/wiki/Template:Bit_and_byte_prefixes).

Ahh sorry, I did that without thinking. Updated patch below.

Anton
--

I noticed a large (100MB) restore took hours to complete. The problem
is memory_xfer_partial repeatedly mallocs and memcpys the entire
100MB buffer for breakpoint shadow handling only to find a small portion
of it is actually written.

The testcase that originally took hours now takes 50 seconds.

--

2013-07-29  Anton Blanchard  <anton@samba.org>

	* target.c (memory_xfer_partial): Cap write to 4KB.

Index: b/gdb/target.c
===================================================================
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1669,6 +1669,13 @@ memory_xfer_partial (struct target_ops *
       void *buf;
       struct cleanup *old_chain;
 
+      /* A large write request is likely to be partially satisfied
+	 by memory_xfer_partial_1.  We will continually malloc
+	 and free a copy of the entire write request for breakpoint
+	 shadow handling even though we only end up writing a small
+	 subset of it.  Cap writes to 4KB to mitigate this.  */
+      len = min (4096, len);
+
       buf = xmalloc (len);
       old_chain = make_cleanup (xfree, buf);
       memcpy (buf, writebuf, len);

Re: [PATCH] Improve performance of large restore commands

[Pedro Alves]

OK.

Thanks,
-- 
Pedro Alves