Add obstack for canonical file name hash table

Hi,
looking into malloc overhead I noticed that we do a lot of small
allocations to hold file names comming from location info. This patch
puts it into an obstack so it interleaves memory allocated by scc_hash
less frequently.
(Still we end up interleaving 64k pages which are permanent - in fact
this table seems to leak from WPA and temporary during stream in)

Bootstrapped/regtested x86_64-linux. OK?

Honza

	* lto-streamer-in.c (file_name_obstack): New obstack.
	(canon_file_name): Use it.
	(lto_reader_init): Initialize it.

On Tue, 5 Nov 2019, Jan Hubicka wrote:

> Hi,
> looking into malloc overhead I noticed that we do a lot of small
> allocations to hold file names comming from location info. This patch
> puts it into an obstack so it interleaves memory allocated by scc_hash
> less frequently.
> (Still we end up interleaving 64k pages which are permanent - in fact
> this table seems to leak from WPA and temporary during stream in)
> 
> Bootstrapped/regtested x86_64-linux. OK?

I think the obstack deserves a big fat comment that it cannot be
reclaimed since the linemap retains permanent pointers into it.
That also suggests to put the string_slot into a separate obstack
or better, make the hasher (and other string_slot hashers)
embed the string_slot struct in the hash?  We'd save an allocation
everywhere.

Richard.

> Honza
> 
> 	* lto-streamer-in.c (file_name_obstack): New obstack.
> 	(canon_file_name): Use it.
> 	(lto_reader_init): Initialize it.
> Index: lto-streamer-in.c
> ===================================================================
> --- lto-streamer-in.c	(revision 277796)
> +++ lto-streamer-in.c	(working copy)
> @@ -57,6 +57,7 @@ freeing_string_slot_hasher::remove (valu
>  
>  /* The table to hold the file names.  */
>  static hash_table<freeing_string_slot_hasher> *file_name_hash_table;
> +static struct obstack file_name_obstack;
>  
>  
>  /* Check that tag ACTUAL has one of the given values.  NUM_TAGS is the
> @@ -113,8 +114,9 @@ canon_file_name (const char *string)
>        char *saved_string;
>        struct string_slot *new_slot;
>  
> -      saved_string = (char *) xmalloc (len + 1);
> -      new_slot = XCNEW (struct string_slot);
> +      saved_string = XOBNEWVEC (&file_name_obstack, char, len + 1);
> +      new_slot = XOBNEWVAR (&file_name_obstack,
> +			    struct string_slot, sizeof (struct string_slot));
>        memcpy (saved_string, string, len + 1);
>        new_slot->s = saved_string;
>        new_slot->len = len;
> @@ -1723,6 +1725,7 @@ lto_reader_init (void)
>    lto_streamer_init ();
>    file_name_hash_table
>      = new hash_table<freeing_string_slot_hasher> (37);
> +  gcc_obstack_init (&file_name_obstack);
>  }
>  
>  
>

> On Tue, 5 Nov 2019, Jan Hubicka wrote:
> 
> > Hi,
> > looking into malloc overhead I noticed that we do a lot of small
> > allocations to hold file names comming from location info. This patch
> > puts it into an obstack so it interleaves memory allocated by scc_hash
> > less frequently.
> > (Still we end up interleaving 64k pages which are permanent - in fact
> > this table seems to leak from WPA and temporary during stream in)
> > 
> > Bootstrapped/regtested x86_64-linux. OK?
> 
> I think the obstack deserves a big fat comment that it cannot be
> reclaimed since the linemap retains permanent pointers into it.
> That also suggests to put the string_slot into a separate obstack

The hasher is sort of ethernal, too, since at any time we want to be
able to load more from input streams, so we can not really free it.
Well, I guess just prior streaming we can, so I will split it.
> or better, make the hasher (and other string_slot hashers)
> embed the string_slot struct in the hash?  We'd save an allocation
> everywhere.

Well, if we want to free hasher, then we want to keep string separate +
comment on obstack, right?  I sill update patch tonight.

Honza
> 
> Richard.
> 
> > Honza
> > 
> > 	* lto-streamer-in.c (file_name_obstack): New obstack.
> > 	(canon_file_name): Use it.
> > 	(lto_reader_init): Initialize it.
> > Index: lto-streamer-in.c
> > ===================================================================
> > --- lto-streamer-in.c	(revision 277796)
> > +++ lto-streamer-in.c	(working copy)
> > @@ -57,6 +57,7 @@ freeing_string_slot_hasher::remove (valu
> >  
> >  /* The table to hold the file names.  */
> >  static hash_table<freeing_string_slot_hasher> *file_name_hash_table;
> > +static struct obstack file_name_obstack;
> >  
> >  
> >  /* Check that tag ACTUAL has one of the given values.  NUM_TAGS is the
> > @@ -113,8 +114,9 @@ canon_file_name (const char *string)
> >        char *saved_string;
> >        struct string_slot *new_slot;
> >  
> > -      saved_string = (char *) xmalloc (len + 1);
> > -      new_slot = XCNEW (struct string_slot);
> > +      saved_string = XOBNEWVEC (&file_name_obstack, char, len + 1);
> > +      new_slot = XOBNEWVAR (&file_name_obstack,
> > +			    struct string_slot, sizeof (struct string_slot));
> >        memcpy (saved_string, string, len + 1);
> >        new_slot->s = saved_string;
> >        new_slot->len = len;
> > @@ -1723,6 +1725,7 @@ lto_reader_init (void)
> >    lto_streamer_init ();
> >    file_name_hash_table
> >      = new hash_table<freeing_string_slot_hasher> (37);
> > +  gcc_obstack_init (&file_name_obstack);
> >  }
> >  
> >  
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

On Tue, 5 Nov 2019, Jan Hubicka wrote:

> > On Tue, 5 Nov 2019, Jan Hubicka wrote:
> > 
> > > Hi,
> > > looking into malloc overhead I noticed that we do a lot of small
> > > allocations to hold file names comming from location info. This patch
> > > puts it into an obstack so it interleaves memory allocated by scc_hash
> > > less frequently.
> > > (Still we end up interleaving 64k pages which are permanent - in fact
> > > this table seems to leak from WPA and temporary during stream in)
> > > 
> > > Bootstrapped/regtested x86_64-linux. OK?
> > 
> > I think the obstack deserves a big fat comment that it cannot be
> > reclaimed since the linemap retains permanent pointers into it.
> > That also suggests to put the string_slot into a separate obstack
> 
> The hasher is sort of ethernal, too, since at any time we want to be
> able to load more from input streams, so we can not really free it.
> Well, I guess just prior streaming we can, so I will split it.
> > or better, make the hasher (and other string_slot hashers)
> > embed the string_slot struct in the hash?  We'd save an allocation
> > everywhere.
> 
> Well, if we want to free hasher, then we want to keep string separate +
> comment on obstack, right?  I sill update patch tonight.

Yes, we want to have an obstack with just the strings and a comment
that we have to keep that.

Richard.

> Honza
> > 
> > Richard.
> > 
> > > Honza
> > > 
> > > 	* lto-streamer-in.c (file_name_obstack): New obstack.
> > > 	(canon_file_name): Use it.
> > > 	(lto_reader_init): Initialize it.
> > > Index: lto-streamer-in.c
> > > ===================================================================
> > > --- lto-streamer-in.c	(revision 277796)
> > > +++ lto-streamer-in.c	(working copy)
> > > @@ -57,6 +57,7 @@ freeing_string_slot_hasher::remove (valu
> > >  
> > >  /* The table to hold the file names.  */
> > >  static hash_table<freeing_string_slot_hasher> *file_name_hash_table;
> > > +static struct obstack file_name_obstack;
> > >  
> > >  
> > >  /* Check that tag ACTUAL has one of the given values.  NUM_TAGS is the
> > > @@ -113,8 +114,9 @@ canon_file_name (const char *string)
> > >        char *saved_string;
> > >        struct string_slot *new_slot;
> > >  
> > > -      saved_string = (char *) xmalloc (len + 1);
> > > -      new_slot = XCNEW (struct string_slot);
> > > +      saved_string = XOBNEWVEC (&file_name_obstack, char, len + 1);
> > > +      new_slot = XOBNEWVAR (&file_name_obstack,
> > > +			    struct string_slot, sizeof (struct string_slot));
> > >        memcpy (saved_string, string, len + 1);
> > >        new_slot->s = saved_string;
> > >        new_slot->len = len;
> > > @@ -1723,6 +1725,7 @@ lto_reader_init (void)
> > >    lto_streamer_init ();
> > >    file_name_hash_table
> > >      = new hash_table<freeing_string_slot_hasher> (37);
> > > +  gcc_obstack_init (&file_name_obstack);
> > >  }
> > >  
> > >  
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
> 
>

Hi,
here is updated patch. Now the hash is freed though it is not
extraordinarily large (for firefox about 20k entries which roughly
correspond to number of source files)

Bootstrapped/regtested x86_64-linux. OK?

	* lto-streamer-in.c: Include alloc-pool.h.
	(freeing_string_slot_hasher): Remove.
	(string_slot_allocator): New object allocator.
	(file_name_hash_table): Turn to hash_table<string_slot_hasher>.
	(file_name_obstack): New obstack.
	(canon_file_name): Allocate in obstack and allocator.
	(lto_reader_init): Initialize obstack and allocator.
	(lto_free_file_name_hash): New function.
	* lto-streamer.h (lto_free_file_name_hash): New.
	* lto.c (do_whole_program_analysis): Call lto_free_file_name_hash.
Index: lto-streamer-in.c
===================================================================
--- lto-streamer-in.c	(revision 277796)
+++ lto-streamer-in.c	(working copy)
@@ -42,21 +42,18 @@ along with GCC; see the file COPYING3.
 #include "cgraph.h"
 #include "cfgloop.h"
 #include "debug.h"
+#include "alloc-pool.h"
 
-
-struct freeing_string_slot_hasher : string_slot_hasher
-{
-  static inline void remove (value_type *);
-};
-
-inline void
-freeing_string_slot_hasher::remove (value_type *v)
-{
-  free (v);
-}
+/* Allocator used to hold string slot entries for line map streaming.  */
+static struct object_allocator<struct string_slot> *string_slot_allocator;
 
 /* The table to hold the file names.  */
-static hash_table<freeing_string_slot_hasher> *file_name_hash_table;
+static hash_table<string_slot_hasher> *file_name_hash_table;
+
+/* This obstack holds file names used in locators. Line map datastructures
+   points here and thus it needs to be kept allocated as long as linemaps
+   exists.  */
+static struct obstack file_name_obstack;
 
 
 /* Check that tag ACTUAL has one of the given values.  NUM_TAGS is the
@@ -113,8 +110,8 @@ canon_file_name (const char *string)
       char *saved_string;
       struct string_slot *new_slot;
 
-      saved_string = (char *) xmalloc (len + 1);
-      new_slot = XCNEW (struct string_slot);
+      saved_string = XOBNEWVEC (&file_name_obstack, char, len + 1);
+      new_slot = string_slot_allocator->allocate ();
       memcpy (saved_string, string, len + 1);
       new_slot->s = saved_string;
       new_slot->len = len;
@@ -1722,7 +1719,23 @@ lto_reader_init (void)
 {
   lto_streamer_init ();
   file_name_hash_table
-    = new hash_table<freeing_string_slot_hasher> (37);
+    = new hash_table<string_slot_hasher> (37);
+  string_slot_allocator = new object_allocator <struct string_slot>
+				("line map file name hash");
+  gcc_obstack_init (&file_name_obstack);
+}
+
+/* Free hash table used to stream in location file names.  */
+
+void
+lto_free_file_name_hash (void)
+{
+  delete file_name_hash_table;
+  file_name_hash_table = NULL;
+  delete string_slot_allocator;
+  string_slot_allocator = NULL;
+  /* file_name_obstack must stay allocated since it is referred to by
+     line map table.  */
 }
 
 
Index: lto-streamer.h
===================================================================
--- lto-streamer.h	(revision 277796)
+++ lto-streamer.h	(working copy)
@@ -874,6 +874,7 @@ extern void lto_streamer_hooks_init (voi
 /* In lto-streamer-in.c */
 extern void lto_input_cgraph (struct lto_file_decl_data *, const char *);
 extern void lto_reader_init (void);
+extern void lto_free_file_name_hash (void);
 extern void lto_input_function_body (struct lto_file_decl_data *,
 				     struct cgraph_node *,
Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 277796)
+++ lto/lto.c	(working copy)
@@ -475,6 +476,9 @@ do_whole_program_analysis (void)
   /* We are about to launch the final LTRANS phase, stop the WPA timer.  */
   timevar_pop (TV_WHOPR_WPA);
 
+  /* We are no longer going to stream in anything.  Free some memory.  */
+  lto_free_file_name_hash ();
+
   timevar_push (TV_WHOPR_PARTITIONING);
 
   gcc_assert (!dump_file);

On Tue, 5 Nov 2019, Jan Hubicka wrote:

> Hi,
> here is updated patch. Now the hash is freed though it is not
> extraordinarily large (for firefox about 20k entries which roughly
> correspond to number of source files)
> 
> Bootstrapped/regtested x86_64-linux. OK?

OK.

Note I was arguing that the "string_slot" indirection should go away
and we should put the whole string_slot into the hash-table rather
than just a pointer to a string_slot.  We're using it in quite a few
places (some allocating the string_slot from an obstack).  That would
have simplified the lto-streamer-in.c part of the patch but of course
requires surgery elsewhere (unless you make such an embedded hasher
just for the file location use).

> 	* lto-streamer-in.c: Include alloc-pool.h.
> 	(freeing_string_slot_hasher): Remove.
> 	(string_slot_allocator): New object allocator.
> 	(file_name_hash_table): Turn to hash_table<string_slot_hasher>.
> 	(file_name_obstack): New obstack.
> 	(canon_file_name): Allocate in obstack and allocator.
> 	(lto_reader_init): Initialize obstack and allocator.
> 	(lto_free_file_name_hash): New function.
> 	* lto-streamer.h (lto_free_file_name_hash): New.
> 	* lto.c (do_whole_program_analysis): Call lto_free_file_name_hash.
> Index: lto-streamer-in.c
> ===================================================================
> --- lto-streamer-in.c	(revision 277796)
> +++ lto-streamer-in.c	(working copy)
> @@ -42,21 +42,18 @@ along with GCC; see the file COPYING3.
>  #include "cgraph.h"
>  #include "cfgloop.h"
>  #include "debug.h"
> +#include "alloc-pool.h"
>  
> -
> -struct freeing_string_slot_hasher : string_slot_hasher
> -{
> -  static inline void remove (value_type *);
> -};
> -
> -inline void
> -freeing_string_slot_hasher::remove (value_type *v)
> -{
> -  free (v);
> -}
> +/* Allocator used to hold string slot entries for line map streaming.  */
> +static struct object_allocator<struct string_slot> *string_slot_allocator;
>  
>  /* The table to hold the file names.  */
> -static hash_table<freeing_string_slot_hasher> *file_name_hash_table;
> +static hash_table<string_slot_hasher> *file_name_hash_table;
> +
> +/* This obstack holds file names used in locators. Line map datastructures
> +   points here and thus it needs to be kept allocated as long as linemaps
> +   exists.  */
> +static struct obstack file_name_obstack;
>  
>  
>  /* Check that tag ACTUAL has one of the given values.  NUM_TAGS is the
> @@ -113,8 +110,8 @@ canon_file_name (const char *string)
>        char *saved_string;
>        struct string_slot *new_slot;
>  
> -      saved_string = (char *) xmalloc (len + 1);
> -      new_slot = XCNEW (struct string_slot);
> +      saved_string = XOBNEWVEC (&file_name_obstack, char, len + 1);
> +      new_slot = string_slot_allocator->allocate ();
>        memcpy (saved_string, string, len + 1);
>        new_slot->s = saved_string;
>        new_slot->len = len;
> @@ -1722,7 +1719,23 @@ lto_reader_init (void)
>  {
>    lto_streamer_init ();
>    file_name_hash_table
> -    = new hash_table<freeing_string_slot_hasher> (37);
> +    = new hash_table<string_slot_hasher> (37);
> +  string_slot_allocator = new object_allocator <struct string_slot>
> +				("line map file name hash");
> +  gcc_obstack_init (&file_name_obstack);
> +}
> +
> +/* Free hash table used to stream in location file names.  */
> +
> +void
> +lto_free_file_name_hash (void)
> +{
> +  delete file_name_hash_table;
> +  file_name_hash_table = NULL;
> +  delete string_slot_allocator;
> +  string_slot_allocator = NULL;
> +  /* file_name_obstack must stay allocated since it is referred to by
> +     line map table.  */
>  }
>  
>  
> Index: lto-streamer.h
> ===================================================================
> --- lto-streamer.h	(revision 277796)
> +++ lto-streamer.h	(working copy)
> @@ -874,6 +874,7 @@ extern void lto_streamer_hooks_init (voi
>  /* In lto-streamer-in.c */
>  extern void lto_input_cgraph (struct lto_file_decl_data *, const char *);
>  extern void lto_reader_init (void);
> +extern void lto_free_file_name_hash (void);
>  extern void lto_input_function_body (struct lto_file_decl_data *,
>  				     struct cgraph_node *,
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 277796)
> +++ lto/lto.c	(working copy)
> @@ -475,6 +476,9 @@ do_whole_program_analysis (void)
>    /* We are about to launch the final LTRANS phase, stop the WPA timer.  */
>    timevar_pop (TV_WHOPR_WPA);
>  
> +  /* We are no longer going to stream in anything.  Free some memory.  */
> +  lto_free_file_name_hash ();
> +
>    timevar_push (TV_WHOPR_PARTITIONING);
>  
>    gcc_assert (!dump_file);
>