github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 05:50:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

⚡ Share one branch summary cache across all three readers

Browse Source 
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.
This commit is contained in:
Álvaro Tejero Cantero 2026-08-14 08:11:58 +02:00
parent c78e6f6eea
commit 74174deb0d
No known key found for this signature in database
3 changed files with 120 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

107
backend/src/app/rpc/commands/files_branch.clj
View File

@ -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)

34
backend/src/app/rpc/commands/files_pull_request.clj
View File

@ -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)

43
backend/test/backend_tests/rpc_file_pull_request_test.clj
View File

@ -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)