RFR: 2312: Do not require re-review for a simple merge [v2]

[Pavel Rappo]

On Wed, 10 Jul 2024 18:39:13 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewCoverage.java line 49:
>> 
>>> 47:     }
>>> 48: 
>>> 49:     public boolean covers(Review review, PullRequest pr) {
>> 
>> From what I can tell, this method is called twice for each Review in a CheckRun, once when generating the commit and once when generating the body. It makes several invocations of `git` in each call, some of which are clearly repeating between calls. We need to look at how we can cache the results to avoid bloating CheckRuns too much. Since the class is instantiated once for each CheckRun (and command), it should be fine to implement caching in the class itself for most of these calls, or rather for the result of a particular review hash. The pr head and target hash won't change during one WorkItem, so the only input value that differs between calls to one instance is the review hash. With that in mind, the pr should be a field rather than a method parameter.
>> 
>> I would really like to solve such performance issues up front for new features and avoid adding more unnecessary bloat that will need cleanup later.
>
> In fact, during a single invocation of `CheckRun.checkStatus`, `ReviewCoverage.covers` is called **four** times for the same review and PR. The first time to identify if a reviewer should be added to the "Reviewed-by" list in a commit message. The second time to identify if a review should be labelled with "Re-review required" or "Review applies to ..." in the "Reviewers" bulleted list in the PR body. The third and fourth times to decide whether the "Reviewed-by" list in the commit message should be amended:
> 
>   1. CheckRun:1329
>   2. CheckRun:1411
>   3. CheckRun:1416 (x 2)
> 
> FYI, when I introduced trivial caching, the savings in my test scenario were very modest. Without any cache, on my machine, `org.openjdk.skara.bots.pr.CheckTests#acceptSimpleMerges` on average took 18,166 ms to complete. With the cache, on the same machine, that same test took 17,784 ms. This is 2% improvement.
> 
> I haven't tried to address your other concern for `PullRequestUtils::targetHash` yet.

I tried to cache `targetHash`. The average time went down a bit more: now it's 17,627 ms. I'll push what I have, and you can see for yourself if you like it or not.

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1672#discussion_r1673137796