mirror of
https://github.com/penpot/penpot.git
synced 2026-10-29 14:00:37 +00:00
⚡ Make component sync about twice as fast
A profile of the benchmark worlds showed where a sync spends its time. - The token sync of shapes that apply no tokens took 38% of a sync in the 1000-copy world: for every synced shape, generate-update-tokens walked every sync attribute, resolved its group and its token attributes, and only then found that neither the main shape nor the copy applies a token. It now returns at once, and the token attributes of each sync attribute are computed once, at load. - A sync looks through every object of each page and component to find the copies it updates, so in a file with many shapes and few copies the search costs more than the sync itself. The new benchmark world main-edit-among-many-shapes syncs a main edit into 10 copies in a page with 20,000 plain rects; its baseline, measured before the change below, joins the recorded one with a note, and its result joins the golden master. generate-sync-container called the uses-assets? multimethod on every object through a lazy filter; the predicate is now chosen once per container (asset-user?) and the objects walked with reduce-kv into a vector, in the same order. - compare-children searched the remaining children (d/seek) for every child out of place, and dropped a child taken out of order with a lazy remove, so a reorder took quadratic time and nested one lazy sequence per child taken. It now looks children up by their reference or id (index-positions) and skips those taken out of order when the walk reaches them, with the same callbacks in the same order. The sync of a reorder stays slow: when a main with 1,000 children moves its first one to the end, each copy still gets 999 moves (F85), and applying them is what costs (4.75 s to 4.26 s for 10 copies). Fewer moves change what a sync writes, so that waits for Phase 4c. No behavior change: the golden masters and the suites are unchanged. Benchmark (lower quantile, ms; baseline / now / ratio): deepest-edit-through-three-levels 35.2 / 18.6 / 0.53 edit-through-a-chain-of-dependents 68.9 / 36.4 / 0.53 library-update-into-copies 83.1 / 43.0 / 0.52 main-edit-among-many-shapes 4.8 / 2.3 / 0.48 main-edit-into-10-copies 1.8 / 0.9 / 0.48 main-edit-into-100-copies 15.4 / 8.4 / 0.54 main-edit-into-1000-copies 153.1 / 90.1 / 0.59 variant-switch-into-copies 32.1 / 20.0 / 0.63 (main-edit-among-many-shapes: baseline measured before the search change.) AI-assisted-by: claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
alonso.torres
parent
64c666b336
commit
a8fefe3980
@ -59,7 +59,8 @@ its work.
|
||||
## Component sync benchmark
|
||||
|
||||
`common/dev/bench/component_sync.clj` times component sync on fixed-size worlds (one main into
|
||||
10/100/1000 copies, 3 nesting levels, a chain of dependents, a variant switch, a library update).
|
||||
10/100/1000 copies, 3 nesting levels, a chain of dependents, a variant switch, a library update,
|
||||
10 copies among 20000 plain shapes).
|
||||
Each world checks its result (how many rects end red) so it never times a no-op. Run from
|
||||
`common/`: `scripts/bench-components` compares with `dev/bench/component_sync_baseline.edn` and
|
||||
exits 1 when a world's lower quantile is more than 10% slower; `scripts/bench-components record`
|
||||
|
||||
@ -27,6 +27,10 @@
|
||||
[app.common.test-helpers.ids-map :as thi]
|
||||
[app.common.test-helpers.shapes :as ths]
|
||||
[app.common.test-helpers.variants :as thv]
|
||||
[app.common.types.file :as ctf]
|
||||
[app.common.types.pages-list :as ctpl]
|
||||
[app.common.types.shape :as cts]
|
||||
[app.common.uuid :as uuid]
|
||||
[clojure.edn :as edn]
|
||||
[clojure.java.io :as io]
|
||||
[clojure.java.shell :as shell]
|
||||
@ -110,6 +114,38 @@
|
||||
:parent-label root-label)
|
||||
(thc/make-component label root-label)))
|
||||
|
||||
(defn- add-plain-shapes
|
||||
"`file` with `n` plain rects at the root of its page, added in one step."
|
||||
[file n]
|
||||
(let [page (thf/current-page file)
|
||||
rects (map (fn [i]
|
||||
(cts/setup-shape {:type :rect :id (uuid/next) :name (str "Rect " i)
|
||||
:x i :y i :width 10 :height 10
|
||||
:parent-id uuid/zero :frame-id uuid/zero}))
|
||||
(range n))]
|
||||
(ctf/update-file-data
|
||||
file
|
||||
(fn [data]
|
||||
(ctpl/update-page data (:id page)
|
||||
(fn [page]
|
||||
(-> page
|
||||
(update :objects into (map (juxt :id identity)) rects)
|
||||
(update-in [:objects uuid/zero :shapes] into (map :id rects)))))))))
|
||||
|
||||
(defn- main-edit-among-many-shapes
|
||||
"One main whose child changes, synced into 10 copies in a page that also
|
||||
holds 20000 plain shapes, which the sync must look through."
|
||||
[]
|
||||
(let [[file ids] (-> (thf/sample-file :file1)
|
||||
(tho/add-simple-component :c0 :root0 :child0)
|
||||
(add-plain-shapes 20000)
|
||||
(instantiate-many :c0 "copy" 10)
|
||||
(edit :child0 red))]
|
||||
{:key :main-edit-among-many-shapes
|
||||
:describe "a main child changes; 10 copies among 20000 plain shapes sync"
|
||||
:run #(sync-cascade file {} ids)
|
||||
:reds 11}))
|
||||
|
||||
(defn- main-edit-into-copies
|
||||
"One main whose child changes, synced into `n` copies."
|
||||
[n]
|
||||
@ -219,7 +255,8 @@
|
||||
deepest-edit-through-three-levels
|
||||
edit-through-a-chain-of-dependents
|
||||
variant-switch-into-copies
|
||||
library-update-into-copies])
|
||||
library-update-into-copies
|
||||
main-edit-among-many-shapes])
|
||||
|
||||
(defn- worlds
|
||||
[]
|
||||
|
||||
@ -13,6 +13,10 @@
|
||||
{:mean-ms 75.119, :lower-q-ms 68.931, :passes 3},
|
||||
:library-update-into-copies
|
||||
{:mean-ms 89.295, :lower-q-ms 83.083, :passes 1},
|
||||
;; added in Phase 3 (2026-10-14), measured on the code before the object
|
||||
;; scan of a sync changed, on the same machine
|
||||
:main-edit-among-many-shapes
|
||||
{:mean-ms 5.465, :lower-q-ms 4.837, :passes 1},
|
||||
:main-edit-into-10-copies
|
||||
{:mean-ms 1.847, :lower-q-ms 1.79, :passes 1},
|
||||
:main-edit-into-100-copies
|
||||
|
||||
@ -91,7 +91,7 @@
|
||||
|
||||
(declare generate-sync-container)
|
||||
(declare generate-sync-shape)
|
||||
(declare uses-assets?)
|
||||
(declare asset-user?)
|
||||
|
||||
(declare generate-sync-shape-direct)
|
||||
(declare sync-shape-recursive)
|
||||
@ -187,8 +187,12 @@
|
||||
(container-log :debug (:id container) :msg "Sync page in local file" :page-id (:id container))
|
||||
(container-log :debug (:id container) :msg "Sync component in local library" :component-id (:id container)))
|
||||
|
||||
(let [linked-shapes (->> (vals (:objects container))
|
||||
(filter #(uses-assets? asset-type asset-id % library-id)))]
|
||||
(let [asset-user? (asset-user? asset-type asset-id library-id)
|
||||
linked-shapes (persistent!
|
||||
(reduce-kv (fn [shapes _ shape]
|
||||
(cond-> shapes (asset-user? shape) (conj! shape)))
|
||||
(transient [])
|
||||
(:objects container)))]
|
||||
(loop [shapes (seq linked-shapes)
|
||||
changes (-> changes
|
||||
(pcb/with-container container)
|
||||
@ -210,27 +214,21 @@
|
||||
changes)))
|
||||
changes))))
|
||||
|
||||
(defmulti uses-assets?
|
||||
"Checks if a shape uses some asset of the given type in the given library."
|
||||
(fn [asset-type _ _ _] asset-type))
|
||||
|
||||
(defmethod uses-assets? :components
|
||||
[_ component-id shape library-id]
|
||||
(if (nil? component-id)
|
||||
(ctk/uses-library-components? shape library-id)
|
||||
(ctk/instance-of? shape library-id component-id)))
|
||||
|
||||
(defmethod uses-assets? :colors
|
||||
[_ color-id shape library-id]
|
||||
(if (nil? color-id)
|
||||
(cts/uses-library-colors? shape library-id)
|
||||
(cts/uses-library-color? shape library-id color-id)))
|
||||
|
||||
(defmethod uses-assets? :typographies
|
||||
[_ typography-id shape library-id]
|
||||
(if (nil? typography-id)
|
||||
(cty/uses-library-typographies? shape library-id)
|
||||
(cty/uses-library-typography? shape library-id typography-id)))
|
||||
(defn- asset-user?
|
||||
"A predicate of the shapes that use the asset `asset-id` of type
|
||||
`asset-type` in the library `library-id`, or any asset of that type in it
|
||||
when `asset-id` is nil."
|
||||
[asset-type asset-id library-id]
|
||||
(case asset-type
|
||||
:components (if (nil? asset-id)
|
||||
#(ctk/uses-library-components? % library-id)
|
||||
#(ctk/instance-of? % library-id asset-id))
|
||||
:colors (if (nil? asset-id)
|
||||
#(cts/uses-library-colors? % library-id)
|
||||
#(cts/uses-library-color? % library-id asset-id))
|
||||
:typographies (if (nil? asset-id)
|
||||
#(cty/uses-library-typographies? % library-id)
|
||||
#(cty/uses-library-typography? % library-id asset-id))))
|
||||
|
||||
(defmulti generate-sync-shape
|
||||
"Generate changes to synchronize one shape from all assets of the given type
|
||||
@ -716,6 +714,15 @@
|
||||
|
||||
;; ---- Operation generation helpers ----
|
||||
|
||||
(defn- index-positions
|
||||
"A map of each value of `key-fn` among the elements of the vector `v` to
|
||||
their positions, in order."
|
||||
[key-fn v]
|
||||
(reduce-kv (fn [index position x]
|
||||
(update index (key-fn x) (fnil conj []) position))
|
||||
{}
|
||||
v))
|
||||
|
||||
(defn- compare-children
|
||||
"Pair the children of a copy shape with the children of its main and call
|
||||
the callback of each case. A child pairs with the main child it refers to;
|
||||
@ -723,7 +730,12 @@
|
||||
`both-cb` in a reset, and does not pair in an inverse sync, so the main
|
||||
child it replaces is removed and the swapped child is added in its place.
|
||||
An inverse sync (which passes `reset?`) never calls `swapped-cb`, so it
|
||||
passes nil."
|
||||
passes nil.
|
||||
|
||||
The children are walked in order, side by side. A child out of place is
|
||||
looked up by its reference (`index-positions`), not searched, so the walk
|
||||
takes linear time; a child taken out of order is skipped when the walk
|
||||
reaches it."
|
||||
[changes shape-inst children-inst children-main container-inst only-inst-cb only-main-cb both-cb swapped-cb moved-cb inverse? reset?]
|
||||
(shape-log :trace (:id shape-inst) container-inst :msg "Compare children")
|
||||
(let [pair?
|
||||
@ -734,12 +746,48 @@
|
||||
synced-pair?
|
||||
(fn [child-main child-inst]
|
||||
(and (ctk/is-main-of? child-main child-inst)
|
||||
(or reset? (not (ctk/swapped? child-inst)))))]
|
||||
(loop [children-inst (seq (or children-inst []))
|
||||
children-main (seq (or children-main []))
|
||||
changes changes]
|
||||
(let [child-inst (first children-inst)
|
||||
child-main (first children-main)]
|
||||
(or reset? (not (ctk/swapped? child-inst)))))
|
||||
|
||||
insts (vec children-inst)
|
||||
mains (vec children-main)
|
||||
insts-by-ref (index-positions :shape-ref insts)
|
||||
mains-by-id (index-positions :id mains)
|
||||
|
||||
;; the first position from `position` on whose child was not taken
|
||||
;; out of order
|
||||
next-position
|
||||
(fn [v position taken]
|
||||
(loop [position position]
|
||||
(if (and (< position (count v))
|
||||
(contains? taken (:id (nth v position))))
|
||||
(recur (inc position))
|
||||
position)))
|
||||
|
||||
;; the first child from `position` on, with `key` in `index`, not
|
||||
;; taken and accepted by `pred`
|
||||
find-child
|
||||
(fn [index key v position taken pred]
|
||||
(some (fn [p]
|
||||
(let [child (nth v p)]
|
||||
(when (and (>= p position)
|
||||
(not (contains? taken (:id child)))
|
||||
(pred child))
|
||||
child)))
|
||||
(get index key)))
|
||||
|
||||
remaining
|
||||
(fn [v position taken]
|
||||
(remove #(contains? taken (:id %)) (subvec v (min position (count v)))))]
|
||||
|
||||
(loop [i 0
|
||||
j 0
|
||||
taken-insts #{}
|
||||
taken-mains #{}
|
||||
changes changes]
|
||||
(let [i (next-position insts i taken-insts)
|
||||
j (next-position mains j taken-mains)
|
||||
child-inst (get insts i)
|
||||
child-main (get mains j)]
|
||||
(shape-log :trace (:id shape-inst) container-inst
|
||||
:msg "Comparing"
|
||||
:main (str (:name child-main) " " (pretty-uuid (:id child-main)))
|
||||
@ -749,37 +797,47 @@
|
||||
changes
|
||||
|
||||
(nil? child-inst)
|
||||
(reduce only-main-cb changes children-main)
|
||||
(reduce only-main-cb changes (remaining mains j taken-mains))
|
||||
|
||||
(nil? child-main)
|
||||
(reduce only-inst-cb changes children-inst)
|
||||
(reduce only-inst-cb changes (remaining insts i taken-insts))
|
||||
|
||||
:else
|
||||
(if (pair? child-main child-inst)
|
||||
(recur (next children-inst)
|
||||
(next children-main)
|
||||
(recur (inc i)
|
||||
(inc j)
|
||||
taken-insts
|
||||
taken-mains
|
||||
(if (synced-pair? child-main child-inst)
|
||||
(both-cb changes child-inst child-main)
|
||||
(swapped-cb changes child-inst child-main)))
|
||||
|
||||
(let [child-inst' (d/seek #(pair? child-main %) children-inst)
|
||||
child-main' (d/seek #(pair? % child-inst) children-main)]
|
||||
(let [child-inst' (find-child insts-by-ref (:id child-main) insts i taken-insts
|
||||
#(pair? child-main %))
|
||||
child-main' (find-child mains-by-id (:shape-ref child-inst) mains j taken-mains
|
||||
#(pair? % child-inst))]
|
||||
(cond
|
||||
(nil? child-inst')
|
||||
(recur children-inst
|
||||
(next children-main)
|
||||
(recur i
|
||||
(inc j)
|
||||
taken-insts
|
||||
taken-mains
|
||||
(only-main-cb changes child-main))
|
||||
|
||||
(nil? child-main')
|
||||
(recur (next children-inst)
|
||||
children-main
|
||||
(recur (inc i)
|
||||
j
|
||||
taken-insts
|
||||
taken-mains
|
||||
(only-inst-cb changes child-inst))
|
||||
|
||||
:else
|
||||
(if inverse?
|
||||
(let [synced? (synced-pair? child-main' child-inst)]
|
||||
(recur (next children-inst)
|
||||
(remove #(= (:id %) (:id child-main')) children-main)
|
||||
(recur (inc i)
|
||||
j
|
||||
taken-insts
|
||||
(conj taken-mains (:id child-main'))
|
||||
(cond-> changes
|
||||
synced?
|
||||
(both-cb child-inst child-main')
|
||||
@ -788,8 +846,10 @@
|
||||
:always
|
||||
(moved-cb child-inst child-main'))))
|
||||
(let [synced? (synced-pair? child-main child-inst')]
|
||||
(recur (remove #(= (:id %) (:id child-inst')) children-inst)
|
||||
(next children-main)
|
||||
(recur i
|
||||
(inc j)
|
||||
(conj taken-insts (:id child-inst'))
|
||||
taken-mains
|
||||
(cond-> changes
|
||||
synced?
|
||||
(both-cb child-inst' child-main)
|
||||
@ -1208,37 +1268,43 @@
|
||||
:ignore-touched true}]}))
|
||||
(pcb/apply-changes-local)))))
|
||||
|
||||
;; The token attributes of each attribute whose tokens sync; the
|
||||
;; `layout-grid-cells` are not synced automatically.
|
||||
(def ^:private token-attrs-by-sync-attr
|
||||
(into {}
|
||||
(comp (remove #(= :layout-grid-cells %))
|
||||
(map (juxt identity cto/shape-attr->token-attrs)))
|
||||
(keys ctk/sync-attrs)))
|
||||
|
||||
(defn generate-update-tokens
|
||||
"Copy into `dest-shape` the tokens `origin-shape` applies to the attributes
|
||||
it syncs: with `omit-touched?`, only those whose group is not in `touched`;
|
||||
with `valid-attrs`, only those attributes (all when nil)."
|
||||
with `valid-attrs`, only those attributes (all when nil). Nothing to copy
|
||||
or remove when neither shape applies tokens."
|
||||
[changes container dest-shape origin-shape touched omit-touched? valid-attrs]
|
||||
(let [attrs
|
||||
(->> (keys ctk/sync-attrs)
|
||||
;; We don't update the flex-child attrs
|
||||
(remove #(= :layout-grid-cells %)))
|
||||
(if (and (empty? (:applied-tokens origin-shape))
|
||||
(empty? (:applied-tokens dest-shape)))
|
||||
changes
|
||||
(let [;; the styles of a text live in its content, so its tokens follow
|
||||
;; the text override groups
|
||||
text? (cfh/text-shape? dest-shape)
|
||||
touched (cond-> touched
|
||||
text? (-> set (into (txt/text-overrides touched))))
|
||||
|
||||
;; the styles of a text live in its content, so its tokens follow
|
||||
;; the text override groups
|
||||
text? (cfh/text-shape? dest-shape)
|
||||
touched (cond-> touched
|
||||
text? (-> set (into (txt/text-overrides touched))))
|
||||
|
||||
applied-tokens
|
||||
(reduce (fn [applied-tokens attr]
|
||||
(let [sync-group (or (when text? (txt/style-unit-of attr))
|
||||
(ctk/resolve-sync-group (:type origin-shape) attr)
|
||||
(ctk/resolve-sync-group (:type dest-shape) attr))
|
||||
token-attrs (cto/shape-attr->token-attrs attr)]
|
||||
(if (and (or (not omit-touched?) (not (touched sync-group)))
|
||||
(or (empty? valid-attrs) (contains? valid-attrs attr)))
|
||||
(into applied-tokens token-attrs)
|
||||
applied-tokens)))
|
||||
#{}
|
||||
attrs)]
|
||||
(cond-> changes
|
||||
(seq applied-tokens)
|
||||
(update-tokens container dest-shape origin-shape applied-tokens))))
|
||||
applied-tokens
|
||||
(reduce-kv (fn [applied-tokens attr token-attrs]
|
||||
(let [sync-group (or (when text? (txt/style-unit-of attr))
|
||||
(ctk/resolve-sync-group (:type origin-shape) attr)
|
||||
(ctk/resolve-sync-group (:type dest-shape) attr))]
|
||||
(if (and (or (not omit-touched?) (not (touched sync-group)))
|
||||
(or (empty? valid-attrs) (contains? valid-attrs attr)))
|
||||
(into applied-tokens token-attrs)
|
||||
applied-tokens)))
|
||||
#{}
|
||||
token-attrs-by-sync-attr)]
|
||||
(cond-> changes
|
||||
(seq applied-tokens)
|
||||
(update-tokens container dest-shape origin-shape applied-tokens)))))
|
||||
|
||||
(defn add-update-attr-changes
|
||||
"Add to `changes` the operations of `attr-operations` on `dest-shape`, and
|
||||
|
||||
@ -134,6 +134,8 @@
|
||||
{:transcript "create-component {:component \"main\", :id sync-main-component}\n make-variant-container {:variant-set \"vset\", :variant-id sync-vset-vcontainer, :count 4}\n make-nested-component-with-variant {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n make-nested-component {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n make-nested-component {:component \"main\", :level 2, :outer sync-main-outercomp-2}\n one-of -> skip\n switch-variant {:target-component sync-vset-v2-component}\n one-of -> skip\n test", :states ["43b4a591dc02b01b" "d844ece54beb0bfe"]}
|
||||
{:transcript "create-component {:component \"main\", :id sync-main-component}\n make-variant-container {:variant-set \"vset\", :variant-id sync-vset-vcontainer, :count 4}\n make-nested-component-with-variant {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n make-nested-component {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n make-nested-component {:component \"main\", :level 2, :outer sync-main-outercomp-2}\n one-of -> skip\n one-of -> skip\n switch-variant {:target-component sync-vset-v3-component}\n test", :states ["4e56e67caa87f262" "85d6d4a265a35a44"]}
|
||||
{:transcript "create-component {:component \"main\", :id sync-main-component}\n make-variant-container {:variant-set \"vset\", :variant-id sync-vset-vcontainer, :count 4}\n make-nested-component-with-variant {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n make-nested-component {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n make-nested-component {:component \"main\", :level 2, :outer sync-main-outercomp-2}\n one-of -> skip\n one-of -> skip\n one-of -> skip\n test", :states ["a3344fab60ad0124"]}]
|
||||
:main-edit-among-many-shapes
|
||||
[{:transcript "main-edit-among-many-shapes", :states ["ddbd413ae850e029"]}]
|
||||
:main-edit-into-10-copies
|
||||
[{:transcript "main-edit-into-10-copies", :states ["55176ea7a3052277"]}]
|
||||
:main-edit-into-100-copies
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user