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

[Erik Joelsson]

On Wed, 10 Jul 2024 22:44:08 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> 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.

I agree that it may not amount to much, especially not in a local test where the Git repository is minimal. I would expect some of these operations to be noticeably more expensive in properly sized repositories like the JDK. It's also about not suffering from the death of a thousand cuts. It's very easy for improvements like this to grow resource usage over time. So I appreciate you making the effort to implement caching. At least this way we know we aren't adding an unreasonable amount of extra work to a CheckRun.

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

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