From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7052 invoked by alias); 22 Apr 2011 22:42:05 -0000 Received: (qmail 6939 invoked by uid 22791); 22 Apr 2011 22:42:03 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.44.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 22 Apr 2011 22:41:47 +0000 Received: from hpaq5.eem.corp.google.com (hpaq5.eem.corp.google.com [172.25.149.5]) by smtp-out.google.com with ESMTP id p3MMfk8a012080 for ; Fri, 22 Apr 2011 15:41:46 -0700 Received: from pvg3 (pvg3.prod.google.com [10.241.210.131]) by hpaq5.eem.corp.google.com with ESMTP id p3MMfhJ1013575 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Fri, 22 Apr 2011 15:41:44 -0700 Received: by pvg3 with SMTP id 3so589404pvg.32 for ; Fri, 22 Apr 2011 15:41:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.142.134.19 with SMTP id h19mr870632wfd.359.1303512102828; Fri, 22 Apr 2011 15:41:42 -0700 (PDT) Received: by 10.142.234.20 with HTTP; Fri, 22 Apr 2011 15:41:42 -0700 (PDT) In-Reply-To: <4DB0CE6E.4080105@redhat.com> References: <20110412184923.33F942225D6@jade.mtv.corp.google.com> <4DAF5782.90009@redhat.com> <4DB0CE6E.4080105@redhat.com> Date: Sat, 23 Apr 2011 00:05:00 -0000 Message-ID: Subject: Re: [patch] Split Parse Timevar (issue4378056) From: Lawrence Crowl To: Jason Merrill Cc: reply@codereview.appspotmail.com, dnovillo@google.com, gcc-patches@gcc.gnu.org Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-IsSubscribed: yes 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/msg01913.txt.bz2 On 4/21/11, Jason Merrill wrote: > 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. True, but I expect it usually does. At any rate, I will revert the timevar to push/pop. >>>> +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. Okay. >>>> +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. Do you have a quick hint for how to make that distinction? >>>> -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. Okay. This on in particular was making the output wide. >>>> -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. :) Ah, but it wasn't the long pole and hence more clarity didn't hurt. >> 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. Okay. >>>> @@ -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. Would you prefer moving that initialization out or placing the start/stop into different routines? -- Lawrence Crowl