Date: Tue, 9 Feb 2021 14:41:15 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: Jessica Clarke <jrtc27@freebsd.org> Cc: Kevin Bowling <kevin.bowling@kev009.com>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly Message-ID: <CAGudoHG%2BiKB4q=QoFEadtHsXy9ODX%2BkEHn-hU71F8yGEbzP5MA@mail.gmail.com> In-Reply-To: <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <CAK7dMtA5rbT6QoCK2HQfzLY2f%2B3kbhdJp0ERmRpc_CVAog%2B6bg@mail.gmail.com> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2/9/21, Jessica Clarke <jrtc27@freebsd.org> wrote: >> On 8 Feb 2021, at 23:13, Kevin Bowling <kevin.bowling@kev009.com> 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 <jrtc27@freebsd.org> >> wrote: >>> >>> On 8 Feb 2021, at 19:15, Mateusz Guzik <mjg@FreeBSD.org> wrote: >>>> >>>> The branch main has been updated by mjg: >>>> >>>> URL: >>>> https://cgit.FreeBSD.org/src/commit/?id=3Daf366d353b84bdc4e730f0fc5638= 53abc338271c >>>> >>>> commit af366d353b84bdc4e730f0fc563853abc338271c >>>> Author: Mateusz Guzik <mjg@FreeBSD.org> >>>> AuthorDate: 2021-02-08 17:01:48 +0000 >>>> Commit: Mateusz Guzik <mjg@FreeBSD.org> >>>> 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHG%2BiKB4q=QoFEadtHsXy9ODX%2BkEHn-hU71F8yGEbzP5MA>