From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17923 invoked by alias); 20 Apr 2011 22:00:59 -0000 Received: (qmail 17914 invoked by uid 22791); 20 Apr 2011 22:00:58 -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; Wed, 20 Apr 2011 22:00:37 +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 p3KM0ZXG021117 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 20 Apr 2011 18:00:35 -0400 Received: from [127.0.0.1] (ovpn-113-102.phx2.redhat.com [10.3.113.102]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p3KM0YL3012315; Wed, 20 Apr 2011 18:00:34 -0400 Message-ID: <4DAF5782.90009@redhat.com> Date: Wed, 20 Apr 2011 23:33: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> In-Reply-To: <20110412184923.33F942225D6@jade.mtv.corp.google.com> 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/msg01732.txt.bz2 On 04/12/2011 11:49 AM, Lawrence Crowl wrote: > This patch is available for review at http://codereview.appspot.com/4378056 I tried to comment there, but it didn't seem to be working; looking at the side-by-side diffs didn't show any changes, and double-clicking on a line in the patch form didn't let me add a comment. > + 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. > +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. > @@ -17194,7 +17225,7 @@ instantiate_decl (tree d, int defer_ok, > - timevar_push (TV_PARSE); > + timevar_push (TV_PARSE_GLOBAL); This too. > @@ -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. > +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? > +DEFTIMEVAR (TV_PARSE_INMETH , "parser inl. meth. body") Is it really important to distinguish this from other functions? > -DEFTIMEVAR (TV_NAME_LOOKUP , "name lookup") > -DEFTIMEVAR (TV_OVERLOAD , "overload resolution") > -DEFTIMEVAR (TV_TEMPLATE_INSTANTIATION, "template instantiation") > +DEFTIMEVAR (TV_INSTANTIATE_TEMPLATE , "instantiate template") > +DEFTIMEVAR (TV_NAME_LOOKUP , "|name lookup") > +DEFTIMEVAR (TV_RESOLVE_OVERLOAD , "|overload resolution") Why these changes? > @@ -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. > @@ -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? Jason