Re: [PATCH] khash: clarify that allocations never fail

[Jeff King <peff@peff.net>]

On Sun, Jul 04, 2021 at 11:41:23AM +0200, René Scharfe wrote:

> Btw. the found code is:
> 
> 	start = xmalloc(length);
> 	if (start == NULL) {
> 		errno = ENOMEM;
> 		return MAP_FAILED;
> 	}
> 
> start cannot be NULL, so the check is dead code.

Yep, so it's doubly silly.

> > IMHO that should not be using xmalloc() at all. It is a replacement for
> > system mmap, which can fail with ENOMEM, and we should be able to do the
> > same. Using xmalloc here is probably losing an opportunity to close
> > another pack window to free up memory for a new one.
> 
> Do you mean using malloc(3) directly instead of xmalloc() would no
> longer try to release pack windows?  xmalloc() hasn't done that anymore
> since 9827d4c185 (packfile: drop release_pack_memory(), 2019-08-12).

No, I meant that by using xmalloc() here, if the allocation fails, we'll
immediately die(). Whereas the caller of mmap() could get the ENOMEM
error, then unmap an in-use pack window and try again.

However, I forgot that we don't actually do that (yet). We unmap windows
if we go over our own packed_git_window_size counter, but not when mmap
fails. Eric posted a patch recently to change that, though (at which
point the die() in xmalloc() would be working against us).

(Actually, we did _try_ to do something like that in xmmap prior to
9827d4c185, but I don't think it actually worked since it was based on
our own internal limit, and not what the OS would allow).

> xmalloc() still brings support for zero-sized allocations on platforms
> whose malloc(3) doesn't like them, and it enforces GIT_ALLOC_LIMIT.
> mmap() is supposed give up with EINVAL if the length is 0, so the
> first feature is not actually helping.  And GIT_ALLOC_LIMIT is not
> documented and only useful for testing, right?

Yeah, I think failing on a 0-length mmap is OK, since that's what real
systems do (and this is a wrapper). Our xmmap_gently() handles this,
which is the right spot.

I don't think of GIT_ALLOC_LIMIT as something we're committed to
publicly supporting, though I have (ab)used it once or twice myself.
However, I'm not sure if it makes sense here. True, on a system without
mmap it is a heap allocation, but to me it's conceptually very different
than a normal allocation (and on a system with mmap, we have no problem
at all requesting arbitrarily large maps). If we did want to put a limit
here, I think we'd want to do it at the xmmap layer, using a separate
variable.

-Peff