From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31866 invoked by alias); 22 Apr 2011 00:40:37 -0000 Received: (qmail 31858 invoked by uid 22791); 22 Apr 2011 00:40:36 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 22 Apr 2011 00:40:17 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p3M0eGfA029701 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 21 Apr 2011 20:40:16 -0400 Received: from [127.0.0.1] (ovpn-113-57.phx2.redhat.com [10.3.113.57]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p3M0eFP2023389; Thu, 21 Apr 2011 20:40:15 -0400 Message-ID: <4DB0CE6E.4080105@redhat.com> Date: Fri, 22 Apr 2011 02:34:00 -0000 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Lightning/1.0b2 Thunderbird/3.1.9 MIME-Version: 1.0 To: Lawrence Crowl CC: reply@codereview.appspotmail.com, dnovillo@google.com, gcc-patches@gcc.gnu.org Subject: Re: [patch] Split Parse Timevar (issue4378056) References: <20110412184923.33F942225D6@jade.mtv.corp.google.com> <4DAF5782.90009@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2011-04/txt/msg01838.txt.bz2 On 04/21/2011 07:17 PM, Lawrence Crowl wrote: >>> @@ -1911,7 +1911,7 @@ ggc_collect (void) >>> - timevar_push (TV_GC); >>> + timevar_start (TV_GC); >> >> Why this change? GC time shouldn't be counted against whatever we >> happen to be parsing when it happens. > > If not, then code that generates lots of garbage does not get > charged for the cost to collect it. I thought it best to separate > these issues. Sure, but the problem is that the collection doesn't always happen in the same place that generated most of the garbage. >>> +DEFTIMEVAR (TV_PHASE_C_WRAPUP_CHECK , "phase C wrapup& check") >>> +DEFTIMEVAR (TV_PHASE_CP_DEFERRED , "phase C++ deferred") >> >> Why do these need to be different timevars? > > The are measuring different things. They are less different now > than they were during earlier development. We can make them the > same if you want. I think we could describe both as language-specific finalization. >>> +DEFTIMEVAR (TV_PARSE_INMETH , "parser inl. meth. body") >> >> Is it really important to distinguish this from other functions? > > This distinction is here to help evaluate potential speedup due to > lazy parsing. It might make some sense to separate functions and > inline functions, which also wouldn't have to be parsed immediately. That makes sense. Inlines in the class aren't significantly different from inlines outside the class, but inlines are significantly different from non-inlines for our purposes. >>> -DEFTIMEVAR (TV_TEMPLATE_INSTANTIATION, "template instantiation") >>> +DEFTIMEVAR (TV_INSTANTIATE_TEMPLATE , "instantiate template") >> >> Why these changes? > > Just to shorten the names. I'd prefer to keep it in the noun form. >>> -DEFTIMEVAR (TV_NAME_LOOKUP , "name lookup") >>> -DEFTIMEVAR (TV_OVERLOAD , "overload resolution") >>> +DEFTIMEVAR (TV_NAME_LOOKUP , "|name lookup") >>> +DEFTIMEVAR (TV_RESOLVE_OVERLOAD , "|overload resolution") And here you significantly lengthened one. :) > The "|" (also in TV_GC) indicates that these vars are collecting > time concurrently with the other non-phase variables. It is intended > to remind readers not to add those times into totals. Hmm, I guess that makes sense, but it should be documented. And perhaps move these timevars to the beginning or end so that they don't look like subsets of template instantiation. >>> @@ -564,6 +564,8 @@ compile_file (void) >>> + timevar_start (TV_PHASE_PARSING); >> >> Why does this happen before... >> >>> + timevar_push (TV_PARSE_GLOBAL); >> >> ...this? I would think the bits in there should be part of _SETUP. > > We could do that, though it would involve splitting the start/stop > calls into different functions. That seemed hard to manage. > As it stands, TV_PHASE_SETUP is entirely before compile_file() > and TV_PHASE_FINALIZE is entirely after. Thoughts? The code is cleaner the way you have it, but not as correct, as there's some initialization being charged to parsing. Jason