Message ID | 7ffd29af-218a-32e3-ac6a-207f0bc8a382@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 28365 invoked by alias); 17 Feb 2017 01:19: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 28345 invoked by uid 89); 17 Feb 2017 01:19:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=callees, Hx-languages-length:2185, dwarf2_cu, *die 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; Fri, 17 Feb 2017 01:19:10 +0000 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A70C27E9E4; Fri, 17 Feb 2017 01:19:10 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id C49926370A; Fri, 17 Feb 2017 01:19:09 +0000 (UTC) Subject: Re: [PATCH 3/8] Code cleanup: Split dwarf2_ranges_read to a callback To: Jan Kratochvil <jan.kratochvil@redhat.com>, gdb-patches@sourceware.org References: <148693097396.9024.2288256732840761882.stgit@host1.jankratochvil.net> <148693098804.9024.9905586065345063380.stgit@host1.jankratochvil.net> Cc: Victor Leschuk <vleschuk@accesssoftek.com> From: Pedro Alves <palves@redhat.com> Message-ID: <7ffd29af-218a-32e3-ac6a-207f0bc8a382@redhat.com> Date: Fri, 17 Feb 2017 01:19:06 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <148693098804.9024.9905586065345063380.stgit@host1.jankratochvil.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Pedro Alves
Feb. 17, 2017, 1:19 a.m. UTC
On 02/12/2017 08:23 PM, Jan Kratochvil wrote: > -/* Get low and high pc attributes from DW_AT_ranges attribute value OFFSET. > - Return 1 if the attributes are present and valid, otherwise, return 0. > - If RANGES_PST is not NULL we should setup `objfile->psymtabs_addrmap'. */ > +/* Call CALLBACK from DW_AT_ranges attribute value OFFSET. > + Return 1 if the attributes are present and valid, otherwise, return 0. */ > > static int > -dwarf2_ranges_read (unsigned offset, CORE_ADDR *low_return, > - CORE_ADDR *high_return, struct dwarf2_cu *cu, > - struct partial_symtab *ranges_pst) > +dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu, > + std::function<void (CORE_ADDR range_beginning, > + CORE_ADDR range_end)> callback) std::function is a lot of unnecessary overhead here. Unless you manage to trigger to small-function optimization (which you won't here, the callbacks are too big), this is forcing a heap allocation inside std::function for every call to dwarf2_ranges_process. Let's only use std::function for it's intended use-case of when the design calls for taking, or more usually, storing, a callable whose type is not knowable at compile type. In this case, the type is determinable at compile type, as everything is local to the same file and the callees are always known. So make dwarf2_ranges_process a template, and the overhead disappears, like: ~~~~~~~~~~~~~~~~~~ OK with that change. Thanks, Pedro Alves
Comments
On Fri, 17 Feb 2017 02:19:06 +0100, Pedro Alves wrote: > std::function is a lot of unnecessary overhead here. Unless you > manage to trigger to small-function optimization (which you won't here, > the callbacks are too big), this is forcing a heap allocation inside > std::function for every call to dwarf2_ranges_process. These microoptimizations lead to the still broken fundamental data types and algorithms of GDB, having to wait for minutes to hours to print an expression (although usually it is not printable anyway). perf would show the problem if it is really significant. It would be less significant with tcmalloc which GNU libc systems still do not use by default. > Let's only use std::function for it's intended use-case of when the > design calls for taking, or more usually, storing, a callable whose > type is not knowable at compile type. You are defining a new C++ specification here? I have checked the code as I wrote it is a perfect use of std::function<> according to the ISO C++11 specification. Thanks you have noticed current GCC has missed-optimization in this case and that it can be workarounded by a template as you have suggested. It leads to worse compilation diagnostics and we can hope this hack can be removed in the future. Still I agree this workaround of GCC is worth the performance gain. Jan
On 02/19/2017 09:26 PM, Jan Kratochvil wrote: > On Fri, 17 Feb 2017 02:19:06 +0100, Pedro Alves wrote: >> std::function is a lot of unnecessary overhead here. Unless you >> manage to trigger to small-function optimization (which you won't here, >> the callbacks are too big), this is forcing a heap allocation inside >> std::function for every call to dwarf2_ranges_process. > > These microoptimizations lead to the still broken fundamental data types and > algorithms of GDB, having to wait for minutes to hours to print an expression > (although usually it is not printable anyway). "lead" here is totally a non sequitur... > perf would show the problem if it is really significant. It would be less > significant with tcmalloc which GNU libc systems still do not use by default. See urls below. There are benchmarks all over the web. The performance problems with misuse of std::function are well known. Best to nip a problem in the bud. > > >> Let's only use std::function for it's intended use-case of when the >> design calls for taking, or more usually, storing, a callable whose >> type is not knowable at compile type. > > You are defining a new C++ specification here? I have checked the code as > I wrote it is a perfect use of std::function<> according to the ISO C++11 > specification. The specification doesn't talk about how the classes it specifies are meant to be used. Only their interfaces and behaviors. You could do something ridiculous like only use "void*" as types of pointers throughout a program, and cast back to the proper dynamic type before dereferencing them, and call it a "perfect" use according to the spec, but that doesn't mean that that would be a be good idea. A "perfect" use for std::function would be using it when you need to save a list of callables in a container in a field of a class, to call later. E.g., it'd be a good fit to replace GDB's observers (observer_attach..., observer_notify..., etc.) For functions that take a callable as argument, but never store the callable anywhere, just use it, the workaround IF you don't want or can't use a template and you need the type-erasing of std::function, is to wrap the callable around an std::reference_wrapper at the callee site, using std::ref. But that's quite inconvenient to use, because std::ref doesn't accept rvalue references. Meaning, #1, you have to remember to use it, #2, you have to give the lambda's a name, like: auto some_callback = [&] (......) { ..... }; function_that_takes_callback_as_a_filter (foo, bar, std::ref (some_callback)); instead of the desired: function_that_takes_callback_as_a_filter (foo, bar, [&] (......) { ..... }; So for the cases where we have a function where we: #1 - can't or don't want to use a template. #2 - want to accept any callable type (, thus need a type-erasing callable wrapper type like std::function). #3 - are sure the lifetime of the callable is as long as the function the callable is being passed to. #4 - are OK to take the callable by rvalue reference. then what we really need is a "function_view" type. I.e., something like std::function that always takes a reference to a callable instead of storing the callable inside. Like std::string_view is to std::string. The standard is lacking here. I'm not saying anything new here. Several projects have such a thing (e.g., llvm::function_ref), I've seen it discussed in the std-proposals list, and I've seen blogs about such things. If you do the proper web searches, you'll find explanations of all the above elsewhere. To spare you the trouble, I just did a search now for you, and here's what I found: on std::function vs templates: http://stackoverflow.com/questions/14677997/stdfunction-vs-template on std::function vs std::ref: http://blog.demofox.org/2015/02/25/avoiding-the-performance-hazzards-of-stdfunction/ Here's LLVM's llvm::function_ref: http://llvm.org/docs/doxygen/html/STLExtras_8h_source.html#l00060 Here's a recent blog on yet another "function_view" implementation: https://vittorioromeo.info/index/blog/passing_functions_to_functions.html As it happens, I have a use for something like this in symtab.c, so I think I'll add something like it to GDB, and avoid having to discuss this again. > Thanks you have noticed current GCC has missed-optimization in this case and > that it can be workarounded by a template as you have suggested. It leads to > worse compilation diagnostics and we can hope this hack can be removed in the > future. Still I agree this workaround of GCC is worth the performance gain. #1 - It's not a hack. #2 - an optimization I imagine could make a difference here would be "new" elision, per N3664, which gcc doesn't do yet. But in any case, it's much better if the type system reflects semantics, instead of relying on some optimization that is not guaranteed to trigger, even if the compiler supports it. #3 - It's not a hack. Thanks, Pedro Alves
diff --git c/gdb/dwarf2read.c w/gdb/dwarf2read.c index 1137541..94d5de2 100644 --- c/gdb/dwarf2read.c +++ w/gdb/dwarf2read.c @@ -11877,10 +11877,13 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu) /* Call CALLBACK from DW_AT_ranges attribute value OFFSET. Return 1 if the attributes are present and valid, otherwise, return 0. */ +/* Callback's type should be: + void (CORE_ADDR range_beginning, CORE_ADDR range_end) +*/ +template <typename Callback> static int dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu, - std::function<void (CORE_ADDR range_beginning, - CORE_ADDR range_end)> callback) + Callback callback) { ~~~~~~~~~~~~~~~~~~