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