From owner-svn-src-head@FreeBSD.ORG Tue Nov 22 18:02:25 2011 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 19ECD106566B; Tue, 22 Nov 2011 18:02:25 +0000 (UTC) (envelope-from lists@eitanadler.com) Received: from mail-fx0-f54.google.com (mail-fx0-f54.google.com [209.85.161.54]) by mx1.freebsd.org (Postfix) with ESMTP id 8F29E8FC16; Tue, 22 Nov 2011 18:02:23 +0000 (UTC) Received: by faap15 with SMTP id p15so999822faa.13 for ; Tue, 22 Nov 2011 10:02:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eitanadler.com; s=0xdeadbeef; h=mime-version:sender:in-reply-to:references:from:date :x-google-sender-auth:message-id:subject:to:cc:content-type :content-transfer-encoding; bh=zBBaUWeThsLI8+tJSSIImjfeqSdp8MD+YHwZHGY2sa0=; b=J3zpAJaXf5FoKxFOqJH38k/zc7a5tRx6v6Bwrbh99GyYcNbA0iN3GhBEnL/AhbNmw+ Fv+N18glw6kSrIPwa9mUmKMRRkFrvElPBd9wqB/fdYSdtS0FAOXyWqqQwyi7o1X1TP1X 6lu9KuXcJUKVZQbe92iy1PH+i0URYhIq3L5Hg= Received: by 10.180.91.137 with SMTP id ce9mr9220149wib.5.1321984942189; Tue, 22 Nov 2011 10:02:22 -0800 (PST) MIME-Version: 1.0 Sender: lists@eitanadler.com Received: by 10.216.21.133 with HTTP; Tue, 22 Nov 2011 10:01:51 -0800 (PST) In-Reply-To: <20111122153332.GA20145@zim.MIT.EDU> References: <201111220250.pAM2oPWC070856@svn.freebsd.org> <20111122153332.GA20145@zim.MIT.EDU> From: Eitan Adler Date: Tue, 22 Nov 2011 13:01:51 -0500 X-Google-Sender-Auth: dsOzowFM9ntVAr0XOUapjEO6HrI Message-ID: To: Eitan Adler , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: dim@freebsd.org, Brooks Davis , theraven@freebsd.org, bde@freebsd.org Subject: Re: svn commit: r227812 - head/lib/libc/string X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Nov 2011 18:02:25 -0000 Meta comment: I should have sent this patch to -hackers prior to seeking approval to commit. I learned my lesson and will seek wider review before making such changes in the future. On Tue, Nov 22, 2011 at 6:21 AM, Bruce Evans wrote: I saw your email about the style changes. I'd like to flesh out what changes should be made first before making another commit>> I guess I'm a little confused. =C2=A0Do we really have profiling>> information at this level that suggests the overhead of the branch is>> significant? I thought most hardware had pretty good>> branch-prediction, particularly with speculative execution. I made this change at the direction of theraven@. I assume he knows a little bit more about compilers than I do. ;)It seems from the rest of this thread that even if this were an optimization it still shouldn't be done at source code code level. > Every time I have profiled the string functions, changes like this always= > have an insignificant effect, even in benchmarks. =C2=A0Normal code spend= s> perhaps 0.01% of its time calling string functions, so the improvements>= from optimizing string functions are 0.01% of an insignificant amount. The problem with profiling this type of change is that it is hard to find a good representative benchmark. I could easily write code that will show you that adding the equality check is a good idea or that it is a horrible idea. IMHO it saves enough time when they are equal, but loses almost no time when the strings are not equal. >> Wouldn't something like __predict_false() have more value for>> performa= nce, It seems that at most this couldn't hurt. On Tue, Nov 22, 2011 at 10:33 AM, David Schultz wrote: > Third, it's not clear that checking whether s1 =3D=3D s2 is even an > optimization. =C2=A0Most programs simply aren't going to pass identical > pointers as arguments to strcmp(), so for the overwhelming > majority of cases, this is just a wasted test and a wasted slot in > a branch predict table. =C2=A0(FWIW, I doubt that a realistic benchmark > would demonstrate any measurable difference.) It is fairly common for programs to compare a value passed to a function to a array of strings. Also note that for the strn*cmp functions we already have a branch so this isn't always a wasted slot. Here is what I'd like to do next: - fix bde@'s style nits - change the | to a || and remove the comment - but leave the equality check as is. - find a src committer to approve the patch - go back to working on ports for a while ;) Is this the right course of action? Or should I just revert both commits entirely? --=20 Eitan Adler Ports committer X11, Bugbusting teams