Date: Sun, 14 Feb 2021 19:55:12 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Alexey Dokuchaev <danfe@freebsd.org>, Jessica Clarke <jrtc27@freebsd.org>, 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: <CAGudoHFr24d%2B-9QeikzbSgFKHtq8hfugQ48OAQw5WiK4DskzyA@mail.gmail.com> In-Reply-To: <3e6dfdae-48a8-2d6c-1f42-c92554d74f82@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> <CAGudoHG%2BiKB4q=QoFEadtHsXy9ODX%2BkEHn-hU71F8yGEbzP5MA@mail.gmail.com> <20210209145348.GA70871@FreeBSD.org> <3e6dfdae-48a8-2d6c-1f42-c92554d74f82@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2/9/21, John Baldwin <jhb@freebsd.org> wrote: > On 2/9/21 6:53 AM, Alexey Dokuchaev wrote: >> On Tue, Feb 09, 2021 at 02:41:15PM +0100, Mateusz Guzik wrote: >>> ... >>> More, if reviews were mandatory, I would expect their quality to go >>> down even further, making them even less likely to prevent breakage. >> >> Exactly that. In fact, the good reviews are typically coming from >> people who care. But those you'll get regardless of whether you've >> asked for them. > > No, that's not quite true. Committing without asking for any review at > all is not the same as requesting review and then timing out when it > doesn't occur. > > Also, as has been noted multiple times now, people do point out questions > that can't easily be fixed post-commit such as too-terse commit logs. > Those are quite easily caught in review if one makes the effort to ask. > > If we want to cherry-pick examples, we can also find examples where > reviews do find issues pre-commit. Look at all the back and forth on > Warner's doc change about libraries and symbol versioning in D28486 for > an example. > > The discussion in D28453 has led to a better approach I still need to > update the review with that moves the handling of pollable sims one > layer up. > > Rob Wing found an issue I had missed in my bhyve config change (D26035) > in terms of new warnings from iasl during pre-commit testing. > > Kostik posted a possible patch trying to address a PR in D28485 that is > probably not valid (see my review comments) and thus saved having > something committed that then had to be reverted. > > Kostik's review on D28342 forced me to rework the change to only scan > segments and not ELF sections since valid ELF executables and DSO's > aren't required to have section headers. > > Review on D27454 led to acclerated AES-GCM for ARMv8 that was targeted > at KTLS being reimplemented in a more generic fashion that also > accelerates IPsec and other users of AES-GCM in the kernel. > > I could keep going listing changes that benefit from cooperation among > developers. However, cooperation does mean one has to be a bit more > patient and be willing to work on follow-on work while letting review > feedback come in. Using tools like git make this fairly easy as you > can apply fixups to the earlier changes and rebase the follow-on changes > under active development afterwards. It is true that you can't get > meaningful review on all changes (or all aspects of a change), but I > think the notion that review _never_ helps is not supported by the > evidence. I think there is a gross misunderstanding here, I don't know if going both ways. First, I never claimed reviews are useless (as performed in this project or in general). To quote from my previous e-mail: > 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. For a general note: I would love a culture where one can count on a thorough and timely review of every change, but as you yourself noted one cannot expect that in this project. Most of your examples point out design choices which is what I said reviews as performed here can be fine, and apart from one don't counter anything I posted. Can a review find bugs? Of course. Does deliver on it? In my experience too rarely. This poses a question what's the harm getting a review anyway, which is answered at the end. Examples I enumerated myself, one of which has you as a reviewer, are not odd exceptions I had to hunt for and if you are not convinced I can keep going. For any of said changes, if it was my commit without any 'reviewed by', someone would be ready to respond claiming the change would have benefited from a review. Yet, a review was there and it did not help. This brings me to: > The fact that Jess found a bug in the assembly code in question the day > after it was committed indicates that pre-commit review would have > been beneficial for this commit. This can be said about just any commit. If not reviewed, a review perhaps would point out an improvement to be made (or a bug to be fixed). But in the same spirit, for any of the reviews I mentioned where a review failed to deliver on anything, a better one would find the problem. And even for reviews which found something, something could have probably been done. Most notably in https://reviews.freebsd.org/D28102 it really does not take long to realize there is a problem if you look at the entire function, which I suspect none of the 4 reviewers did. Yet the change got accepted by all of them. I consider reviews of this sort to be a bigger problem than people not getting reviews. Thus the question is what's sufficient to commit something in good faith. In particular, given your own statement not everything can be meaningfully reviewed, it comes down to a judgment call by a developer whether to get one. This is on top of figuring out what testing should have been performed. Let's take a look at this commit: For testing I assumed running the code and the glibc test suite would be sufficient -- the latter turns out to be significantly less exhaustive than necessary at least for this routine, lesson learned. Before doing anything more with the code I'm going to have to find a better suite or extend this one. What about a review? Should you read history of amd64/amd64/support.S you will find I rewrote or otherwise heavily patched memcmp, memset, memmove/memcpy, copyinstr and copyin/copyout. Almost all of these changes were reviewed. Reviewer demanded the use of macros to dedup some code, which I complied with (which you could call a benefit from a review). Yet, some of the patches had bugs and they did not get pointed out. As strlen is a self-contained routine, there are no lasting design choices to made there which would impact other code and the mycroft trick is an established method of implementing it, I did not see much use of a getting a review. Maybe it would find a bug, maybe it would not, but getting this particular routine reviewed did not seem useful. What would *definitely* helped is better testing and this is where time should have been spent here. Given all of this, what's the harm on erring on getting a review anyway? Example harm is seen in aforementioned pmap change where the patch is rubber stamped and everyone is absolved of any responsibility. To be clear, gallatin should have posted the review and I would do to given that I don't work in the area (and maybe I would make the same mistake). Here I blame reviewers for not spotting a bug which I would argue is easier to see when looking at a patch than when writing one. This still happened with reviews not even being mandatory. With more reviews, I expect precisely kind of thing to happen more often. You really have to ask yourself, of all the reviewed patches which got committed and did blow up, how many came in already in that general shape. What significance, if any, comes with 'reviewed by'? I also think your perception of how often I don't get a review compared to other people is skewed by granularity of my changes -- what other developers would collapse into one change (maybe committed without review) tends to be a small patchset for me. Finally, it would be good if you took a critical look at other people. It's hard to shake an impression you are just picking on me. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHFr24d%2B-9QeikzbSgFKHtq8hfugQ48OAQw5WiK4DskzyA>