[2/6] Code cleanup: dwarf2read.c: Eliminate ::file_write

  There's no real need for all this indirection.

gdb/ChangeLog:
2017-06-12  Pedro Alves  <palves@redhat.com>

	* dwarf2read.c (file_write(FILE *, const void *, size_t)): Delete.
	(file_write (FILE *, const std::vector<Elem>&)): Delete.
	(data_buf::file_write): Call ::fwrite directly.
---
 gdb/ChangeLog    |  6 ++++++
 gdb/dwarf2read.c | 22 ++--------------------
 2 files changed, 8 insertions(+), 20 deletions(-)
  

On Mon, 12 Jun 2017 18:14:07 +0200, Pedro Alves wrote:
> There's no real need for all this indirection.

> -    ::file_write (file, m_vec);
> +    if (::fwrite (m_vec.data (), 1, m_vec.size (), file) != m_vec.size ())
> +      error (_("couldn't write data to file"));

Aren't 28 characters with the variable mentioned once easier to write and more
bug-proof than to write each time 115 characters mentioning the same variable
3 times?


Jan
  

On Mon, 12 Jun 2017 18:14:07 +0200, Pedro Alves wrote:
> There's no real need for all this indirection.
...
> -/* Write SIZE bytes from the buffer pointed to by DATA to FILE, with
> -   error checking.  */
> -
> -static void
> -file_write (FILE *file, const void *data, size_t size)
> -{
> -  if (fwrite (data, 1, size, file) != size)
> -    error (_("couldn't data write to file"));
> -}
> -
> -/* Write the contents of VEC to FILE, with error checking.  */
> -
> -template<class Elem>
> -static void
> -file_write (FILE *file, const std::vector<Elem> &vec)
> -{
> -  file_write (file, vec.data (), vec.size() * sizeof (vec[0]));
> -}
> -
>  /* In-memory buffer to prepare data to be written later to a file.  */
>  class data_buf
>  {
> @@ -23252,7 +23233,8 @@ public:
>    /* Write the buffer to FILE.  */
>    void file_write (FILE *file) const
>    {
> -    ::file_write (file, m_vec);
> +    if (::fwrite (m_vec.data (), 1, m_vec.size (), file) != m_vec.size ())
> +      error (_("couldn't write data to file"));
>    }

It is a regression as one needs to multiply vector.size() by sizeof(vector[0]).
Which is also why I separated determining the memory block boundaries from
checking the write success.


Jan
  

On 06/17/2017 06:34 PM, Jan Kratochvil wrote:
> On Mon, 12 Jun 2017 18:14:07 +0200, Pedro Alves wrote:
>> There's no real need for all this indirection.
> 
>> -    ::file_write (file, m_vec);
>> +    if (::fwrite (m_vec.data (), 1, m_vec.size (), file) != m_vec.size ())
>> +      error (_("couldn't write data to file"));
> 
> Aren't 28 characters with the variable mentioned once easier to write and more
> bug-proof than to write each time 115 characters 

That's not a correct comparison, because you're not accounting for all the
characters of the wrapper functions there were removed.  If you count those,
then certainly we end up with fewer characters overall.  It might be different if
the wrapper functions were used in more than one place, but they aren't.

> mentioning the same variable 3 times?

The old code mentioned the same variable 3 times as well:

 -template<class Elem>
 -static void
 -file_write (FILE *file, const std::vector<Elem> &vec)
 -{
 -  file_write (file, vec.data (), vec.size() * sizeof (vec[0]));
                      ^^^          ^^^                  ^^^
 -}


I ran into this because with the byte_vector change, we'd have to
to modify this template's template parameter and prototype.
So the extra indirection ended up getting in the way, and it's
much simpler to just get rid of it.

Thanks,
Pedro Alves
  

On 06/18/2017 07:36 PM, Jan Kratochvil wrote:

>> @@ -23252,7 +23233,8 @@ public:
>>    /* Write the buffer to FILE.  */
>>    void file_write (FILE *file) const
>>    {
>> -    ::file_write (file, m_vec);
>> +    if (::fwrite (m_vec.data (), 1, m_vec.size (), file) != m_vec.size ())
>> +      error (_("couldn't write data to file"));
>>    }
> 
> It is a regression as one needs to multiply vector.size() by sizeof(vector[0]).
> Which is also why I separated determining the memory block boundaries from
> checking the write success.

No it's not.  The fwrite call is no longer in generic code where
the element type is unknown.  Here m_vec is a vector of gdb_byte, and
gdb_byte is guaranteed to have sizeof == 1.

Thanks,
Pedro Alves
  

On Mon, 19 Jun 2017 11:27:46 +0200, Pedro Alves wrote:
> On 06/18/2017 07:36 PM, Jan Kratochvil wrote:
> 
> >> @@ -23252,7 +23233,8 @@ public:
> >>    /* Write the buffer to FILE.  */
> >>    void file_write (FILE *file) const
> >>    {
> >> -    ::file_write (file, m_vec);
> >> +    if (::fwrite (m_vec.data (), 1, m_vec.size (), file) != m_vec.size ())
> >> +      error (_("couldn't write data to file"));
> >>    }
> > 
> > It is a regression as one needs to multiply vector.size() by sizeof(vector[0]).
> > Which is also why I separated determining the memory block boundaries from
> > checking the write success.
> 
> No it's not.  The fwrite call is no longer in generic code where
> the element type is unknown.  Here m_vec is a vector of gdb_byte, and
> gdb_byte is guaranteed to have sizeof == 1.

Ah, OK.  So I would like there some: static_assert (sizeof (m_vec[0]) == 1);

The file_write template+function is used also in a later patch of that series
where sizeof(element) is not 1.  I have reimplemented it with ::fwrite in this
"v2" series after this your removal but I do not agree with that, IMO it was
safer+easier with the template+function you just removed.


Jan
  

> The file_write template+function is used also in a later patch of that series
> where sizeof(element) is not 1.  I have reimplemented it with ::fwrite in this
> "v2" series after this your removal but I do not agree with that, IMO it was
> safer+easier with the template+function you just removed.

If there's more than one place this could be handy, then I'm fine with
adding a wrapper function.  But as I said, it can't be exactly the
same as the original one, because it simply wouldn't compile.

Thanks,
Pedro Alves
  

On Mon, 19 Jun 2017 11:47:51 +0200, Pedro Alves wrote:
> But as I said, it can't be exactly the
> same as the original one, because it simply wouldn't compile.

I haven't found a benchmark whether the gdb::byte_vector optimization was
really worth complicating the codebase.  GDB has more serious performance
problems than such microoptimizations.

One of the goals of the move to C++ was to remove all the GDB-specific
language constructs making it easier to contribute.  Now GDB is becoming
written in its own language again, just based on C++ this time.


Jan
  

On 06/19/2017 11:03 AM, Jan Kratochvil wrote:
> On Mon, 19 Jun 2017 11:47:51 +0200, Pedro Alves wrote:
>> But as I said, it can't be exactly the
>> same as the original one, because it simply wouldn't compile.
> 
> I haven't found a benchmark whether the gdb::byte_vector optimization was
> really worth complicating the codebase.  GDB has more serious performance
> problems than such microoptimizations.

That's not a valid argument for justifying performance regressions.  
byte_vector avoids premature pessimization, at no real expense of 
readability, and makes it easier to actually use std::vector (because
that's what byte_vector is) in more cases without worrying about 
whether that'd cause a regression vs a dynamic array, as I've showed in
the patch that introduced it.

The C++ breakpoints series from a couple weeks back has some
performance fixes, and I'll be happy to review more patches that help
with any of those performance problems you may be alluding to.

> One of the goals of the move to C++ was to remove all the GDB-specific
> language constructs making it easier to contribute.  Now GDB is becoming
> written in its own language again, just based on C++ this time.

That's a gross exaggeration.  In any case, a byte buffer is not semantically
the same as a vector of integers.  And custom containers and other similar data
structures don't seem to have caused an issue for competing toolchains, btw.

Thanks,
Pedro Alves
  

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> GDB has more serious performance
> problems than such microoptimizations.

Hi Jan,
Do you have some examples or bugs?  GDB performance improvement is one
of my todo, so if you have some examples, or bugs, that would be quite
helpful.
  

On Mon, 19 Jun 2017 14:06:53 +0200, Yao Qi wrote:
> Do you have some examples or bugs?  GDB performance improvement is one
> of my todo, so if you have some examples, or bugs, that would be quite
> helpful.

Any backtraces take up to a minute.  That is because GDB expands only whole
CUs (and not specific DIEs) and one CU can be pretty huge in C++ programs,
moreover GDB has to expand tens of CUs to expand just one CU due to some
interdependencies between CUs enforced by dwarf2read.c.  Less serious but
another existing problem may be that dwz DW_TAG_imported_unit is implemented
only in DWARF reader and not in inferior symbol/type system of GDB, therefore
all DW_TAG_imported_unit units get duplicated/multiplicated inside GDB, being
expanded each time again.

Occasional print of an expression takes a minute which may be due to:
	gdb's performance is so terrible that it is unusable
	https://bugzilla.redhat.com/show_bug.cgi?id=1401091

Then conditions of breakpoints get evaluated by GDB and not in inferior which
is commonly unusable to wait for hours until the wished even happens and one
has to rather put an "if" statement into a recompiled debuggee.

Then there is the inability of execute C++ statements which should get fixed
by the 'compile' project but then it will be at least 2 times slower than
needed due to the use of gcc instead of clang.


Jan