increasing the number of GCC reviewers

increasing the number of GCC reviewers

[Basile STARYNKEVITCH]

Hello All,

I am unfortunately not attending the GCC summit which happens right now 
in Montreal.

But apparently, there seems to be a lack of code reviewers for GCC. The 
few people who do review code seems to have a lot of review in their 
batch queue.

Perhaps could be discussed at the summit some way to increase the set of 
reviewers, i.e. the set of people able to say Ok to a patch submitted on 
gcc-patches@

Or is that question a steering committee priviledge? If it is, could 
someone ask it to them (the SC members)?

I am wrong in believing that code review is a real bottleneck today?

Regards.

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: increasing the number of GCC reviewers

[Andrew Haley]

Basile STARYNKEVITCH wrote:

> I am unfortunately not attending the GCC summit which happens right now
> in Montreal.
> 
> But apparently, there seems to be a lack of code reviewers for GCC. The
> few people who do review code seems to have a lot of review in their
> batch queue.
> 
> Perhaps could be discussed at the summit some way to increase the set of
> reviewers, i.e. the set of people able to say Ok to a patch submitted on
> gcc-patches@

As I understand it, the set of reviewers allowed to say OK to a patch is
limited to the set of people capable of reviewing these patches.  That is,
there is a limited set of people with enough real knowledge of gcc to
approve a patch.  Adding people who do not really understand gcc will
not help.

Andrew.

Re: increasing the number of GCC reviewers

[Basile STARYNKEVITCH]

Andrew Haley wrote:
> Basile STARYNKEVITCH wrote:
>>
>> Perhaps could be discussed at the summit some way to increase the set of
>> reviewers, i.e. the set of people able to say Ok to a patch submitted on
>> gcc-patches@
>>     
>
> As I understand it, the set of reviewers allowed to say OK to a patch is
> limited to the set of people capable of reviewing these patches.  That is,
> there is a limited set of people with enough real knowledge of gcc to
> approve a patch.  

I know a lot of people who fully understand many of the patches I did 
submit, and who are not able to say Ok (because' their plain maintainer 
status disallows that).
And there are some patches which I did comment about, which I believe I 
did understand, and of course which I am not allowed to approve. I am 
certainly not alone in that case!

My feeling is on the contrary that the set of people having a real 
knowledge of gcc (or at least of substantial parts of it [*]) is much 
bigger than the set of reviewers allowed to say OK.

Regards.

PS. [note *] GCC is a huge software, so understanding well a part of it 
could be enough to understand some patches. And GCC is a huge software, 
so the set of people understanding all of it is shrinking; perhaps even 
it could be empty! (I never met any person claiming to understand all of 
GCC; for sure I will never understand all of it.).

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: increasing the number of GCC reviewers

[Basile STARYNKEVITCH]

Basile STARYNKEVITCH wrote:
> Andrew Haley wrote:
>> Basile STARYNKEVITCH wrote: 
>
> PS. [note *] GCC is a huge software, so understanding well a part of 
> it could be enough to understand some patches. 

> And GCC is a huge software
I meant GCC is growing a lot. Its increase rate (about 1MLOC in less 
than 3 years) is impressive.
> so the set of people understanding all of it is shrinking; perhaps 
> even it could be empty! (I never met any person claiming to understand 
> all of GCC; for sure I will never understand all of it.).
>


-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: increasing the number of GCC reviewers

[Andrew Haley]

Basile STARYNKEVITCH wrote:
> Andrew Haley wrote:
>> Basile STARYNKEVITCH wrote:
>>>
>>> Perhaps could be discussed at the summit some way to increase the set of
>>> reviewers, i.e. the set of people able to say Ok to a patch submitted on
>>> gcc-patches@
>>
>> As I understand it, the set of reviewers allowed to say OK to a
>> patch is limited to the set of people capable of reviewing these
>> patches.  That is, there is a limited set of people with enough
>> real knowledge of gcc to approve a patch.

> I know a lot of people who fully understand many of the patches I
> did submit, and who are not able to say Ok (because' their plain
> maintainer status disallows that).

> And there are some patches which I did comment about, which I
> believe I did understand, and of course which I am not allowed to
> approve. I am certainly not alone in that case!

Well, maybe.  But it's not what you know, it's what you know that ain't
so.  And I am pretty sure that I'm more cautious today about some of the
patches I believe I understand than I was when I started many years go.

> My feeling is on the contrary that the set of people having a real
> knowledge of gcc (or at least of substantial parts of it [*]) is much
> bigger than the set of reviewers allowed to say OK.

That's certainly true, but there's a big difference between having real
knowledge of gcc and having enough real knowledge to approve a patch.

It is quite possibly the case that some maintainers should be "promoted".
But it isn't sufficient to have a blanket policy of "let's have more
reviewers".  We need something more like "I think Fred Bloggs knows gcc
well enough to approve patches to reload" or "I am Fred Bloggs and I
know gcc well enough to approve patches to reload."

That would be much more productive.

Andrew.

Re: increasing the number of GCC reviewers

[Adam Nemet]

Andrew Haley <aph@redhat.com> writes:
> We need something more like "I think Fred Bloggs knows gcc well enough
> to approve patches to reload" or "I am Fred Bloggs and I know gcc well
> enough to approve patches to reload."

And whom should such email be sent to?  The SC is best reached on gcc@
but I don't think that recommending someone publicly is necessarly a
good idea.  E.g. what if the SC does not appoint the person; does that
mean that the SC decided that he or she was not qualified enough?

IMO the best way would be to nominate someone to the SC directly and
then if the SC decides to support the nomination they can check with the
person if he or she would accept the appointment.

Adam

Re: increasing the number of GCC reviewers

[Andrew Haley]

Adam Nemet wrote:
> Andrew Haley <aph@redhat.com> writes:
>> We need something more like "I think Fred Bloggs knows gcc well enough
>> to approve patches to reload" or "I am Fred Bloggs and I know gcc well
>> enough to approve patches to reload."
> 
> And whom should such email be sent to?  The SC is best reached on gcc@
> but I don't think that recommending someone publicly is necessarly a
> good idea.  E.g. what if the SC does not appoint the person; does that
> mean that the SC decided that he or she was not qualified enough?
> 
> IMO the best way would be to nominate someone to the SC directly and
> then if the SC decides to support the nomination they can check with the
> person if he or she would accept the appointment.

I think it's a much better idea to contact Fred (or Freda, for that matter)
Bloggs to ask them if they want to maintain reload.  :-)

If you really need to contact any SC members in private, you can do
that.  It's not as though their identities are secret.

Andrew.

Re: increasing the number of GCC reviewers

[Ben Elliston]

On Tue, 2009-06-09 at 19:00 +0100, Andrew Haley wrote:

> I think it's a much better idea to contact Fred (or Freda, for that matter)
> Bloggs to ask them if they want to maintain reload.  :-)

Wouldn't it be Alan Smithee to maintain reload? :-)

Ben

Re: increasing the number of GCC reviewers

[Dave Korn]

Ben Elliston wrote:
> On Tue, 2009-06-09 at 19:00 +0100, Andrew Haley wrote:
> 
>> I think it's a much better idea to contact Fred (or Freda, for that matter)
>> Bloggs to ask them if they want to maintain reload.  :-)
> 
> Wouldn't it be Alan Smithee to maintain reload? :-)
> 
> Ben

  Burn, Reload, Burn?

    cheers,
      DaveK

Re: increasing the number of GCC reviewers

[Joe Buck]

On Tue, Jun 09, 2009 at 10:54:06AM -0700, Adam Nemet wrote:
> Andrew Haley <aph@redhat.com> writes:
> > We need something more like "I think Fred Bloggs knows gcc well enough
> > to approve patches to reload" or "I am Fred Bloggs and I know gcc well
> > enough to approve patches to reload."
> 
> And whom should such email be sent to?  The SC is best reached on gcc@
> but I don't think that recommending someone publicly is necessarly a
> good idea.  E.g. what if the SC does not appoint the person; does that
> mean that the SC decided that he or she was not qualified enough?

> IMO the best way would be to nominate someone to the SC directly and
> then if the SC decides to support the nomination they can check with the
> person if he or she would accept the appointment.

You could contact any SC member by private mail if you think the topic
is too sensitive to discuss in public.  It would work best to pick an
SC member who is familiar with the nominee's work (and we'd have to
know that Fred Bloggs wants the job).

Re: increasing the number of GCC reviewers

[Paolo Bonzini]

>> My feeling is on the contrary that the set of people having a real
>> knowledge of gcc (or at least of substantial parts of it [*]) is much
>> bigger than the set of reviewers allowed to say OK.
> 
> That's certainly true, but there's a big difference between having real
> knowledge of gcc and having enough real knowledge to approve a patch.

It's also a matter of time.  I wouldn't trust myself with the ability to 
review patches in broader areas, because I wouldn't have enough time to 
do it properly.

Paolo

Re: increasing the number of GCC reviewers

[Basile STARYNKEVITCH]

FWIW, I am not taking this question personally (I don't really claim 
that I could become any kind of reviewer; I believe in general that 
reviewing abilities should be evaluated by others.). I just think the 
set of reviewers should significantly grow.


Andrew Haley wrote:
>
>   
>> My feeling is on the contrary that the set of people having a real
>> knowledge of gcc (or at least of substantial parts of it [*]) is much
>> bigger than the set of reviewers allowed to say OK.
>>     
>
>   
I am not at the summit. So I don't know if my perception "there are not 
enough reviewers" [0] is shared by others or not. I suppose it is agreed 
(that the set of reviewers should be increased [1])  If not, ignore the 
rest. I really don't know if other people believe (as I do) that the set 
of reviewers should significantly increase.

My perception is that many reviewers have too much reviews [2] in their 
queue, and that these tasks might overwhelm or bore them. But since I am 
not a reviewer, I cannot reliably understand what it is to be one. For 
instance, my feeling is that Diego Novillo -whom I know, and I admire a 
lot- (and some other GCC gurus) is almost exhausted by his pending 
review queue.

> That's certainly true, but there's a big difference between having real
> knowledge of gcc and having enough real knowledge to approve a patch.
>   
What might perhaps be discussed at the summit is possibly this (perhaps 
too strong) requirement on the reviewer's level. If there are too few 
reviewers, and if making a big lot of reviews is boring (or just too 
demanding or too tiring) to them, then we might consider lowering the 
threshold to become a reviewer (e.g. dispatch review abilities to more 
people, or perhaps define some fine grain policy on future reviewers; I 
could imagine that some people could review just a few GCC source files).
> It is quite possibly the case that some maintainers should be "promoted".
> But it isn't sufficient to have a blanket policy of "let's have more
> reviewers".  
But we first should agree on the wish than an increase of the set of 
reviewers is desirable.

> We need something more like "I think Fred Bloggs knows gcc
> well enough to approve patches to reload" or "I am Fred Bloggs and I
> know gcc well enough to approve patches to reload."
>   

I am not sure to parse correctly this sentence. Sorry, English is a 
foreign language to me. Is "reload" some functionality (PCH?) you refer 
inside GCC, or is it the task of making reviews on patches submitted on 
gcc-patches@ ? I was just thinking about stuff like "Fred Bloggs knows 
enough to approve patches submitted on gcc-patches@ to files gcc/ggc*.[ch]"


And it could happen that the plugin infrastructure might in the future 
move some code out of GCC core (and into plugins). In that future 
situation, the set of reviewers might not need to increase.

Regards.

Note 0: for me a reviewer is any person admitted to say ok to some (even 
very few) patches.

Note 1: My intuition is that the number of reviewers should be 
proportional (at least; and one could believe to O(n log n) where n is 
the size of GCC) to the GCC source size. I am not sure (& I don't know 
if) it has increased by 30% in 3 years, as did the source code!

Note 2: I have no idea if the patch-to-be-reviewed queue of each 
reviewer has increased since 2 years ago! I intuitively feel it did 
increase a lot, i.e. reviewers have much more pressure on them. Maybe I 
am wrong!

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: increasing the number of GCC reviewers

[Richard Guenther]

On Tue, Jun 9, 2009 at 2:10 PM, Basile
STARYNKEVITCH<basile@starynkevitch.net> wrote:
> FWIW, I am not taking this question personally (I don't really claim that I
> could become any kind of reviewer; I believe in general that reviewing
> abilities should be evaluated by others.). I just think the set of reviewers
> should significantly grow.
>
>
> Andrew Haley wrote:
>>
>>
>>>
>>> My feeling is on the contrary that the set of people having a real
>>> knowledge of gcc (or at least of substantial parts of it [*]) is much
>>> bigger than the set of reviewers allowed to say OK.
>>>
>>
>>
>
> I am not at the summit. So I don't know if my perception "there are not
> enough reviewers" [0] is shared by others or not. I suppose it is agreed
> (that the set of reviewers should be increased [1])  If not, ignore the
> rest. I really don't know if other people believe (as I do) that the set of
> reviewers should significantly increase.
>
> My perception is that many reviewers have too much reviews [2] in their
> queue, and that these tasks might overwhelm or bore them. But since I am not
> a reviewer, I cannot reliably understand what it is to be one. For instance,
> my feeling is that Diego Novillo -whom I know, and I admire a lot- (and some
> other GCC gurus) is almost exhausted by his pending review queue.
>
>> That's certainly true, but there's a big difference between having real
>> knowledge of gcc and having enough real knowledge to approve a patch.
>>
>
> What might perhaps be discussed at the summit is possibly this (perhaps too
> strong) requirement on the reviewer's level. If there are too few reviewers,
> and if making a big lot of reviews is boring (or just too demanding or too
> tiring) to them, then we might consider lowering the threshold to become a
> reviewer (e.g. dispatch review abilities to more people, or perhaps define
> some fine grain policy on future reviewers; I could imagine that some people
> could review just a few GCC source files).
>>
>> It is quite possibly the case that some maintainers should be "promoted".
>> But it isn't sufficient to have a blanket policy of "let's have more
>> reviewers".
>
> But we first should agree on the wish than an increase of the set of
> reviewers is desirable.
>
>> We need something more like "I think Fred Bloggs knows gcc
>> well enough to approve patches to reload" or "I am Fred Bloggs and I
>> know gcc well enough to approve patches to reload."
>>
>
> I am not sure to parse correctly this sentence. Sorry, English is a foreign
> language to me. Is "reload" some functionality (PCH?) you refer inside GCC,
> or is it the task of making reviews on patches submitted on gcc-patches@ ? I
> was just thinking about stuff like "Fred Bloggs knows enough to approve
> patches submitted on gcc-patches@ to files gcc/ggc*.[ch]"
>
>
> And it could happen that the plugin infrastructure might in the future move
> some code out of GCC core (and into plugins). In that future situation, the
> set of reviewers might not need to increase.
>
> Regards.
>
> Note 0: for me a reviewer is any person admitted to say ok to some (even
> very few) patches.
>
> Note 1: My intuition is that the number of reviewers should be proportional
> (at least; and one could believe to O(n log n) where n is the size of GCC)
> to the GCC source size. I am not sure (& I don't know if) it has increased
> by 30% in 3 years, as did the source code!
>
> Note 2: I have no idea if the patch-to-be-reviewed queue of each reviewer
> has increased since 2 years ago! I intuitively feel it did increase a lot,
> i.e. reviewers have much more pressure on them. Maybe I am wrong!

You are wrong.

;)

Richard.

> --
> Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
> email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
> 8, rue de la Faiencerie, 92340 Bourg La Reine, France
> *** opinions {are only mines, sont seulement les miennes} ***
>
>

Re: increasing the number of GCC reviewers

[Andrew Haley]

Basile STARYNKEVITCH wrote:
> FWIW, I am not taking this question personally (I don't really claim
> that I could become any kind of reviewer; I believe in general that
> reviewing abilities should be evaluated by others.). I just think the
> set of reviewers should significantly grow.

But that needs more experts.  The set of reviewers is not limited by
the steering committee but by the number of experts in the world that
are willing to review patches.

>>> My feeling is on the contrary that the set of people having a real
>>> knowledge of gcc (or at least of substantial parts of it [*]) is much
>>> bigger than the set of reviewers allowed to say OK.
>>>>
>>   
> I am not at the summit. So I don't know if my perception "there are not
> enough reviewers" [0] is shared by others or not. I suppose it is agreed
> (that the set of reviewers should be increased [1])  If not, ignore the
> rest. I really don't know if other people believe (as I do) that the set
> of reviewers should significantly increase.

Everyone agrees that life would be better if there were more suitably
qualified gcc maintainers to approve patches.  I don't have to ask
anyone to know that.

>> It is quite possibly the case that some maintainers should be "promoted".
>> But it isn't sufficient to have a blanket policy of "let's have more
>> reviewers".  
> But we first should agree on the wish than an increase of the set of
> reviewers is desirable.

It's not in any doubt.  What is in doubt is that we should reduce
the quality threshold in order to get them.

>> We need something more like "I think Fred Bloggs knows gcc
>> well enough to approve patches to reload" or "I am Fred Bloggs and I
>> know gcc well enough to approve patches to reload."

> I am not sure to parse correctly this sentence. Sorry, English is a
> foreign language to me. Is "reload" some functionality (PCH?) you refer
> inside GCC, or is it the task of making reviews on patches submitted on
> gcc-patches@ ? I was just thinking about stuff like "Fred Bloggs knows
> enough to approve patches submitted on gcc-patches@ to files gcc/ggc*.[ch]"

This is going to sound rude, but if you don't know what reload is
you're not able to talk about gcc maintenance.

Andrew.

Re: increasing the number of GCC reviewers

[Basile STARYNKEVITCH]

Andrew Haley wrote:
> Basile STARYNKEVITCH wrote:
>   
>> FWIW, I am not taking this question personally (I don't really claim
>> that I could become any kind of reviewer; I believe in general that
>> reviewing abilities should be evaluated by others.). I just think the
>> set of reviewers should significantly grow.
>> I am not sure to parse correctly this sentence. Sorry, English is a
>> foreign language to me. Is "reload" some functionality (PCH?) you refer
>> inside GCC, or is it the task of making reviews on patches submitted on
>> gcc-patches@ ? I was just thinking about stuff like "Fred Bloggs knows
>> enough to approve patches submitted on gcc-patches@ to files gcc/ggc*.[ch]"
>>     
>
> This is going to sound rude, but if you don't know what reload is
> you're not able to talk about gcc maintenance.
>
>   

Reload is probably in the register allocator, which indeed is in the 
backend part I know nothing about (and I don't care much).

Your opinion is not rude, but I still believe one don"t need to 
understand all of the GCC internals to talk about the review process. I 
even believe that some people (e.g. those working on other big software 
like Gnome/GTK, KDE/QT, Mozilla, ...) could have valid opinions & 
suggestions on it without even knowing anything about GCC, but only 
understanding the review procedures.

I even disagree on your opinion. I believe I might even become in a few 
years some kind of gcc/ggc*.[ch] secondary reviewer. I don't want to 
become one (being a reviewer is probably more a burden than an honor, 
and probably consume a lot of time, and might be often boring.). 
However, I am pretty sure that nobody needs to understand register 
allocation to review GGC related patches, or Ada or C++ front end 
patches, or Fortran front-end patches, or CPP patches, or perhaps even 
some gcc/passes.c or gcc/tree-passes.h patches like the plugin related 
ones.).

This is precisely my point. It should be perfectly acceptable that some 
people be authorized to approve some few patches without understanding 
the whole of GCC, and even without knowing all of it.

Now, I understand you or others can disagree with my opinion. You may 
think that most reviewers should know most of GCC (I disagree with that).

My point is that GCC could quite soon grow to 5 or 6MLOC, and the set of 
people understanding all of them (or simply more than half of them) 
will, if it is not yet empty, decrease dramatically.

Perhaps some GCC decision making people (SC?) might even want to really 
decrease the size of core GCC. This could be possible with the plugin 
feature. As an extreme & silly example, they (or is it we?) could decide 
to move out of GCC core and into future GCC plugins all the 
optimizations done only at -O3 or only with explicit -f* flags. I don't 
really advocate that (again I don't have any informed opinion; I almost 
never used -O3 and know few people who do use it!) but it could be possible.


Regards

-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***

Re: increasing the number of GCC reviewers

[Andrew Haley]

Basile STARYNKEVITCH wrote:
> Andrew Haley wrote:
>> Basile STARYNKEVITCH wrote:
>>
>> This is going to sound rude, but if you don't know what reload is
>> you're not able to talk about gcc maintenance.
> 
> Reload is probably in the register allocator, which indeed is in the
> backend part I know nothing about (and I don't care much).

OK, that's pretty close.

> Your opinion is not rude, but I still believe one don"t need to
> understand all of the GCC internals to talk about the review process.

No, they don't.  But they need to have some kind of a clue about how
it works, and what the pieces are.

> I even disagree on your opinion. I believe I might even become in a few
> years some kind of gcc/ggc*.[ch] secondary reviewer.

Sure, I don't see why not.  It'll take work, but it's perfectly possible.

> This is precisely my point. It should be perfectly acceptable that some
> people be authorized to approve some few patches without understanding
> the whole of GCC, and even without knowing all of it.

GCC isn't really like that.  Changes in one part can affect things much
later on, and you really have to know why.  That doesn't mean you have
to understand all of the compiler, but you need to have a lot of
knowledge.

> Now, I understand you or others can disagree with my opinion. You may
> think that most reviewers should know most of GCC (I disagree with that).

No-one knows most of GCC.  At least, I very much doubt it.

Rather than saying "the set of reviewers should significantly grow", let me
challenge you.  Suggest someone who should be added to that set.

Andrew.

Re: increasing the number of GCC reviewers

[Steven Bosscher]

On Tue, Jun 9, 2009 at 9:29 PM, Andrew Haley<aph@redhat.com> wrote:
>> This is precisely my point. It should be perfectly acceptable that some
>> people be authorized to approve some few patches without understanding
>> the whole of GCC, and even without knowing all of it.
>
> GCC isn't really like that.  Changes in one part can affect things much
> later on, and you really have to know why.  That doesn't mean you have
> to understand all of the compiler, but you need to have a lot of
> knowledge.

This is a problem with GCC's lack of modularity, not with Basile's
point of view.

Ciao!
Steven

Re: increasing the number of GCC reviewers

[Richard Kenner]

> > GCC isn't really like that. Changes in one part can affect things much
> > later on, and you really have to know why. That doesn't mean you have
> > to understand all of the compiler, but you need to have a lot of
> > knowledge.
> 
> This is a problem with GCC's lack of modularity, not with Basile's
> point of view.

I don't think it's a totally modularity issue.  Compilers, by their nature,
are some of the most complicated and interdependent programs around.  Sure,
one could improve the modularity of GCC, but a key to evaluating a change
is to know if the change is being done in the right place.  In order to be
able to make that determination, you have to know about all the other
places that might also exist.  Modularity isn't going to help THAT problem
much.

Re: increasing the number of GCC reviewers

[Joseph S. Myers]

On Wed, 10 Jun 2009, Richard Kenner wrote:

> > > GCC isn't really like that. Changes in one part can affect things much
> > > later on, and you really have to know why. That doesn't mean you have
> > > to understand all of the compiler, but you need to have a lot of
> > > knowledge.
> > 
> > This is a problem with GCC's lack of modularity, not with Basile's
> > point of view.
> 
> I don't think it's a totally modularity issue.  Compilers, by their nature,
> are some of the most complicated and interdependent programs around.  Sure,
> one could improve the modularity of GCC, but a key to evaluating a change
> is to know if the change is being done in the right place.  In order to be
> able to make that determination, you have to know about all the other
> places that might also exist.  Modularity isn't going to help THAT problem
> much.

This is one area where reviews of the form "OK if there are (no objections 
/ no objections from an area X maintainer) within 48 hours" can be useful: 
if a maintainer can judge that a patch will safely fix a problem but it's 
possible that a maintainer from another area, or a more specific 
maintainer, might judge that a different approach for a fix would be 
better.  For that matter, a maintainer could comment on the choice of area 
in which to fix the bug without knowing about the precise details of how 
it should be fixed there; there are lots of partial review possibilities 
that already happen (and we don't need to classify them in detail, but may 
wish to encourage them where patches fall between maintainer areas).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: increasing the number of GCC reviewers

[Diego Novillo]

On Wed, Jun 10, 2009 at 08:15, Richard Kenner<kenner@vlsi1.ultra.nyu.edu> wrote:

>> This is a problem with GCC's lack of modularity, not with Basile's
>> point of view.
>
> I don't think it's a totally modularity issue.  Compilers, by their nature,
> are some of the most complicated and interdependent programs around.

I agree.  Modularity mostly gives you modular complexity, it does not
reduce the inherent complexity of the compiler.  It does make some
aspects of reviewing easier to handle, of course.


Diego.

Re: increasing the number of GCC reviewers

[Diego Novillo]

Basile,

You don't need to convince us that we need more reviewers.  We all
agree on that.  You simply need to suggest a reviewer for some set of
files that

- Knows that set of files very well
- Is familiar with GCC development
- Is willing to review patches and be a maintainer for those files
- Has made enough contributions to those files and whose work is well
regarded by the developer community

You'll quickly find out that this makes for a fairly small set of
people.  Long term, I don't think we would do us a service if we
relaxed any of those requirements too much.


Diego.

Re: increasing the number of GCC reviewers

[Gabriel Dos Reis]

On Tue, Jun 9, 2009 at 3:22 PM, Diego Novillo<dnovillo@google.com> wrote:

> You'll quickly find out that this makes for a fairly small set of
> people.  Long term, I don't think we would do us a service if we
> relaxed any of those requirements too much.

Excellent summary.

-- Gaby

Re: increasing the number of GCC reviewers

[Tom Tromey]

>>>>> "Basile" == Basile STARYNKEVITCH <basile@starynkevitch.net> writes:

Basile> I believe I might even become in a few years some kind of
Basile> gcc/ggc*.[ch] secondary reviewer. I don't want to become one
Basile> (being a reviewer is probably more a burden than an honor, and
Basile> probably consume a lot of time, and might be often boring.)

This is probably part of the problem.
Maybe there are other people who don't want to do it :-)

Tom

Re: increasing the number of GCC reviewers

[Ben Elliston]

On Tue, 2009-06-09 at 21:13 +0200, Basile STARYNKEVITCH wrote:

> This is precisely my point. It should be perfectly acceptable that some 
> people be authorized to approve some few patches without understanding 
> the whole of GCC, and even without knowing all of it.

I sympathise with this point of view, Basile.  Over the time I have
worked on GCC, there has been a lot of modularity improvements and
improving the robustness of interfaces throughout the compiler.  If this
continues, it should be possible to understand changes in more
isolation.  While there will always be consequences that need to be
understood, but that should not apply to every change.

Ben

Re: increasing the number of GCC reviewers

[Ian Lance Taylor]

Basile STARYNKEVITCH <basile@starynkevitch.net> writes:

> I am unfortunately not attending the GCC summit which happens right
> now in Montreal.
>
> But apparently, there seems to be a lack of code reviewers for
> GCC. The few people who do review code seems to have a lot of review
> in their batch queue.
>
> Perhaps could be discussed at the summit some way to increase the set
> of reviewers, i.e. the set of people able to say Ok to a patch
> submitted on gcc-patches@
>
> Or is that question a steering committee priviledge? If it is, could
> someone ask it to them (the SC members)?

Your e-mail was raised (by Richi) during the SC panel discussion.  The
discussion in the room was much like the discussion on the list: we
would all like to have more reviewers; the problem is finding people who
are willing and able to do the job.  The SC is not a bottleneck here.

During the panel discussion, Mark Mitchell said that if anybody wants to
raise an issue with the SC, they can send him e-mail directly and he
will raise it.  I'm sure that is true of other SC members as well.

I believe that the most useful immediate thing we could do to speed up
gcc development would be to move to a distributed version control
system.  That should make it much easier for people to swap patches
around.  It should make it possible for reviewers to take a patch, make
some changes themselves, and send it back to the original submitter for
more work.  It would make it much simpler for people to reliably test
other people's patches, singly or in combination.  None of this would
eliminate the traditional role of maintainers in the gcc system;
however, I believe it would make some decisions simpler.

Ian

Re: increasing the number of GCC reviewers

[Joseph S. Myers]

On Tue, 9 Jun 2009, Ian Lance Taylor wrote:

> I believe that the most useful immediate thing we could do to speed up
> gcc development would be to move to a distributed version control
> system.

We haven't even finished the last version control system transition 
(wwwdocs is still using CVS), it's not long since we started it and there 
is as yet no one clear winner in the world of DVCSes, so another 
transition would seem rather premature.

> That should make it much easier for people to swap patches
> around.

I don't see what the present difficulty here is supposed to be.  For small 
to moderate-sized self-contained patches the precise system used makes 
little difference.  For large to huge patch series from contributors with 
write access, they are already able to create whatever branches they may 
wish.  Distributed systems would facilitate large series from contributors 
without write access, but in practice I think such contributions from 
non-regular contributors tend to run into other unrelated issues (and 
hosting a public tree the size of GCC isn't really something to consider 
"easy" whatever system is used).

> It should make it possible for reviewers to take a patch, make
> some changes themselves, and send it back to the original submitter for
> more work.

This is already possible, but I don't think difficulties with it are a 
significant problem with patch review at present.  If the original 
submitter is to submit further patches then having them understand the 
deficiencies enough to fix them themselves is generally better than fixing 
them for them.  (The simpler case of fixing the submitted patch and 
committing the fixed version is already quite common.)

> It would make it much simpler for people to reliably test
> other people's patches, singly or in combination.

As I observe it difficulties in dealing with other people's patches arise 
not because of version control choices but because of interactions between 
logical changes expressed textually in patches, especially global 
mechanical changes, and semantic patch tools seem to me to be more 
promising to help in such areas.

I have observed patch review and its failures in action through following 
the main GCC lists since the start of EGCS.  It's not the large and 
complicated patches than in my experience have difficulty getting 
reviewed; those generally come from people with extensive experience of 
GCC development, and if those people cannot approve their own patches 
there are other people with sufficient interest in them who do so.  It's 
smaller patches, and patches from outsiders, that have the greater 
difficulty, while it's complicated patches from sophisticated developers 
that more advanced version control systems help more with.

If a patch does not attract the interest and attention of reviewers when 
posted, there is no system (human or technical) to notice this and to 
track that it still needs review and who might be a suitable reviewer, 
other than the submitter pinging it.  If a patch is submitted by an 
outsider who hasn't followed all the documentation on submitting patches, 
and may not have properly tested it, or described how it was tested, or 
written a proper ChangeLog entry, or formatted it according to the coding 
standards, or followed the correct abstractions, the fact it doesn't look 
like a normal submission and requires greater effort to review makes it 
less likely to be reviewed.  If a patch from an outsider is nevertheless 
approved, the approver will typically say it is OK, and it will be up to 
the outsider to reply that they don't have write access and get someone to 
commit it.

At the human level I suspect it would help to have people who watch for 
submissions from non-regulars (including those attached to Bugzilla) and 
help them prepare patches following all the usual conventions and get them 
reviewed (checking for copyright assignments at an early stage as needed) 
and make sure the submissions don't get lost.  At the technical level, 
while submissions on gcc-patches take a wide variety of forms, approvals 
are more restricted; it ought to be possible for software to do a 
reasonably good job of tracking which submissions have been reviewed / 
approved / committed (including noticing people trying to submit patches 
through Bugzilla), and of identifying the most likely relevant maintainers 
to review patches, aided by humans in keeping the data clean.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: increasing the number of GCC reviewers

[Ian Lance Taylor]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> At the human level I suspect it would help to have people who watch for 
> submissions from non-regulars (including those attached to Bugzilla) and 
> help them prepare patches following all the usual conventions and get them 
> reviewed (checking for copyright assignments at an early stage as needed) 
> and make sure the submissions don't get lost.  At the technical level, 
> while submissions on gcc-patches take a wide variety of forms, approvals 
> are more restricted; it ought to be possible for software to do a 
> reasonably good job of tracking which submissions have been reviewed / 
> approved / committed (including noticing people trying to submit patches 
> through Bugzilla), and of identifying the most likely relevant maintainers 
> to review patches, aided by humans in keeping the data clean.

I agree, and I think it is in fact precisely at this step where a
distributed version control system can be most helpful.  But I'm not
going to belabor the point.

Ian

Re: increasing the number of GCC reviewers

[Tom Tromey]

>>>>> "Joseph" == Joseph S Myers <joseph@codesourcery.com> writes:

Ian> I believe that the most useful immediate thing we could do to speed up
Ian> gcc development would be to move to a distributed version control
Ian> system.

Joseph> We haven't even finished the last version control system
Joseph> transition (wwwdocs is still using CVS), it's not long since
Joseph> we started it and there is as yet no one clear winner in the
Joseph> world of DVCSes, so another transition would seem rather
Joseph> premature.

There will never be a clear winner.  git and hg are in wide use on big
projects.  Either one would probably work reasonably for gcc.  Neither
is likely to kill off the other.

Tom

Re: increasing the number of GCC reviewers

[Daniel Berlin]

On Tue, Jun 9, 2009 at 8:51 PM, Joseph S. Myers<joseph@codesourcery.com> wrote:
> On Tue, 9 Jun 2009, Ian Lance Taylor wrote:
>
>> I believe that the most useful immediate thing we could do to speed up
>> gcc development would be to move to a distributed version control
>> system.
>
> We haven't even finished the last version control system transition
> (wwwdocs is still using CVS), it's not long since we started it and there
> is as yet no one clear winner in the world of DVCSes, so another
> transition would seem rather premature.
There will never be a clear winner here, because none of them is
"better enough" than the others, nor is it likely they ever will be.
At least hg and git have significant mindshare, and seem to attract
different kinds of communities.

Personally, I think at this point, moving to git would make the most
sense (as much as I love hg). It's much more forgiving than it used to
be, most things support it, and there are some cool possibilities,
like gerrit (http://code.google.com/p/gerrit/).  It is precisely built
to handle the problem of finding the right reviewers, making sure
patches don't fall through the cracks, while still making sure it's
easy to submit patches and have maintainers merge them.
I'm happy to set up a demo if anyone wants to see it.

As for advantages, having used both hg and git, the only thing i ever
use svn for anymore is to occasionally patch into a clean tree to do a
commit.

Lastly, as for wwwdocs, it's more a case of "migration of wwwdocs was
left to those who care about it", which is a very small set of people.
Combined with the fact that it has a bunch of preprocessing/etc
scripts that get run over it, and nobody wants to do the conversion.

Re: increasing the number of GCC reviewers

[Joseph S. Myers]

On Tue, 9 Jun 2009, Daniel Berlin wrote:

> be, most things support it, and there are some cool possibilities,
> like gerrit (http://code.google.com/p/gerrit/).  It is precisely built

I think a critical feature of any fancy code review system (or of how it 
is configured for GCC) used for a significant amount of GCC review is that 
all the patches and reviews are automatically sent to gcc-patches in plain 
text so that they get archived in the static list archives and can 
continue to be referred to that way, grepped, etc., after the next N 
transitions.  Email replies to patch submissions should also continue to 
work smoothly.  Given these requirements, anyone can use or not use any 
code review system as they please.  Bugzilla has worked very well in this 
regard in allowing multiple workflows (reading new submissions and 
comments through email and replying there / searching for past bugs and 
replying through the web interface).

> to handle the problem of finding the right reviewers, making sure
> patches don't fall through the cracks, while still making sure it's

I believe (based on observation of what has been done so far) people will 
submit patches in any way possible, including several we do not advise at 
all such as attaching patches against a GCC 3.x release tarball to a bug 
in Bugzilla or sending those patches to gcc-bugs, and making sure patches 
don't fall through the cracks means detecting patches sent in all these 
ways and tracking their status.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: increasing the number of GCC reviewers

[Paolo Bonzini]

Joseph S. Myers wrote:
> On Tue, 9 Jun 2009, Daniel Berlin wrote:
> 
>> be, most things support it, and there are some cool possibilities,
>> like gerrit (http://code.google.com/p/gerrit/).  It is precisely built
> 
> I think a critical feature of any fancy code review system (or of how it 
> is configured for GCC) used for a significant amount of GCC review is that 
> all the patches and reviews are automatically sent to gcc-patches in plain 
> text so that they get archived in the static list archives and can 
> continue to be referred to that way, grepped, etc., after the next N 
> transitions.  Email replies to patch submissions should also continue to 
> work smoothly.

Yes, Daniel just mentioned a possibility.  It is good to consider 
possible developments even if for now you're just keeping the old workflow.

gerrit was developed at Google, but git's own development (and Linux's) 
proceeds only on mailing lists, and for this reason git has a command like

	git send-email --cc joseph@codesourcery.com master..HEAD

to submit a private branch all at once to gcc-patches@ and CC you. :-)

It is true however that currently we are not encouraging outsiders to 
contribute, because old timers work on mostly large patches (or large 
sequences of patches) that reviewers know about.  For the same reason, 
it is easier for small patches to fall through the cracks than large ones.

Paolo

Re: increasing the number of GCC reviewers

[Joseph S. Myers]

On Wed, 10 Jun 2009, Paolo Bonzini wrote:

> It is true however that currently we are not encouraging outsiders to
> contribute, because old timers work on mostly large patches (or large
> sequences of patches) that reviewers know about.  For the same reason, it is
> easier for small patches to fall through the cracks than large ones.

And for all the other reasons I mentioned about patches from outsiders 
being liable to be harder to review.  And when they are reviewed the 
contributor may lose interest and not go through all the steps needed to 
address issues from the review.  So I don't think any tracking system 
should treat "needs changes from the submitter" as a state meaning 
"maintainers can ignore this indefinitely until a new submission"; patches 
in that state from new contributors could just as much do with human help 
from more experienced contributors to get the patches into shape (and I 
think such human help for badly formed contributions would be just as 
valuable in attracting new contributors as any technical solutions).

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: increasing the number of GCC reviewers

[Weddington, Eric]

 

> -----Original Message-----
> From: Joseph S. Myers [mailto:joseph@codesourcery.com] 
> Sent: Tuesday, June 09, 2009 6:51 PM
> To: Ian Lance Taylor
> Cc: Basile STARYNKEVITCH; GCC Mailing List
> Subject: Re: increasing the number of GCC reviewers
> 
> 
> At the human level I suspect it would help to have people who 
> watch for 
> submissions from non-regulars (including those attached to 
> Bugzilla) and 
> help them prepare patches following all the usual conventions 
> and get them 
> reviewed (checking for copyright assignments at an early 
> stage as needed) 
> and make sure the submissions don't get lost.  At the 
> technical level, 
> while submissions on gcc-patches take a wide variety of 
> forms, approvals 
> are more restricted; it ought to be possible for software to do a 
> reasonably good job of tracking which submissions have been 
> reviewed / 
> approved / committed (including noticing people trying to 
> submit patches 
> through Bugzilla), and of identifying the most likely 
> relevant maintainers 
> to review patches, aided by humans in keeping the data clean.

From my experience having patches go to a mailing list is a sure way to have them get lost. When it goes into someone's inbox, it's likely to get pushed down, and "out of sight, out of mind" quickly. While the ML is archived, it is not as useful to search through as having a specific patch tracker/database, e.g. as found on SourceForge or Savannah projects. AFAIK the only gcc patch tracker being used is not used on a mandatory basis. 

While I'm not suggesting that gcc use SF/Savannah, it seems odd that gcc has a bug database, but no patch tracking database.

RE: increasing the number of GCC reviewers

[Joseph S. Myers]

On Wed, 10 Jun 2009, Weddington, Eric wrote:

> From my experience having patches go to a mailing list is a sure way to 
> have them get lost. When it goes into someone's inbox, it's likely to 
> get pushed down, and "out of sight, out of mind" quickly. While the ML 
> is archived, it is not as useful to search through as having a specific 
> patch tracker/database, e.g. as found on SourceForge or Savannah 
> projects. AFAIK the only gcc patch tracker being used is not used on a 
> mandatory basis.
> 
> While I'm not suggesting that gcc use SF/Savannah, it seems odd that gcc 
> has a bug database, but no patch tracking database.

Sure, a database is useful; I identified Bugzilla as a model that has 
worked well.  Email works very well for the initial feed of all GCC 
development traffic since the last time it was checked including bugs, 
comments on bugs, patches and comments on patches, and provides the first 
pass of acquainting people with new developments coming in and allowing 
quick responses; the database for finding later bugs meeting given 
criteria that didn't get dealt with in the initial email pass.  The 
database works well for tracking over time narrowly focused discussion on 
a particular well-defined bug; rather less well when the discussion 
diverges and there is no longer a clear concept of exactly what bug the 
database entry relates to (I think the same would apply to any system 
tracking patch discussion: it would get less useful when the discussion 
diverges away from a patch to deal with a clearly defined issue).  If 
Bugzilla included patch attachments in the body of emails it sent rather 
than as URLs only, it might even work OK for patch review, especially if 
it could somehow tell when messages should go to gcc-patches and when to 
gcc-bugs.

If I want to find a particular past *development* discussion, I don't 
necessarily remember which list it appeared on but may have other context 
for it; grepping my mailboxes for it sometimes helps, as may searching for 
information in the online archives, and in both cases it is very useful 
that bug discussions appear in gcc-bugs mailboxes and archives rather than 
needing a different search system in a different database to be used for 
those.

Sending a message to my inbox and *not* to the relevant mailing list is a 
very good way of making it less likely I'll respond to it; I'll ignore 
direct copies of messages that "should have" gone to a mailing list on the 
basis that I'll deal with the list copy later when reading my list 
mailbox, but I may not notice that the message did not in fact go to a 
list after all.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: increasing the number of GCC reviewers

[Weddington, Eric]

 

> -----Original Message-----
> From: Joseph Myers [mailto:joseph@codesourcery.com] 
> Sent: Wednesday, June 10, 2009 8:52 AM
> To: Weddington, Eric
> Cc: Ian Lance Taylor; Basile STARYNKEVITCH; GCC Mailing List
> Subject: RE: increasing the number of GCC reviewers
> 
> > While I'm not suggesting that gcc use SF/Savannah, it seems 
> odd that gcc 
> > has a bug database, but no patch tracking database.
> 
> Sure, a database is useful; I identified Bugzilla as a model that has 
> worked well.

I would think that setting up a bugzilla database for patches would might allow some clarification of the approval process. Fields could be set up to categorize the patch into one or more approval areas (e.g. patch needs approval from target maintainer and middle-end maintainer), and ways to mark the patch with what is needed (e.g., needs ChangeLog, GNU coding standards, fix in specified area (w/ reference to comment), etc.), general status of patch (e.g. NEW, WAITING FOR CHANGE, APPROVED, REJECTED, whatever), priority and target milestone.

Perhaps with having a database of patches, reviewers could search for "open patches" and pick and choose a patch to review as they like and time permitting, much like it's done with the bug database today. If people do not want to be promoted to reviewers today because of the fear that they cannot keep up with gcc-patches (that somehow they have to keep track of every single patch coming in), then maybe a patch database is way to allow them to ease into participating in the review process and to do the partial reviews that would still help move things along.