PING^2: resubmitted IRA improvement patches

[Vladimir Makarov]

  On 10/05/2010 10:57 PM, David Edelsohn wrote:
> Many FOSS projects and companies operate very successfully and
> efficiently on a "code review required" basis.  Nothing is perfect,
> but I generally have heard good things from projects that implement
> that model.  Having a second pair of eyes double-check a patch can
> help a lot.
>
Many FOSS projects are successful *with* "code reviewer required basis" 
does not mean they are successful *because of* it.  I feel that GCC is 
doing better than five years ago.  And IMHO that is mostly because GCC 
got a critical mass of experienced professionally working developers 
(although I guess improving reviewing process helped a bit but IMO again 
not because of the policy but more because the developers pay more 
attention to it).
> Small patches by developers in their own area of expertise probably
> will not gain a lot from a review, but I have witnessed many cases
> when someone pointed out an API that the developer should have used or
> a problematic corner case the developer missed.
>
I don't think that only reviewer approach is right thing.  What if two 
reviewers do not agree on a third party patch.  I saw recently a nasty 
conflict on this base on GCC mailing list (I was myself in the same 
situation recently in much less severe form).  What if a patch creator 
(and also a reviewer in the patch code area too) does not agree with his 
patch review and he believes he knows importance of the patch better.

Instead of just a bazaar (although there are not negative sides of such 
development) you might be creating a bazaar squared.

I think a natural way to resolve this issue is to have some soft 
authority based in other words some credible person with status higher 
than just reviewer.  You could call it a maintainer.

> Larger patches, especially ones that change or extend the algorithmic
> design of GCC, deserve more care and attention.  Working on GCC
> requires multiple types of knowledge: GCC's internals, compiler
> optimization algorithms, algorithm implementation details and tuning.
> No one knows everything and a marketplace of ideas should produce a
> better solution.
>

> The GCC SC current policy for appointing "maintainers" and "reviewers"
> broadly can be described as: "maintainers" are for narrow areas, like
> GC, OpenMP, language front ends and ports, and "reviewers" for broader
> areas, like middle-end, RTL and dataflow.  In other words appointing
> maintainers in areas that may not have enough people with expertise to
> review each other's patches and avoided by higher-level reviewers who
> do not feel qualified to review the patches.  If there are enough
> qualified reviewers for a narrower area, "reviewer" is preferred.
>
I feel there is inconsistency in you policy implementation.   There are 
still important big parts of compiler which have maintainers even there 
are enough people with a good experience in these parts to provide at 
least two reviewers.  I could name a few of them

important ports like x86/x86_64, powerpc and arm
important front-ends
autovectorizer/loop optimizer (that project which got a recent 
appointment.  I mean recent as since IRA reviewer appointments).
> A lot of developers were "grandfathered" in to maintainer roles that
> should be reviewers.  And a lot of developers who are or were less
> active for a period of time never resigned their roles.  These
> situations have not caused noticeable problems, other than envy, so
> the SC has chosen not to address it.  However, note that all Global
> Write Privilege maintainers were converted to Global Reviewers.  The
> SC welcomes any other groups of maintainers to voluntarily convert to
> reviewers.
>
I wanted to express my feeling on this long ago.

I think in general removing global maintainers was a good thing.  But a 
bad thing also happened.  You removed Richard Henderson as a global 
maintainer, a quite actively working person who has unique wide range 
knowledge of GCC, who has never abused this privilege and never had own 
political agenda.

As for "SC welcomes any other groups of maintainers to voluntarily 
convert to reviewers."  David, I feel that you gave a lot of thoughts in 
the new policy, believe in it,  and probably was a major engine in this 
changes.  I think you should show a leadership and give up you power 
maintainer status.  I think that is concerning other SC members.

If you don't show this leadership and that for me is another argument to 
support introduction some policy about SC members rotation if it is 
legally possible.
> If you think some developers or potential reviewers are not aware of
> an upcoming patch, why not pro-actively contact them instead of
> waiting for them to come to you?  A little bit of extra effort by a
> patch developer can go a long way.
>
> More effective GCC developers generally figure out what other
> developers are knowledgeable in an area, how those developers prefer
> to communicate (private email, public mailing list, IRC, bugzilla,
> phone, skype, lunch, conferences) and contact them using the other
> developers' preferred communication medium.  They chat, brainstorm,
> solicit ideas and experience, and generally ensure that everyone is on
> the same page.  They alert the appropriate people ahead of time --
> beyond the minimal, official effort.  Communicate early and often.
>
I have a feeling sometimes CCing to official reviewers, discourage other 
reviewers to look at the patch.

I could also write in your way (and I already wrote) that most effective 
reviewers at least read mailing list and look at the patches of their 
reviewer area.

I am aware about lack of knowledgeable IRA reviewers despite the facts 
that there are four of them.  I'll work on bringing more knowledgeable 
IRA reviewers and I know good candidates.
> I think the GCC community should look for ways to attract more
> reviewers and how to make reviewers more efficient instead of looking
> for ways to allow developers to avoid patch reviews.
I don't avoid patch reviews I always welcome them (including in my area 
maintainership).
>    How can we
> update and improve GCC's patch review work-flow?
I think addressing issues I wrote here would be good.