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/

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)

[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)

[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)

[cmang]

On 2012/08/24 23:20:21, davidxl wrote:

On Fri, Aug 24, 2012 at 3:56 PM,   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/