From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11670 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] optimize malloc0 Date: Wed, 5 Jul 2017 12:13:10 -0400 Message-ID: <20170705161310.GA1627@brightrain.aerifal.cx> References: <20170626214339.10942-1-amonakov@ispras.ru> <20170704214554.GS1627@brightrain.aerifal.cx> <20170704233910.GW1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1499271207 25861 195.159.176.226 (5 Jul 2017 16:13:27 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 5 Jul 2017 16:13:27 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-11683-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jul 05 18:13:23 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1dSmvX-0006KJ-L7 for gllmg-musl@m.gmane.org; Wed, 05 Jul 2017 18:13:19 +0200 Original-Received: (qmail 3078 invoked by uid 550); 5 Jul 2017 16:13:22 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 2036 invoked from network); 5 Jul 2017 16:13:22 -0000 Content-Disposition: inline In-Reply-To: Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:11670 Archived-At: 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