[m-rev.] Post commit review: Remove available-space field from region struct

[Quan Phan]

On Thu, Dec 20, 2007 at 06:36:38PM +1100, Zoltan Somogyi wrote:
> On 19-Dec-2007, Quan Phan <quan.phan at cs.kuleuven.be> wrote:
> > pointer to last word of the data area can be computed by
> > last_page->MR_regionpage_space + MR_REGION_PAGE_SPACE_SIZE - 1.
> 
> Yes, it can. However, when you allocate a new region page, you will be
> (a) initializing the pointer to the next page (to null)
> (b) filling in the first cell allocated from that region page.
> 
> If the pointer to the next page is at the end of the region page, you are
> guaranteed to get separate cache misses for (a) and (b). If the pointer
> to the next page is at the start of the region page, a single cache miss
> will handle both, provided the cell is small enough.
Ok, I have little working experience with cache behaviour. So I made this
change. 

> I would say that the
> conditionally included profiling field, MR_regionpage_allocated_size,
> should also be put near the start.
Now I don't see why I decided to put MR_regionpage_allocated_size into
MR_RegionPage. That field is to collect memory allocated into a region,
so it should be better in MR_RegionHeader. I made this change.

> 
> Note that the code currently has several casts between MR_Region and
> MR_RegionPage, all of which will need updating if you move the fields.
> However, the casts should be replaced anyway by a conversion macro,
> and this can be updated to apply any required adjustments by casting
> not the region page, but its MR_regionpage_space field.
> 
> I think there is another potential performance problem in the current value
> of MR_REGION_PAGE_SPACE_SIZE. We want every MR_RegionPage structure to have a
> power of two size, since that makes both allocators and caches as efficient
> as possible. However, MR_RegionPage's size is now 257 words (258 with RBMM
> profiling).
> 
> For now, I suggest setting MR_REGION_PAGE_SPACE_SIZE to 255. In the longer
> term, I would suggest replacing MR_REGION_PAGE_SPACE_SIZE with another macro,
> MR_REGION_PAGE_SIZE (set to a power of two), and defining MR_RegionPage like
> this:
> 
> struct MR_RegionPage_Struct {
> 	MR_RegionPage	*MR_regionpage_next;
> #if ...
> 	MR_Integer	MR_regionpage_allocated_size;
> #endif
> 	MR_Word		MR_regionpage_space[1];
> }
> 
> The size of the array would of course be a lie, but C doesn't mind, and
> we can easily specify MR_REGION_PAGE_SIZE * sizeof(MR_Word) in the call
> to malloc. Or we could set the array size at MR_REGION_PAGE_SIZE minus one
> or two words.
It would take no change to the current code if we use both macros
with MR_REGION_PAGE_SPACE_SIZE = MR_REGION_PAGE_SIZE - 1. You said to me
before that the MMC has a limitation on the number of argument a functor
can have. I think if it is reasonable we can derive the value of PAGE_SIZE
from that number. 

 
> >  /*
> >  ** This method is to be called at the start of the then part of an ite with
> >  ** semidet condition (most of the times).
> > -** At that point we will only check if the region is disj-protected or not.
> > +** XXX At that point we will only check if the region is disj-protected or not.
> > +** At that point we do not have to check whether this ite-protected region is 
> > +** disj-protected or not. It is because the region is protected for this ite
> > +** only if it has not been protected before the ite and from the start of
> > +** this ite to the point when this method is called, i.e., the condition, the
> > +** region cannot be disj-protected by any disjunctions because this condition
> > +** is semidet.
> >  */
> 
> The idea of a more detailed comment is good, but I don't understand it;
> the sentence structure is too complex

I rephrased it.

> 
> >  void
> >  MR_remove_undisjprotected_region_ite_then_semidet(MR_Region *region)
> >  {
> > +    /*
> >      MR_region_debug_try_remove_region(region);
> >      if ( !MR_region_is_disj_protected(region) ) {
> >          MR_region_destroy_region(region);
> > @@ -293,6 +299,9 @@
> >          region->MR_region_logical_removed = 1;
> >          MR_region_debug_logically_remove_region(region);
> >      }
> > +    */
> > +    MR_region_destroy_region(region);
> > +    MR_region_debug_destroy_region(region);
> >  }
> 
> Why comment out the code? If not needed now, you should delete it.
> If you ever need to resurrect it, you can do so using the old versions
> in CVS.

Deleted now.

 
> > -                    protected_region->MR_region_ite_protected =             \
> > -                        top_ite_frame->MR_riff_previous_ite_frame;          \
> > +                    protected_region->MR_region_ite_protected = NULL;       \
> 
> This is because we don't re-protect previously protected regions, right?
You are right, an ite-protected region is in one and only one ite frame. 

> 
> The diff is otherwise fine. I will post my diff to mercury-reviews in a
> minute for you to review after commit. If you find any problems with it,
> you can commit a fix without a review.
I will commit all the changes mentioned about in a few minutes.

Regards,
Quan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------