Avoid unnecesary GGC runs during LTO

Hi,
while looking into -ftime-report, I noticed that ggc can take up to 10% of WPA memory
while it does almost nothing: it is run just after streaming that explicitly
frees memory that becomes unreachable.  The first GGC run usually saves at
most 1% of memory and then it is never run again.
I believe this ought to also help in case we get into swap, since ltranses will
also ggc less.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* lto.c (read_cgraph_and_symbols): Grow ggc memory after streaming.
	* ggc.h (ggc_grow): New function.
	* ggc-none.c (ggc_grow): New function.
	* ggc-page.c (ggc_grow): Likewise.

On Fri, 11 Apr 2014, Jan Hubicka wrote:

> 
> Hi,
> while looking into -ftime-report, I noticed that ggc can take up to 10% of WPA memory
> while it does almost nothing: it is run just after streaming that explicitly
> frees memory that becomes unreachable.  The first GGC run usually saves at
> most 1% of memory and then it is never run again.
> I believe this ought to also help in case we get into swap, since ltranses will
> also ggc less.
> 
> Bootstrapped/regtested x86_64-linux, OK?
> 
> Honza
> 
> 	* lto.c (read_cgraph_and_symbols): Grow ggc memory after streaming.
> 	* ggc.h (ggc_grow): New function.
> 	* ggc-none.c (ggc_grow): New function.
> 	* ggc-page.c (ggc_grow): Likewise.
> Index: ggc.h
> ===================================================================
> --- ggc.h	(revision 209170)
> +++ ggc.h	(working copy)
> @@ -225,6 +225,9 @@ extern const char *ggc_alloc_string_stat
>     function is called, not during allocations.  */
>  extern void ggc_collect	(void);
>  
> +/* Assume that all GGC memory is reachable and grow the limits for next collection. */
> +extern void ggc_grow (void);
> +
>  /* Register an additional root table.  This can be useful for some
>     plugins.  Does nothing if the passed pointer is NULL. */
>  extern void ggc_register_root_tab (const struct ggc_root_tab *);
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 209170)
> +++ lto/lto.c	(working copy)
> @@ -2999,6 +3000,10 @@ read_cgraph_and_symbols (unsigned nfiles
>    gimple_canonical_types = NULL;
>    delete canonical_type_hash_cache;
>    canonical_type_hash_cache = NULL;
> +
> +  /* At this stage we know that majority of GGC memory is reachable.  
> +     Growing the limits prevents unnecesary invocation of GGC.  */
> +  ggc_grow ();
>    ggc_collect ();

Isn't the collect here pointless?  I see not in ENABLE_CHECKING, but
shouldn't this be abstracted away, thus call ggc_collect from ggc_grow?
Or maybe rather even for ENABLE_CHECKING adjust G.allocated_last_gc
and simply drop the ggc_collect above ().

Anyway, this is sth for stage1 at this point.

Thanks,
Richard.

>    /* Set the hooks so that all of the ipa passes can read in their data.  */
> Index: ggc-none.c
> ===================================================================
> --- ggc-none.c	(revision 209170)
> +++ ggc-none.c	(working copy)
> @@ -63,3 +63,8 @@ ggc_free (void *p)
>  {
>    free (p);
>  }
> +
> +void
> +ggc_grow (void)
> +{
> +}
> Index: ggc-page.c
> ===================================================================
> --- ggc-page.c	(revision 209170)
> +++ ggc-page.c	(working copy)
> @@ -2095,6 +2095,19 @@ ggc_collect (void)
>      fprintf (G.debug_file, "END COLLECTING\n");
>  }
>  
> +/* Assume that all GGC memory is reachable and grow the limits for next collection. */
> +
> +void
> +ggc_grow (void)
> +{
> +#ifndef ENABLE_CHECKING
> +  G.allocated_last_gc = MAX (G.allocated_last_gc,
> +			     G.allocated);
> +#endif
> +  if (!quiet_flag)
> +    fprintf (stderr, " {GC start %luk} ", (unsigned long) G.allocated / 1024);
> +}
> +
>  /* Print allocation statistics.  */
>  #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
>  		  ? (x) \
> 
>

On 04/11/2014 08:07 AM, Jan Hubicka wrote:
> Hi,
> while looking into -ftime-report, I noticed that ggc can take up to 10% of WPA memory
> while it does almost nothing: it is run just after streaming that explicitly
> frees memory that becomes unreachable.  The first GGC run usually saves at
> most 1% of memory and then it is never run again.
> I believe this ought to also help in case we get into swap, since ltranses will
> also ggc less.
>
> Bootstrapped/regtested x86_64-linux, OK?
Hi!

I applied both patches you sent today and there are Firefox LTO -O3 
results: 
https://drive.google.com/file/d/0B0pisUJ80pO1ajRzLWFneTJpcE0/edit?usp=sharing
It shows that you saved a bit memory in WPA.

Martin

>
> Honza
>
> 	* lto.c (read_cgraph_and_symbols): Grow ggc memory after streaming.
> 	* ggc.h (ggc_grow): New function.
> 	* ggc-none.c (ggc_grow): New function.
> 	* ggc-page.c (ggc_grow): Likewise.
> Index: ggc.h
> ===================================================================
> --- ggc.h	(revision 209170)
> +++ ggc.h	(working copy)
> @@ -225,6 +225,9 @@ extern const char *ggc_alloc_string_stat
>      function is called, not during allocations.  */
>   extern void ggc_collect	(void);
>   
> +/* Assume that all GGC memory is reachable and grow the limits for next collection. */
> +extern void ggc_grow (void);
> +
>   /* Register an additional root table.  This can be useful for some
>      plugins.  Does nothing if the passed pointer is NULL. */
>   extern void ggc_register_root_tab (const struct ggc_root_tab *);
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 209170)
> +++ lto/lto.c	(working copy)
> @@ -2999,6 +3000,10 @@ read_cgraph_and_symbols (unsigned nfiles
>     gimple_canonical_types = NULL;
>     delete canonical_type_hash_cache;
>     canonical_type_hash_cache = NULL;
> +
> +  /* At this stage we know that majority of GGC memory is reachable.
> +     Growing the limits prevents unnecesary invocation of GGC.  */
> +  ggc_grow ();
>     ggc_collect ();
>   
>     /* Set the hooks so that all of the ipa passes can read in their data.  */
> Index: ggc-none.c
> ===================================================================
> --- ggc-none.c	(revision 209170)
> +++ ggc-none.c	(working copy)
> @@ -63,3 +63,8 @@ ggc_free (void *p)
>   {
>     free (p);
>   }
> +
> +void
> +ggc_grow (void)
> +{
> +}
> Index: ggc-page.c
> ===================================================================
> --- ggc-page.c	(revision 209170)
> +++ ggc-page.c	(working copy)
> @@ -2095,6 +2095,19 @@ ggc_collect (void)
>       fprintf (G.debug_file, "END COLLECTING\n");
>   }
>   
> +/* Assume that all GGC memory is reachable and grow the limits for next collection. */
> +
> +void
> +ggc_grow (void)
> +{
> +#ifndef ENABLE_CHECKING
> +  G.allocated_last_gc = MAX (G.allocated_last_gc,
> +			     G.allocated);
> +#endif
> +  if (!quiet_flag)
> +    fprintf (stderr, " {GC start %luk} ", (unsigned long) G.allocated / 1024);
> +}
> +
>   /* Print allocation statistics.  */
>   #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
>   		  ? (x) \

> > +
> > +  /* At this stage we know that majority of GGC memory is reachable.  
> > +     Growing the limits prevents unnecesary invocation of GGC.  */
> > +  ggc_grow ();
> >    ggc_collect ();
> 
> Isn't the collect here pointless?  I see not in ENABLE_CHECKING, but
> shouldn't this be abstracted away, thus call ggc_collect from ggc_grow?
> Or maybe rather even for ENABLE_CHECKING adjust G.allocated_last_gc
> and simply drop the ggc_collect above ().

I am fine with both.  I basically decided to keep the explicit ggc_collect() to
make it clear (from lto.c source code) that we are GGC safe at this point and
to have way to double check that we do not produce too much of garbage with
checking disabled. (so with -Q I will see how much it is collected at that place).

We can embed it into ggc_grow and document that w/o checking it is equivalent
to ggc_cooect.
> 
> Anyway, this is sth for stage1 at this point.

OK,
Honza
> 
> Thanks,
> Richard.
> 
> >    /* Set the hooks so that all of the ipa passes can read in their data.  */
> > Index: ggc-none.c
> > ===================================================================
> > --- ggc-none.c	(revision 209170)
> > +++ ggc-none.c	(working copy)
> > @@ -63,3 +63,8 @@ ggc_free (void *p)
> >  {
> >    free (p);
> >  }
> > +
> > +void
> > +ggc_grow (void)
> > +{
> > +}
> > Index: ggc-page.c
> > ===================================================================
> > --- ggc-page.c	(revision 209170)
> > +++ ggc-page.c	(working copy)
> > @@ -2095,6 +2095,19 @@ ggc_collect (void)
> >      fprintf (G.debug_file, "END COLLECTING\n");
> >  }
> >  
> > +/* Assume that all GGC memory is reachable and grow the limits for next collection. */
> > +
> > +void
> > +ggc_grow (void)
> > +{
> > +#ifndef ENABLE_CHECKING
> > +  G.allocated_last_gc = MAX (G.allocated_last_gc,
> > +			     G.allocated);
> > +#endif
> > +  if (!quiet_flag)
> > +    fprintf (stderr, " {GC start %luk} ", (unsigned long) G.allocated / 1024);
> > +}
> > +
> >  /* Print allocation statistics.  */
> >  #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
> >  		  ? (x) \
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE / SUSE Labs
> SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
> GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer

> > > +
> > > +  /* At this stage we know that majority of GGC memory is reachable.  
> > > +     Growing the limits prevents unnecesary invocation of GGC.  */
> > > +  ggc_grow ();
> > >    ggc_collect ();
> > 
> > Isn't the collect here pointless?  I see not in ENABLE_CHECKING, but
> > shouldn't this be abstracted away, thus call ggc_collect from ggc_grow?
> > Or maybe rather even for ENABLE_CHECKING adjust G.allocated_last_gc
> > and simply drop the ggc_collect above ().
> 
> I am fine with both.  I basically decided to keep the explicit ggc_collect() to
> make it clear (from lto.c source code) that we are GGC safe at this point and
> to have way to double check that we do not produce too much of garbage with
> checking disabled. (so with -Q I will see how much it is collected at that place).
> 
> We can embed it into ggc_grow and document that w/o checking it is equivalent
> to ggc_cooect.
> > 
> > Anyway, this is sth for stage1 at this point.
> 
> OK,
> Honza

Ping...
the patches saves 33 GGC runs during libxul.so link, that is not that bad ;)

Honza
> > 
> > Thanks,
> > Richard.
> > 
> > >    /* Set the hooks so that all of the ipa passes can read in their data.  */
> > > Index: ggc-none.c
> > > ===================================================================
> > > --- ggc-none.c	(revision 209170)
> > > +++ ggc-none.c	(working copy)
> > > @@ -63,3 +63,8 @@ ggc_free (void *p)
> > >  {
> > >    free (p);
> > >  }
> > > +
> > > +void
> > > +ggc_grow (void)
> > > +{
> > > +}
> > > Index: ggc-page.c
> > > ===================================================================
> > > --- ggc-page.c	(revision 209170)
> > > +++ ggc-page.c	(working copy)
> > > @@ -2095,6 +2095,19 @@ ggc_collect (void)
> > >      fprintf (G.debug_file, "END COLLECTING\n");
> > >  }
> > >  
> > > +/* Assume that all GGC memory is reachable and grow the limits for next collection. */
> > > +
> > > +void
> > > +ggc_grow (void)
> > > +{
> > > +#ifndef ENABLE_CHECKING
> > > +  G.allocated_last_gc = MAX (G.allocated_last_gc,
> > > +			     G.allocated);
> > > +#endif
> > > +  if (!quiet_flag)
> > > +    fprintf (stderr, " {GC start %luk} ", (unsigned long) G.allocated / 1024);
> > > +}
> > > +
> > >  /* Print allocation statistics.  */
> > >  #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
> > >  		  ? (x) \
> > > 
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE / SUSE Labs
> > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
> > GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer

On April 17, 2014 6:03:13 PM CEST, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > > +
>> > > +  /* At this stage we know that majority of GGC memory is
>reachable.  
>> > > +     Growing the limits prevents unnecesary invocation of GGC. 
>*/
>> > > +  ggc_grow ();
>> > >    ggc_collect ();
>> > 
>> > Isn't the collect here pointless?  I see not in ENABLE_CHECKING,
>but
>> > shouldn't this be abstracted away, thus call ggc_collect from
>ggc_grow?
>> > Or maybe rather even for ENABLE_CHECKING adjust G.allocated_last_gc
>> > and simply drop the ggc_collect above ().
>> 
>> I am fine with both.  I basically decided to keep the explicit
>ggc_collect() to
>> make it clear (from lto.c source code) that we are GGC safe at this
>point and
>> to have way to double check that we do not produce too much of
>garbage with
>> checking disabled. (so with -Q I will see how much it is collected at
>that place).
>> 
>> We can embed it into ggc_grow and document that w/o checking it is
>equivalent
>> to ggc_cooect.
>> > 
>> > Anyway, this is sth for stage1 at this point.
>> 
>> OK,
>> Honza
>
>Ping...
>the patches saves 33 GGC runs during libxul.so link, that is not that
>bad ;)

What is the updated patch you propose?

Richard

>Honza
>> > 
>> > Thanks,
>> > Richard.
>> > 
>> > >    /* Set the hooks so that all of the ipa passes can read in
>their data.  */
>> > > Index: ggc-none.c
>> > >
>===================================================================
>> > > --- ggc-none.c	(revision 209170)
>> > > +++ ggc-none.c	(working copy)
>> > > @@ -63,3 +63,8 @@ ggc_free (void *p)
>> > >  {
>> > >    free (p);
>> > >  }
>> > > +
>> > > +void
>> > > +ggc_grow (void)
>> > > +{
>> > > +}
>> > > Index: ggc-page.c
>> > >
>===================================================================
>> > > --- ggc-page.c	(revision 209170)
>> > > +++ ggc-page.c	(working copy)
>> > > @@ -2095,6 +2095,19 @@ ggc_collect (void)
>> > >      fprintf (G.debug_file, "END COLLECTING\n");
>> > >  }
>> > >  
>> > > +/* Assume that all GGC memory is reachable and grow the limits
>for next collection. */
>> > > +
>> > > +void
>> > > +ggc_grow (void)
>> > > +{
>> > > +#ifndef ENABLE_CHECKING
>> > > +  G.allocated_last_gc = MAX (G.allocated_last_gc,
>> > > +			     G.allocated);
>> > > +#endif
>> > > +  if (!quiet_flag)
>> > > +    fprintf (stderr, " {GC start %luk} ", (unsigned long)
>G.allocated / 1024);
>> > > +}
>> > > +
>> > >  /* Print allocation statistics.  */
>> > >  #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
>> > >  		  ? (x) \
>> > > 
>> > > 
>> > 
>> > -- 
>> > Richard Biener <rguenther@suse.de>
>> > SUSE / SUSE Labs
>> > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
>> > GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer

> On April 17, 2014 6:03:13 PM CEST, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> > > +
> >> > > +  /* At this stage we know that majority of GGC memory is
> >reachable.  
> >> > > +     Growing the limits prevents unnecesary invocation of GGC. 
> >*/
> >> > > +  ggc_grow ();
> >> > >    ggc_collect ();
> >> > 
> >> > Isn't the collect here pointless?  I see not in ENABLE_CHECKING,
> >but
> >> > shouldn't this be abstracted away, thus call ggc_collect from
> >ggc_grow?
> >> > Or maybe rather even for ENABLE_CHECKING adjust G.allocated_last_gc
> >> > and simply drop the ggc_collect above ().
> >> 
> >> I am fine with both.  I basically decided to keep the explicit
> >ggc_collect() to
> >> make it clear (from lto.c source code) that we are GGC safe at this
> >point and
> >> to have way to double check that we do not produce too much of
> >garbage with
> >> checking disabled. (so with -Q I will see how much it is collected at
> >that place).
> >> 
> >> We can embed it into ggc_grow and document that w/o checking it is
> >equivalent
> >> to ggc_cooect.
> >> > 
> >> > Anyway, this is sth for stage1 at this point.
> >> 
> >> OK,
> >> Honza
> >
> >Ping...
> >the patches saves 33 GGC runs during libxul.so link, that is not that
> >bad ;)
> 
> What is the updated patch you propose?

I was trying to explain, why I kept explicit ggc_collect just after ggc_grow:

I want to make it clear that we are ggc safe at that point. I also want to see
the ggc run happening w/o checking to have -Q report how much of garbage we see
at this stage so I can keep eye on it.

I can hide ENABLE_CHECKING ggc_collect call in ggc_grow and update
documentation if your preffer.

Honza
> 
> Richard
> 
> >Honza
> >> > 
> >> > Thanks,
> >> > Richard.
> >> > 
> >> > >    /* Set the hooks so that all of the ipa passes can read in
> >their data.  */
> >> > > Index: ggc-none.c
> >> > >
> >===================================================================
> >> > > --- ggc-none.c	(revision 209170)
> >> > > +++ ggc-none.c	(working copy)
> >> > > @@ -63,3 +63,8 @@ ggc_free (void *p)
> >> > >  {
> >> > >    free (p);
> >> > >  }
> >> > > +
> >> > > +void
> >> > > +ggc_grow (void)
> >> > > +{
> >> > > +}
> >> > > Index: ggc-page.c
> >> > >
> >===================================================================
> >> > > --- ggc-page.c	(revision 209170)
> >> > > +++ ggc-page.c	(working copy)
> >> > > @@ -2095,6 +2095,19 @@ ggc_collect (void)
> >> > >      fprintf (G.debug_file, "END COLLECTING\n");
> >> > >  }
> >> > >  
> >> > > +/* Assume that all GGC memory is reachable and grow the limits
> >for next collection. */
> >> > > +
> >> > > +void
> >> > > +ggc_grow (void)
> >> > > +{
> >> > > +#ifndef ENABLE_CHECKING
> >> > > +  G.allocated_last_gc = MAX (G.allocated_last_gc,
> >> > > +			     G.allocated);
> >> > > +#endif
> >> > > +  if (!quiet_flag)
> >> > > +    fprintf (stderr, " {GC start %luk} ", (unsigned long)
> >G.allocated / 1024);
> >> > > +}
> >> > > +
> >> > >  /* Print allocation statistics.  */
> >> > >  #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
> >> > >  		  ? (x) \
> >> > > 
> >> > > 
> >> > 
> >> > -- 
> >> > Richard Biener <rguenther@suse.de>
> >> > SUSE / SUSE Labs
> >> > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
> >> > GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
>

On April 17, 2014 7:18:05 PM CEST, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On April 17, 2014 6:03:13 PM CEST, Jan Hubicka <hubicka@ucw.cz>
>wrote:
>> >> > > +
>> >> > > +  /* At this stage we know that majority of GGC memory is
>> >reachable.  
>> >> > > +     Growing the limits prevents unnecesary invocation of
>GGC. 
>> >*/
>> >> > > +  ggc_grow ();
>> >> > >    ggc_collect ();
>> >> > 
>> >> > Isn't the collect here pointless?  I see not in ENABLE_CHECKING,
>> >but
>> >> > shouldn't this be abstracted away, thus call ggc_collect from
>> >ggc_grow?
>> >> > Or maybe rather even for ENABLE_CHECKING adjust
>G.allocated_last_gc
>> >> > and simply drop the ggc_collect above ().
>> >> 
>> >> I am fine with both.  I basically decided to keep the explicit
>> >ggc_collect() to
>> >> make it clear (from lto.c source code) that we are GGC safe at
>this
>> >point and
>> >> to have way to double check that we do not produce too much of
>> >garbage with
>> >> checking disabled. (so with -Q I will see how much it is collected
>at
>> >that place).
>> >> 
>> >> We can embed it into ggc_grow and document that w/o checking it is
>> >equivalent
>> >> to ggc_cooect.
>> >> > 
>> >> > Anyway, this is sth for stage1 at this point.
>> >> 
>> >> OK,
>> >> Honza
>> >
>> >Ping...
>> >the patches saves 33 GGC runs during libxul.so link, that is not
>that
>> >bad ;)
>> 
>> What is the updated patch you propose?
>
>I was trying to explain, why I kept explicit ggc_collect just after
>ggc_grow:
>
>I want to make it clear that we are ggc safe at that point. I also want
>to see
>the ggc run happening w/o checking to have -Q report how much of
>garbage we see
>at this stage so I can keep eye on it.
>
>I can hide ENABLE_CHECKING ggc_collect call in ggc_grow and update
>documentation if your preffer.

I'd prefer that.  OK with that change.

Thanks,
Richard.

>Honza
>> 
>> Richard
>> 
>> >Honza
>> >> > 
>> >> > Thanks,
>> >> > Richard.
>> >> > 
>> >> > >    /* Set the hooks so that all of the ipa passes can read in
>> >their data.  */
>> >> > > Index: ggc-none.c
>> >> > >
>> >===================================================================
>> >> > > --- ggc-none.c	(revision 209170)
>> >> > > +++ ggc-none.c	(working copy)
>> >> > > @@ -63,3 +63,8 @@ ggc_free (void *p)
>> >> > >  {
>> >> > >    free (p);
>> >> > >  }
>> >> > > +
>> >> > > +void
>> >> > > +ggc_grow (void)
>> >> > > +{
>> >> > > +}
>> >> > > Index: ggc-page.c
>> >> > >
>> >===================================================================
>> >> > > --- ggc-page.c	(revision 209170)
>> >> > > +++ ggc-page.c	(working copy)
>> >> > > @@ -2095,6 +2095,19 @@ ggc_collect (void)
>> >> > >      fprintf (G.debug_file, "END COLLECTING\n");
>> >> > >  }
>> >> > >  
>> >> > > +/* Assume that all GGC memory is reachable and grow the
>limits
>> >for next collection. */
>> >> > > +
>> >> > > +void
>> >> > > +ggc_grow (void)
>> >> > > +{
>> >> > > +#ifndef ENABLE_CHECKING
>> >> > > +  G.allocated_last_gc = MAX (G.allocated_last_gc,
>> >> > > +			     G.allocated);
>> >> > > +#endif
>> >> > > +  if (!quiet_flag)
>> >> > > +    fprintf (stderr, " {GC start %luk} ", (unsigned long)
>> >G.allocated / 1024);
>> >> > > +}
>> >> > > +
>> >> > >  /* Print allocation statistics.  */
>> >> > >  #define SCALE(x) ((unsigned long) ((x) < 1024*10 \
>> >> > >  		  ? (x) \
>> >> > > 
>> >> > > 
>> >> > 
>> >> > -- 
>> >> > Richard Biener <rguenther@suse.de>
>> >> > SUSE / SUSE Labs
>> >> > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
>> >> > GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
>>