From 74174deb0d408d3859f61b460d80d8d1f7849fd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Fri, 14 Aug 2026 08:11:58 +0200 Subject: [PATCH] :zap: Share one branch summary cache across all three readers The pull-request listing and the file-open info query each recomputed the same [ahead behind conflicts] the branches listing had already cached, so a file with ten open pull requests paid the whole cold cost the branch panel no longer pays. All three now go through cached-diff-counts, keyed on the same (base-snapshot-id, source-revn, branch-revn) tuple, and none of them realizes main when every row it needs is cached. --- backend/src/app/rpc/commands/files_branch.clj | 107 +++++++++--------- .../app/rpc/commands/files_pull_request.clj | 34 ++++-- .../rpc_file_pull_request_test.clj | 43 +++++++ 3 files changed, 120 insertions(+), 64 deletions(-) diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 650fc021bf..db946f09c0 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -587,20 +587,6 @@ ;; conflicts are symmetric; only possible when both sides diverged (if (and (pos? ahead-revn) (pos? behind-revn)) (count (:conflicts fwd)) 0)]))) -(defn branch-diff-counts - "The counts of `branch-diff-counts!`, degrading to zeros with a warning - when the base snapshot cannot be resolved: this is a read-only listing, - and merge/update DO refuse loudly in that situation (`get-base-data`)." - [cfg main-data branch] - (try - (branch-diff-counts! cfg main-data branch) - (catch Throwable cause - (l/wrn :hint "unable to compute branch diff counts" - :branch-file-id (str (:branch-file-id branch)) - :source-file-id (str (:source-file-id branch)) - :cause cause) - [0 0 0]))) - (def ^:private branch-summary-cache "The per-branch `[ahead behind conflicts]` summary, keyed by `(base-snapshot-id, source-revn, branch-revn)` — exactly the inputs the @@ -611,13 +597,51 @@ The window is a working day rather than minutes because the fetch this exists for is a file open: an entry is a uuid and five numbers, so keeping it costs nothing, and a shorter window pays the whole cold cost - again for a designer who comes back after lunch." + again for a designer who comes back after lunch. + + Both surfaces reporting these numbers share it: the branches listing and + the pull-request listing ask the same question about the same pair, so + whichever runs first warms the other." (ucache/create :max-size 8192 :keepalive "8h")) (defn- summary-cache-key [{:keys [base-snapshot-id source-revn branch-revn]}] [base-snapshot-id source-revn branch-revn]) +(defn cached-summary + "The cached summary of a branch row, or nil when it would have to be + computed. Public because a listing decides whether to realize main + before it pays for it, and a listing whose rows are all cached needs no + main at all." + [row] + (ucache/get branch-summary-cache (summary-cache-key row))) + +(defn cached-diff-counts + "`branch-diff-counts!` behind the summary cache, degrading to zeros with + a warning when the base snapshot cannot be resolved: a listing is + read-only, and merge/update DO refuse loudly in that situation + (`get-base-data`). A value computed from an error never enters the cache. + + `branch` carries the key tuple and the pair being compared, with + `source-file-id` naming main and `branch-file-id` the branch, whichever + listing is asking." + [cfg main-data branch] + (let [[ahead-revn behind-revn] (revn-deltas branch)] + (try + (ucache/get branch-summary-cache + (summary-cache-key branch) + (fn [_] + (branch-diff-counts! cfg main-data + (assoc branch + :ahead-revn ahead-revn + :behind-revn behind-revn)))) + (catch Throwable cause + (l/wrn :hint "unable to compute branch diff counts" + :branch-file-id (str (:branch-file-id branch)) + :source-file-id (str (:source-file-id branch)) + :cause cause) + [0 0 0])))) + (sv/defmethod ::get-file-branches "List the branches of a file. `ahead`/`behind` are entity-level change counts (matching the compare dialog), gated by the cheap revn deltas so @@ -627,7 +651,9 @@ The per-branch summary is cached by `(base-snapshot-id, source-revn, branch-revn)`: a repeated listing with no intervening save performs no comparison work, and the tuple names exactly the inputs the computation - reads, so a stale key cannot exist and the cache needs no invalidation." + reads, so a stale key cannot exist and the cache needs no invalidation. + The pull-request listing reads the same cache, so either surface can + arrive warm." {::doc/added "2.16" ::sm/params schema:get-file-branches} [cfg {:keys [::rpc/profile-id file-id include-archived]}] @@ -640,38 +666,17 @@ ;; are not going to be merged as-is, so the expensive diff ;; would be wasted work (and their base may be released) open? (fn [row] (= "open" (:status row))) - cached (fn [row] (ucache/get branch-summary-cache (summary-cache-key row))) ;; realize main once, only if some open branch diverged ;; AND its summary is not cached need? (some (fn [r] (and (open? r) - (nil? (cached r)) + (nil? (cached-summary r)) (let [[a b] (revn-deltas r)] (or (pos? a) (pos? b))))) rows) - main-data (when need? (:data (bfc/get-file cfg file-id :realize? true))) - counts (fn [row] - (let [k (summary-cache-key row) - [ahead-revn behind-revn] (revn-deltas row)] - (or (ucache/get branch-summary-cache k) - (try - (ucache/get branch-summary-cache - k - (fn [_] - (branch-diff-counts! - cfg main-data - {:id (:id row) - :source-file-id file-id - :branch-file-id (:branch-file-id row) - :base-snapshot-id (:base-snapshot-id row) - :ahead-revn ahead-revn - :behind-revn behind-revn}))) - (catch Throwable cause - (l/wrn :hint "unable to compute branch diff counts" - :branch-file-id (str (:branch-file-id row)) - :source-file-id (str file-id) - :cause cause) - [0 0 0])))))] + main-data (when need? (:data (bfc/get-file cfg file-id :realize? true)))] (mapv (fn [row] - (let [[ahead behind conflicts] (if (open? row) (counts row) [0 0 0])] + (let [[ahead behind conflicts] (if (open? row) + (cached-diff-counts cfg main-data row) + [0 0 0])] (-> row (assoc :ahead ahead :behind behind :conflicts conflicts) (dissoc :branch-revn :source-revn :base-snapshot-id :base-branch-revn)))) @@ -1315,21 +1320,19 @@ (db/run! cfg (fn [{:keys [::db/conn] :as cfg}] (files/check-read-permissions! cfg profile-id file-id) - (when-let [{:keys [base-snapshot-id source-file-id status] :as row} + (when-let [{:keys [source-file-id status] :as row} (db/exec-one! conn [sql:get-file-branch-info file-id])] ;; `ahead`/`behind` count actual entity-level changes (what ;; the compare dialog lists), NOT the raw revn delta — a ;; single edit can span many save-revns and noise is filtered. - (let [[ahead-revn behind-revn] (revn-deltas row) - [ahead behind conflicts] + ;; The frontend asks this on every file open, so it reads the + ;; same summary cache as the two listings. + (let [[ahead behind conflicts] (if (= "open" status) - (branch-diff-counts cfg nil - {:id (:id row) - :source-file-id source-file-id - :branch-file-id file-id - :base-snapshot-id base-snapshot-id - :ahead-revn ahead-revn - :behind-revn behind-revn}) + (cached-diff-counts cfg nil + (assoc row + :source-file-id source-file-id + :branch-file-id file-id)) [0 0 0])] (-> row (assoc :ahead ahead :behind behind :conflicts conflicts) diff --git a/backend/src/app/rpc/commands/files_pull_request.clj b/backend/src/app/rpc/commands/files_pull_request.clj index 9b214a4491..b041f0570b 100644 --- a/backend/src/app/rpc/commands/files_pull_request.clj +++ b/backend/src/app/rpc/commands/files_pull_request.clj @@ -159,22 +159,24 @@ "Attach the derived attributes the UI needs: reviews (+ aggregate `review-state`), `outdated` (the branch moved past the review snapshot, cheap revn gate) and entity-level ahead/behind/conflicts counts against - main (only for open pull requests, gated like the branches listing)." - [cfg main-data reviews-map {:keys [id status source-file-id target-file-id - base-snapshot-id review-revn branch-revn] + main (only for open pull requests, gated like the branches listing). + + The counts come from the branch summary cache, keyed on the same tuple + the branches listing uses, so ten open pull requests on one file cost + one computation per branch and nothing at all once that panel has been + opened." + [cfg main-data reviews-map {:keys [id status file-branch-id source-file-id target-file-id + review-revn branch-revn] :as row}] (let [reviews (get reviews-map id []) open? (= "open" status) - [ahead-revn behind-revn] (fbranch/revn-deltas row) [ahead behind conflicts] (if open? - (fbranch/branch-diff-counts cfg main-data - {:id id - :source-file-id target-file-id - :branch-file-id source-file-id - :base-snapshot-id base-snapshot-id - :ahead-revn ahead-revn - :behind-revn behind-revn}) + (fbranch/cached-diff-counts cfg main-data + (assoc row + :id file-branch-id + :source-file-id target-file-id + :branch-file-id source-file-id)) [0 0 0])] (-> row (assoc :reviews (mapv #(dissoc % :pull-request-id) reviews)) @@ -401,7 +403,12 @@ pull request is annotated with its reviews, the aggregate `review-state`, the `outdated` flag and the entity-level ahead/behind/conflicts counts (gated by the cheap revn deltas, with - main realized once and shared, like the branches listing)." + main realized once and shared, like the branches listing). + + Those counts come from the branch summary cache that the branches + listing fills, keyed on the same tuple: a repeated listing with no + intervening save performs no comparison work and realizes no file, and + ten open pull requests on one branch pay for one computation." {::doc/added "2.16" ::sm/params schema:get-file-pull-requests} [cfg {:keys [::rpc/profile-id file-id include-closed]}] @@ -415,7 +422,10 @@ (boolean include-closed)]) reviews-map (get-reviews-map cfg rows) open? (fn [row] (= "open" (:status row))) + ;; realize main once, and only for a diverged pull + ;; request whose summary is not already cached need? (some (fn [r] (and (open? r) + (nil? (fbranch/cached-summary r)) (let [[a b] (fbranch/revn-deltas r)] (or (pos? a) (pos? b))))) rows) diff --git a/backend/test/backend_tests/rpc_file_pull_request_test.clj b/backend/test/backend_tests/rpc_file_pull_request_test.clj index 65bfcf8d1f..375faa066b 100644 --- a/backend/test/backend_tests/rpc_file_pull_request_test.clj +++ b/backend/test/backend_tests/rpc_file_pull_request_test.clj @@ -7,6 +7,7 @@ (ns backend-tests.rpc-file-pull-request-test (:require [app.common.features :as cfeat] + [app.common.files.branch-merge :as bm] [app.common.time :as ct] [app.common.uuid :as uuid] [app.config :as cf] @@ -538,6 +539,48 @@ (t/is (= 1 (count (:result out)))) (t/is (= "closed" (-> out :result first :status)))))))) +(t/deftest pull-request-listing-shares-the-branch-summary-cache + (with-redefs [cf/flags pr-flags] + (let [author (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id author) + :project-id (:default-project-id author)}) + branch (create-branch* author file "feature") + branches (fn [] (:result (th/command! {::th/type :get-file-branches + ::rpc/profile-id (:id author) + :file-id (:id file)}))) + prs (fn [] (:result (th/command! {::th/type :get-file-pull-requests + ::rpc/profile-id (:id author) + :file-id (:id file)}))) + summary (fn [row] (select-keys row [:ahead :behind :conflicts]))] + + ;; diverge both sides so the comparison is real work + (add-color* author (:branch-file-id branch) "Brand") + (add-color* author (:id file) "Accent") + (t/is (nil? (:error (create-pr* author (:id branch) {})))) + + (let [orig-merge bm/compute-merge + calls (atom 0)] + (with-redefs [bm/compute-merge (fn [& args] (swap! calls inc) (apply orig-merge args))] + (let [cold-branch (first (branches)) + cold-calls @calls + _ (reset! calls 0) + warm-pr (first (prs)) + pr-calls @calls + _ (reset! calls 0) + repeat-pr (first (prs))] + + (t/testing "the branches listing pays for the comparison" + (t/is (pos? cold-calls)) + (t/is (= {:ahead 1 :behind 1 :conflicts 0} (summary cold-branch)))) + + (t/testing "the pull-request listing arrives warm and agrees" + (t/is (zero? pr-calls)) + (t/is (= (summary cold-branch) (summary warm-pr)))) + + (t/testing "and a repeat of it performs no comparison work either" + (t/is (zero? @calls)) + (t/is (= (summary warm-pr) (summary repeat-pr)))))))))) + (t/deftest branching-flag-gates-pull-request-commands (let [author (th/create-profile* 1 {:is-active true}) file (th/create-file* 1 {:profile-id (:id author)