Re: [pph] Do not read pph files more than once (issue4983055)

[Gabriel Charette <gcharette1@gmail.com>]

Oops forgot to reply all the first time...

On Fri, Sep 9, 2011 at 4:54 PM, Diego Novillo <dnovillo@google.com> wrote:
> This was not causing any failures, but it is pretty wasteful to read
> the same PPH more than once.
>
> We cannot just skip them, however.  We need to read the line table to
> properly modify the line table for the current translation unit.

I don't think that's right. If the header is not re-read (i.e. ifdef
guarded out), it should not show in the line_table either. I think you
simply want to do this check at the top of pph_read_file itself.

>
>  /* Read PPH file FILENAME.  Return the in-memory pph_stream instance.  */
>
> -pph_stream *
> +void
>  pph_read_file (const char *filename)
>  {
>   pph_stream *stream;
> @@ -1667,7 +1696,7 @@ pph_read_file (const char *filename)
>   else
>     error ("Cannot open PPH file for reading: %s: %m", filename);
>
> -  return stream;
> +  pph_add_read_image (stream);
>  }

This needs to be after the check for an already read image I think
(having it here wouldn't create test failures, but would create
duplicate entries for skipped headers (as right now they are still
added to pph_read_images when skipped I think)).

Cheers!,
Gab

On Fri, Sep 9, 2011 at 4:54 PM, Diego Novillo <dnovillo@google.com> wrote:
> This was not causing any failures, but it is pretty wasteful to read
> the same PPH more than once.
>
> We cannot just skip them, however.  We need to read the line table to
> properly modify the line table for the current translation unit.
>
> Tested on x86_64.  Committed to branch.
>
>
> Diego.
>
>
>        * pph-streamer-in.c (pph_image_already_read): New.
>        (pph_read_file_1): Call pph_image_already_read.  If it returns
>        true, return after reading the line table.
>        (pph_add_read_image): New.
>        (pph_read_file): Change return value to void.  Update all callers.
>        Call pph_add_read_image.
>        * pph-streamer-out.c (pph_add_include): Remove second argument.
>        Update all callers.
>        Do not add INCLUDE to pph_read_images.
>        * pph-streamer.h (pph_add_include): Update prototype.
>
> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
> index b111850..ea44460 100644
> --- a/gcc/cp/pph-streamer-in.c
> +++ b/gcc/cp/pph-streamer-in.c
> @@ -1462,7 +1462,6 @@ pph_in_include (pph_stream *stream)
>  {
>   int old_loc_offset;
>   const char *include_name;
> -  pph_stream *include;
>   source_location prev_start_loc = pph_in_source_location (stream);
>
>   /* Simulate highest_location to be as it would be at this point in a non-pph
> @@ -1475,8 +1474,7 @@ pph_in_include (pph_stream *stream)
>   old_loc_offset = pph_loc_offset;
>
>   include_name = pph_in_string (stream);
> -  include = pph_read_file (include_name);
> -  pph_add_include (include, false);
> +  pph_read_file (include_name);
>
>   pph_loc_offset = old_loc_offset;
>  }
> @@ -1583,6 +1581,23 @@ pph_in_line_table_and_includes (pph_stream *stream)
>  }
>
>
> +/* If FILENAME has already been read, return the stream associated with it.  */
> +
> +static pph_stream *
> +pph_image_already_read (const char *filename)
> +{
> +  pph_stream *include;
> +  unsigned i;
> +
> +  /* FIXME pph, implement a hash map to avoid this linear search.  */
> +  FOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, include)
> +    if (strcmp (include->name, filename) == 0)
> +      return include;
> +
> +  return NULL;
> +}
> +
> +
>  /* Helper for pph_read_file.  Read contents of PPH file in STREAM.  */
>
>  static void
> @@ -1605,6 +1620,11 @@ pph_read_file_1 (pph_stream *stream)
>      read.  */
>   cpp_token_replay_loc = pph_in_line_table_and_includes (stream);
>
> +  /* If we have read STREAM before, we do not need to re-read the rest
> +     of its body.  We only needed to read its line table.  */
> +  if (pph_image_already_read (stream->name))
> +    return;
> +
>   /* Read all the identifiers and pre-processor symbols in the global
>      namespace.  */
>   pph_in_identifiers (stream, &idents_used);
> @@ -1650,13 +1670,22 @@ pph_read_file_1 (pph_stream *stream)
>      STREAM will need to be read again the next time we want to read
>      the image we are now generating.  */
>   if (pph_out_file && !pph_reading_includes)
> -    pph_add_include (stream, true);
> +    pph_add_include (stream);
> +}
> +
> +
> +/* Add STREAM to the list of read images.  */
> +
> +static void
> +pph_add_read_image (pph_stream *stream)
> +{
> +  VEC_safe_push (pph_stream_ptr, heap, pph_read_images, stream);
>  }
>
>
>  /* Read PPH file FILENAME.  Return the in-memory pph_stream instance.  */
>
> -pph_stream *
> +void
>  pph_read_file (const char *filename)
>  {
>   pph_stream *stream;
> @@ -1667,7 +1696,7 @@ pph_read_file (const char *filename)
>   else
>     error ("Cannot open PPH file for reading: %s: %m", filename);
>
> -  return stream;
> +  pph_add_read_image (stream);
>  }
>
>
> diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
> index 264294c..1a32814 100644
> --- a/gcc/cp/pph-streamer-out.c
> +++ b/gcc/cp/pph-streamer-out.c
> @@ -1845,18 +1845,13 @@ pph_add_decl_to_symtab (tree decl, enum pph_symtab_action action,
>  }
>
>
> -/* Add INCLUDE to the list of files included by  the main pph_out_stream
> -   if IS_MAIN_STREAM_INCLUDE, as well as to the global list of all read
> -   includes.  */
> +/* Add INCLUDE to the list of files included by pph_out_stream.  */
>
>  void
> -pph_add_include (pph_stream *include, bool is_main_stream_include)
> +pph_add_include (pph_stream *include)
>  {
> -  if (is_main_stream_include)
> -    VEC_safe_push (pph_stream_ptr, heap,
> -                  pph_out_stream->encoder.w.includes, include);
> -
> -  VEC_safe_push (pph_stream_ptr, heap, pph_read_images, include);
> +  VEC_safe_push (pph_stream_ptr, heap, pph_out_stream->encoder.w.includes,
> +                include);
>  }
>
>
> diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
> index 7205d64..7f98764 100644
> --- a/gcc/cp/pph-streamer.h
> +++ b/gcc/cp/pph-streamer.h
> @@ -255,7 +255,7 @@ void pph_flush_buffers (pph_stream *);
>  void pph_init_write (pph_stream *);
>  void pph_write_tree (struct output_block *, tree, bool);
>  void pph_add_decl_to_symtab (tree, enum pph_symtab_action, bool, bool);
> -void pph_add_include (pph_stream *, bool);
> +void pph_add_include (pph_stream *);
>  void pph_writer_init (void);
>  void pph_writer_finish (void);
>  void pph_write_location (struct output_block *, location_t);
> @@ -269,7 +269,7 @@ struct binding_table_s *pph_in_binding_table (pph_stream *);
>  void pph_init_read (pph_stream *);
>  tree pph_read_tree (struct lto_input_block *, struct data_in *);
>  location_t pph_read_location (struct lto_input_block *, struct data_in *);
> -pph_stream *pph_read_file (const char *);
> +void pph_read_file (const char *);
>  void pph_reader_finish (void);
>
>  /* In pt.c.  */
>
> --
> This patch is available for review at http://codereview.appspot.com/4983055
>