Re: [PATCH] Line map table allocation

[David Malcolm <dmalcolm at redhat dot com>]

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