[llvm-dev] [RFC] Script to match open Phabricator reviews with potential reviewers

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] [RFC] Script to match open Phabricator reviews with potential reviewers

Dean Michael Berris via llvm-dev
Hi,

At the last EuroLLVM, I gave a lightning talk about code review
statistics on Phabricator reviews and what we could derive from that
to try and reduce waiting-for-review bottlenecks. (see

One of the items I pointed to is a script we've been using internally
for a little while to try and match open Phabricator reviews to people
who might be able to review them well. I received quite a few requests
to share that script, so I've decided to do so, see https://reviews.llvm.org/D46192.

The script uses 2 similar heuristics to try and match open reviews with
potential reviewers:

- If there is overlap between the lines of code touched by the
  patch-under-review and lines of code that a person has written, that
  person may be a good reviewer.
- If there is overlap between the files touched by the patch-under-review
  and the source files that a person has made changes to, that person may
  be a good reviewer.

The script provides a percentage for each of the above heuristics and
emails a summary. For example, this morning, I received the following
summary from the script in my inbox for patches-under-review where some
change was made in the past 24 hours:

SUMMARY FOR [hidden email] (found 8 reviews):
[3.37%/41.67%] https://reviews.llvm.org/D46018 '[GlobalISel][IRTranslator] Split aggregates during IR translation' by Amara Emerson
[0.00%/100.00%] https://reviews.llvm.org/D46111 '[ARM] Enable misched for R52.' by Dave Green
[0.00%/50.00%] https://reviews.llvm.org/D45770 '[AArch64] Disable spill slot scavenging when stack realignment required.' by Paul Walker
[0.00%/40.00%] https://reviews.llvm.org/D42759 '[CGP] Split large data structres to sink more GEPs' by Haicheng Wu
[0.00%/25.00%] https://reviews.llvm.org/D45189 '[MachineOutliner][AArch64] Keep track of functions that use a red zone in AArch64MachineFunctionInfo and use that instead of checking for noredzone in the MachineOutliner' by Jessica Paquette
[0.00%/25.00%] https://reviews.llvm.org/D46107 '[AArch64] Codegen for v8.2A dot product intrinsics' by Oliver Stannard
[0.00%/12.50%] https://reviews.llvm.org/D45541 '[globalisel] Update GlobalISel emitter to match new representation of extending loads' by Daniel Sanders
[0.00%/6.25%] https://reviews.llvm.org/D44386 '[x86] Introduce the pconfig/enclv instructions' by Gabor Buella


The first percentage in square brackers is the percentage of lines in
the patch-under-review that changes lines that I wrote. The second
percentage is the percentage of files that I made at least some
changes to out of all of the files touched by the patch-under-review.

Both the script and the heuristics are far from perfect, but I've
heard positive feedback from the few colleagues my script has been
sending a summary to every day - hearing that this does help them to
quickly find patches-under-review they can help to review.

Some more details are at https://reviews.llvm.org/D46192.

I hope that by sharing this, more and better ideas will arise to
automatically match open reviews with people able to review them.

Thanks,

Kristof


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] Script to match open Phabricator reviews with potential reviewers

Dean Michael Berris via llvm-dev
I just saw this, and I have to say -- thanks, Kristof!

Do you know if this is something that could be automated in
Phabricator, instead of something that people run on their own? Or is
the intent of this to be something that ran regularly (say, weekly or
daily) that would email people (or the list) that could be doing the
reviews for some of the open patches?

On Sat, Apr 28, 2018 at 1:01 AM, Kristof Beyls via llvm-dev
<[hidden email]> wrote:

> Hi,
>
> At the last EuroLLVM, I gave a lightning talk about code review
> statistics on Phabricator reviews and what we could derive from that
> to try and reduce waiting-for-review bottlenecks. (see
> https://llvm.org/devmtg/2018-04/talks.html#Lightning_2).
>
> One of the items I pointed to is a script we've been using internally
> for a little while to try and match open Phabricator reviews to people
> who might be able to review them well. I received quite a few requests
> to share that script, so I've decided to do so, see
> https://reviews.llvm.org/D46192.
>
> The script uses 2 similar heuristics to try and match open reviews with
> potential reviewers:
>
> - If there is overlap between the lines of code touched by the
>   patch-under-review and lines of code that a person has written, that
>   person may be a good reviewer.
> - If there is overlap between the files touched by the patch-under-review
>   and the source files that a person has made changes to, that person may
>   be a good reviewer.
>
> The script provides a percentage for each of the above heuristics and
> emails a summary. For example, this morning, I received the following
> summary from the script in my inbox for patches-under-review where some
> change was made in the past 24 hours:
>
> SUMMARY FOR [hidden email] (found 8 reviews):
> [3.37%/41.67%] https://reviews.llvm.org/D46018 '[GlobalISel][IRTranslator]
> Split aggregates during IR translation' by Amara Emerson
> [0.00%/100.00%] https://reviews.llvm.org/D46111 '[ARM] Enable misched for
> R52.' by Dave Green
> [0.00%/50.00%] https://reviews.llvm.org/D45770 '[AArch64] Disable spill slot
> scavenging when stack realignment required.' by Paul Walker
> [0.00%/40.00%] https://reviews.llvm.org/D42759 '[CGP] Split large data
> structres to sink more GEPs' by Haicheng Wu
> [0.00%/25.00%] https://reviews.llvm.org/D45189 '[MachineOutliner][AArch64]
> Keep track of functions that use a red zone in AArch64MachineFunctionInfo
> and use that instead of checking for noredzone in the MachineOutliner' by
> Jessica Paquette
> [0.00%/25.00%] https://reviews.llvm.org/D46107 '[AArch64] Codegen for v8.2A
> dot product intrinsics' by Oliver Stannard
> [0.00%/12.50%] https://reviews.llvm.org/D45541 '[globalisel] Update
> GlobalISel emitter to match new representation of extending loads' by Daniel
> Sanders
> [0.00%/6.25%] https://reviews.llvm.org/D44386 '[x86] Introduce the
> pconfig/enclv instructions' by Gabor Buella
>
>
> The first percentage in square brackers is the percentage of lines in
> the patch-under-review that changes lines that I wrote. The second
> percentage is the percentage of files that I made at least some
> changes to out of all of the files touched by the patch-under-review.
>
> Both the script and the heuristics are far from perfect, but I've
> heard positive feedback from the few colleagues my script has been
> sending a summary to every day - hearing that this does help them to
> quickly find patches-under-review they can help to review.
>
> Some more details are at https://reviews.llvm.org/D46192.
>
> I hope that by sharing this, more and better ideas will arise to
> automatically match open reviews with people able to review them.
>
> Thanks,
>
> Kristof
>
>
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>



--
Dean
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] Script to match open Phabricator reviews with potential reviewers

Dean Michael Berris via llvm-dev

> On 3 May 2018, at 01:48, Dean Michael Berris <[hidden email]> wrote:
>
> I just saw this, and I have to say -- thanks, Kristof!
>
> Do you know if this is something that could be automated in
> Phabricator, instead of something that people run on their own? Or is
> the intent of this to be something that ran regularly (say, weekly or
> daily) that would email people (or the list) that could be doing the
> reviews for some of the open patches?

I’m afraid I don’t know much about Phabricators internals.
That being said, I can’t think of a fundamental reason why this couldn’t be added to Phabricator, and indeed it does look like it might fit better integrated into Phabricator, rather than as a separate script that has to get to the data through the somewhat limited REST APIs of Phabricator.
It’d be great if someone with Phabricator implementation experience could share their thoughts on this.

In the mean while, I think the script as is is (very) experimental.
In other words, yes, I think that we could run it regularly as an llvm.org service.
But it may be better first that a few volunteers run it for their organizations and collect feedback on what seems to be useful about the suggestions made by the script and what doesn’t seem useful.
That way, if and when we turn this on as a community-wide llvm.org service, we’ll have hopefully cleaned up the majority of the roughest edges.

Thanks,

Kristof

>
> On Sat, Apr 28, 2018 at 1:01 AM, Kristof Beyls via llvm-dev
> <[hidden email]> wrote:
>> Hi,
>>
>> At the last EuroLLVM, I gave a lightning talk about code review
>> statistics on Phabricator reviews and what we could derive from that
>> to try and reduce waiting-for-review bottlenecks. (see
>> https://llvm.org/devmtg/2018-04/talks.html#Lightning_2).
>>
>> One of the items I pointed to is a script we've been using internally
>> for a little while to try and match open Phabricator reviews to people
>> who might be able to review them well. I received quite a few requests
>> to share that script, so I've decided to do so, see
>> https://reviews.llvm.org/D46192.
>>
>> The script uses 2 similar heuristics to try and match open reviews with
>> potential reviewers:
>>
>> - If there is overlap between the lines of code touched by the
>>  patch-under-review and lines of code that a person has written, that
>>  person may be a good reviewer.
>> - If there is overlap between the files touched by the patch-under-review
>>  and the source files that a person has made changes to, that person may
>>  be a good reviewer.
>>
>> The script provides a percentage for each of the above heuristics and
>> emails a summary. For example, this morning, I received the following
>> summary from the script in my inbox for patches-under-review where some
>> change was made in the past 24 hours:
>>
>> SUMMARY FOR [hidden email] (found 8 reviews):
>> [3.37%/41.67%] https://reviews.llvm.org/D46018 '[GlobalISel][IRTranslator]
>> Split aggregates during IR translation' by Amara Emerson
>> [0.00%/100.00%] https://reviews.llvm.org/D46111 '[ARM] Enable misched for
>> R52.' by Dave Green
>> [0.00%/50.00%] https://reviews.llvm.org/D45770 '[AArch64] Disable spill slot
>> scavenging when stack realignment required.' by Paul Walker
>> [0.00%/40.00%] https://reviews.llvm.org/D42759 '[CGP] Split large data
>> structres to sink more GEPs' by Haicheng Wu
>> [0.00%/25.00%] https://reviews.llvm.org/D45189 '[MachineOutliner][AArch64]
>> Keep track of functions that use a red zone in AArch64MachineFunctionInfo
>> and use that instead of checking for noredzone in the MachineOutliner' by
>> Jessica Paquette
>> [0.00%/25.00%] https://reviews.llvm.org/D46107 '[AArch64] Codegen for v8.2A
>> dot product intrinsics' by Oliver Stannard
>> [0.00%/12.50%] https://reviews.llvm.org/D45541 '[globalisel] Update
>> GlobalISel emitter to match new representation of extending loads' by Daniel
>> Sanders
>> [0.00%/6.25%] https://reviews.llvm.org/D44386 '[x86] Introduce the
>> pconfig/enclv instructions' by Gabor Buella
>>
>>
>> The first percentage in square brackers is the percentage of lines in
>> the patch-under-review that changes lines that I wrote. The second
>> percentage is the percentage of files that I made at least some
>> changes to out of all of the files touched by the patch-under-review.
>>
>> Both the script and the heuristics are far from perfect, but I've
>> heard positive feedback from the few colleagues my script has been
>> sending a summary to every day - hearing that this does help them to
>> quickly find patches-under-review they can help to review.
>>
>> Some more details are at https://reviews.llvm.org/D46192.
>>
>> I hope that by sharing this, more and better ideas will arise to
>> automatically match open reviews with people able to review them.
>>
>> Thanks,
>>
>> Kristof
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
>
>
> --
> Dean

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev