Message ID | 1497284051-13795-2-git-send-email-palves@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 88154 invoked by alias); 12 Jun 2017 16:14:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@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 Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 86770 invoked by uid 89); 12 Jun 2017 16:14:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, T_RP_MATCHES_RCVD, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Jun 2017 16:14:09 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3A212C01CB7F for <gdb-patches@sourceware.org>; Mon, 12 Jun 2017 16:14:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3A212C01CB7F Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 3A212C01CB7F Received: from cascais.lan (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE4F880F6C for <gdb-patches@sourceware.org>; Mon, 12 Jun 2017 16:14:12 +0000 (UTC) From: Pedro Alves <palves@redhat.com> To: gdb-patches@sourceware.org Subject: [PATCH 2/6] Code cleanup: dwarf2read.c: Eliminate ::file_write Date: Mon, 12 Jun 2017 17:14:07 +0100 Message-Id: <1497284051-13795-2-git-send-email-palves@redhat.com> In-Reply-To: <1497284051-13795-1-git-send-email-palves@redhat.com> References: <1497284051-13795-1-git-send-email-palves@redhat.com> In-Reply-To: <8efc0742-1014-4fe0-6948-f40a9c5c4975@redhat.com> References: <8efc0742-1014-4fe0-6948-f40a9c5c4975@redhat.com> |
Commit Message
Pedro Alves
June 12, 2017, 4:14 p.m. UTC
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(-)
Comments
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
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 316e03a..e3e980d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,11 @@ 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. + +2017-06-12 Pedro Alves <palves@redhat.com> + * dwarf2read.c (uniquify_cu_indices): Use std::unique and std::vector::erase. diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 0c9e275..55b3033 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -23195,25 +23195,6 @@ dwarf2_per_objfile_free (struct objfile *objfile, void *d) /* The "save gdb-index" command. */ -/* 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")); } private: