This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Resending in plain text mode so that it goes through to mailing list...


On Wed, Jul 25, 2012 at 11:32 AM, Teresa Johnson <tejohnson@google.com> wrote:
>
>
>
>
> On Tue, Jul 24, 2012 at 3:03 PM, Chris Manghane <cmang@google.com> wrote:
>>
>> This patch modifies pmu-profile to allow gooda_feedback access to
>>
>>
> Needs some more context about perf.data -> gcda conversion.
>
>>
>> important gcov to gcda conversion functions and also
>
>
> Not really gcov to gcda conversion functions, but gcov io functions.
>
>> modifies the gcov pmu format store indices
>
> "format to store indices into a..."
>
>>
>> in a string table of filenames as opposed to storing the filename inside of every pmu entry. This reduces the amount of size bloat in gcda, especially since the gcda files do a per asm line analysis and this leads to many entries sharing the same filename data anyways. gooda_feedback considers this and outputs the string following the load latency and branch misprediction information.
>>
>> The changes made belong to one of the following categories:
>>     1. Removing static declaration/defition of certain function that are needed by gooda_feedback (pmu-profile.c)
>>     2. Modifying the gcov format to use indices instead of string for the filename (gcov-io.h)
>>     3. Modifying gcda writing functions to write the index instead of the filename (pmu-profile.c)
>>     4. Removing references to filename (gcov-io.c, pmu-profile.c)
>>
>> This was tested by using crosstool-validate with --testers=crosstool. No other testcases seemed necessary since this patch only modifies data that is relevant to pfmon, which is considered dead now.
>
>
> Needs more testing (discussed with Chris offline).
>
>>
>>
>> The patch should be applied to google/gcc-4_7
>>
>> The CL for gooda_feedback can be found at Google ref c/31972005
>>
>>
>> 2012-07-24  Chris Manghane  <cmang@google.com>
>>
>>         * libgcc/pmu-profile.c (static int parse_pfmon_load_latency): filename field no longer exists
>
>
> Also mention the removal of "static" from the declarations.
>
>>
>>         (parse_load_latency_line): filename field no longer exists
>>         (parse_branch_mispredict_line): filename field no longer exists
>
>
> For repeating descriptions use or "Ditto." or "Likewise."
>
>>         (__gcov_stop_pmu_profiler): filename field no longer exists
>>         (gcov_write_ll_line): now writes string table index instead of filename
>>         (gcov_write_branch_mispredict_line): now writes string table index instead of filename
>>         (gcov_write_load_latency_infos): filename field no longer exists
>>         (gcov_write_branch_mispredict_infos): filename field no longer exists
>>         (gcov_tag_pmu_tool_header_length): filename field no longer exists
>>         (gcov_write_tool_header): filename field no longer exists
>>         * gcc/gcov.c (release_structures): filename field no longer exists
>>         (filter_pmu_data_lines): filename field no longer exists
>>         * gcc/gcov-io.c (gcov_read_pmu_load_latency_info): removed filename field and added string table index
>>         (gcov_read_pmu_branch_mispredict_info): removed filename field and added string table index
>>         (print_load_latency_line): filename field no longer exists
>>         (print_branch_mispredict_line): filename field no longer exists
>>         * gcc/gcov-io.h: added new tag for string table printing
>>         * gcc/gcov-dump.c (tag_pmu_load_latency_info): filename field no longer exists
>>         (tag_pmu_branch_mispredict_info): filename field no longer exists
>>
>> Index: libgcc/pmu-profile.c
>> ===================================================================
>> --- libgcc/pmu-profile.c        (revision 189823)
>> +++ libgcc/pmu-profile.c        (working copy)
>> @@ -232,11 +232,11 @@ static int parse_pfmon_load_latency (char *filenam
>>  static int parse_pfmon_branch_mispredicts (char *filename, void *pmu_data);
>>  static gcov_unsigned_t gcov_tag_pmu_tool_header_length (gcov_pmu_tool_header_t
>>                                                          *header);
>> -static void gcov_write_tool_header (gcov_pmu_tool_header_t *header);
>> -static void gcov_write_load_latency_infos (void *info);
>> -static void gcov_write_branch_mispredict_infos (void *info);
>> -static void gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_info);
>> -static void gcov_write_branch_mispredict_line (const gcov_pmu_brm_info_t
>> +void gcov_write_tool_header (gcov_pmu_tool_header_t *header);
>> +void gcov_write_load_latency_infos (void *info);
>> +void gcov_write_branch_mispredict_infos (void *info);
>> +void gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_info);
>> +void gcov_write_branch_mispredict_line (const gcov_pmu_brm_info_t
>>                                                 *brm_info);
>
>
> They should be moved to gcov-io.h with linkage GCOV_LINKAGE which will pick extern when building libgcov (see examples in gcov-io.h).
>
>>
>>  static int start_addr2line_symbolizer (pid_t pid);
>>  static void end_addr2line_symbolizer (void);
>> @@ -749,14 +749,12 @@ parse_load_latency_line (char *line, gcov_pmu_ll_i
>
>
> I think it would be cleaner just to remove all the pfmon specific handling as part of this checkin, since that is obsolete and wouldn't work with your changes anyway.
>
>>
>>        if (!sep)
>>          {
>>            /* Assume entire string is srcfile.  */
>> -          ll_info->filename = (char *)sym_info;
>>            ll_info->line = 0;
>>          }
>>        else
>>          {
>>            /* Terminate the filename string at the separator.  */
>>            *sep = 0;
>> -          ll_info->filename = (char *)sym_info;
>>            /* Convert rest of the sym info to a line number.  */
>>            ll_info->line = atol (sep+1);
>>          }
>> @@ -765,7 +763,6 @@ parse_load_latency_line (char *line, gcov_pmu_ll_i
>>    else
>>      {
>>        /* No symbolizer available.  */
>> -      ll_info->filename = NULL;
>>        ll_info->line = 0;
>>        ll_info->discriminator = 0;
>>      }
>> @@ -815,14 +812,12 @@ parse_branch_mispredict_line (char *line, gcov_pmu
>>        if (!sep)
>>          {
>>            /* Assume entire string is srcfile.  */
>> -          brm_info->filename = sym_info;
>>            brm_info->line = 0;
>>          }
>>        else
>>          {
>>            /* Terminate the filename string at the separator.  */
>>            *sep = 0;
>> -          brm_info->filename = sym_info;
>>            /* Convert rest of the sym info to a line number.  */
>>            brm_info->line = atol (sep+1);
>>          }
>> @@ -831,7 +826,6 @@ parse_branch_mispredict_line (char *line, gcov_pmu
>>    else
>>      {
>>        /* No symbolizer available.  */
>> -      brm_info->filename = NULL;
>>        brm_info->line = 0;
>>        brm_info->discriminator = 0;
>>      }
>> @@ -1361,11 +1355,10 @@ __gcov_stop_pmu_profiler (void)
>>
>>  /* Write the load latency information LL_INFO into the gcda file.  */
>>
>> -static void
>> +void
>>  gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_info)
>>  {
>> -  gcov_unsigned_t len = GCOV_TAG_PMU_LOAD_LATENCY_LENGTH (ll_info->filename);
>> -  gcov_write_tag_length (GCOV_TAG_PMU_LOAD_LATENCY_INFO, len);
>> +  gcov_write_tag_length (GCOV_TAG_PMU_LOAD_LATENCY_INFO, 1);
>
>
> The length of 1 isn't correct - it should be the length of the data associated with this tag. See  GCOV_TAG_PMU_LOAD_LATENCY_LENGTH for how it is computing this (and you can just modify that to remove the filename parameter and add 1 for your new tag).
>
>>    gcov_write_unsigned (ll_info->counts);
>>    gcov_write_unsigned (ll_info->self);
>>    gcov_write_unsigned (ll_info->cum);
>> @@ -1379,31 +1372,29 @@ gcov_write_ll_line (const gcov_pmu_ll_info_t *ll_i
>>    gcov_write_counter (ll_info->code_addr);
>>    gcov_write_unsigned (ll_info->line);
>>    gcov_write_unsigned (ll_info->discriminator);
>> -  gcov_write_string (ll_info->filename);
>> +  gcov_write_unsigned (ll_info->filetag);
>>  }
>>
>>
>>  /* Write the branch mispredict information BRM_INFO into the gcda file.  */
>>
>> -static void
>> +void
>>  gcov_write_branch_mispredict_line (const gcov_pmu_brm_info_t *brm_info)
>>  {
>> -  gcov_unsigned_t len = GCOV_TAG_PMU_BRANCH_MISPREDICT_LENGTH (
>> -      brm_info->filename);
>> -  gcov_write_tag_length (GCOV_TAG_PMU_BRANCH_MISPREDICT_INFO, len);
>> +  gcov_write_tag_length (GCOV_TAG_PMU_BRANCH_MISPREDICT_INFO, 1);
>
>
> Same thing here with the length computation (see above comment).
>
>>
>>    gcov_write_unsigned (brm_info->counts);
>>    gcov_write_unsigned (brm_info->self);
>>    gcov_write_unsigned (brm_info->cum);
>>    gcov_write_counter (brm_info->code_addr);
>>    gcov_write_unsigned (brm_info->line);
>>    gcov_write_unsigned (brm_info->discriminator);
>> -  gcov_write_string (brm_info->filename);
>> +  gcov_write_unsigned (brm_info->filetag);
>>  }
>>
>>  /* Write load latency information INFO into the gcda file.  The gcda
>>     file has already been opened and is available for writing.  */
>>
>> -static void
>> +void
>>  gcov_write_load_latency_infos (void *info)
>>  {
>>    unsigned i;
>> @@ -1429,7 +1420,7 @@ gcov_write_load_latency_infos (void *info)
>>  /* Write branch mispredict information INFO into the gcda file.  The
>>     gcda file has already been opened and is available for writing.  */
>>
>> -static void
>> +void
>>  gcov_write_branch_mispredict_infos (void *info)
>>  {
>>    unsigned i;
>> @@ -1470,7 +1461,7 @@ gcov_tag_pmu_tool_header_length (gcov_pmu_tool_hea
>>  /* Write tool header into the gcda file. It assumes that the gcda file
>>     has already been opened and is available for writing.  */
>>
>> -static void
>> +void
>>  gcov_write_tool_header (gcov_pmu_tool_header_t *header)
>>  {
>>    gcov_unsigned_t len = gcov_tag_pmu_tool_header_length (header);
>> Index: gcc/gcov.c
>> ===================================================================
>> --- gcc/gcov.c  (revision 189823)
>> +++ gcc/gcov.c  (working copy)
>> @@ -1008,8 +1008,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 +1024,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 */
>> @@ -2277,58 +2273,6 @@ filter_pmu_data_lines (source_t *src)
>>    ll_infos->ll_array = 0;
>>    brm_infos->brm_array = 0;
>>
>> -  /* Go over all the load latency entries and save the ones
>> -     corresponding to this source file.  */
>
>
> Don't want to remove this handling, as it is doing the job of deciding which pmu data matches the current file being compiled. You just need to change the handling of how to access the filename.
>
>>
>> -  for (i = 0; i < pmu_global_info.ll_infos.ll_count; ++i)
>> -    {
>> -      gcov_pmu_ll_info_t *ll_info = pmu_global_info.ll_infos.ll_array[i];
>> -      if (0 == strcmp (src->name, ll_info->filename))
>> -        {
>> -          if (!ll_infos->ll_array)
>> -            {
>> -              ll_infos->ll_count = 0;
>> -              ll_infos->alloc_ll_count = 64;
>> -              ll_infos->ll_array = XCNEWVEC (gcov_pmu_ll_info_t *,
>> -                                             ll_infos->alloc_ll_count);
>> -            }
>> -          /* Found a matching entry, save it.  */
>> -          ll_infos->ll_count++;
>> -          if (ll_infos->ll_count >= ll_infos->alloc_ll_count)
>> -            {
>> -              /* need to realloc */
>> -              ll_infos->ll_array = (gcov_pmu_ll_info_t **)
>> -                xrealloc (ll_infos->ll_array, 2 * ll_infos->alloc_ll_count);
>> -            }
>> -          ll_infos->ll_array[ll_infos->ll_count - 1] = ll_info;
>> -        }
>> -    }
>> -
>> -  /* Go over all the branch mispredict entries and save the ones
>> -     corresponding to this source file.  */
>> -  for (i = 0; i < pmu_global_info.brm_infos.brm_count; ++i)
>> -    {
>> -      gcov_pmu_brm_info_t *brm_info = pmu_global_info.brm_infos.brm_array[i];
>> -      if (0 == strcmp (src->name, brm_info->filename))
>> -        {
>> -          if (!brm_infos->brm_array)
>> -            {
>> -              brm_infos->brm_count = 0;
>> -              brm_infos->alloc_brm_count = 64;
>> -              brm_infos->brm_array = XCNEWVEC (gcov_pmu_brm_info_t *,
>> -                                               brm_infos->alloc_brm_count);
>> -            }
>> -          /* Found a matching entry, save it.  */
>> -          brm_infos->brm_count++;
>> -          if (brm_infos->brm_count >= brm_infos->alloc_brm_count)
>> -            {
>> -              /* need to realloc */
>> -              brm_infos->brm_array = (gcov_pmu_brm_info_t **)
>> -                xrealloc (brm_infos->brm_array, 2 * brm_infos->alloc_brm_count);
>> -            }
>> -          brm_infos->brm_array[brm_infos->brm_count - 1] = brm_info;
>> -        }
>> -    }
>> -
>>    /* Sort the load latency data according to the line numbers because
>>       we later iterate over sources in line number order. Normally we
>>       expect the PMU tool to provide sorted data, but a few entries can
>> Index: gcc/gcov-io.c
>> ===================================================================
>> --- gcc/gcov-io.c       (revision 189823)
>> +++ gcc/gcov-io.c       (working copy)
>> @@ -242,7 +242,6 @@ GCOV_LINKAGE void
>>  gcov_read_pmu_load_latency_info (gcov_pmu_ll_info_t *ll_info,
>>                                   gcov_unsigned_t len ATTRIBUTE_UNUSED)
>>  {
>> -  const char *filename;
>>    ll_info->counts = gcov_read_unsigned ();
>>    ll_info->self = gcov_read_unsigned ();
>>    ll_info->cum = gcov_read_unsigned ();
>> @@ -256,11 +255,7 @@ gcov_read_pmu_load_latency_info (gcov_pmu_ll_info_
>>    ll_info->code_addr = gcov_read_counter ();
>>    ll_info->line = gcov_read_unsigned ();
>>    ll_info->discriminator = gcov_read_unsigned ();
>> -  filename = gcov_read_string ();
>> -  if (filename)
>> -    ll_info->filename = gcov_canonical_filename (xstrdup (filename));
>> -  else
>> -    ll_info->filename = 0;
>> +  ll_info->filetag = gcov_read_unsigned ();
>>  }
>>
>>  /* Read LEN words and construct branch mispredict info BRM_INFO.  */
>> @@ -269,18 +264,13 @@ GCOV_LINKAGE void
>>  gcov_read_pmu_branch_mispredict_info (gcov_pmu_brm_info_t *brm_info,
>>                                        gcov_unsigned_t len ATTRIBUTE_UNUSED)
>>  {
>> -  const char *filename;
>>    brm_info->counts = gcov_read_unsigned ();
>>    brm_info->self = gcov_read_unsigned ();
>>    brm_info->cum = gcov_read_unsigned ();
>>    brm_info->code_addr = gcov_read_counter ();
>>    brm_info->line = gcov_read_unsigned ();
>>    brm_info->discriminator = gcov_read_unsigned ();
>> -  filename = gcov_read_string ();
>> -  if (filename)
>> -    brm_info->filename = gcov_canonical_filename (xstrdup (filename));
>> -  else
>> -    brm_info->filename = 0;
>> +  brm_info->filetag = gcov_read_unsigned ();
>>  }
>>
>>  /* Read LEN words from an open gcov file and construct data into pmu
>> @@ -779,7 +769,7 @@ print_load_latency_line (FILE *fp, const gcov_pmu_
>>    if (!ll_info)
>>      return;
>>    fprintf (fp, " %u %.2f%% %.2f%% %.2f%% %.2f%% %.2f%% %.2f%% %.2f%% "
>> -           "%.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %s %d %d",
>> +           "%.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %d %d %d",
>
>
> Instead of changing this to print the tag, it would be useful to print both the tag and the filename (as accessed via your string table). Ditto below for the branch mispredict info.
>
>>
>>             ll_info->counts,
>>             convert_unsigned_to_pct (ll_info->self),
>>             convert_unsigned_to_pct (ll_info->cum),
>> @@ -791,7 +781,7 @@ print_load_latency_line (FILE *fp, const gcov_pmu_
>>             convert_unsigned_to_pct (ll_info->gt_1024),
>>             convert_unsigned_to_pct (ll_info->wself),
>>             ll_info->code_addr,
>> -           ll_info->filename,
>> +           ll_info->filetag,
>>             ll_info->line,
>>             ll_info->discriminator);
>>    if (newline == add_newline)
>> @@ -807,12 +797,12 @@ print_branch_mispredict_line (FILE *fp, const gcov
>>  {
>>    if (!brm_info)
>>      return;
>> -  fprintf (fp, " %u %.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %s %d %d",
>> +  fprintf (fp, " %u %.2f%% %.2f%% " HOST_WIDEST_INT_PRINT_HEX " %d %d %d",
>>             brm_info->counts,
>>             convert_unsigned_to_pct (brm_info->self),
>>             convert_unsigned_to_pct (brm_info->cum),
>>             brm_info->code_addr,
>> -           brm_info->filename,
>> +           brm_info->filetag,
>>             brm_info->line,
>>             brm_info->discriminator);
>>    if (newline == add_newline)
>
>
> Need to add i/o handling of new string table.
>
>> Index: gcc/gcov-io.h
>> ===================================================================
>> --- gcc/gcov-io.h       (revision 189823)
>> +++ gcc/gcov-io.h       (working copy)
>> @@ -449,6 +449,7 @@ typedef HOST_WIDEST_INT gcov_type;
>>    (gcov_string_length (filename) + 5 + 2)
>>  #define GCOV_TAG_PMU_TOOL_HEADER ((gcov_unsigned_t)0xa9000000)
>>  #define GCOV_TAG_MODULE_INFO ((gcov_unsigned_t)0xab000000)
>> +#define GCOV_TAG_PMU_STRING_TABLE_ENTRY ((gcov_unsigned_t)0xad000000)
>>
>>  /* Counters that are collected.  */
>>  #define GCOV_COUNTER_ARCS      0  /* Arc transitions.  */
>> @@ -635,7 +636,7 @@ typedef struct gcov_pmu_load_latency_info
>>    gcov_type code_addr;        /* the actual miss address (pc+1 for Intel) */
>>    gcov_unsigned_t line;       /* line number corresponding to this miss */
>>    gcov_unsigned_t discriminator;   /* discriminator information for this miss */
>> -  char *filename;       /* filename corresponding to this miss */
>> +  gcov_unsigned_t filetag;       /* location in string table of filename corresponding to event */
>>  } gcov_pmu_ll_info_t;
>>
>>  /* This structure is used during runtime as well as in gcov.  */
>> @@ -665,7 +666,7 @@ typedef struct gcov_pmu_branch_mispredict_info
>>    gcov_type code_addr;        /* the actual mispredict address */
>>    gcov_unsigned_t line;       /* line number corresponding to this event */
>>    gcov_unsigned_t discriminator;   /* discriminator for this event */
>> -  char *filename;       /* filename corresponding to this event */
>> +  gcov_unsigned_t filetag;       /* location in string table of filename corresponding to event */
>>  } gcov_pmu_brm_info_t;
>>
>>  /* This structure is used during runtime as well as in gcov.  */
>> Index: gcc/gcov-dump.c
>> ===================================================================
>> --- gcc/gcov-dump.c     (revision 189823)
>> +++ gcc/gcov-dump.c     (working copy)
>> @@ -573,7 +573,6 @@ tag_pmu_load_latency_info (const char *filename AT
>>
>>    gcov_pmu_ll_info_t ll_info;
>>    gcov_read_pmu_load_latency_info (&ll_info, length);
>>    print_load_latency_line (stdout, &ll_info, no_newline);
>> -  free (ll_info.filename);
>>  }
>>
>>  /* Read gcov tag GCOV_TAG_PMU_BRANCH_MISPREDICT_INFO from the gcda
>> @@ -586,7 +585,6 @@ tag_pmu_branch_mispredict_info (const char *filena
>>    gcov_pmu_brm_info_t brm_info;
>>    gcov_read_pmu_branch_mispredict_info (&brm_info, length);
>>    print_branch_mispredict_line (stdout, &brm_info, no_newline);
>> -  free (brm_info.filename);
>>  }
>
>
> Need to add printing of new string table.
>
> Teresa
>>
>>
>>
>>
>> --
>> This patch is available for review at http://codereview.appspot.com/6427063
>
>
>
>
> --
> Teresa Johnson | Software Engineer |  tejohnson@google.com |  408-460-2413
>



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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]