I56ae707c changed the behavior of how approvals get copied. Prior to
that commit, Gerrit would only copy approvals of the prior patch set.
The commit changed the behavior to investigate all prior patch sets. The
benefit is that in the following situation, PS3 will get approvals
copied:
- PS1
- PS2 - REWORK of PS1
- PS3 - REWORK of PS2, Trivial rebase of PS1
Unfortunately, the change introduced a serious performance regression
for Git pushes to Gerrit and change edits. The effort of computing the
change kind for a new patch set was increased from O(1) to
O(num-prior-patch-sets).
Computing the change kind is a potentially (especially distinguishing
between TRIVIAL_REBASE and MERGE_FIRST_PARENT_UPDATE) long-running and
computationally intensive operation. For example, on chromium/src
hosted on googlesource.com its p50 is 2.2 seconds. Uploading patch set
10 of a change means that Gerrit will spend 9x2.2s ~ 20s computing
change kind while loading approvals for the current patch set.
The use case metioned in I56ae707c covers rare edge cases. There is no
documentation that indicates that Gerrit even supports these. The
performance penalty on the other side is real and applies to the vast
majority of Git pushes to Gerrit.
Irrespective of this change, we can think about cases to make
change_kind cheaper to compute, but even with optimizations, we have to
eliminate the number of patch sets being a complexity factor.
NoteDb doesn't store copied approvals. There is an argument to be made
that it should. If NoteDb would store copied approvals, we would have to
make the same tradeoff (consider only n and n-1) as this commit makes.
Storing copied approvals in NoteDb seems worthwhile, but is out of the
scope of this commit.
After this commit, the computation cost will still be O(num-patch-sets),
but through changing the ChangeKind cache access pattern, we scale down
the number of ChangeKind computations from O(num-patch-sets) to O(1)
assuming the cache is ephemeral. Storing approvals in NoteDb would allow
us to cut the rest of the computation to O(1) as well. Since all of it
happens in memory though, the cost is not as high.
This commit preserves the behavior that approvals are computed on the
fly. So if a project's config changes the behavior of when approvals get
copied, that will affect all changes immediately. It's debatable if this
aligns with intention as it just does half the job: It does affect
changes that users look at, but not values indexed in the ChangeIndex.
Changing this behavior is out of the scope of this commit.
There is test coverage for approval copying in StickyApprovalsIT.
This test adds a test to ensure we are not regressing. It would be
better if the production logic would ensure that with checkState calls,
but since ChangeKindCache deals with Git commits, that would at least
force us to load patch sets there which seems undesirable for now.
Change-Id: I34157a6bed52418536a417421a7127423619596b
da70bea56d