From owner-dev-commits-src-all@freebsd.org Tue Feb 9 13:41:18 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 D633553F119; Tue, 9 Feb 2021 13:41:18 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com [IPv6:2a00:1450:4864:20::42f]) (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 4DZkZ656HFz51t7; Tue, 9 Feb 2021 13:41:18 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x42f.google.com with SMTP id q7so21750612wre.13; Tue, 09 Feb 2021 05:41:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=tHgukRA1lyjBxBml3TUdnTAZFM0XeHjQ+Mnj2uRa1bM=; b=TkMuKsSwCmj/GlC6AJ4WS3w/zQBZdzMAkJM1KuMjZQvoxIz9jWky4UzIVPNQGUlsK0 8ifX727GB2TbRuwxx2iEBPBUZMRzmkcXPHbprYelnSsJ1IFU9mHBCYE0yVZqSkeXU8O2 t0gaMyunwyAOBHZAK3zCRWnLvCh1kOG/DvbV0FzJUaugw7OpNtizH1sCRunovkX1e6Jt 8erdAUlWUChksGAp2o0we0xriBIUiR0rn3w9YLmoa8xZ2DUkl/vMbV9YlTfWh0gz2nSd xNZ+2Y3g/2uPH4Jz94GuMHcFWW0Y41SPsn01WzUT/0QAQGIuO759q7PQLIRfcB/n9Mqy B+Vg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=tHgukRA1lyjBxBml3TUdnTAZFM0XeHjQ+Mnj2uRa1bM=; b=odhWDQzjYLnLbMPbFRvFnogHTYSbc4U9sm0YjryhhU4zGauH4QNLL9dE5M5485huuS 3DMoCkJGBNv48fSTIrTxboZG3JuRzP2y2zUQAJ8o9HYHvWECXq2O0lPxS8HVsc+AITxc hBOj5z9aJOxPDK0MqNrYR/8dq9FFO8ri4fkvQn9iIFdmyHPVoXPLr8qRRq5s6ftpp3wu PIU8GVplhEYnU3iGnPUYIgdLkkn+8pXvwbPAarKYUGCPIstgu/JcqCwx1aDjF9uxeLpn 2z80PmONjhmuPHp0o36IADnTs9I6mB2EsLUHn47xknrzGswxWRmfJXTJqIR7pHn6c2US gtRA== X-Gm-Message-State: AOAM531FCuzwTHCBnCxcq1et8yVOaIl/e8hOotwsXvvBwygd+VX/5QTq GjYTIzw2BEqFWMn7vT6hdCMVRLXcddepxL8lqSpOxOS3tG4= X-Google-Smtp-Source: ABdhPJxndz56Jo+G75YIIC/ePAu5KJPqiTHHCBkZ8zh14BKa731K765lEJbGxq0j7zrneyjMRBRW/QS2ElTn6vqUWN4= X-Received: by 2002:a05:6000:1565:: with SMTP id 5mr26203661wrz.109.1612878077323; Tue, 09 Feb 2021 05:41:17 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a5d:464c:0:0:0:0:0 with HTTP; Tue, 9 Feb 2021 05:41:15 -0800 (PST) In-Reply-To: <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> From: Mateusz Guzik Date: Tue, 9 Feb 2021 14:41:15 +0100 Message-ID: Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly To: Jessica Clarke Cc: Kevin Bowling , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4DZkZ656HFz51t7 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 13:41:18 -0000 On 2/9/21, Jessica Clarke wrote: >> On 8 Feb 2021, at 23:13, Kevin Bowling wrote: >> >> 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 they 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. > There is a lot to say here and I don't know if I'll manage to make my points come across. First I'll address reviews in general and then I'll comment on strlen. The key is that reviews in this project notoriously fail to reduce/prevent breakage. I know this because several patches written by me which got accepted with little to no objection turned out to have problems which would have been found if review was better. I also know because I see the very same problem with reviews involving other people. Note I'm not talking about expecting reviews to guarantee bug-free commits. I'm talking about bugs one would expect a review to find, but did not. It got to the point where I put no stock in 'reviewed by'. Examples below. Ultimately what reduces/prevents breakage is testing and that's what I'm normally doing. Of course not all problems can be found in testing, so this boils down to making a call whether a review is warranted and what it can do to begin with. I do think a review is mandatory when making a non-cosmetic change to an area worked on by someone else. Similarly, if there are multiple people active somewhere, it's probably best to coordinate. Review may be a great idea if a design choice has to be made or certain people have expertise from the problem domain, even if they are not active somewhere. I don't believe the project has manpower to perform good quality code reviews of every single change. The developer has to make a judgment call whether the change warrants a review and sometimes they will make a mistake here, just like even a good review will sometimes fail to spot problems it otherwise would. I do believe shallow reviews allow people to dodge criticism for breakage -- after all, someone else accepted the change. Examples from recent past (all fixed): https://reviews.freebsd.org/D28091 Add -NODEBUG variant of GENERIC-MMCCAM kernel configuration. Some discussion, accepted by 2 people, has a typo in the kernel config breaking tinderbox https://reviews.freebsd.org/D28102 amd64: compare TLB shootdown target to all_cpus Nothing pointed out, accepted by 4 people, crashes on boot when the kernel is built with INVARIANTS. https://reviews.freebsd.org/D28284 elfctl: allow features to be specified by value Nothing pointed out, accepted by 1 person, has '!=3D' instead of '|=3D' breaking the change. https://reviews.freebsd.org/D28306 nfs client: block vnode_pager_setsize() calls from nfscl_loadattrcache in nfs_write There was a question about something but the patch got accepted, breaks writes by partially pushing garbage into files. https://reviews.freebsd.org/D27609 ndis(4): remove as previous announced Nothing pointed out, accepted by 1 person. Breaks the build in 2 ways and leaves leftovers (grep git log for ndis and you will see what I mean). Change like this is arguably to review, but bare minimum which could have been done is to ask if they built tinderbox and perhaps git grep -i ndis on your own. This in my experience is the standard. Which brings me to C strlen. Before committing my patch to the C variant I did the due diligence for little endian -- I have glibc test suite up and running, I have a small test program on my own and finally I ran several workloads on it. For big endian I only ran the small test program on ref13-ppc64 and called it a day. That was a failure to do the due diligence -- I should spent time to set up the test suite in that environment, but could not be arsed to do it. I should have also asked someone running the hardware to the code by just using it. Bad call on my end, a bug was reported and I promptly reverted the change. Would a review prevent the problem? That is plausible, but given track record indicated above, I would not count on it. More, if reviews were mandatory, I would expect their quality to go down even further, making them even less likely to prevent breakage. And now we reach this change. I did the due diligence in testing (glibc, my test jig, actual workloads). Because of this, while I can never claim the routine is bug free, I doubt bugs (if any) would be found in a review and this is why I did not ask for one. In contrast, here are examples from my TODO list where I do plan to get a review if I write a patch: - nofault/onfault so that C routines can handle interaction with userspace memory -- several decisions to make here - hot patching of sdt probes -- same as above - copyinstr using the mycroft trick -- possibly questionable hacks pop up That said, I do own to my mistake with C strlen (shallow testing for big endian) and I do stand behind not getting a review for this patch. I believe you have good intentions, but are simply suffering from confirmation bias. Perhaps the breakage introduced by me is something which popped up for you. If you want an example of something which is frustrating, I suggest you take a look at how often the build is broken and who keeps reporting or fixing it. >> On Mon, Feb 8, 2021 at 12:37 PM Jessica Clarke >> wrote: >>> >>> 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=3Daf366d353b84bdc4e730f0fc5638= 53abc338271c >>>> >>>> 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 code >>>> patched to use it is still slower than the routine implemented here >>>> 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 is >>> 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@freebsd.org" > > --=20 Mateusz Guzik