From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90096 invoked by alias); 17 Nov 2016 22:15:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 90036 invoked by uid 89); 17 Nov 2016 22:15:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=transferred X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Nov 2016 22:15:04 +0000 Received: by simark.ca (Postfix, from userid 33) id E0F2C1E93D; Thu, 17 Nov 2016 17:15:02 -0500 (EST) To: Pedro Alves Subject: Re: [PATCH] gdb: Use C++11 std::chrono X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 17 Nov 2016 22:15:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1479402927-4639-1-git-send-email-palves@redhat.com> References: <1479402927-4639-1-git-send-email-palves@redhat.com> Message-ID: <284edd6f2f85cd2c7cb9306a07a15b2a@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.2 X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00502.txt.bz2 On 2016-11-17 12:15, Pedro Alves wrote: > This patch fixes a few problems with GDB's time handling. > [...] Indeed, it makes the code very nice and readable. My only opinion about std::chrono so far was that it was so much more painful to do std::this_thread::sleep_for(std::chrono::seconds(2)) than sleep(2). :) > +/* Count the total amount of time spent executing in user mode. */ > > +user_cpu_time_clock::time_point > +user_cpu_time_clock::now () noexcept > +{ > + return time_point (microseconds (get_run_time ())); Looking at get_run_time() in libiberty, it seems like it returns user time + system time. Doesn't it contradict the naming of this clock and the comment of this method? > - /* If gettimeofday doesn't exist, and as a portability solution > it has > - been replaced with, e.g., time, then it doesn't make sense to print > - the microseconds field. Is there a way to check for that? */ > - fprintf (stderr, "%ld:%06ld ", (long) tm.tv_sec, (long) > tm.tv_usec); > + fprintf (stderr, "%ld:%06ld ", s.count (), us.count ()); Unrelated to your change, but I find it weird to format the time as "seconds:useconds". I wouldn't object if you sneakily changed it to "seconds.useconds". :) > @@ -2390,8 +2383,8 @@ mi_load_progress (const char *section_name, > unsigned long total_sent, > unsigned long grand_total) > { > - struct timeval time_now, delta, update_threshold; > - static struct timeval last_update; > + using namespace std::chrono; > + static steady_clock::time_point last_update; > static char *previous_sect_name = NULL; > int new_section; > struct ui_out *saved_uiout; > @@ -2416,18 +2409,9 @@ mi_load_progress (const char *section_name, > > uiout = current_uiout; > > - update_threshold.tv_sec = 0; > - update_threshold.tv_usec = 500000; > - gettimeofday (&time_now, NULL); > - > - delta.tv_usec = time_now.tv_usec - last_update.tv_usec; > - delta.tv_sec = time_now.tv_sec - last_update.tv_sec; > - > - if (delta.tv_usec < 0) > - { > - delta.tv_sec -= 1; > - delta.tv_usec += 1000000L; > - } > + microseconds update_threshold (500000); > + steady_clock::time_point time_now = steady_clock::now (); > + steady_clock::duration delta = time_now - last_update; Can this be simplified to avoid having the delta variable? Since we can easily compare two time_points, we can probably make it look like: if (steady_clock::now () >= next_update) { next_update = steady_clock::now() + update_threshold; ... } or if (steady_clock::now () >= (last_update + update_threshold)) { last_update = steady_clock::now (); ... } Also, wouldn't "update_period" or "update_rate" be more precise than "update_threshold"? > static void > timestamp (struct mi_timestamp *tv) > { > - gettimeofday (&tv->wallclock, NULL); > -#ifdef HAVE_GETRUSAGE > - getrusage (RUSAGE_SELF, &rusage); > - tv->utime.tv_sec = rusage.ru_utime.tv_sec; > - tv->utime.tv_usec = rusage.ru_utime.tv_usec; > - tv->stime.tv_sec = rusage.ru_stime.tv_sec; > - tv->stime.tv_usec = rusage.ru_stime.tv_usec; > -#else > - { > - long usec = get_run_time (); > - > - tv->utime.tv_sec = usec/1000000L; > - tv->utime.tv_usec = usec - 1000000L*tv->utime.tv_sec; > - tv->stime.tv_sec = 0; > - tv->stime.tv_usec = 0; > - } > -#endif > + using namespace std::chrono; It's not written in the coding style guide, but I think it would be nice to have a newline between the "using" declaration and the following lines, like we do with variable declarations. > -/* Report how fast the transfer went. */ > +/* Report on STREAM the performance of a memory transfer operation, > + such as 'load'. DATA_COUNT is the number of bytes transferred. > + WRITE_COUNT is the number of separate write operations, or 0, if > + that information is not available. TIME is the range of time in > + which the operation lasted. */ > > -void > +static void > print_transfer_performance (struct ui_file *stream, > unsigned long data_count, > unsigned long write_count, > - const struct timeval *start_time, > - const struct timeval *end_time) > + const time_range &time) Is there a reason you couldn't use a std::chrono::duration instead of a time_range here? The function doesn't seem to care about the particular time_points, only their difference. > - timestamp = xstrprintf ("%ld:%ld %s%s", > - (long) tm.tv_sec, (long) tm.tv_usec, > - linebuffer, > - need_nl ? "\n": ""); > - make_cleanup (xfree, timestamp); > - fputs_unfiltered (timestamp, stream); > + std::string timestamp = string_printf ("%ld:%ld %s%s", Same comment about secs:usecs vs secs.usecs.