Line map table allocation

Message ID 5e8d4a6f-7785-e4b2-1685-2483475c7cec@acm.org
State New
Headers show
Series
  • Line map table allocation
Related show

Commit Message

Nathan Sidwell Aug. 6, 2018, 4:58 p.m.
Here's a line-map patch to make the new_linemap logic simpler.  On the 
modules branch I need to allocate blocks of linemaps, and found the 
current allocation scheme a little confusing to adjust.  This'll make 
that subsequent change simpler.

While there, I set the default allocator (xmalloc) in the init routine, 
rather than check it for each allocation.  I doubt the loss of a 
devirtualization possibility is significant (we're doing allocation 
wrong if it is).

booted & tested on x86_64-linux

ok?

nathan
-- 
Nathan Sidwell

Comments

David Malcolm Aug. 7, 2018, 7:43 p.m. | #1
On Mon, 2018-08-06 at 12:58 -0400, Nathan Sidwell wrote:
> Here's a line-map patch to make the new_linemap logic simpler.  On

> the 

> modules branch I need to allocate blocks of linemaps, and found the 

> current allocation scheme a little confusing to adjust.  This'll

> make 

> that subsequent change simpler.

> 

> While there, I set the default allocator (xmalloc) in the init

> routine, 

> rather than check it for each allocation.  I doubt the loss of a 

> devirtualization possibility is significant (we're doing allocation 

> wrong if it is).

> 

> booted & tested on x86_64-linux

> 

> ok?


> 2018-08-06  Nathan Sidwell  <nathan@acm.org>

> 

> 	* line-map.c: (linemap_init): Set default allocator here.

> 	(new_linemap): Rather than here.  Refactor allocation logic.

> 

> Index: line-map.c

> ===================================================================

> --- line-map.c	(revision 263332)

> +++ line-map.c	(working copy)

> @@ -346,6 +346,8 @@ linemap_init (struct line_maps *set,

>  #else

>    new (set) line_maps();

>  #endif

> +  /* Set default allocator.  */

> +  set->reallocator = (line_map_realloc) xrealloc;


Set default *reallocator*, surely?

I wonder if we still need that cast.  I see include/libiberty.h has:

  extern void *xrealloc (void *, size_t) ATTRIBUTE_RETURNS_NONNULL;

and libcpp/include/linemap.h has:

  typedef void *(*line_map_realloc) (void *, size_t);

which appear to be identical to me.  But there's enough macro cruft
elsewhere involving realloc that I'm nervous about removing the cast.

>    set->highest_location = RESERVED_LOCATION_COUNT - 1;

>    set->highest_line = RESERVED_LOCATION_COUNT - 1;

>    set->location_adhoc_data_map.htab =

> @@ -376,81 +378,58 @@ linemap_check_files_exited (struct line_

>  static struct line_map *

>  new_linemap (struct line_maps *set,  source_location start_location)

>  {

> -  struct line_map *result;

> -  bool macro_map_p = start_location >= LINE_MAP_MAX_LOCATION;

> +  bool macro_p = start_location >= LINE_MAP_MAX_LOCATION;



> +  unsigned alloc = LINEMAPS_ALLOCATED (set, macro_p);

> +  unsigned used = LINEMAPS_USED (set, macro_p);


These two names are too terse for my taste; whilst reading the rest of
the patch I had to double-check "is this a count of structs or of
bytes?".

How about "num_alloc_maps" and "num_used_maps"?

> -  if (LINEMAPS_USED (set, macro_map_p) == LINEMAPS_ALLOCATED (set, macro_map_p))

> +  if (used == alloc)

>      {

> -      /* We ran out of allocated line maps. Let's allocate more.  */

> -      size_t alloc_size;

> -

> -      /* Cast away extern "C" from the type of xrealloc.  */

> -      line_map_realloc reallocator = (set->reallocator

> -				      ? set->reallocator

> -				      : (line_map_realloc) xrealloc);

> -      line_map_round_alloc_size_func round_alloc_size =

> -	set->round_alloc_size;

> -

> -      size_t map_size = (macro_map_p

> -			 ? sizeof (line_map_macro)

> -			 : sizeof (line_map_ordinary));

> +      /* We need more space!  */

> +      if (!alloc)

> +	alloc = 128;

> +      alloc *= 2;

> +

> +      size_t map_size;


Whilst we're refactoring, please rename this to "size_per_map".

> +      void *buffer;

> +      if (macro_p)

> +	{

> +	  map_size = sizeof (line_map_macro);

> +	  buffer = set->info_macro.maps;

> +	}

> +      else

> +	{

> +	  map_size = sizeof (line_map_ordinary);

> +	  buffer = set->info_ordinary.maps;

> +	}

>  

>        /* We are going to execute some dance to try to reduce the

>  	 overhead of the memory allocator, in case we are using the

>  	 ggc-page.c one.

>  	 

>  	 The actual size of memory we are going to get back from the

> -	 allocator is the smallest power of 2 that is greater than the

> -	 size we requested.  So let's consider that size then.  */

> -

> -      alloc_size =

> -	(2 * LINEMAPS_ALLOCATED (set, macro_map_p) +  256)

> -	* map_size;

> -

> -      /* Get the actual size of memory that is going to be allocated

> -	 by the allocator.  */

> -      alloc_size = round_alloc_size (alloc_size);

> +	 allocator may well be larger than what we ask for.  Use this

> +	 hook to find what that size is.  */

> +      size_t alloc_size = set->round_alloc_size (alloc * map_size);


If I'm reading this right, there's a slight change here in how we grow
the buffers: previously, num_alloc_maps grew to:

  2 * num_alloc_maps + 256

whereas now it grows to:

  2 * num_alloc_maps, with an initial size of 256.

(That addition of 256 in the old behavior appears to date back to
r44584 on 2001-08-03, which created line-map.c)

I think this growth change is OK.

>  

>        /* Now alloc_size contains the exact memory size we would get if

>  	 we have asked for the initial alloc_size amount of memory.

>  	 Let's get back to the number of macro map that amounts

>  	 to.  */


The above comment contains a pre-existing erroneous reference to "macro
map".  Please change to "maps" there.

> -      LINEMAPS_ALLOCATED (set, macro_map_p) =

> -	alloc_size / map_size;

> -

> -      /* And now let's really do the re-allocation.  */

> -      if (macro_map_p)

> -	{

> -	  set->info_macro.maps

> -	    = (line_map_macro *) (*reallocator) (set->info_macro.maps,

> -						 (LINEMAPS_ALLOCATED (set, macro_map_p)

> -						  * map_size));

> -	  result = &set->info_macro.maps[LINEMAPS_USED (set, macro_map_p)];

> -	}

> -      else

> -	{

> -	  set->info_ordinary.maps =

> -	    (line_map_ordinary *) (*reallocator) (set->info_ordinary.maps,

> -						  (LINEMAPS_ALLOCATED (set, macro_map_p)

> -						   * map_size));

> -	  result = &set->info_ordinary.maps[LINEMAPS_USED (set, macro_map_p)];

> -	}

> -      memset (result, 0,

> -	      ((LINEMAPS_ALLOCATED (set, macro_map_p)

> -		- LINEMAPS_USED (set, macro_map_p))

> -	       * map_size));

> -    }

> -  else

> -    {

> -      if (macro_map_p)

> -	result = &set->info_macro.maps[LINEMAPS_USED (set, macro_map_p)];

> +      unsigned num_maps = alloc_size / map_size;

> +      buffer = set->reallocator (buffer, num_maps * map_size);

> +      memset ((char *)buffer + used * map_size, 0, (num_maps - used) * map_size);

> +      if (macro_p)

> +	set->info_macro.maps = (line_map_macro *)buffer;

>        else

> -	result = &set->info_ordinary.maps[LINEMAPS_USED (set, macro_map_p)];

> +	set->info_ordinary.maps = (line_map_ordinary *)buffer;

> +      LINEMAPS_ALLOCATED (set, macro_p) = num_maps;

>      }

>  

> -  result->start_location = start_location;

> +  line_map *result = (macro_p ? (line_map *)&set->info_macro.maps[used]

> +		      : (line_map *)&set->info_ordinary.maps[used]);

> +  LINEMAPS_USED (set, macro_p)++;

>  

> -  LINEMAPS_USED (set, macro_map_p)++;

> +  result->start_location = start_location;

>  

>    return result;

>  }


Otherwise, looks good to me.

OK for trunk, with the nits noted above fixed.

Thanks
Dave
Nathan Sidwell Aug. 7, 2018, 9:27 p.m. | #2
On 08/07/2018 03:43 PM, David Malcolm wrote:

> OK for trunk, with the nits noted above fixed.


Committing this, thanks

nathan

-- 
Nathan Sidwell
2018-08-07  Nathan Sidwell  <nathan@acm.org>

	* line-map.c: (linemap_init): Set default allocator here.
	(new_linemap): Rather than here.  Refactor allocation logic.

Index: line-map.c
===================================================================
--- line-map.c	(revision 263365)
+++ line-map.c	(working copy)
@@ -346,6 +346,8 @@ linemap_init (struct line_maps *set,
 #else
   new (set) line_maps();
 #endif
+  /* Set default reallocator (used for initial alloc too).  */
+  set->reallocator = xrealloc;
   set->highest_location = RESERVED_LOCATION_COUNT - 1;
   set->highest_line = RESERVED_LOCATION_COUNT - 1;
   set->location_adhoc_data_map.htab =
@@ -376,81 +378,59 @@ linemap_check_files_exited (struct line_
 static struct line_map *
 new_linemap (struct line_maps *set,  source_location start_location)
 {
-  struct line_map *result;
-  bool macro_map_p = start_location >= LINE_MAP_MAX_LOCATION;
+  bool macro_p = start_location >= LINE_MAP_MAX_LOCATION;
+  unsigned num_maps_allocated = LINEMAPS_ALLOCATED (set, macro_p);
+  unsigned num_maps_used = LINEMAPS_USED (set, macro_p);
 
-  if (LINEMAPS_USED (set, macro_map_p) == LINEMAPS_ALLOCATED (set, macro_map_p))
+  if (num_maps_used == num_maps_allocated)
     {
-      /* We ran out of allocated line maps. Let's allocate more.  */
-      size_t alloc_size;
-
-      /* Cast away extern "C" from the type of xrealloc.  */
-      line_map_realloc reallocator = (set->reallocator
-				      ? set->reallocator
-				      : (line_map_realloc) xrealloc);
-      line_map_round_alloc_size_func round_alloc_size =
-	set->round_alloc_size;
-
-      size_t map_size = (macro_map_p
-			 ? sizeof (line_map_macro)
-			 : sizeof (line_map_ordinary));
+      /* We need more space!  */
+      if (!num_maps_allocated)
+	num_maps_allocated = 128;
+      num_maps_allocated *= 2;
+
+      size_t size_of_a_map;
+      void *buffer;
+      if (macro_p)
+	{
+	  size_of_a_map = sizeof (line_map_macro);
+	  buffer = set->info_macro.maps;
+	}
+      else
+	{
+	  size_of_a_map = sizeof (line_map_ordinary);
+	  buffer = set->info_ordinary.maps;
+	}
 
       /* We are going to execute some dance to try to reduce the
 	 overhead of the memory allocator, in case we are using the
 	 ggc-page.c one.
 	 
 	 The actual size of memory we are going to get back from the
-	 allocator is the smallest power of 2 that is greater than the
-	 size we requested.  So let's consider that size then.  */
-
-      alloc_size =
-	(2 * LINEMAPS_ALLOCATED (set, macro_map_p) +  256)
-	* map_size;
-
-      /* Get the actual size of memory that is going to be allocated
-	 by the allocator.  */
-      alloc_size = round_alloc_size (alloc_size);
+	 allocator may well be larger than what we ask for.  Use this
+	 hook to find what that size is.  */
+      size_t alloc_size
+	= set->round_alloc_size (num_maps_allocated * size_of_a_map);
 
       /* Now alloc_size contains the exact memory size we would get if
 	 we have asked for the initial alloc_size amount of memory.
-	 Let's get back to the number of macro map that amounts
-	 to.  */
-      LINEMAPS_ALLOCATED (set, macro_map_p) =
-	alloc_size / map_size;
-
-      /* And now let's really do the re-allocation.  */
-      if (macro_map_p)
-	{
-	  set->info_macro.maps
-	    = (line_map_macro *) (*reallocator) (set->info_macro.maps,
-						 (LINEMAPS_ALLOCATED (set, macro_map_p)
-						  * map_size));
-	  result = &set->info_macro.maps[LINEMAPS_USED (set, macro_map_p)];
-	}
-      else
-	{
-	  set->info_ordinary.maps =
-	    (line_map_ordinary *) (*reallocator) (set->info_ordinary.maps,
-						  (LINEMAPS_ALLOCATED (set, macro_map_p)
-						   * map_size));
-	  result = &set->info_ordinary.maps[LINEMAPS_USED (set, macro_map_p)];
-	}
-      memset (result, 0,
-	      ((LINEMAPS_ALLOCATED (set, macro_map_p)
-		- LINEMAPS_USED (set, macro_map_p))
-	       * map_size));
-    }
-  else
-    {
-      if (macro_map_p)
-	result = &set->info_macro.maps[LINEMAPS_USED (set, macro_map_p)];
+	 Let's get back to the number of map that amounts to.  */
+      unsigned num_maps = alloc_size / size_of_a_map;
+      buffer = set->reallocator (buffer, num_maps * size_of_a_map);
+      memset ((char *)buffer + num_maps_used * size_of_a_map, 0,
+	      (num_maps - num_maps_used) * size_of_a_map);
+      if (macro_p)
+	set->info_macro.maps = (line_map_macro *)buffer;
       else
-	result = &set->info_ordinary.maps[LINEMAPS_USED (set, macro_map_p)];
+	set->info_ordinary.maps = (line_map_ordinary *)buffer;
+      LINEMAPS_ALLOCATED (set, macro_p) = num_maps;
     }
 
-  result->start_location = start_location;
+  line_map *result = (macro_p ? (line_map *)&set->info_macro.maps[num_maps_used]
+		      : (line_map *)&set->info_ordinary.maps[num_maps_used]);
+  LINEMAPS_USED (set, macro_p)++;
 
-  LINEMAPS_USED (set, macro_map_p)++;
+  result->start_location = start_location;
 
   return result;
 }

Patch

2018-08-06  Nathan Sidwell  <nathan@acm.org>

	* line-map.c: (linemap_init): Set default allocator here.
	(new_linemap): Rather than here.  Refactor allocation logic.

Index: line-map.c
===================================================================
--- line-map.c	(revision 263332)
+++ line-map.c	(working copy)
@@ -346,6 +346,8 @@  linemap_init (struct line_maps *set,
 #else
   new (set) line_maps();
 #endif
+  /* Set default allocator.  */
+  set->reallocator = (line_map_realloc) xrealloc;
   set->highest_location = RESERVED_LOCATION_COUNT - 1;
   set->highest_line = RESERVED_LOCATION_COUNT - 1;
   set->location_adhoc_data_map.htab =
@@ -376,81 +378,58 @@  linemap_check_files_exited (struct line_
 static struct line_map *
 new_linemap (struct line_maps *set,  source_location start_location)
 {
-  struct line_map *result;
-  bool macro_map_p = start_location >= LINE_MAP_MAX_LOCATION;
+  bool macro_p = start_location >= LINE_MAP_MAX_LOCATION;
+  unsigned alloc = LINEMAPS_ALLOCATED (set, macro_p);
+  unsigned used = LINEMAPS_USED (set, macro_p);
 
-  if (LINEMAPS_USED (set, macro_map_p) == LINEMAPS_ALLOCATED (set, macro_map_p))
+  if (used == alloc)
     {
-      /* We ran out of allocated line maps. Let's allocate more.  */
-      size_t alloc_size;
-
-      /* Cast away extern "C" from the type of xrealloc.  */
-      line_map_realloc reallocator = (set->reallocator
-				      ? set->reallocator
-				      : (line_map_realloc) xrealloc);
-      line_map_round_alloc_size_func round_alloc_size =
-	set->round_alloc_size;
-
-      size_t map_size = (macro_map_p
-			 ? sizeof (line_map_macro)
-			 : sizeof (line_map_ordinary));
+      /* We need more space!  */
+      if (!alloc)
+	alloc = 128;
+      alloc *= 2;
+
+      size_t map_size;
+      void *buffer;
+      if (macro_p)
+	{
+	  map_size = sizeof (line_map_macro);
+	  buffer = set->info_macro.maps;
+	}
+      else
+	{
+	  map_size = sizeof (line_map_ordinary);
+	  buffer = set->info_ordinary.maps;
+	}
 
       /* We are going to execute some dance to try to reduce the
 	 overhead of the memory allocator, in case we are using the
 	 ggc-page.c one.
 	 
 	 The actual size of memory we are going to get back from the
-	 allocator is the smallest power of 2 that is greater than the
-	 size we requested.  So let's consider that size then.  */
-
-      alloc_size =
-	(2 * LINEMAPS_ALLOCATED (set, macro_map_p) +  256)
-	* map_size;
-
-      /* Get the actual size of memory that is going to be allocated
-	 by the allocator.  */
-      alloc_size = round_alloc_size (alloc_size);
+	 allocator may well be larger than what we ask for.  Use this
+	 hook to find what that size is.  */
+      size_t alloc_size = set->round_alloc_size (alloc * map_size);
 
       /* Now alloc_size contains the exact memory size we would get if
 	 we have asked for the initial alloc_size amount of memory.
 	 Let's get back to the number of macro map that amounts
 	 to.  */
-      LINEMAPS_ALLOCATED (set, macro_map_p) =
-	alloc_size / map_size;
-
-      /* And now let's really do the re-allocation.  */
-      if (macro_map_p)
-	{
-	  set->info_macro.maps
-	    = (line_map_macro *) (*reallocator) (set->info_macro.maps,
-						 (LINEMAPS_ALLOCATED (set, macro_map_p)
-						  * map_size));
-	  result = &set->info_macro.maps[LINEMAPS_USED (set, macro_map_p)];
-	}
-      else
-	{
-	  set->info_ordinary.maps =
-	    (line_map_ordinary *) (*reallocator) (set->info_ordinary.maps,
-						  (LINEMAPS_ALLOCATED (set, macro_map_p)
-						   * map_size));
-	  result = &set->info_ordinary.maps[LINEMAPS_USED (set, macro_map_p)];
-	}
-      memset (result, 0,
-	      ((LINEMAPS_ALLOCATED (set, macro_map_p)
-		- LINEMAPS_USED (set, macro_map_p))
-	       * map_size));
-    }
-  else
-    {
-      if (macro_map_p)
-	result = &set->info_macro.maps[LINEMAPS_USED (set, macro_map_p)];
+      unsigned num_maps = alloc_size / map_size;
+      buffer = set->reallocator (buffer, num_maps * map_size);
+      memset ((char *)buffer + used * map_size, 0, (num_maps - used) * map_size);
+      if (macro_p)
+	set->info_macro.maps = (line_map_macro *)buffer;
       else
-	result = &set->info_ordinary.maps[LINEMAPS_USED (set, macro_map_p)];
+	set->info_ordinary.maps = (line_map_ordinary *)buffer;
+      LINEMAPS_ALLOCATED (set, macro_p) = num_maps;
     }
 
-  result->start_location = start_location;
+  line_map *result = (macro_p ? (line_map *)&set->info_macro.maps[used]
+		      : (line_map *)&set->info_ordinary.maps[used]);
+  LINEMAPS_USED (set, macro_p)++;
 
-  LINEMAPS_USED (set, macro_map_p)++;
+  result->start_location = start_location;
 
   return result;
 }