Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[cmang]

Hi Rong,

Could you take a look at the patch I mailed to gcc-patches when you get
a chance?

It reduces the gcda size bloat issue by replacing gcov_pmu data with a
filetag field that holds the position of the correct filename inside of
the newly added string table.

Thanks,
Chris

http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[davidxl]

Ok for google branches.

David




http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[Teresa Johnson]

On Mon, Aug 27, 2012 at 10:55 AM,  <cmang@google.com> wrote:
> On 2012/08/24 23:20:21, davidxl wrote:
>>
>> On Fri, Aug 24, 2012 at 3:56 PM,  <mailto:cmang@google.com> wrote:
>> >
>> > http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h
>> > File gcc/gcov-io.h (right):
>> >
>> >
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
>>
>> > gcc/gcov-io.h:688: gcov_unsigned_t index;   /* The corresponding
>
> string
>>
>> > table index */
>> > On 2012/08/24 22:30:30, davidxl wrote:
>> >>
>> >> Is this field necessary?
>> >
>> >
>> > For the purposes of gcov_dump output we wanted to output not only
>
> the
>>
>> > string table entry but also its index in the string table just in
>
> case
>>
>> > there were any problems with ordering that might map filetags to the
>> > wrong filename. Because of this and the fact that gcov.c and
>
> gcov-dump.c
>>
>> > read one entry at a time, it seemed better to have the index be
>> > self-contained within the struct to avoid extra logic.
>
>
>
>> That is fine -- but you probably need to add assertion check when the
>> string table is read in.
>
>
>> >
>> > Also, since the string table entry has to be written with its
>> > corresponding index for gcov-dump, the gcov_write_string_table_entry
>> > function could have a similar syntax to the ll/brm writing
>
> functions.
>>
>> >
>> >
>> >
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
>>
>> > gcc/gcov-io.h:689: char* filename;          /* The filename that
>
> belongs
>>
>> > at this index */
>> > On 2012/08/24 22:30:30, davidxl wrote:
>> >>
>> >> Can this field name be generalized?
>> >
>> >
>> > Generalized in what way? I used this name because it was used before
>
> in
>>
>> > the old gcda format.
>
>
>> Since it is string table, it can be something like str_ or strval_
>
> etc.
>
>
> Done. filename will be changed to str in an upcoming patch.
>
>
>> >
>> >
>> >
>
> http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
>>
>> > File libgcc/pmu-profile.c (right):
>> >
>> >
>
>
> http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
>>
>> > libgcc/pmu-profile.c:83:
>> > On 2012/08/24 22:30:30, davidxl wrote:
>> >>
>> >> Who is the caller to this interface?
>> >
>> >
>> >> It should be declared in gcov-io.h
>> >
>> >
>> > The only current caller of this is
>> > https://critique.corp.google.com/#review/31972005. This is also the
>
> same
>>
>> > for the other two ll/brm writing functions. Should I declare those
>
> in
>>
>> > gcov-io.h as well?
>
>
>> Any utilities functions that may be be used by clients other than gcov
>> tool should be in the common header.
>
>
>
> However, I don't really know how to answer this. It seems that if I move
> the mentioned utility functions to gcov-io.h, I should also move their
> helper functions, like convert_unsigned_to_pct, to gcov-io.h as well and
> completely remove pmu-profile.c. Does this seem like a reasonable
> solution?

I think the idea is to put the declarations into gcov-io.h since they
are no longer static. But you could leave the implementation in
pmu-profile.c

Teresa

>
>> David
>
>
>> >
>> > http://codereview.appspot.com/6427063/
>
>
>
>
> http://codereview.appspot.com/6427063/



-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[cmang]

On 2012/08/24 23:20:21, davidxl wrote:
> On Fri, Aug 24, 2012 at 3:56 PM,  <mailto:cmang@google.com> wrote:
> >
> > http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h
> > File gcc/gcov-io.h (right):
> >
> >
http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
> > gcc/gcov-io.h:688: gcov_unsigned_t index;   /* The corresponding
string
> > table index */
> > On 2012/08/24 22:30:30, davidxl wrote:
> >>
> >> Is this field necessary?
> >
> >
> > For the purposes of gcov_dump output we wanted to output not only
the
> > string table entry but also its index in the string table just in
case
> > there were any problems with ordering that might map filetags to the
> > wrong filename. Because of this and the fact that gcov.c and
gcov-dump.c
> > read one entry at a time, it seemed better to have the index be
> > self-contained within the struct to avoid extra logic.


> That is fine -- but you probably need to add assertion check when the
> string table is read in.

> >
> > Also, since the string table entry has to be written with its
> > corresponding index for gcov-dump, the gcov_write_string_table_entry
> > function could have a similar syntax to the ll/brm writing
functions.
> >
> >
> >
http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
> > gcc/gcov-io.h:689: char* filename;          /* The filename that
belongs
> > at this index */
> > On 2012/08/24 22:30:30, davidxl wrote:
> >>
> >> Can this field name be generalized?
> >
> >
> > Generalized in what way? I used this name because it was used before
in
> > the old gcda format.

> Since it is string table, it can be something like str_ or strval_
etc.


Done. filename will be changed to str in an upcoming patch.

> >
> >
> >
http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
> > File libgcc/pmu-profile.c (right):
> >
> >

http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
> > libgcc/pmu-profile.c:83:
> > On 2012/08/24 22:30:30, davidxl wrote:
> >>
> >> Who is the caller to this interface?
> >
> >
> >> It should be declared in gcov-io.h
> >
> >
> > The only current caller of this is
> > https://critique.corp.google.com/#review/31972005. This is also the
same
> > for the other two ll/brm writing functions. Should I declare those
in
> > gcov-io.h as well?

> Any utilities functions that may be be used by clients other than gcov
> tool should be in the common header.


However, I don't really know how to answer this. It seems that if I move
the mentioned utility functions to gcov-io.h, I should also move their
helper functions, like convert_unsigned_to_pct, to gcov-io.h as well and
completely remove pmu-profile.c. Does this seem like a reasonable
solution?

> David

> >
> > http://codereview.appspot.com/6427063/



http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[Xinliang David Li]

On Fri, Aug 24, 2012 at 3:56 PM,  <cmang@google.com> wrote:
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h
> File gcc/gcov-io.h (right):
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
> gcc/gcov-io.h:688: gcov_unsigned_t index;   /* The corresponding string
> table index */
> On 2012/08/24 22:30:30, davidxl wrote:
>>
>> Is this field necessary?
>
>
> For the purposes of gcov_dump output we wanted to output not only the
> string table entry but also its index in the string table just in case
> there were any problems with ordering that might map filetags to the
> wrong filename. Because of this and the fact that gcov.c and gcov-dump.c
> read one entry at a time, it seemed better to have the index be
> self-contained within the struct to avoid extra logic.


That is fine -- but you probably need to add assertion check when the
string table is read in.

>
> Also, since the string table entry has to be written with its
> corresponding index for gcov-dump, the gcov_write_string_table_entry
> function could have a similar syntax to the ll/brm writing functions.
>
>
> http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
> gcc/gcov-io.h:689: char* filename;          /* The filename that belongs
> at this index */
> On 2012/08/24 22:30:30, davidxl wrote:
>>
>> Can this field name be generalized?
>
>
> Generalized in what way? I used this name because it was used before in
> the old gcda format.

Since it is string table, it can be something like str_ or strval_ etc.

>
>
> http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
> File libgcc/pmu-profile.c (right):
>
> http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
> libgcc/pmu-profile.c:83:
> On 2012/08/24 22:30:30, davidxl wrote:
>>
>> Who is the caller to this interface?
>
>
>> It should be declared in gcov-io.h
>
>
> The only current caller of this is
> https://critique.corp.google.com/#review/31972005. This is also the same
> for the other two ll/brm writing functions. Should I declare those in
> gcov-io.h as well?

Any utilities functions that may be be used by clients other than gcov
tool should be in the common header.

David

>
> http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[cmang]

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h
File gcc/gcov-io.h (right):

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
gcc/gcov-io.h:688: gcov_unsigned_t index;   /* The corresponding string
table index */
On 2012/08/24 22:30:30, davidxl wrote:
> Is this field necessary?

For the purposes of gcov_dump output we wanted to output not only the
string table entry but also its index in the string table just in case
there were any problems with ordering that might map filetags to the
wrong filename. Because of this and the fact that gcov.c and gcov-dump.c
read one entry at a time, it seemed better to have the index be
self-contained within the struct to avoid extra logic.

Also, since the string table entry has to be written with its
corresponding index for gcov-dump, the gcov_write_string_table_entry
function could have a similar syntax to the ll/brm writing functions.

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
gcc/gcov-io.h:689: char* filename;          /* The filename that belongs
at this index */
On 2012/08/24 22:30:30, davidxl wrote:
> Can this field name be generalized?

Generalized in what way? I used this name because it was used before in
the old gcda format.

http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
File libgcc/pmu-profile.c (right):

http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
libgcc/pmu-profile.c:83:
On 2012/08/24 22:30:30, davidxl wrote:
> Who is the caller to this interface?

> It should be declared in gcov-io.h

The only current caller of this is
https://critique.corp.google.com/#review/31972005. This is also the same
for the other two ll/brm writing functions. Should I declare those in
gcov-io.h as well?

http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[davidxl]

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h
File gcc/gcov-io.h (right):

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
gcc/gcov-io.h:688: gcov_unsigned_t index;   /* The corresponding string
table index */
Is this field necessary?

http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
gcc/gcov-io.h:689: char* filename;          /* The filename that belongs
at this index */
Can this field name be generalized?

http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
File libgcc/pmu-profile.c (right):

http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
libgcc/pmu-profile.c:83:
Who is the caller to this interface?

It should be declared in gcov-io.h

http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[davidxl]

On 2012/08/24 21:51:24, cmang wrote:
> Fixed formatting issues.

The gcov.c is still not uploaded properly.

> ===================================================================
> --- gcc/gcov.c	(revision 190359)
> +++ gcc/gcov.c	(working copy)
> @@ -222,6 +222,7 @@ typedef struct pmu_data
>   {
>     ll_infos_t ll_infos;
>     brm_infos_t brm_infos;
> +  string_table_t string_table;
>   } pmu_data_t;

>   /* Describes a single line of source. Contains a chain of basic
blocks
> @@ -975,6 +976,7 @@ release_structures (void)
>     function_t *fn;
>     ll_infos_t *ll_infos = &pmu_global_info.ll_infos;
>     brm_infos_t *brm_infos = &pmu_global_info.brm_infos;
> +  string_table_t *string_table = &pmu_global_info.string_table;

>     for (ix = n_sources; ix--;)
>       {
> @@ -1008,8 +1010,6 @@ release_structures (void)
>         /* delete each element */
>         for (i = 0; i < ll_infos->ll_count; ++i)
>           {
> -          if (ll_infos->ll_array[i]->filename)
> -            XDELETE (ll_infos->ll_array[i]->filename);
>             XDELETE (ll_infos->ll_array[i]);
>           }
>         /* delete the array itself */
> @@ -1026,8 +1026,6 @@ release_structures (void)
>         /* delete each element */
>         for (i = 0; i < brm_infos->brm_count; ++i)
>           {
> -          if (brm_infos->brm_array[i]->filename)
> -            XDELETE (brm_infos->brm_array[i]->filename);
>             XDELETE (brm_infos->brm_array[i]);
>           }
>         /* delete the array itself */
> @@ -1035,6 +1033,22 @@ release_structures (void)
>         brm_infos->brm_array = NULL;
>         brm_infos->brm_count = 0;
>       }
> +
> +  /* Cleanup PMU string table entries. */
> +  if (string_table->st_count)
> +    {
> +      unsigned i;
> +
> +      /* delete each element */
> +      for (i = 0; i < string_table->st_count; ++i)
> +        {
> +          XDELETE (string_table->st_array[i]);
> +        }

Remove {}.




http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[Xinliang David Li]

I don't see any code where a string table is declared nor created ...

David


On Fri, Aug 24, 2012 at 2:26 PM, Chris Manghane <cmang@google.com> wrote:
> Also, what did you mean by there being missing string table management code?
>
>
> On Fri, Aug 24, 2012 at 2:23 PM, <cmang@google.com> wrote:
>>
>>
>> Ok, I fixed most of the problem you found. I'm not sure what happened
>> with gcov.c, but I'll try uploading it again.
>>
>>
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c
>> File gcc/gcov-io.c (right):
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode280
>> gcc/gcov-io.c:280: gcov_read_pmu_string_table_entry
>> (gcov_pmu_st_entry_t* st_entry,
>> On 2012/08/24 20:42:03, davidxl wrote:
>>>
>>> Fix format:
>>
>>
>>> ..entry_t  *st_entry,
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode281
>> gcc/gcov-io.c:281: gcov_unsigned_t len ATTRIBUTE_UNUSED)
>> On 2012/08/24 20:42:03, davidxl wrote:
>>>
>>> Why having an unused parameter? Can it be used in assertion check?
>>
>>
>> I'm following the format of the pmu_branch_mispredict/load_latency_info
>> function defined above. Should I remove the ATTRIBUTE_UNUSED symbol from
>> them as well?
>>
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode830
>> gcc/gcov-io.c:830: print_pmu_string_table_entry (FILE *fp, const
>> gcov_pmu_st_entry_t* st_entry,
>> On 2012/08/24 20:42:03, davidxl wrote:
>>>
>>> Fix format.
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode831
>> gcc/gcov-io.c:831: const enum print_newline newline) {
>> On 2012/08/24 20:42:03, davidxl wrote:
>>>
>>> '{' goes to the new line.
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h
>> File gcc/gcov-io.h (right):
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h#newcode699
>> gcc/gcov-io.h:699: Used for bookkeeping.  */
>> On 2012/08/24 20:42:03, davidxl wrote:
>>>
>>> typo.
>>
>>
>> Sorry, I don't notice the typo here. This line is copied from
>> load_latency/branch_mispredict_infos.
>>
>>
>> http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h#newcode916
>> gcc/gcov-io.h:916: const enum print_newline);
>> On 2012/08/24 20:42:03, davidxl wrote:
>>>
>>> Fix indentation.
>>
>>
>> Done.
>>
>> http://codereview.appspot.com/6427063/
>
>

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[cmang]

Ok, I fixed most of the problem you found. I'm not sure what happened
with gcov.c, but I'll try uploading it again.


http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c
File gcc/gcov-io.c (right):

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode280
gcc/gcov-io.c:280: gcov_read_pmu_string_table_entry
(gcov_pmu_st_entry_t* st_entry,
On 2012/08/24 20:42:03, davidxl wrote:
> Fix format:

> ..entry_t  *st_entry,

Done.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode281
gcc/gcov-io.c:281: gcov_unsigned_t len ATTRIBUTE_UNUSED)
On 2012/08/24 20:42:03, davidxl wrote:
> Why having an unused parameter? Can it be used in assertion check?

I'm following the format of the pmu_branch_mispredict/load_latency_info
function defined above. Should I remove the ATTRIBUTE_UNUSED symbol from
them as well?

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode830
gcc/gcov-io.c:830: print_pmu_string_table_entry (FILE *fp, const
gcov_pmu_st_entry_t* st_entry,
On 2012/08/24 20:42:03, davidxl wrote:
> Fix format.

Done.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode831
gcc/gcov-io.c:831: const enum print_newline newline) {
On 2012/08/24 20:42:03, davidxl wrote:
> '{' goes to the new line.

Done.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h
File gcc/gcov-io.h (right):

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h#newcode699
gcc/gcov-io.h:699: Used for bookkeeping.  */
On 2012/08/24 20:42:03, davidxl wrote:
> typo.

Sorry, I don't notice the typo here. This line is copied from
load_latency/branch_mispredict_infos.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h#newcode916
gcc/gcov-io.h:916: const enum print_newline);
On 2012/08/24 20:42:03, davidxl wrote:
> Fix indentation.

Done.

http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[davidxl]

Where is the string table management code?  The gcov.c file is not
properly uploaded either.

David


http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c
File gcc/gcov-io.c (right):

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode280
gcc/gcov-io.c:280: gcov_read_pmu_string_table_entry
(gcov_pmu_st_entry_t* st_entry,
Fix format:

..entry_t  *st_entry,

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode281
gcc/gcov-io.c:281: gcov_unsigned_t len ATTRIBUTE_UNUSED)
Why having an unused parameter? Can it be used in assertion check?

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode830
gcc/gcov-io.c:830: print_pmu_string_table_entry (FILE *fp, const
gcov_pmu_st_entry_t* st_entry,
Fix format.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.c#newcode831
gcc/gcov-io.c:831: const enum print_newline newline) {
'{' goes to the new line.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h
File gcc/gcov-io.h (right):

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h#newcode699
gcc/gcov-io.h:699: Used for bookkeeping.  */
typo.

http://codereview.appspot.com/6427063/diff/5001/gcc/gcov-io.h#newcode916
gcc/gcov-io.h:916: const enum print_newline);
Fix indentation.

http://codereview.appspot.com/6427063/

Re: [google] Modification of gcov pmu format to reduce gcda size bloat (issue 6427063)

[cmang]

Hi David,

Could you take a look at the patch I mailed to gcc-patches when you get
a chance?

Thanks,
Chris

http://codereview.appspot.com/6427063/