Re: Bug#245678: zsh: built-in rm -rf fills up the memory

[Peter Stephenson <pws@xxxxxxx>]

Bart Schaefer wrote:
> > However, I don't think even *that* is the basic problem.  I think this:
> > 
> >     old = (old + H_ISIZE - 1) & ~(H_ISIZE - 1);
> >     new = (new + H_ISIZE - 1) & ~(H_ISIZE - 1);
> > 
> > is wrong --- it should be the heap size here, not H_ISIZE which is
> > simply sizeof(union mem_align), which is probably only 4 or 8 words.
> 
> It took me a while staring at this to decide that it was doing the right 
> thing, but I came to the conclusion that it's only actually _wrong_ if
> HEAP_ARENA_SIZE is _not_ a multiple of H_ISIZE.  And even then I'm pretty
> sure "wrong" just means "inefficient".

I'd have to follow through with a debugger, really... But I can't see
any mechanism to prevent it reallocating for every few bytes of increase
once the size goes beyond HEAP_ARENA_SIZE, which can't be the
intention.  The chunk #ifdef'd out for USE_MMAP is the first point where
we see if we have enough space for `new' already (except for special
cases old == new, we allocated the space then shrank, new is NULL).
So for new and old larger than HEAP_ARENA_SIZE (which is bound to
happen after the first attempt to grow), we call realloc() every time
the difference crosses an H_ISIZE boundary.  What have I missed?

> > We only need to reallocate to heap size boundaries
> 
> That's correct, but we _do_ only reallocate to heap size boundaries in the
> non-MMAP case.  I think.  It's only the MMAP'd code, which as I said was
> pasted in long after the original algorithm was written, that botches it.

Yes, but we then call realloc unnecessarily in between actually growing
the heap, unless my logic above is wrong.  It will reallocate even
though the new size of the heap is the same as the old one.

> > I might be tempted by proposed solutions involving the complete removal
> > of hrealloc.
> 
> Which would mean what, exactly?  Never grow heap blocks?

hrealloc is only used as an optimisation.  If necessary we could always
allocate new space.  In the case of the files module we could use
zalloc and zrealloc and let the system worry about the problem.  Then we
lose MMAP altogether, of course.

-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************