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.