Re: [Help-smalltalk] Read-only oldspace

[Paolo Bonzini]

> Paolo can you check the patch, here I've changed a bit the behavior of
> make_ro; I preserve F_FIXED, F_OLD, or young information thus when
> making an object writable I can restore it in the good space.
> It seems to be the definitive version

Just a few comments...

> +      /* become writeable */
> +      if ((oop->flags & F_LOADED) == 0)
> +        {
> +          if (oop->flags & F_FIXED)
> +            newObj = (gst_object) _gst_mem_alloc (_gst_mem.fixed, size);
> +          else if (oop->flags & F_OLD)
> +            newObj = (gst_object) _gst_mem_alloc (_gst_mem.old, size);
> +          else
> +            newObj = (gst_object) _gst_alloc_new_obj (size);

This can use _gst_alloc_obj.

> +  if (oop->flags & F_READONLY)
> +    {
> +      oop->flags |= F_FIXED;

Add a comment somewhere that rospace is never compacted.  As a 
consequence...

>  gst_object
> +_gst_alloc_ro_obj (size_t size,
> +                    OOP *p_oop)
> +{
> +  gst_object p_instance;
> +
> +  size = ROUNDED_BYTES (size);
> +
> +  /* If the object is big enough, we put it directly in oldspace.  */
> +  p_instance = (gst_object) _gst_mem_alloc (_gst_mem.ro_old, size);
> +  if COMMON (p_instance)
> +    goto ok;
> +
> +  _gst_global_gc (size);
> +  p_instance = (gst_object) _gst_mem_alloc (_gst_mem.ro_old, size);
> +  if COMMON (p_instance)
> +    goto ok;
> +
> +  _gst_compact (0);
> +  p_instance = (gst_object) _gst_mem_alloc (_gst_mem.ro_old, size);

... the compaction step here is not necessary.

> @@ -889,25 +1054,43 @@ oldspace_before_freeing (heap_data *h, heap_block *blk, 
> size_t sz)
>  }
>
>  heap_data *
> -oldspace_nomemory (heap_data *h, size_t sz)
> +grow_nomemory (heap_data *h, size_t sz)
>  {
>    if (!_gst_gc_running)
>      _gst_global_gc (sz);
>    else
>      {
>        /* Already garbage collecting, emergency growth just to satisfy
> -        tenuring necessities.  */
> -      int grow_amount_to_satisfy_rate = _gst_mem.old->heap_limit
> +         tenuring necessities.  */
> +      int grow_amount_to_satisfy_rate = h->heap_limit
>             * (100.0 + _gst_mem.space_grow_rate) / 100;
> -      int grow_amount_to_satisfy_threshold =
> -          (sz + _gst_mem.old->heap_total)
> -          * 100.0 /_gst_mem.grow_threshold_percent;
> +      int grow_amount_to_satisfy_threshold =
> +           (sz + h->heap_total)
> +           * 100.0 /_gst_mem.grow_threshold_percent;
>
> -      _gst_mem.old->heap_limit = MAX (grow_amount_to_satisfy_rate,
> -                                     grow_amount_to_satisfy_threshold);
> +      h->heap_limit = MAX (grow_amount_to_satisfy_rate,
> +                           grow_amount_to_satisfy_threshold);
>      }
>
> -  return _gst_mem.old;
> +  return h;

This change to the "return" is wrong -- yes, it's tricky, I also 
misremembered it the other day and probably didn't explain it well 
enough on IRC.  _gst_mem.old could have changed due to the GC.  So just 
make this function void...

> +}
> +
> +heap_data *
> +ro_oldspace_nomemory (heap_data *h, size_t sz)
> +{
> +  return grow_nomemory (_gst_mem.ro_old, sz);

... and return the appropriate space from these functions 
(_gst_mem.ro_old here and likewise elsewhere).

> +          if (flags & F_READONLY)
> +            object = (gst_object) _gst_mem_alloc (_gst_mem.ro_old, size);
> +         else if (flags & F_FIXED)

This is bad unfortunately.  There are a _lot_ of readonly objects, and 
allocating them will cause a measurable slowdown of image load.  I'm not 
sure how you can fix that, perhaps by first storing readonly objects, 
then page aligning the file, and then storing other objects.  Remember 
that any slowdown incurred by image save is acceptable.

Otherwise looks good!  Have you measured a practical performance 
difference with it?

Paolo