Message ID | 5e8d4a6f-7785-e4b2-1685-2483475c7cec@acm.org |
---|---|
State | New |
Headers | show |
Series | Line map table allocation | expand |
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
On 08/07/2018 03:43 PM, David Malcolm wrote:
> OK for trunk, with the nits noted above fixed.
Committing this, thanks
nathan
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; }