Skip to content

Commit 2e48305

Browse files
Avoid redundant anchor lookups in Fragment.anchor() and unkeyed v-for (#188)
* Add pytest-benchmark suite and per-PR benchmark CI workflow Adds a bench/ suite (element creation, component mount depth, attribute update at depth, keyed/unkeyed v-for reconciliation, and unkeyed list grow/shrink) using pytest-benchmark with DictRenderer and synchronous event loop, mirroring the setup in the sibling observ repo. The new benchmark workflow runs the suite twice on every PR - once against the master version of collagraph/ and once against the PR version - and fails when mean time regresses more than 5%. The CI test job is scoped to the tests directory so the matrix does not execute benchmarks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Disable GC during timed list-benchmark calls to reduce variance The allocation burst of list reconciliation triggers collection pauses in some rounds, which inflated stddev on the grow benchmarks to ~40% of the mean - far too noisy for the 5% regression gate in CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Speed up mount path: cheap arity check, reuse first(), leaner emit - weak(): read __code__.co_argcount instead of building a full inspect.Signature for every wrapped watcher callback; this runs for every dynamic bind during create/mount. - ComponentFragment.mount(): use the existing first() helper to resolve the component root element instead of a hand-rolled BFS with a per-mount deque import. Adds a regression test for a component whose root element sits behind a v-if wrapper. - Component.emit(): avoid creating defaultdict entries for events that have no handlers and skip the set copy when empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cache Fragment._component_parent() lookups The parent-chain walk ran on every reactive attribute update (via _set_attr/_rem_attr calling component.updated()) and on every ref (un)registration, making update cost scale with fragment depth. The result is now cached per fragment. The cache is invalidated in the parent property setter, on unmount(destroy=True), and - via a new recursive helper - after DynamicFragment._create_fragment_for_tag() transfers live children to a new active fragment on a <component :is> tag switch, which reassigns _parent directly and changes the owning component of every transferred descendant. Two regression tests cover the tag-switch reparenting: updated() attribution and string-ref registration must follow the new owner. Both fail when the invalidation hook is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Avoid redundant anchor lookups in Fragment.anchor() and unkeyed v-for - anchor(): a membership check followed by .index() scanned the parent's children twice; use a single .index() in a try/except, for both children and slot_contents. - Unkeyed ListFragment updates mounted every appended fragment with a freshly computed self.anchor(), which walks the tree per item even though the anchor (the first element after the list) cannot change while appending items from the list's own subtree. Compute it lazily once per update run, turning growth by n items from O(n^2) anchor work into O(n). Same hoist in ListFragment.mount(). Adds a regression test asserting that growing an unkeyed v-for with a trailing sibling inserts elements in order, each anchored before that sibling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 2b6a634 commit 2e48305

2 files changed

Lines changed: 76 additions & 10 deletions

File tree

collagraph/fragment.py

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -145,22 +145,31 @@ def anchor(self) -> Any | None:
145145
assert parent is not None
146146

147147
# Check if this fragment is in parent's children
148-
if self in parent.children:
148+
try:
149149
idx = parent.children.index(self)
150+
except ValueError:
151+
idx = None
152+
153+
if idx is not None:
150154
length = len(parent.children) - 1
151155
while 0 <= idx < length:
152156
idx += 1
153157
if element := parent.children[idx].first():
154158
return element
155159
# Fragment might be slot content - check parent's slot_contents if it's
156160
# a ComponentFragment
157-
elif isinstance(parent, ComponentFragment) and self in parent.slot_contents:
158-
idx = parent.slot_contents.index(self)
159-
length = len(parent.slot_contents) - 1
160-
while 0 <= idx < length:
161-
idx += 1
162-
if element := parent.slot_contents[idx].first():
163-
return element
161+
elif isinstance(parent, ComponentFragment):
162+
try:
163+
idx = parent.slot_contents.index(self)
164+
except ValueError:
165+
idx = None
166+
167+
if idx is not None:
168+
length = len(parent.slot_contents) - 1
169+
while 0 <= idx < length:
170+
idx += 1
171+
if element := parent.slot_contents[idx].first():
172+
return element
164173

165174
# No sibling anchor found at this level. If the parent doesn't have
166175
# its own element (e.g., ComponentFragment, ControlFlowFragment), climb
@@ -706,11 +715,18 @@ def update_children(self):
706715
fragment.unmount()
707716
self.values.pop(index)
708717

718+
# The anchor is loop-invariant: it is the first element
719+
# *after* this ListFragment, and the loop below only
720+
# appends items from the list's own subtree before it.
721+
# Computed lazily so update-only runs don't pay for it.
722+
anchor = _UNSET
709723
for i, item in enumerate(items):
710724
if i < len(self.children):
711725
# Update the content for existing values
712726
self.values[i]["context"] = item
713727
else:
728+
if anchor is _UNSET:
729+
anchor = self.anchor()
714730
# Create a new fragment + context
715731
context = reactive({"context": item})
716732
self.values.append(context)
@@ -719,15 +735,18 @@ def update_children(self):
719735
)
720736
self.children.append(fragment)
721737
fragment.parent = self
722-
fragment.mount(target, anchor=self.anchor())
738+
fragment.mount(target, anchor=anchor)
723739

724740
# Then we add a watch_effect for the children
725741
# which adds/removes/updates all the child fragments
726742
self._watchers["list"] = watch_effect(update_children)
727743

744+
anchor = _UNSET
728745
for child in self.children:
729746
if not child.element:
730-
child.mount(target, anchor=self.anchor())
747+
if anchor is _UNSET:
748+
anchor = self.anchor()
749+
child.mount(target, anchor=anchor)
731750

732751
self._mounted = True
733752

tests/test_directive_for.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,3 +675,50 @@ class Items(cg.Component):
675675
first_item, second_item = container["children"]
676676
assert first_item["attrs"]["value"] == "a"
677677
assert second_item["attrs"]["value"] == "b"
678+
679+
680+
def test_for_unkeyed_grow_insert_order(parse_source):
681+
"""
682+
Growing an unkeyed v-for in one reactive update must insert the new
683+
elements in order, each anchored before the element that follows the
684+
list (regression test for hoisting the loop-invariant anchor out of
685+
the append loop).
686+
"""
687+
from tests.conftest import CustomElement, TrackingRenderer
688+
689+
App, _ = parse_source(
690+
"""
691+
<template>
692+
<node v-for="item in items" :content="item" />
693+
<footer content="footer" />
694+
</template>
695+
<script>
696+
import collagraph as cg
697+
class App(cg.Component):
698+
pass
699+
</script>
700+
"""
701+
)
702+
703+
renderer = TrackingRenderer()
704+
gui = Collagraph(renderer, event_loop_type=EventLoopType.SYNC)
705+
container = CustomElement(type="root")
706+
state = reactive({"items": []})
707+
gui.render(App, container, state)
708+
709+
renderer.reset_counters()
710+
state["items"] = ["a", "b", "c"]
711+
712+
inserts = [op for op in renderer.operations if op[0] == "insert"]
713+
assert inserts == [
714+
("insert", "a", "before footer"),
715+
("insert", "b", "before footer"),
716+
("insert", "c", "before footer"),
717+
]
718+
assert [child.type for child in container.children] == [
719+
"node",
720+
"node",
721+
"node",
722+
"footer",
723+
]
724+
assert [child.content for child in container.children[:3]] == ["a", "b", "c"]

0 commit comments

Comments
 (0)