Re: [patch] mem_node shrink

[Dmitry Antipov <dmantipov@yandex.ru>]

Richard Stallman wrote:

> The idea looks like an improvement.  I think there needs to be a comment
> explaining that the widths of these fields are supposed to add up to
> the same as an EMACS_INT.
> 
> And is EMACS_INT the right thing?  EMACS_INT is `long' in some cases.
> Should it be plain `int' instead?  Should it be a type that's as wide
> as a pointer or as size_t?
> 
> Should the size value be measured in units of Lisp_Object instead
> of bytes?
> 
>     +  if (size > MOST_POSITIVE_FIXNUM)
>     +    abort ();
>     +#endif
> 
> That's not reliably the correct test.  It happens to be right, at
> present, because the width of the field is BITS_PER_EMACS_INT - 4, and
> it is unsigned, so it has the same number of bits as a positive Lisp
> integer.  But that is just coincidence.
> 
> So I think MOST_POSITIVE_FIXNUM should be replaced with something
> guaranteed to be right.  Define a constant to serve as the width of
> that field, and use the same constant here in something like -((-1) <<
> MEM_NODE_SIZE_WIDTH).

If GC_MALLOC_CHECK is defined, xmalloc and xrealloc calls mem_insert.  On 64-bit system,
if neither USE_MMAP_FOR_BUFFERS nor REL_ALLOC are used, xmalloc and xrealloc may grow
buffer text up to the size which fits into integer, but not into 28-bit bitfield.  So,
the bitfield should be wide enough, i.e. have width BITS_PER_EMACS_INT - 4.

If GC_MALLOC_CHECK isn't defined, mem_insert is probably never called for an area
which is >= 256M even on a 64-bit machine, so it should be safe to use BITS_PER_INT - 4.

This stuff should be checked carefully - I don't have an access to any 64-bit system for
now, but hopefully will have it in a few days...

> Wouldn't it be cleaner for mem_insert to do  max (..., 1) ?

IMHO, inserting a 'region' of zero size is a nonsense because:

  a) In general, malloc (0) is most probably a bug (although some implementations
     returns a non-NULL pointer to the space which can be accessed), it should be
     aborted, and other code shouldn't assume that returned pointer is valid;

  b) In the case of Emacs, xrealloc (ptr, 0) should be equal to free (ptr) regardless
     of the ptr value (and return NULL). Rationale may be found, for example, within
     shrink_regexp_cache: it may call xrealloc (ptr, 0) and, if cp->buf.buffer is NULL
     (which happens during bootstrap), this xrealloc yields to malloc (0). If malloc
     allocates something and returns non-NULL in this case, we may have an allocated
     cp->buf.buffer even if cp->buf.allocated == cp->buf.used == 0, which is definitely
     stupid (but not a fatal bug).

Dmitry