Date: Mon, 8 Feb 2021 20:20:11 -0700 From: Warner Losh <imp@bsdimp.com> To: Kevin Bowling <kevin.bowling@kev009.com> Cc: Jessica Clarke <jrtc27@freebsd.org>, Mateusz Guzik <mjg@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>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly Message-ID: <CANCZdfoDxy2JwegL__WXnBWibxK-ZPBz6=MS8_hpovfXJSX1YA@mail.gmail.com> In-Reply-To: <CAK7dMtBFEXTy=1vW_r8Q5qpenbLcnMSkLN83Hxs93VOke28rqw@mail.gmail.com> 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> <CAK7dMtBFEXTy=1vW_r8Q5qpenbLcnMSkLN83Hxs93VOke28rqw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 documented 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 "would you have reviewed it?" line. I personally look at every single src review. 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 not a blank check to ignore project norms, nor for others to attack those that suggest it. The code is only about 50 lines of assembler, well within the range of code that often gets a good review. Suggesting that it would have benefitted from a review is not picking on mjg. Anyway, this is the last thing I'll post here... These threads get way out 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 <kevin.bowling@kev009.com> wrote: > I understand your position and Warner=E2=80=99s from the documentation. = The > problem which is not described is that frustration is asymptotically high= er > in the other direction without volunteering to do work. As another examp= le > I could reply and ask for unit tests for any change (tests are obviously > helpful too) but unless I am willing to help it is just a suggestion and > 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 a= nd > person. Otherwise it=E2=80=99s volunteering other people=E2=80=99s time = and reduces the > willpower to fix these performance areas. > > The head model supports occasional break and revert. I think this is > important 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 > happens by anyone right now juxtaposed to his track record of improvement= s > and quickness to address issues or revert where issue arose. > > Regards, > Kevin > > On Mon, Feb 8, 2021 at 4:19 PM 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. >> >> Jess >> >> > 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=3Daf366d353b84bdc4e730f0fc563853= abc338271c >> >>> >> >>> 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 her= e >> >>> 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" >> >>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoDxy2JwegL__WXnBWibxK-ZPBz6=MS8_hpovfXJSX1YA>