* Fix summary generation with fork
@ 2013-11-19 1:23 Jan Hubicka
2013-11-19 5:03 ` Jan Hubicka
2013-11-19 20:14 ` Rong Xu
0 siblings, 2 replies; 3+ messages in thread
From: Jan Hubicka @ 2013-11-19 1:23 UTC (permalink / raw)
To: xur, gcc-patches, marxin.liska
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.
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);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Fix summary generation with fork
2013-11-19 1:23 Fix summary generation with fork Jan Hubicka
@ 2013-11-19 5:03 ` Jan Hubicka
2013-11-19 20:14 ` Rong Xu
1 sibling, 0 replies; 3+ messages in thread
From: Jan Hubicka @ 2013-11-19 5:03 UTC (permalink / raw)
To: Jan Hubicka; +Cc: xur, gcc-patches, marxin.liska
> 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.
>
> 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.
Hi,
it seems I forgot the following hunk in my tree. With fork we now get
errors about corrupted profile info: run_max * runs < sum_max.
Obviously problem here is again about definition what is run and what
is maximum per run. I think run_max is quite pointless part of our summary
so I think I will drop it, but first I need to look into the histogram code.
I apologize for the breakage.
Honza
Index: ChangeLog
===================================================================
--- ChangeLog (revision 204984)
+++ ChangeLog (working copy)
@@ -1,3 +1,7 @@
+2013-11-18 Jan Hubicka <jh@suse.cz>
+
+ * profile.c (compute_branch_probabilities): Do not sanity check run_max.
+
2013-11-18 Bernd Schmidt <bernds@codesourcery.com>
* cgraphunit.c (ipa_passes): Don't execute all_lto_gen_passes.
Index: profile.c
===================================================================
--- profile.c (revision 204984)
+++ profile.c (working copy)
@@ -528,11 +528,6 @@ compute_branch_probabilities (unsigned c
/* Very simple sanity checks so we catch bugs in our profiling code. */
if (!profile_info)
return;
- if (profile_info->run_max * profile_info->runs < profile_info->sum_max)
- {
- error ("corrupted profile info: run_max * runs < sum_max");
- exec_counts = NULL;
- }
if (profile_info->sum_all < profile_info->sum_max)
{
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Fix summary generation with fork
2013-11-19 1:23 Fix summary generation with fork Jan Hubicka
2013-11-19 5:03 ` Jan Hubicka
@ 2013-11-19 20:14 ` Rong Xu
1 sibling, 0 replies; 3+ messages in thread
From: Rong Xu @ 2013-11-19 20:14 UTC (permalink / raw)
To: Jan Hubicka; +Cc: GCC Patches, Martin Liška
On Mon, Nov 18, 2013 at 2:15 PM, Jan Hubicka <hubicka@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);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-19 18:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-19 1:23 Fix summary generation with fork Jan Hubicka
2013-11-19 5:03 ` Jan Hubicka
2013-11-19 20:14 ` Rong Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).