Hi all. I'm assuming most people reading this email are familiar with LLVM's
code review process <LLVM Developer Policy — LLVM 16.0.0git documentation;
as well as LLVM Weekly, the development newsletter I've written and sent out
every Monday since Jan 2014. Since that time, it's provided something of a
"signal boost" for important mailing list discussions and commits. I feel it
could play a similar role in helping patches that are stuck waiting for code
reviews, or drawing attention to submissions from first time contributors.
There may be alternative or complementary approaches to tackling this
perceived problem we should discuss - I'm coming from a position of trying to
apply the tools I have at my disposal. Also see my previous thoughts on this
issue <http://lists.llvm.org/pipermail/llvm-dev/2016-November/106696.html>\.
I don't think it's controversial to suggest that while the code review process
works fantastically well a lot of the time, some patches fall through the
cracks and long delays in review feedback can put people off contributing to
LLVM. As was pointed out in response to the last RFC, very long review times
are problems for long-time contributors as well as newcomers.
My proposal is simple: add a new 'Review corner' section to LLVM Weekly to
help highlight patches that need more reviewer input. There are two main
categories I'd like to focus on:
1) patches from first-time contributors
2) patches where review activity has died off (i.e. they're 'stuck').
Obviously this is something that I can just go ahead and do, but I'd
appreciate feedback on whether this would be useful, as well as the specifics
of my proposed approach. I'd argue such a listing does provide value above and
beyond the firehose of {llvm,cfe}-commits, as the two mentioned categories are
not easily discoverable.
How to select the patches to include? I'm going to rule out manual curation -
LLVM Weekly is already a large time commitment and I'm not convinced trawling
llvm-commits is the way to about this even if time weren't a problem. Instead
I suggest that authors of patches that have got stuck in the review process
should submit their work for inclusion via a simple web form. If reviews have
stalled for a given period of time (1/2 weeks?), just submit a link to the
patch, and up to two sentences describing what the patch does and why people
might want to take a look at it. This might also be used to highlight patches
that aren't short of reviews but could benefit from a wider audience
(obviously if changes are large enough, an RFC should be submitted to
llvm-dev/cfe-dev as usual). The Phabricator API could be used to identify
patches from first time contributors for automatic inclusion, and ideally to
track the stats related to previously included patches (perhaps generate and
include credits for those who stepped up and helped review the previous week's
list?).
So, what do people think, is this worth a go? I recognise this proposal makes
a fairly large assumption, that lack of visibility is the problem to be
solved. If the underlying problem is that not enough people have the time or
willingness to review code, no amount of signal boosting is going to improve
things.
Thank you very much for raising this crucial issue again.
And I think the review corner would be a good way to solve it.
However, I’m worry about that the traffic of the list would eventually become too high, and looses the ‘highlighter’ feature that it should have
After all, no one wants the ‘headline of unreviewed patches' to be a 1000+ items list.
I suggest that we can aid this problem from another aspect at the same time: Notify the reviewer actively.
We can inference the candidate code owner/reviewer who is responsible for the patch from the submitted code
And provide the submitter an option to notify those reviewers by email.
I think with the aforementioned approach, we can keep the review corner while preventing it to become another {llvm,cfe}-commits
Also, I think it can help new contributors with some problems related to code owners and code review assignees. For example, sometimes a new submitter either doesn't know who should be assigned for a code review, or the part he/she committed doesn’t seem to be covered by any code owner listed in CODE_OWNER.txt (e.g Transforms/Utils, IIRC).
Thank you very much for raising this crucial issue again.
And I think the review corner would be a good way to solve it.
However, I’m worry about that the traffic of the list would eventually become too high, and looses the ‘highlighter’ feature that it should have
After all, no one wants the ‘headline of unreviewed patches' to be a 1000+ items list.
Hi Bekket, thanks for the response.
I've been following llvm-commits pretty intently for the last 6 weeks
or so. My impression is that the list of patches that are stuck on
reviews and received a 'ping' in that time isn't too huge. I think the
important thing to remember is that the review process is working well
for a large volume of patches, so we're talking about highlighting a
fairly small subset.
I suggest that we can aid this problem from another aspect at the same time: Notify the reviewer actively.
We can inference the candidate code owner/reviewer who is responsible for the patch from the submitted code
And provide the submitter an option to notify those reviewers by email.
I think with the aforementioned approach, we can keep the review corner while preventing it to become another {llvm,cfe}-commits
Also, I think it can help new contributors with some problems related to code owners and code review assignees. For example, sometimes a new submitter either doesn't know who should be assigned for a code review, or the part he/she committed doesn’t seem to be covered by any code owner listed in CODE_OWNER.txt (e.g Transforms/Utils, IIRC).
Do you get the impression people have problems following the advice of
looking at the commit history for the files they're modifying and
adding reviewers based on that?
Alex Bradbury <asb@asbradbury.org> 於 2017年8月27日 上午6:43 寫道：
Hi Alex,
Thank you very much for raising this crucial issue again.
And I think the review corner would be a good way to solve it.
However, I’m worry about that the traffic of the list would eventually become too high, and looses the ‘highlighter’ feature that it should have
After all, no one wants the ‘headline of unreviewed patches' to be a 1000+ items list.
Hi Bekket, thanks for the response.
I've been following llvm-commits pretty intently for the last 6 weeks
or so. My impression is that the list of patches that are stuck on
reviews and received a 'ping' in that time isn't too huge. I think the
important thing to remember is that the review process is working well
for a large volume of patches, so we're talking about highlighting a
fairly small subset.
Ah, that’s great.
I suggest that we can aid this problem from another aspect at the same time: Notify the reviewer actively.
We can inference the candidate code owner/reviewer who is responsible for the patch from the submitted code
And provide the submitter an option to notify those reviewers by email.
I think with the aforementioned approach, we can keep the review corner while preventing it to become another {llvm,cfe}-commits
Also, I think it can help new contributors with some problems related to code owners and code review assignees. For example, sometimes a new submitter either doesn't know who should be assigned for a code review, or the part he/she committed doesn’t seem to be covered by any code owner listed in CODE_OWNER.txt (e.g Transforms/Utils, IIRC).
Do you get the impression people have problems following the advice of
looking at the commit history for the files they're modifying and
adding reviewers based on that?
Sometimes contributors in the commit history are not able to review, or if they’re not the code owner, they’re not mandatory to take responsibility for reviewing the patch.
Will, I think this problem would somehow lead back to the original one, where there are patches left unreviewed though they’re assigned to somebody.
But I think finding the right people to review is still an important issue.
Thank you for trying to solve a very real problem.
I’m not sure this method will work – but we’ll never know without trying. Either way, your willingness to act and do something is very much appreciated by me personally.
Hans Wennborg suggested on Twitter that bugs could also be included. I
can't write a coherent response in 140 characters, so am responding
here.
I think that highlighting important bugs is also a useful activity,
but I have less faith that there's something LLVM Weekly can do to
help here. There may be some value in highlighting bugs which are
release blockers (though I already try to make sure I link to any such
list when posted by the release manager) or a selection of 'beginner'
bugs, (props to Brian Gesiak for pushing for this category). But
beyond that, how do you decide which bugs to highlight? With patches,
it's normally the case that a highly motivated party (the patch
author) has put in the majority of the work, and a relatively smaller
amount of incremental effort is required from others in the LLVM
community (code review). With bugs it's the other way around - a huge
amount of additional work might be required to properly diagnose and
address a bug report.
I'm totally open to trying something if you think there's a way LLVM
Weekly can have an impact in this area, but I'm less hopeful about the
potential impact in reducing the number of open bugs.
I think this is a great idea.
I could suggest you setting up Google Forms[1] (or something similar). This way you can give access to some volunteers (count me in) who can help you moderate those submissions.
As a side note, Rust has a thing called “This week in Rust”[2], which is basically a community moderated weekly. You may adopt some parts of this approach if it sounds good to you.
Hi all. I'm assuming most people reading this email are familiar with LLVM's
code review process <LLVM Developer Policy — LLVM 18.0.0git documentation;
as well as LLVM Weekly, the development newsletter I've written and sent out
every Monday since Jan 2014. Since that time, it's provided something of a
"signal boost" for important mailing list discussions and commits. I feel it
could play a similar role in helping patches that are stuck waiting for code
reviews, or drawing attention to submissions from first time contributors.
There may be alternative or complementary approaches to tackling this
perceived problem we should discuss - I'm coming from a position of trying to
apply the tools I have at my disposal. Also see my previous thoughts on this
issue <http://lists.llvm.org/pipermail/llvm-dev/2016-November/106696.html>\.
Hans Wennborg suggested on Twitter that bugs could also be included. I
can't write a coherent response in 140 characters, so am responding
here.
The reason I brought it up is because I think the situation is
somewhat similar to stalled reviews; in fact forgotten bug reports are
even worse in a way: if no developers are cc'd on the bug, the pings
don't go anywhere -- no mailing list, no nothing. At least with code
reviews, pings usually go to one of the -commits lists.
I think that highlighting important bugs is also a useful activity,
but I have less faith that there's something LLVM Weekly can do to
help here. There may be some value in highlighting bugs which are
release blockers (though I already try to make sure I link to any such
list when posted by the release manager) or a selection of 'beginner'
bugs, (props to Brian Gesiak for pushing for this category). But
beyond that, how do you decide which bugs to highlight? With patches,
it's normally the case that a highly motivated party (the patch
author) has put in the majority of the work, and a relatively smaller
amount of incremental effort is required from others in the LLVM
community (code review). With bugs it's the other way around - a huge
amount of additional work might be required to properly diagnose and
address a bug report.
I'm totally open to trying something if you think there's a way LLVM
Weekly can have an impact in this area, but I'm less hopeful about the
potential impact in reducing the number of open bugs.
What I was thinking when I replied to your tweet was something like
"bugs filed in the last 7 days which no-one seems to have looked at",
or something similar. Hopefully it should be possible to build the
list automatically.
I'm not sure how large that list would be each week though, or how
useful it would be. Also, this might be something we should fix by
having a better bug triaging process in general.
Hi all. I'm assuming most people reading this email are familiar with LLVM's
code review process <LLVM Developer Policy — LLVM 18.0.0git documentation;
as well as LLVM Weekly, the development newsletter I've written and sent out
every Monday since Jan 2014. Since that time, it's provided something of a
"signal boost" for important mailing list discussions and commits. I feel it
could play a similar role in helping patches that are stuck waiting for code
reviews, or drawing attention to submissions from first time contributors.
There may be alternative or complementary approaches to tackling this
perceived problem we should discuss - I'm coming from a position of trying to
apply the tools I have at my disposal. Also see my previous thoughts on this
issue <http://lists.llvm.org/pipermail/llvm-dev/2016-November/106696.html>\.
Hans Wennborg suggested on Twitter that bugs could also be included. I
can't write a coherent response in 140 characters, so am responding
here.
The reason I brought it up is because I think the situation is
somewhat similar to stalled reviews; in fact forgotten bug reports are
even worse in a way: if no developers are cc'd on the bug, the pings
don't go anywhere -- no mailing list, no nothing. At least with code
reviews, pings usually go to one of the -commits lists.
Good point.
What I was thinking when I replied to your tweet was something like
"bugs filed in the last 7 days which no-one seems to have looked at",
or something similar. Hopefully it should be possible to build the
list automatically.
I'm not sure how large that list would be each week though, or how
useful it would be. Also, this might be something we should fix by
having a better bug triaging process in general.
Thanks for the clarification. Trying to improve bug triage problem
seems like a manageable proposition. I'd guess it's already possible
to construct a bugzilla query showing bugs filed in the past 7 days
that haven't been triaged?
Something like the following?:
"""
## Bug corner
In the past 7 days, XXX bugs have been opened, YYY bugs have been
closed. ZZZ newly-filed bugs remain untriaged - see <link> to help.
Thanks to contributions from CREDIT1, ..., CREDITN, AAA out of BBB
bugs were triaged last week.
"""
FWIW: I built and ran a gcc patch tracker back in the day that parsed
patches out of the mailing list that were waiting review, identified likely
code owners, and built a table, and colored them by status.
It worked remarkably well to see which areas of the compiler needed more
people who could review patches, etc.
(it was not really meant to shame people :P).
It ended up getting very heavily used, and it presented relatively hard
data.
So i suspect what you suggest, or some variant may help.
(It also seems like it would be really easy to do the above in phab now).
As I reported back in June, the overall LLVM open bug count increases
by an average of 4 per day. I don't know how many of the bugs that
stay open get comments, but this gives you a reasonable upper bound
on the size of this kind of weekly summary.
--paulr
I would like to add my support for this effort and take the opportunity to thank you Alex for not only offering to do something about this issue, but for all the incredibly valuable work you do with LLVM Weekly.
Although I keep a fairly close eye on llvm-dev, I frequently find stuff in LLVM Weekly that I find worth a closer look.
I am not sure how common this is, but I simply can’t keep up with llvm-commits, so I don’t try to. If I am not a subscriber/reviewer on a patch, I don’t know it exists. I believe the “Review Corner” would be an awesome addition to the summary you send out on Mondays.
One thing I’d like to bring up is the cadence of re-appearance of patches in the review corner. I believe it wouldn’t necessarily be useful to disqualify patches from future inclusion if they’ve been included before. OTOH, I think having the same patches there every week wouldn’t be very useful either.
Thanks once again for the work you do with LLVM Weekly.
Hi all. I'm assuming most people reading this email are familiar with LLVM's
code review process <LLVM Developer Policy — LLVM 18.0.0git documentation;
as well as LLVM Weekly, the development newsletter I've written and sent out
every Monday since Jan 2014. Since that time, it's provided something of a
"signal boost" for important mailing list discussions and commits. I feel it
could play a similar role in helping patches that are stuck waiting for code
reviews, or drawing attention to submissions from first time contributors.
There may be alternative or complementary approaches to tackling this
perceived problem we should discuss - I'm coming from a position of trying to
apply the tools I have at my disposal. Also see my previous thoughts on this
issue <http://lists.llvm.org/pipermail/llvm-dev/2016-November/106696.html>\.
I don't think it's controversial to suggest that while the code review process
works fantastically well a lot of the time, some patches fall through the
cracks and long delays in review feedback can put people off contributing to
LLVM. As was pointed out in response to the last RFC, very long review times
are problems for long-time contributors as well as newcomers.
My proposal is simple: add a new 'Review corner' section to LLVM Weekly to
help highlight patches that need more reviewer input. There are two main
categories I'd like to focus on:
1) patches from first-time contributors
2) patches where review activity has died off (i.e. they're 'stuck').
Thanks for your efforts to improve this process.
I played around with the Phabricator API a while ago (I think after a post where you suggested improving the experience for first-time contributors) and built a small script that identifies first-time contributors and adds myself as subscriber to those reviews.
I shared the script here ⚙ D37259 [Tools] Add script to identify new contributors from Phabricator (WIP) .
It would need some more work to turn it into a proper script, but I thought it would be a good time to share it now.
I think it should be relatively easy to find reviews where activity has died off using the API (excluding comments & patches submitted to llvm-commits directly).
My proposal is simple: add a new 'Review corner' section to LLVM Weekly to
help highlight patches that need more reviewer input. There are two main
categories I'd like to focus on:
1) patches from first-time contributors
2) patches where review activity has died off (i.e. they're 'stuck').
Thanks for your efforts to improve this process.
I played around with the Phabricator API a while ago (I think after a post
where you suggested improving the experience for first-time contributors)
and built a small script that identifies first-time contributors and adds
myself as subscriber to those reviews.
I shared the script here ⚙ D37259 [Tools] Add script to identify new contributors from Phabricator (WIP) .
It would need some more work to turn it into a proper script, but I thought
it would be a good time to share it now.
Thanks for sharing, this seems like a really useful starting point.
One potential direction would be a simple bot along the lines of the
rust-highfive which assigns at least an initial reviewer from a pool
of people hoping to help people get started contributing (see
for an example). In the last RFC thread, I think Paul Robinson called
these 'first responders'. While on the subject of Phabricator bots,
having one which complains when a patch is posted without llvm-commits
or cfe-commits subscribed would probably help avoid patches falling
through the cracks due to a simple oversight.
I think it should be relatively easy to find reviews where activity has died
off using the API (excluding comments & patches submitted to llvm-commits
directly).
I think using the API to find patches where activity has died off
might be useful for stats tracking. I'd rather rely on the patch
author choosing to make a submission for these stalled reviews though,
as I think part of the deal should be that the patch author asserts:
1) They're still interesting in landing the patch
2) They've ensured the patch applies cleanly on current LLVM/Clang
3) They've addressed any review comments they've received so far
My proposal is simple: add a new 'Review corner' section to LLVM Weekly to
help highlight patches that need more reviewer input. There are two main
categories I'd like to focus on:
1) patches from first-time contributors
2) patches where review activity has died off (i.e. they're 'stuck').
Thanks for your efforts to improve this process.
I played around with the Phabricator API a while ago (I think after a post
where you suggested improving the experience for first-time contributors)
and built a small script that identifies first-time contributors and adds
myself as subscriber to those reviews.
I shared the script here ⚙ D37259 [Tools] Add script to identify new contributors from Phabricator (WIP) .
It would need some more work to turn it into a proper script, but I thought
it would be a good time to share it now.
Thanks for sharing, this seems like a really useful starting point.
One potential direction would be a simple bot along the lines of the
rust-highfive which assigns at least an initial reviewer from a pool
of people hoping to help people get started contributing (see Fail ./x.py on invalid command by vorner · Pull Request #44134 · rust-lang/rust · GitHub
for an example). In the last RFC thread, I think Paul Robinson called
these 'first responders'. While on the subject of Phabricator bots,
having one which complains when a patch is posted without llvm-commits
or cfe-commits subscribed would probably help avoid patches falling
through the cracks due to a simple oversight.
Excellent points. It sounds like a few simple bots could make the process a little bit smoother. It would be great if we could agree if we want some "official" bots that live somewhere in the LLVM repository, to get started properly.
I think it should be relatively easy to find reviews where activity has died
off using the API (excluding comments & patches submitted to llvm-commits
directly).
I think using the API to find patches where activity has died off
might be useful for stats tracking. I'd rather rely on the patch
author choosing to make a submission for these stalled reviews though,
as I think part of the deal should be that the patch author asserts:
1) They're still interesting in landing the patch
2) They've ensured the patch applies cleanly on current LLVM/Clang
3) They've addressed any review comments they've received so far
Seeing as the idea got positive feedback, I'm going to trial it
starting from next week. Please submit patches which are awaiting
review here <http://llvmweekly.org/reviewcorner>\. Let's start simple
and see if it works. I've suggested the following guidelines for
submitting a code review thread for inclusion:
* The patch has gone at least two weeks without substantial review
feedback OR this is your first patch to an LLVM project
* You have updated the patch based on any review comments received so far
* The patch has been compile tested against the current HEAD of the
appropriate LLVM project, and rebased if necessary
* You are willing to write a short two-sentence summary that explains
1) what the patch does, and 2) why people might interested.
As you've hopefully seen, the first iteration of the "Review corner"
made its debut in Monday's LLVM Weekly: http://llvmweekly.org/issue/195
Four 'stuck' reviews were submitted and since then, 3/4 have seen
fresh review activity - thanks to Dimitry Andric, Matthias Braun, and
Anna Zaks. So far so good, but lets try and make it 4/4 (see https://reviews.llvm.org/D36357).
I haven't yet received any review submissions for next week, though
that may be because people prefer to wait until close to the deadline
(Sunday) to see if the review process moves forwards on its own. The
submission form is here LLVM Weekly - Review Corner