Re: [PATCH] optimize malloc0

[Rich Felker <dalias@libc.org>]

On Wed, Jul 05, 2017 at 04:28:28PM +0300, Alexander Monakov wrote:
> 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.

OK. My aim was to discover whether the cleverness was justified, and
in the end I think it mostly was. Until understanding that, I don't
think I had a lot of constructive feedback on how it could be less
"overly clever". Maybe a brief high-level description of what the code
is doing and why, either in the email or a comment, would have helped.

> > 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.

Sorry, I really should have just written what I meant as code. My
intent was to have the MIN(diff_to_end_of_page, diff_to_end_of_chunk)
logic in the conditional for "found a nonzero word".

> > 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.

That's okay, we can follow up on this later if anyone cares about the
size. I don't think it's a big deal.

Rich