[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [patch] mem_node shrink
From: |
Dmitry Antipov |
Subject: |
Re: [patch] mem_node shrink |
Date: |
Thu, 22 Nov 2007 16:57:20 +0300 |
User-agent: |
Thunderbird 2.0.0.6 (X11/20070926) |
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