[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170705161310.GA1627@brightrain.aerifal.cx>
Date: Wed, 5 Jul 2017 12:13:10 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] optimize malloc0
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
