From owner-dev-commits-src-all@freebsd.org Tue Feb 9 03:53:00 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 70AF852E2A3 for ; Tue, 9 Feb 2021 03:53:00 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com [IPv6:2607:f8b0:4864:20::b35]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DZTWJ2R2yz3tK9 for ; Tue, 9 Feb 2021 03:53:00 +0000 (UTC) (envelope-from kevin.bowling@kev009.com) Received: by mail-yb1-xb35.google.com with SMTP id i71so16829776ybg.7 for ; Mon, 08 Feb 2021 19:53:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kev009.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=vt5rdb4uy+sm0WI499fQjszQn4S8XYafHExrbXd8KRA=; b=YCQupFnzLTCb3aupWbwCGkRY4HOVTK3rwnHidO3aO/knHw6UdkD3ynaj70kASxSz29 89BBvZAgEOAuppuZ9mR3ieP25ZF+552BQdI9S5eg9256dnwzUEprGVZUytZvVrkFSv1D KmC3Q5dJb+S/aSQDpNTdZTNC6zxBkNFDEI9TQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=vt5rdb4uy+sm0WI499fQjszQn4S8XYafHExrbXd8KRA=; b=JuN6x31BxNw87HVfUDihcCsYgT5v+tUcwiEN+heGNhqNO4J7qmuQiNDp2MRqOaFdPo M9xB6CTcStTvaAz3kvPu5Q/zztVgCr23X5LApkK4pEEbb2mUyoI4TpwQWZEkHOtPjNa0 7FNIwhgy4sw6LRrEKObj/Wu3NWOooL/Ia41Cp+avOIf9w0j4+LzKp9ruHHABv+z8R/h5 rpiHjfMrxbjljdvxF/GwpxRFnbI9z4B7TXc+Ix7qyBcXxiZMTjszSgBH797TcmjFGF00 MvQ8CXji3dTYkivaP3XZusS4BkFM8Gdpzetvve+gpURNaBcxg52EAQmxaHUqeyFmcTN6 KTFQ== X-Gm-Message-State: AOAM533cFlJdqgUsx+ccv7ieAHIzfFskqtgkE201qdKDQU9xMMRyq3uj OxVLbZCV3WwEu/g0vJlj/XSWzZCkbEQm37T+joHkXA== X-Google-Smtp-Source: ABdhPJzFDroBp60ylIYN1f715eRa/qx36askEtG4rCKTbfnEoz/BAxX+LQ+Elou2vFZiRueFgULutwl6rmit6DIdibA= X-Received: by 2002:a25:4981:: with SMTP id w123mr29850614yba.123.1612842779170; Mon, 08 Feb 2021 19:52:59 -0800 (PST) MIME-Version: 1.0 References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> In-Reply-To: From: Kevin Bowling Date: Mon, 8 Feb 2021 20:52:47 -0700 Message-ID: Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly To: Warner Losh Cc: Jessica Clarke , Mateusz Guzik , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" , "src-committers@freebsd.org" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4DZTWJ2R2yz3tK9 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Feb 2021 03:53:00 -0000 Warner, My intent was not to attack Jessica just as I do not believe her intent was to pick on Mateuz but from different perspectives frustrations can build up to the point that the message is not well received. What I intended was the opposite: to encourage tact in this kind of exchan= ge. If we can start over in the future, the best way to do this is something like: "Mateuz, this is interesting work. I would like to have a chance to review your amd64 assembly changes before you commit them... [because $reasons might be nice to help someone internally process and change their behavior, but without justification sure it's just implied they care about the quality of the code and that is enough]".. I stand behind my position that commanding someone to do something without offer or without having an existing relationship such that they will receive the request in the way it was intended is a good way to burn people out. There are two contributors here. Siding with one is a negative outcome. I am trying to propose an actionable criteria that Mateuz can act on and have good faith in because I know him well enough to know he will do it. Regards, Kevin On Mon, Feb 8, 2021 at 8:20 PM Warner Losh wrote: > > Kevin, > > I'm sure that you think you are being reasonable. But you sure are coming= off as attacking Jessica and I for a polite request to adhere to documente= d project norms. It's not unreasonable to make a request. You are proposing= a crazy and unreasonable standard by attacking Jessica and I with the "wou= ld you have reviewed it?" line. I personally look at every single src revie= w. I don't comment on them all. I don't know if I can until I take a look, = but I at least read the summaries. > > It's cool that mjg has gone ahead and rewritten strlen. However, that's n= ot a blank check to ignore project norms, nor for others to attack those th= at suggest it. The code is only about 50 lines of assembler, well within th= e range of code that often gets a good review. Suggesting that it would hav= e benefitted from a review is not picking on mjg. > > Anyway, this is the last thing I'll post here... These threads get way ou= t of hand, so if you want to post a rebuttal, I'll give you the last word. > > Warner > > On Mon, Feb 8, 2021 at 5:18 PM Kevin Bowling w= rote: >> >> I understand your position and Warner=E2=80=99s from the documentation. = The problem which is not described is that frustration is asymptotically h= igher in the other direction without volunteering to do work. As another e= xample I could reply and ask for unit tests for any change (tests are obvio= usly helpful too) but unless I am willing to help it is just a suggestion a= nd should not be sent as a command. If you are willing to do such reviews = timely, or have command of someone who will, I will coordinate with mjg and= person. Otherwise it=E2=80=99s volunteering other people=E2=80=99s time a= nd reduces the willpower to fix these performance areas. >> >> The head model supports occasional break and revert. I think this is im= portant given the resources FreeBSD has available, since a lot of the less = glamorous work is unpaid and underpaid. >> >> Picking on mjg for this is suspicious given how frequently breakage happ= ens by anyone right now juxtaposed to his track record of improvements and = quickness to address issues or revert where issue arose. >> >> Regards, >> Kevin >> >> On Mon, Feb 8, 2021 at 4:19 PM Jessica Clarke wrote= : >>> >>> > On 8 Feb 2021, at 23:13, Kevin Bowling wro= te: >>> > >>> > FreeBSD does not require pre-commit approval unless called out >>> > specifically. Are you volunteering to review the changes, and if so >>> > where is your guidance? These messages are otherwise unhelpful. >>> >>> It is not a hard requirement, but it is strongly encouraged. Section 7 >>> of the committer's guide says: >>> >>> =E2=80=A2 All non-trivial changes should be reviewed before the= y are >>> committed to the repository. >>> >>> This was a non-trivial change. I was particularly frustrated to see >>> this commit go in without review having previously called out mjg@ for >>> not getting any reviews for his (now reverted) previous strlen change. >>> >>> Jess >>> >>> > On Mon, Feb 8, 2021 at 12:37 PM Jessica Clarke w= rote: >>> >> >>> >> On 8 Feb 2021, at 19:15, Mateusz Guzik wrote: >>> >>> >>> >>> The branch main has been updated by mjg: >>> >>> >>> >>> URL: https://cgit.FreeBSD.org/src/commit/?id=3Daf366d353b84bdc4e730= f0fc563853abc338271c >>> >>> >>> >>> commit af366d353b84bdc4e730f0fc563853abc338271c >>> >>> Author: Mateusz Guzik >>> >>> AuthorDate: 2021-02-08 17:01:48 +0000 >>> >>> Commit: Mateusz Guzik >>> >>> CommitDate: 2021-02-08 19:15:21 +0000 >>> >>> >>> >>> amd64: implement strlen in assembly >>> >>> >>> >>> The C variant in libkern performs excessive branching to find the >>> >>> non-zero byte instead of using the bsfq instruction. The same cod= e >>> >>> patched to use it is still slower than the routine implemented he= re >>> >>> as the compiler keeps neglecting to perform certain optimizations >>> >>> (like using leaq). >>> >>> >>> >>> On top of that the routine can is a starting point for copyinstr >>> >>> which operates on words instead of bytes. >>> >>> >>> >>> Tested with glibc test suite. >>> >>> >>> >>> Sample results (calls/s): >>> >>> >>> >>> Haswell: >>> >>> $(perl -e "print 'A' x 3"): >>> >>> stock: 211198039 >>> >>> patched:338626619 >>> >>> asm: 465609618 >>> >>> >>> >>> $(perl -e "print 'A' x 100"): >>> >>> stock: 83151997 >>> >>> patched: 98285919 >>> >>> asm: 120719888 >>> >>> >>> >>> AMD EPYC 7R32: >>> >>> $(perl -e "print 'A' x 3"): >>> >>> stock: 282523617 >>> >>> asm: 491498172 >>> >>> >>> >>> $(perl -e "print 'A' x 100"): >>> >>> stock: 114857172 >>> >>> asm: 112082057 >>> >> >>> >> No Reviewed by? More than one pair of eyes on non-trivial assembly i= s >>> >> almost always a good idea. >>> >> >>> >> Jess >>> >> >>> >> _______________________________________________ >>> >> dev-commits-src-main@freebsd.org mailing list >>> >> https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main >>> >> To unsubscribe, send any mail to "dev-commits-src-main-unsubscribe@f= reebsd.org" >>>