commit 541ac10c62fbfa3ad2204e9171c99c499ede46e7
author Youssef Elghareeb <ghareeb@google.com> Thu Jun 24 17:49:30 2021 +0200
committer Youssef Elghareeb <ghareeb@google.com> Fri Jun 25 15:11:14 2021 +0200
tree 9152df5d5495e7a4ecd7a896b92ea3558526858e
parent 42cf030d06171ec1f595da24b7cec3648f64b002

DiffOperations: Move the timeout logic inside the diff cache

In this change we do the following:

1) We move the timeout logic from DiffOperations to the
GitFileDiffCache. The timeout is enforced while computing the file
header (similar to the old diff cache, see [2]). If the computation
times out, we cache negative results. We enforce the timeout with a
boolean variable (see (2)). The FileDiffCache (which wraps the
GitFileDiffCache and performs extra logic, e.g. computing rebase edits)
will also cache negative results.

2) Add an extra boolean field to the GitFileDiffCacheKey and the
FileDiffCacheKey called "useTimeout". We only enforce timeouts if the
diff algorithm uses the Myers diff fallback.

The new logic now goes as follows: DiffOperations requests diffs using
the HISTOGRAM_WITH_MYERS algorithm and with useTimeout=true. If we get
back negative results (meaning that the computation times out), we
request the diffs again using the HISTOGRAM_NO_FALLBACK and with
useTimeouts=false (we don't want to enforce timeouts in this case). This
logic is matching what was done in old diff cache.

The reasonning behind this change is the following:

* The timeout logic in the old diff cache was added in the past (many
years ago) because of a bug in the Myers diff algorithm in JGit that
causes an infinite loop. See Ib00de070 and [1]. It looks like the bug
isn't fixed yet. The timeout was only employed while computing the file
header (see [2]) so was not enforced on the whole diff computation. [1]
suggets that if the JGit bug in Myers diff is fixed then we can remove
the timeouts in Gerrit, i.e. we did introduce the timeout logic in
Gerrit as a workaround for this bug.

* It wasn't a good idea to keep the timeout logic in DiffOperations:
This caused the @DiffExecutor to create new threads with every request
to get the diff, whether it's cached or not, and caused an overload to
the diff executor. With the logic of this change, once the result is
cached (whether positive or negative), we will not have to go through
the cache loader (or the timeout) logic again.

[1] bugs.chromium.org/p/gerrit/issues/detail?id=487
[2] https://gerrit.googlesource.com/gerrit/+/0dc27d2a5b512fdcb7ce5f95b412e174fe33c7d2/java/com/google/gerrit/server/patch/filediff/PatchListLoader.java#530

Change-Id: I680454cf93f78b90025bab95ed3717c74dbf32bf