From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1421 invoked by alias); 21 Apr 2011 23:17:47 -0000 Received: (qmail 1413 invoked by uid 22791); 21 Apr 2011 23:17:46 -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; Thu, 21 Apr 2011 23:17:30 +0000 Received: from kpbe14.cbf.corp.google.com (kpbe14.cbf.corp.google.com [172.25.105.78]) by smtp-out.google.com with ESMTP id p3LNHTkW017252 for ; Thu, 21 Apr 2011 16:17:29 -0700 Received: from pzk1 (pzk1.prod.google.com [10.243.19.129]) by kpbe14.cbf.corp.google.com with ESMTP id p3LNH2w5029188 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Thu, 21 Apr 2011 16:17:28 -0700 Received: by pzk1 with SMTP id 1so133501pzk.2 for ; Thu, 21 Apr 2011 16:17:27 -0700 (PDT) MIME-Version: 1.0 Received: by 10.142.201.5 with SMTP id y5mr274440wff.74.1303427847566; Thu, 21 Apr 2011 16:17:27 -0700 (PDT) Received: by 10.142.234.20 with HTTP; Thu, 21 Apr 2011 16:17:27 -0700 (PDT) In-Reply-To: <4DAF5782.90009@redhat.com> References: <20110412184923.33F942225D6@jade.mtv.corp.google.com> <4DAF5782.90009@redhat.com> Date: Fri, 22 Apr 2011 00:40: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/msg01834.txt.bz2 On 4/20/11, Jason Merrill wrote: > On 04/12/2011 11:49 AM, Lawrence Crowl wrote: >> This patch is available for review at >> http://codereview.appspot.com/4378056 >> + timevar_start (TV_RESOLVE_OVERLOAD); > > Putting this in perform_overload_resolution isn't enough; only a couple > of cases of overload resolution actually use it. Any function that > calls tourney will also need this. Okay, I'll add those functions. You've explained why I was getting surprisingly low numbers for overload resolution. >> +lookup_template_class (tree d1, tree arglist, tree in_decl, tree context, >> + int entering_scope, tsubst_flags_t complain) >> +{ >> + tree ret; >> + bool subtime = timevar_cond_start (TV_NAME_LOOKUP); > > Let's count this as TV_INSTANTIATE_TEMPLATE instead. Okay. BTW, that is a change from existing behavior. > >> @@ -17194,7 +17225,7 @@ instantiate_decl (tree d, int defer_ok, >> - timevar_push (TV_PARSE); >> + timevar_push (TV_PARSE_GLOBAL); > > This too. Okay. > >> @@ -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. >> +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. >> +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. >> -DEFTIMEVAR (TV_TEMPLATE_INSTANTIATION, "template instantiation") >> +DEFTIMEVAR (TV_INSTANTIATE_TEMPLATE , "instantiate template") > > Why these changes? Just to shorten the names. They were getting pretty unwieldy. >> -DEFTIMEVAR (TV_NAME_LOOKUP , "name lookup") >> -DEFTIMEVAR (TV_OVERLOAD , "overload resolution") >> +DEFTIMEVAR (TV_NAME_LOOKUP , "|name lookup") >> +DEFTIMEVAR (TV_RESOLVE_OVERLOAD , "|overload resolution") > > Why these changes? 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. > >> @@ -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? >> @@ -16760,6 +16770,7 @@ cp_parser_class_specifier (cp_parser* parser) >> + timevar_pop (TV_PARSE_STRUCT); >> + timevar_pop (TV_PARSE_STRUCT); >> + timevar_pop (TV_PARSE_STRUCT); >> + timevar_pop (TV_PARSE_STRUCT); > > Why not factor this out like you did with so many functions outside the > parser? Probably just inertia early on. I'll change it. -- Lawrence Crowl