[m-rev.] For review: Make improvements to stack segments code.

[Paul Bone]

On Mon, Jun 07, 2010 at 12:41:07AM +1000, Julien Fischer wrote:
>
> Hi Paul,
>
> Here are some initial comments on this change.
>
> On Thu, 27 May 2010, Paul Bone wrote:
>
>>    MR_create_or_reuse_zone (formerly MR_create_zone) checks for a free zone
>>    of at least the required size before allocating a new one.  When zones are
>>    released they are put on the free list.  Garbage collection of free zones
>>    is not yet implemented.
>
> Will the garbage collector still scan the memory zones on the free list?
> (Not free the zones could be problematic in trail segment grades because
> if part of the program does something that allocates a lot of trail
> segments, we may end up with a lot of memory zones sitting on the free
> list after the commit; this may tie up memory that may be needed by
> later parts of the program.)

Yes, Boehm will scan these memory segments.  I will allow these to be freed in
a future change.

>>    As above MR_create_zone is now MR_create_or_reuse_zone,
>>
>>    MR_unget_zone is now MR_release_zone.
>>
>>    MR_construct_zone has been removed.
>
> Say why, i.e. it is no longer required.
>

    MR_construct_zone has been removed, it was only ever called by                                                                MR_create_or_reuse_zone.  MR_create_or_reuse_zone now contains the code for
    MR_construct_zone.

>>    error handlers.
>>
>>    Do better locking on the free_memory_zones and used_memory_zones lists.
>
> Better locking in what sense?
>

    Updates to used_memory_zones now use a pthread mutex so that only one
    thread may be updating the list at once.  This lock is shared with the
    free_memory_zones structure.

>>    Make sure that list modification makes memory writes in a clear order so
>>    that the lists are always well formed.

I re-wrote this:

    Updates to used_memory_zones now use memory barriers to guarantee that
    concurrent reads always read a consistent, but possibly incomplete,
    data-structure.  This is necessary because it is read from a signal handler
    which cannot call pthread_mutex().

>> Index: runtime/mercury_memory_zones.c
>> ===================================================================
>> RCS file: /home/mercury1/repository/mercury/runtime/mercury_memory_zones.c,v
>> retrieving revision 1.36
>> diff -u -p -b -r1.36 mercury_memory_zones.c
>> --- runtime/mercury_memory_zones.c	13 Feb 2010 07:29:10 -0000	1.36
>> +++ runtime/mercury_memory_zones.c	27 May 2010 01:31:04 -0000
>> @@ -63,8 +63,14 @@
>>
>> #ifdef MR_THREAD_SAFE
>>   #include "mercury_thread.h"
>> +  #include "mercury_atomic_ops.h"
>> #endif
>>
>> +#include "mercury_memory_handlers.h"
>> +
>> +/*
>> +** Why is this included here and not above with the other system includes?
>> +*/
>
> Did you leave out an XXX there?

Yes,

>> @@ -469,12 +553,92 @@ MR_unget_zone(MR_MemoryZone *zone)
>>     } else {
>>         prev->MR_zone_next = tmp->MR_zone_next;
>>     }
>> +    MR_UNLOCK(&memory_zones_lock, "remove_zone_frm_used_list");
>> +}
>> +
>> +static void
>> +MR_return_zone_to_free_list(MR_MemoryZone *zone)
>> +{
>> +    MR_MemoryZonesFree      *tmp_list;
>
> Please use a more meaningful name than tmp_list.

    /* The current list in iterations over the list of free lists */
    MR_MemoryZonesFree      *cur_list;

>
>> +    size_t                  size;
>> +
>> +    size = get_zone_alloc_size(zone);
>> +
>> +    MR_LOCK(&memory_zones_lock, "ireturn_zone_to_free_list");
>
> ireturn?
>

I must have typeod this and not spotted it.

>> -MR_MemoryZone *
>> -MR_construct_zone(const char *name, int id, MR_Word *base,
>> -    size_t size, size_t offset, size_t redsize, MR_ZoneHandler handler)
>> +static MR_MemoryZone *
>> +MR_create_new_zone(size_t desired_size, size_t redzone_size)
>> {
>> +    size_t          offset;
>>     MR_MemoryZone   *zone;
>> +    MR_Word         *base;
>>     size_t          total_size;
>>
>> +    offset = MR_next_offset();
>> +    /*
>> +    ** Ignore the offset if it is at least half the desired size of the zone.
>> +    ** This should only happen for very small zones.
>> +    */
>> +    if ((offset * 2) > desired_size) {
>> +        offset = 0;
>> +    }
>> +
>> +    /*
>> +    ** The redzone must be page aligned and page multiple.
>
> Do you mean:
>
> 	... and its size must be a multiple of the page size.

Yes,

Changed this.

>> +    */
>> +    redzone_size = MR_round_up(redzone_size, MR_page_size);
>> +    /*
>> +    ** Include an extra page size for the hardzone.
>> +    */
>> +    total_size = desired_size + redzone_size + MR_page_size;
>> +    /*
>> +    ** The total size must also be rounded to a page boundary, so that it can
>> +    ** be allocated from mmap if we're using accurate GC.
>> +    */
>> +    total_size = MR_round_up(total_size, MR_page_size);
>> +
>> +    base = (MR_Word *) MR_alloc_zone_memory(total_size);
>>     if (base == NULL) {
>> -        MR_fatal_error("MR_construct_zone called with NULL pointer");
>> +        MR_fatal_error("Unable to allocate memory for zone");
>>     }
>>
>> -    zone = MR_get_zone();
>> +    zone = MR_GC_NEW(MR_MemoryZone);
>>
>> -    zone->MR_zone_name = name;
>> -    zone->MR_zone_id = id;
>> -    zone->MR_zone_desired_size = size;
>> -    zone->MR_zone_redzone_size = redsize;
>> +    zone->MR_zone_name = NULL;
>> +#ifdef MR_THREAD_SAFE
>> +    zone->MR_zone_id = MR_atomic_add_and_fetch_uint(&MR_context_id_counter, 1);
>> +#else
>> +    zone->MR_zone_id = ++MR_context_id_counter;
>> +#endif
>> +    zone->MR_zone_desired_size = desired_size;
>> +    zone->MR_zone_redzone_size = redzone_size;
>>
>> #ifdef  MR_CHECK_OVERFLOW_VIA_MPROTECT
>> -    zone->MR_zone_handler = handler;
>> +    zone->MR_zone_handler = NULL;
>> #endif /* MR_CHECK_OVERFLOW_VIA_MPROTECT */
>>
>> -    /*
>> -    ** XXX If `zone' is pulled off the free-list (rather than newly allocated)
>> -    ** then zone->MR_zone_bottom will be pointing to a previously allocated
>> -    ** memory region.  Setting it to point to `base' therefore causes a memory
>> -    ** leak.  `base' should not be allocated in the first place if `zone' is
>> -    ** to be reused.  See also the workaround in MR_unget_zone().
>> -    */
>>     zone->MR_zone_bottom = base;
>>
>> -#ifdef  MR_PROTECTPAGE
>> -    total_size = size + MR_unit;
>> -#else
>> -    total_size = size;
>> -#endif  /* MR_PROTECTPAGE */
>> -
>>     zone->MR_zone_top = (MR_Word *) ((char *) base + total_size);
>>     zone->MR_zone_min = (MR_Word *) ((char *) base + offset);
>> #ifdef  MR_LOWLEVEL_DEBUG
>> @@ -593,6 +795,9 @@ MR_extend_zone(MR_MemoryZone *zone, size
>>         MR_fatal_error("MR_extend_zone called with NULL pointer");
>>     }
>>
>> +    /*
>> +    ** Why use this value for new_total_size?
>> +    */
>
> XXX?
>

Yes.  I've added this.

>> #ifdef  MR_PROTECTPAGE
>>     new_total_size = new_size + 2 * MR_unit;
>> #else
>> @@ -645,25 +850,34 @@ MR_extend_zone(MR_MemoryZone *zone, size
>>     return base_incr;
>> }


>> Index: runtime/mercury_memory_zones.h
>> ===================================================================
>> RCS file: /home/mercury1/repository/mercury/runtime/mercury_memory_zones.h,v
>> retrieving revision 1.20
>> diff -u -p -b -r1.20 mercury_memory_zones.h
>> --- runtime/mercury_memory_zones.h	5 Sep 2008 11:19:33 -0000	1.20
>> +++ runtime/mercury_memory_zones.h	26 May 2010 07:54:44 -0000
>> @@ -134,6 +135,22 @@ struct MR_MemoryZones_Struct {
>>     MR_MemoryZones      *MR_zones_tail;
>> };
>>
>> +/*
>> +** Free memory zones are arranged in a list of lists, The outer list (below)
>> +** associates a size of the zones in each inner list.  It is to be kept in
>> +** sorted order from smallest to largest.  So that a traversal of this list
>> +** returns the zones that are the 'best fit' as the 'first fit'.  The inner
>> +** lists (using the MR_zone_next field of the zones contain zones of all the
>> +** same size.
>> +*/
>
> Elsewhere you refer to the outer list as the major list, and the inner
> list as the minor list.
>

I've made the other comments conformant with this comment.

>> +typedef struct MR_MemoryZonesFree_Struct MR_MemoryZonesFree;
>> +
>> +struct MR_MemoryZonesFree_Struct {
>> +	size_t			MR_zonesfree_size;
>> +	MR_MemoryZone		*MR_zonesfree_head;
>> +	MR_MemoryZonesFree	*MR_zonesfree_tail;
>> +};
>> +
>> 	/*
>> 	** MR_zone_end specifies the end of the area accessible without
>> 	** a page fault. It is used by MR_clear_zone_for_GC().

>> Index: runtime/mercury_misc.c
>> ===================================================================
>> RCS file: /home/mercury1/repository/mercury/runtime/mercury_misc.c,v
>> retrieving revision 1.28
>> diff -u -p -b -r1.28 mercury_misc.c
>> --- runtime/mercury_misc.c	14 Nov 2006 00:15:40 -0000	1.28
>> +++ runtime/mercury_misc.c	26 May 2010 07:54:44 -0000
>> @@ -88,9 +88,13 @@ void
>> MR_fatal_error(const char *fmt, ...)
>> {
>>     va_list args;
>> +    int error = errno;
>>
>>     fflush(stdout);     /* in case stdout and stderr are the same */
>>
>> +    if (error != 0) {
>> +        fprintf(stderr, "Errno: %s\n", strerror(error));
>> +    }
>
> I suggest also printing errno itself there, sometimes the description
> won't be that useful, e.g. when it's something like "Unknown error".
>

There was a presentation about this at LCA this year.  It was pretty amusing.

Abstract: http://www.lca2010.org.nz/programme/schedule/view_talk/50260?day=wednesday

Code: http://libexplain.sourceforge.net/

> Please post a revised log message and diff once you have addressed the
> above.

Okay,  There also seem to be some problems with hlc.gc.par, so I'll address
those before I re-post it.

I've fixed all the other errors you've pointed out.

Thanks.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20100607/55cdbcb3/attachment.sig>