git: af366d353b84 - main - amd64: implement strlen in assembly

[Mateusz Guzik]

On 2/9/21, John Baldwin <jhb at 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>