From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1315 invoked by alias); 12 Sep 2017 11:31:53 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 502 invoked by uid 89); 12 Sep 2017 11:31:52 -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,SPF_PASS autolearn=ham version=3.3.2 spammy=footprint, backbone X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Sep 2017 11:31:50 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 044A6AC1B; Tue, 12 Sep 2017 11:31:47 +0000 (UTC) Subject: Re: [RFC][PATCH] Do refactoring of attribute functions and move them to attribs.[hc]. To: Jakub Jelinek Cc: Jason Merrill , Joseph Myers , gcc-patches List , Richard Biener References: <4cf00ee9-4cd7-1806-481f-966611768a15@suse.cz> <8a0b2ff0-50f9-771d-fb63-01d17cb06b46@suse.cz> <669c960e-f4a8-eae3-efd2-f8c29477a817@suse.cz> <7e541554-10a6-9cae-fca0-042900271768@suse.cz> <20170912075453.GD1701@tucnak> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <1f89ae73-7e84-66db-fec3-1e86e6c816d8@suse.cz> Date: Tue, 12 Sep 2017 11:31:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170912075453.GD1701@tucnak> Content-Type: multipart/mixed; boundary="------------5FF041086C69F76CD00F3291" X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00669.txt.bz2 This is a multi-part message in MIME format. --------------5FF041086C69F76CD00F3291 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-length: 4233 On 09/12/2017 09:54 AM, Jakub Jelinek wrote: > On Thu, Jul 13, 2017 at 03:51:31PM +0200, Martin Liška wrote: >> It's request for comment where I mechanically moved attribute-related function to attribs.[hc]. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Thoughts? > > I've only noticed this now, what is the reason for undoing the size > optimization we had, i.e. inline lookup_attribute that handles the most > common case inline only and inline computes the strlen (an important > optimization, because we almost always call it with a string literal > and strlen of that is a constant) and doing the rest out of line? > >> -/* Given an attribute name ATTR_NAME and a list of attributes LIST, >> - return a pointer to the attribute's list element if the attribute >> - is part of the list, or NULL_TREE if not found. If the attribute >> - appears more than once, this only returns the first occurrence; the >> - TREE_CHAIN of the return value should be passed back in if further >> - occurrences are wanted. ATTR_NAME must be in the form 'text' (not >> - '__text__'). */ >> - >> -static inline tree >> -lookup_attribute (const char *attr_name, tree list) >> -{ >> - gcc_checking_assert (attr_name[0] != '_'); >> - /* In most cases, list is NULL_TREE. */ >> - if (list == NULL_TREE) >> - return NULL_TREE; >> - else >> - /* Do the strlen() before calling the out-of-line implementation. >> - In most cases attr_name is a string constant, and the compiler >> - will optimize the strlen() away. */ >> - return private_lookup_attribute (attr_name, strlen (attr_name), list); >> -} > > The current code instead inlines the whole lookup_attribute which is larger. > Have you looked at the code size effects of that change? > > Jakub > Hi. Sorry for removal of the optimization. You're right it really adds some size (~30K of .text): $ bloaty gcc/cc1plus -- /tmp/cc1plus VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ [ = ] 0 .debug_loc +277Ki +0.4% [ = ] 0 .debug_info +86.9Ki +0.1% [ = ] 0 .debug_ranges +59.9Ki +0.5% +0.2% +32.1Ki .text +32.1Ki +0.2% [ = ] 0 .debug_line +9.36Ki +0.1% +0.0% +2.06Ki .rodata +2.06Ki +0.0% [ = ] 0 .symtab +1.73Ki +0.1% +0.1% +1.72Ki .eh_frame +1.72Ki +0.1% +0.0% +272 .dynstr +272 +0.0% [ = ] 0 .debug_abbrev +140 +0.0% +0.0% +72 .dynsym +72 +0.0% +0.0% +64 .eh_frame_hdr +64 +0.0% [ = ] 0 .debug_aranges +48 +0.0% +0.0% +12 .gnu.hash +12 +0.0% +0.0% +12 .hash +12 +0.0% +0.0% +6 .gnu.version +6 +0.0% -------------- SHRINKING -------------- -10.2% -6 [Unmapped] -317 -20.8% [ = ] 0 .strtab -66 -0.0% [ = ] 0 .debug_str -52 -0.0% +0.1% +36.3Ki TOTAL +471Ki +0.2% $ bloaty gcc/cc1 -- /tmp/cc1 VM SIZE FILE SIZE ++++++++++++++ GROWING ++++++++++++++ [ = ] 0 .debug_loc +232Ki +0.4% [ = ] 0 .debug_info +74.1Ki +0.1% [ = ] 0 .debug_ranges +48.9Ki +0.4% +0.2% +26.9Ki .text +26.9Ki +0.2% [ = ] 0 .debug_line +8.22Ki +0.1% +0.1% +1.79Ki .eh_frame +1.79Ki +0.1% +0.0% +1.75Ki .rodata +1.75Ki +0.0% [ = ] 0 .symtab +1.50Ki +0.1% +0.0% +272 .dynstr +272 +0.0% [ = ] 0 .debug_abbrev +72 +0.0% +0.0% +72 .dynsym +72 +0.0% +0.0% +64 .eh_frame_hdr +64 +0.0% [ = ] 0 .debug_aranges +48 +0.0% +0.0% +12 .gnu.hash +12 +0.0% +0.0% +12 .hash +12 +0.0% +0.0% +6 .gnu.version +6 +0.0% -------------- SHRINKING -------------- [ = ] 0 .debug_str -52 -0.0% [ = ] 0 .strtab -26 -0.0% -+-+-+-+-+-+-+ MIXED +-+-+-+-+-+-+- +19% +10 [Unmapped] -2.87Ki -75.1% +0.1% +30.9Ki TOTAL +392Ki +0.2% Thus I'm suggesting to the how it was. Is the patch OK after testing? Martin --------------5FF041086C69F76CD00F3291 Content-Type: text/x-patch; name="0001-Reduce-lookup_attribute-memory-footprint.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-Reduce-lookup_attribute-memory-footprint.patch" Content-length: 2454 >From a40c06fc06afcb7bb886d7a3106e6da631a48430 Mon Sep 17 00:00:00 2001 From: marxin Date: Tue, 12 Sep 2017 13:30:39 +0200 Subject: [PATCH] Reduce lookup_attribute memory footprint. gcc/ChangeLog: 2017-09-12 Martin Liska * attribs.c (private_lookup_attribute): New function. * attribs.h (private_lookup_attribute): Declared here. (lookup_attribute): Called from this place. --- gcc/attribs.c | 17 +++++++++++++++++ gcc/attribs.h | 17 ++++++----------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/gcc/attribs.c b/gcc/attribs.c index b8f58a74596..9064434e5f2 100644 --- a/gcc/attribs.c +++ b/gcc/attribs.c @@ -1584,3 +1584,20 @@ attribute_list_contained (const_tree l1, const_tree l2) return 1; } + + +tree +private_lookup_attribute (const char *attr_name, size_t attr_len, tree list) +{ + while (list) + { + tree attr = get_attribute_name (list); + size_t ident_len = IDENTIFIER_LENGTH (attr); + if (cmp_attribs (attr_name, attr_len, IDENTIFIER_POINTER (attr), + ident_len)) + break; + list = TREE_CHAIN (list); + } + + return list; +} diff --git a/gcc/attribs.h b/gcc/attribs.h index 06e6993e958..0285b93cbc3 100644 --- a/gcc/attribs.h +++ b/gcc/attribs.h @@ -87,6 +87,11 @@ extern tree handle_dll_attribute (tree *, tree, tree, int, bool *); extern int attribute_list_equal (const_tree, const_tree); extern int attribute_list_contained (const_tree, const_tree); +/* The backbone of lookup_attribute(). ATTR_LEN is the string length + of ATTR_NAME, and LIST is not NULL_TREE. */ +extern tree private_lookup_attribute (const char *attr_name, size_t attr_len, + tree list); + /* For a given IDENTIFIER_NODE, strip leading and trailing '_' characters so that we have a canonical form of attribute names. */ @@ -151,17 +156,7 @@ lookup_attribute (const char *attr_name, tree list) /* Do the strlen() before calling the out-of-line implementation. In most cases attr_name is a string constant, and the compiler will optimize the strlen() away. */ - while (list) - { - tree attr = get_attribute_name (list); - size_t ident_len = IDENTIFIER_LENGTH (attr); - if (cmp_attribs (attr_name, attr_len, IDENTIFIER_POINTER (attr), - ident_len)) - break; - list = TREE_CHAIN (list); - } - - return list; + return private_lookup_attribute (attr_name, attr_len, list); } } -- 2.14.1 --------------5FF041086C69F76CD00F3291--