Re: [patch] mem_node shrink

[Dmitry Antipov]

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