Re: [PATCH] fork-child.c: Avoid unnecessary heap-allocation / string copying (Re: [PATCH v5 3/5] C++-fy and prepare for sharing fork_inferior)

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Wednesday, April 12 2017, Pedro Alves wrote:

> On 04/12/2017 06:04 AM, Sergio Durigan Junior wrote:
>
>>>>>  static void
>>>>> -breakup_args (char *scratch, char **argv)
>>>>> +breakup_args (const std::string &scratch, std::vector<char *> &argv)
>>>>>  {
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +      std::string arg = scratch.substr (cur_pos, next_sep - cur_pos);
>>>>> +
>>>>
>>>> This creates a temporary string (heap allocates) ...
>>>>
>>>>> +      argv.push_back (xstrdup (arg.c_str ()));
>>>>
>>>> ... and here you create yet another copy.
>>>>
>>>> You should be able to avoid it by using e.g., savestring:
>>>>
>>>>     char *arg = savestring (scratch.c_str () + cur_pos, next_sep - cur_pos);
>>>>     argv.push_back (arg);
>>>
>>> Fair enough.  I had my mind on "C++-only mode" when writing this code.
>
> Yup, in C++, it's good to keep unnecessary temporaries
> and hidden heap allocations in mind.  Actually, now that I look a bit
> deeper, I think we can avoid a "premature pessimization" here, keeping
> the same level of clarity.
>
> I think it'd be good to push this patch below.  WDYT?

Hm, I don't know.  I mean, I understand where you're coming from and
what you're trying to achieve, but somehow I thought the old code
(especially the part that modified the argument string in place, using
\0's as separators) was somewhat hacky.  Even though the new code uses
more memory, it it also more readable, at least IMNSHO.  And also more
const-correct, since we can make breakup_args_for_exec receive a const
as the first argument.

But I liked a few bits of your patch.  See below.

> From 64d0035762344ed33858a3b8930a83e7071ea4a8 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 12 Apr 2017 10:55:36 +0100
> Subject: [PATCH] fork-child.c: Avoid unnecessary heap-allocation / string
>  copying
>
> The previous patch converted the argv building from an
> alloca-allocated array of non-owning arg pointers, to a std::vector of
> owning pointers, which results in N string dups, with N being the
> number of arguments in the vector, and then requires manually
> releasing the pointers owned by the vector.
>
> This patch makes the vector hold non-owning pointers, and avoids the
> string dups, by doing one single string copy of the arguments upfront,
> and replacing separators with NULL terminators in place, like we used
> to.
>
> With this, there's no need to remember to call free_vector_argv either.
>
> Tested on x86_64 Fedora 23.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* fork-child.c (breakup_args): Make 'scratch' non-const.
> 	Replace separators with NULL terminators in place.  Change
> 	type of vector.  (fork_inferior): The argument vector now
> 	holds non-owning pointers.  Don't strdup strings into the
> 	vector.  Remove free_vector_argv call.
> ---
>  gdb/fork-child.c | 57 ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/gdb/fork-child.c b/gdb/fork-child.c
> index 6b7386e..8a6f9e6 100644
> --- a/gdb/fork-child.c
> +++ b/gdb/fork-child.c
> @@ -46,10 +46,12 @@ static char *exec_wrapper;
>  /* Break up SCRATCH into an argument vector suitable for passing to
>     execvp and store it in ARGV.  E.g., on "run a b c d" this routine
>     would get as input the string "a b c d", and as output it would
> -   fill in ARGV with the four arguments "a", "b", "c", "d".  */
> +   fill in ARGV with the four arguments "a", "b", "c", "d".  The
> +   pointers pushed to ARGV point directly into SCRATCH, which is
> +   modified in place with the necessary NULL terminators.  */
>  
>  static void
> -breakup_args (const std::string &scratch, std::vector<char *> &argv)
> +breakup_args (std::string &scratch, std::vector<const char *> &argv)
>  {
>    for (size_t cur_pos = 0; cur_pos < scratch.size ();)
>      {
> @@ -62,13 +64,19 @@ breakup_args (const std::string &scratch, std::vector<char *> &argv)
>        /* Find the position of the next separator.  */
>        std::size_t next_sep = scratch.find_first_of (" \t\n", cur_pos);
>  
> -      /* No separator found, which means this is the last
> -	 argument.  */
>        if (next_sep == std::string::npos)
> -	next_sep = scratch.size ();
> +	{
> +	  /* No separator found, which means this is the last
> +	     argument.  */
> +	  next_sep = scratch.size ();
> +	}

This comment should really be inside the block.  Thanks.

> +      else
> +	{
> +	  /* Replace the separator with a terminator.  */
> +	  scratch[next_sep++] = '\0';
> +	}
>  
> -      char *arg = savestring (scratch.c_str () + cur_pos, next_sep - cur_pos);
> -      argv.push_back (arg);
> +      argv.push_back (&scratch[cur_pos]);
>  
>        cur_pos = next_sep;
>      }
> @@ -155,7 +163,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>    static const char *exec_file;
>    char **save_our_env;
>    int shell = 0;
> -  std::vector<char *> argv;
>    const char *inferior_io_terminal = get_inferior_io_terminal ();
>    struct inferior *inf;
>    int i;
> @@ -183,21 +190,29 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>        shell = 1;
>      }
>  
> +  /* The argument vector.  Holds non-owning pointers.  */
> +  std::vector<const char *> c_argv;
> +
> +  /* These must be live up to the exec call, because the arguments in
> +     C_ARGV point inside them.  */
> +  std::string broken_up_args;
> +  std::string shell_command;
> +
>    if (!shell)
>      {
>        /* We're going to call execvp.  Create argument vector.  */
> -      argv.push_back (xstrdup (exec_file));
> -      breakup_args (allargs, argv);
> +      c_argv.push_back (exec_file);
> +      broken_up_args = allargs;
> +      breakup_args (broken_up_args, c_argv);
>      }
>    else
>      {
>        /* We're going to call a shell.  */
> -      std::string shell_command;
>        const char *p;
>        int need_to_quote;
>        const int escape_bang = escape_bang_in_quoted_argument (shell_file);
>  
> -      shell_command = std::string ("exec ");
> +      shell_command = "exec ";
>  
>        /* Add any exec wrapper.  That may be a program name with arguments, so
>  	 the user must handle quoting.  */
> @@ -265,12 +280,12 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>        /* If we decided above to start up with a shell, we exec the
>  	 shell, "-c" says to interpret the next arg as a shell command
>  	 to execute, and this command is "exec <target-program>
> -	 <args>".  We xstrdup all the strings here because they will
> -	 be free'd later in the code.  */
> -      argv.push_back (xstrdup (shell_file));
> -      argv.push_back (xstrdup ("-c"));
> -      argv.push_back (xstrdup (shell_command.c_str ()));
> -      argv.push_back (NULL);
> +	 <args>".  */
> +      c_argv.reserve (4);

I liked the idea of reserving the exact amount of space needed in the
vector.

> +      c_argv.push_back (shell_file);
> +      c_argv.push_back ("-c");
> +      c_argv.push_back (shell_command.c_str ());
> +      c_argv.push_back (NULL);
>      }
>  
>    /* Retain a copy of our environment variables, since the child will
> @@ -376,6 +391,10 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>           path to find $SHELL.  Rich Pixley says so, and I agree.  */
>        environ = env;
>  
> +      /* It is guaranteed that the exec functions do not modify the
> +	 arguments, but they nevertheless expect "char **".  */
> +      char **argv = const_cast<char **> (&c_argv[0]);

Also, I liked your approach to this.

> +
>        if (exec_fun != NULL)
>          (*exec_fun) (argv[0], &argv[0], env);
>        else
> @@ -393,8 +412,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>        _exit (0177);
>      }
>  
> -  free_vector_argv (argv);
> -
>    /* Restore our environment in case a vforked child clob'd it.  */
>    environ = save_our_env;
>  
> -- 
> 2.5.5

Anyway, long story short: I won't oppose if you want to go ahead and
push this patch, but it's not my preferred approach.

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/