Re: [PATCH v2] Optimize memory_xfer_partial for remote

[Pedro Alves <palves at redhat dot com>]

On 06/24/2016 10:21 PM, Don Breazeal wrote:
> 
> and with commit hash: 67c059c29e1fb0cdeacdd2005f955514d8d1fb34
> 

Write:

 ... with commit 67c059c29e1f ("Improve performance of large restore
 commands") ...

so the reader has a clue what the commit is about without having
to check.


> gdb/ChangeLog:
> 2016-06-24  Don Breazeal  <donb@codesourcery.com>
> 
> 	* remote.c (remote_get_memory_xfer_limit): New function.
> 	* target-delegates.c (delegate_get_memory_xfer_limit,
> 	debug_get_memory_xfer_limit, install_delegators,
> 	install_dummy_methods, init_debug_target): New functions
> 	and target_ops initialization from regenerating the file.

The standard practice is to just say:

 	* target-delegates.c: Regenerate.

> 	* target.c (default_get_memory_xfer_limit): New function and
> 	forward declaration.
> 	(memory_xfer_partial): Call target_ops.to_get_memory_xfer_limit.
> 	* target.h (struct target_ops)<to_get_memory_xfer_limit>: New
> 	member.

Space between ")<".

> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 501f3c6..03c7ab7 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10160,6 +10160,12 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
>    return TARGET_XFER_OK;
>  }
>  
> +static ULONGEST
> +remote_get_memory_xfer_limit (struct target_ops *ops)

Intro comment, something like "Implementation of ... method.".

>  
> +/* The default implementation for the to_get_memory_xfer_limit method.
> +   The hard-coded limit here was determined to be a reasonable default
> +   that eliminated exponential slowdown on very large transfers without
> +   unduly compromising performance on smaller transfers.  */

Where's this coming from?  Is this new experimentation you did,
or are you talking about Anton's patch?

> @@ -1301,8 +1314,9 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
>  	 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);
> +	 subset of it.  Cap writes to a limit specified by the target
> +	 to mitigate this.  */
> +      len = min (ops->to_get_memory_xfer_limit (ops), len);
>  

Does this still work if remote is not the top-most target?

E.g., what happens if you do "record" to push a record_statum
target on top?  Do we get the 4KB default limit, or the
remote limit?

Thanks,
Pedro Alves