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

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.


--
This patch is available for review at http://codereview.appspot.com/4983055

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
>

On 11-09-12 10:50 , Gabriel Charette wrote:
> 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.

Thanks.  I'll try this suggestion.

>> @@ -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)).

Yeah, we now end up with the file mentioned twice in the vector of images.


Diego.