Re: [PATCH] optimize malloc0

[Alexander Monakov <amonakov@ispras.ru>]

On Tue, 4 Jul 2017, Rich Felker wrote:
> > > Overall I like this. Reviewing what was discussed on IRC, I called the
> > > loop logic clever and nsz said maybe a bit too clever. On further
> > > reading I think he's right.
> > 
> > Somehow raising this point in the context of the rest of src/malloc seems
> > even worse than common bikeshed.
> 
> I don't think so -- I actually had to re-read the code a few times
> before I understood what it was doing. Yes, maybe there's some
> confusing code in malloc.c, and I'd like to avoid repeating that when
> it's rewritten, but I think concern about making it worse is valid.

My main gripe is with the way this feedback was offered. It shouldn't be
ok to say "nah, too clever" and just leave it at that - at least make an
effort to elaborate or suggest improvements? How should contributors
find a balance between "acceptably non-clever" code and code that lives
up to general expectations of efficiency and conciseness in musl?

That we seem to disagree on this code being simple enough is secondary.

> I was saying that, if we want to do a simple, idiomatic forward loop
> like I described, the need for special-casing the first and last
> partial pages could be avoided by preloading nonzero data in 2
> specific places, so that the same logic that switches to memset for
> the interior pages would also work for the boundary ones.

This doesn't address the need to treat loop/memset boundaries separately
for boundary pages. In fact, what you wrote in the previous email, if
interpreted literally, would clear the whole page at the end of region.

> That sounds nice, but do you have a proposal for how it would work?
> Dummy weak mal0_clear in malloc.c with the working definition in
> calloc.c? Just putting it in a separate TU wouldn't do anything to
> help since malloc.c would still have a reference to it.

Hm, yes, probably something like that, perhaps with also adding a weak
definition of calloc in lite_malloc.c. I haven't really looked into it.

Alexander