On Mon, Nov 18, 2013 at 2:15 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
> Hi,
> this patch fixes problem we noticed with Martin Liska where gcov_dump is
> called
> several times per execution of firefox (on each fork and exec). This causes
> runs to be large and makes functions executed once per program to be
> considered
> cold.
>
> This patch makes us to update runs just once per execution and not on each
> streaming of profile data. While testing it I also noticed that program
> summary is now broken, since crc32 is accumulated per each dumping instead
> just
> once.
You are right. I forgot that gcov_exit() can be called multiple times.
Should have
initialized crc32 per gcov_exit() call.
Thanks for the fix.
>
> I believe the newly introduced static vars should go - there is nothing really
> preventing us from doing two concurent updates and also it just unnecesary
> increases to footprint of libgcov. I converted thus all_prg and crc32 back to
> local vars.
>
> Bootstrapped/regtested x86_64-linux, comitted.
>
> * libgcov-driver.c (get_gcov_dump_complete): Update comments.
> (all_prg, crc32): Remove static vars.
> (gcov_exit_compute_summary): Rewrite to return crc32; do not clear
> all_prg.
> (gcov_exit_merge_gcda): Add crc32 parameter.
> (gcov_exit_merge_summary): Add crc32 and all_prg parameter;
> do not account run if it was already accounted.
> (gcov_exit_dump_gcov): Add crc32 and all_prg parameters.
> (gcov_exit): Initialize all_prg; update.
> Index: libgcov-driver.c
> ===================================================================
> --- libgcov-driver.c (revision 204945)
> +++ libgcov-driver.c (working copy)
> @@ -96,7 +96,7 @@ static size_t gcov_max_filename = 0;
> /* Flag when the profile has already been dumped via __gcov_dump(). */
> static int gcov_dump_complete;
>
> -/* A global functino that get the vaule of gcov_dump_complete. */
> +/* A global function that get the vaule of gcov_dump_complete. */
>
> int
> get_gcov_dump_complete (void)
> @@ -319,12 +319,6 @@ gcov_compute_histogram (struct gcov_summ
>
> /* summary for program. */
> static struct gcov_summary this_prg;
> -#if !GCOV_LOCKED
> -/* summary for all instances of program. */
> -static struct gcov_summary all_prg;
> -#endif
> -/* crc32 for this program. */
> -static gcov_unsigned_t crc32;
> /* gcda filename. */
> static char *gi_filename;
> /* buffer for the fn_data from another program. */
> @@ -333,10 +327,9 @@ static struct gcov_fn_buffer *fn_buffer;
> static struct gcov_summary_buffer *sum_buffer;
>
> /* This funtions computes the program level summary and the histo-gram.
> - It initializes ALL_PRG, computes CRC32, and stores the summary in
> - THIS_PRG. All these three variables are file statics. */
> + It computes and returns CRC32 and stored summari in THIS_PRG. */
>
> -static void
> +static gcov_unsigned_t
> gcov_exit_compute_summary (void)
> {
> struct gcov_info *gi_ptr;
> @@ -346,10 +339,8 @@ gcov_exit_compute_summary (void)
> int f_ix;
> unsigned t_ix;
> gcov_unsigned_t c_num;
> + gcov_unsigned_t crc32 = 0;
>
> -#if !GCOV_LOCKED
> - memset (&all_prg, 0, sizeof (all_prg));
> -#endif
> /* Find the totals for this execution. */
> memset (&this_prg, 0, sizeof (this_prg));
> for (gi_ptr = gcov_list; gi_ptr; gi_ptr = gi_ptr->next)
> @@ -391,6 +382,7 @@ gcov_exit_compute_summary (void)
> }
> }
> gcov_compute_histogram (&this_prg);
> + return crc32;
> }
>
> /* A struct that bundles all the related information about the
> @@ -412,7 +404,8 @@ static int
> gcov_exit_merge_gcda (struct gcov_info *gi_ptr,
> struct gcov_summary *prg_p,
> gcov_position_t *summary_pos_p,
> - gcov_position_t *eof_pos_p)
> + gcov_position_t *eof_pos_p,
> + gcov_unsigned_t crc32)
> {
> gcov_unsigned_t tag, length;
> unsigned t_ix;
> @@ -652,13 +645,19 @@ gcov_exit_write_gcda (const struct gcov_
> Return -1 on error. Return 0 on success. */
>
> static int
> -gcov_exit_merge_summary (const struct gcov_info *gi_ptr, struct gcov_summary
> *prg)
> +gcov_exit_merge_summary (const struct gcov_info *gi_ptr, struct gcov_summary
> *prg,
> + gcov_unsigned_t crc32, struct gcov_summary *all_prg)
> {
> struct gcov_ctr_summary *cs_prg, *cs_tprg;
> -#if !GCOV_LOCKED
> - struct gcov_ctr_summary *cs_all;
> -#endif
> unsigned t_ix;
> + /* If application calls fork or exec multiple times, we end up storing
> + profile repeadely. We should not account this as multiple runs or
> + functions executed once may mistakely become cold. */
> + static int run_accounted = 0;
> +#if !GCOV_LOCKED
> + /* summary for all instances of program. */
> + struct gcov_ctr_summary *cs_all;
> +#endif
>
> /* Merge the summaries. */
> for (t_ix = 0; t_ix < GCOV_COUNTERS_SUMMABLE; t_ix++)
> @@ -668,13 +667,18 @@ gcov_exit_merge_summary (const struct gc
>
> if (gi_ptr->merge[t_ix])
> {
> - if (!cs_prg->runs++)
> + int first = !cs_prg->runs;
> +
> + if (!run_accounted)
> + cs_prg->runs++;
> + run_accounted = 1;
> + if (first)
> cs_prg->num = cs_tprg->num;
> cs_prg->sum_all += cs_tprg->sum_all;
> if (cs_prg->run_max < cs_tprg->run_max)
> cs_prg->run_max = cs_tprg->run_max;
> cs_prg->sum_max += cs_tprg->run_max;
> - if (cs_prg->runs == 1)
> + if (first)
> memcpy (cs_prg->histogram, cs_tprg->histogram,
> sizeof (gcov_bucket_type) * GCOV_HISTOGRAM_SIZE);
> else
> @@ -686,9 +690,8 @@ gcov_exit_merge_summary (const struct gc
> gi_filename);
> return -1;
> }
> -
> #if !GCOV_LOCKED
> - cs_all = &all_prg.ctrs[t_ix];
> + cs_all = &all_prg->ctrs[t_ix];
> if (!cs_all->runs && cs_prg->runs)
> {
> cs_all->num = cs_prg->num;
> @@ -697,7 +700,7 @@ gcov_exit_merge_summary (const struct gc
> cs_all->run_max = cs_prg->run_max;
> cs_all->sum_max = cs_prg->sum_max;
> }
> - else if (!all_prg.checksum
> + else if (!all_prg->checksum
> /* Don't compare the histograms, which may have slight
> variations depending on the order they were updated
> due to the truncating integer divides used in the
> @@ -711,7 +714,7 @@ gcov_exit_merge_summary (const struct gc
> gcov_error ("profiling:%s:Data file mismatch - some "
> "data files may have been concurrently "
> "updated without locking support\n", gi_filename);
> - all_prg.checksum = ~0u;
> + all_prg->checksum = ~0u;
> }
> #endif
> }
> @@ -729,7 +732,8 @@ gcov_exit_merge_summary (const struct gc
> summaries separate. */
>
> static void
> -gcov_exit_dump_gcov (struct gcov_info *gi_ptr, struct gcov_filename_aux *gf)
> +gcov_exit_dump_gcov (struct gcov_info *gi_ptr, struct gcov_filename_aux *gf,
> + gcov_unsigned_t crc32, struct gcov_summary *all_prg)
> {
> struct gcov_summary prg; /* summary for this object over all program. */
> int error;
> @@ -753,7 +757,8 @@ gcov_exit_dump_gcov (struct gcov_info *g
> gcov_error ("profiling:%s:Not a gcov data file\n", gi_filename);
> goto read_fatal;
> }
> - error = gcov_exit_merge_gcda (gi_ptr, &prg, &summary_pos, &eof_pos);
> + error = gcov_exit_merge_gcda (gi_ptr, &prg, &summary_pos, &eof_pos,
> + crc32);
> if (error == -1)
> goto read_fatal;
> }
> @@ -766,7 +771,7 @@ gcov_exit_dump_gcov (struct gcov_info *g
> summary_pos = eof_pos;
> }
>
> - error = gcov_exit_merge_summary (gi_ptr, &prg);
> + error = gcov_exit_merge_summary (gi_ptr, &prg, crc32, all_prg);
> if (error == -1)
> goto read_fatal;
>
> @@ -794,19 +799,24 @@ gcov_exit (void)
> {
> struct gcov_info *gi_ptr;
> struct gcov_filename_aux gf;
> + gcov_unsigned_t crc32;
> + struct gcov_summary all_prg;
>
> /* Prevent the counters from being dumped a second time on exit when the
> application already wrote out the profile using __gcov_dump(). */
> if (gcov_dump_complete)
> return;
>
> - gcov_exit_compute_summary ();
> + crc32 = gcov_exit_compute_summary ();
>
> allocate_filename_struct (&gf);
> +#if !GCOV_LOCKED
> + memset (&all_prg, 0, sizeof (all_prg));
> +#endif
>
> /* Now merge each file. */
> for (gi_ptr = gcov_list; gi_ptr; gi_ptr = gi_ptr->next)
> - gcov_exit_dump_gcov (gi_ptr, &gf);
> + gcov_exit_dump_gcov (gi_ptr, &gf, crc32, &all_prg);
>
> if (gi_filename)
> free (gi_filename);
