Resolve the head once a flush, not once a node
The identity map holds weak references. A branch row nobody keeps a strong reference to is collected between two nodes, so resolving the head inside the placement loop read it back from the database for every node in the flush: 201 SELECTs on `branches` to write 200 actions, and 36 s -> 45 s on the same 297 tests. Nothing about any result changed, which is why only a stopwatch found it, and why there is now a test counting the reads. Also: the two reads left un-pathed on purpose say so where they live — `max_action_index` allocates the legacy `index` and must stay adventure-wide or two branches issue the same number, and export is a flat v1 bundle whose reader has no idea branches exist. And `Adventure.actions` keeps its `index` ordering, because ordering the collection by depth would not make it a story: it is every branch's actions, and a path is a selection out of it. 318 tests green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Dvvqn9ZDR4ixeFPHNbww7
This commit is contained in:
committed by
Parth
co-authored by
Claude Opus 5
parent
563b9af9cf
commit
05a2a77e4c
@@ -134,6 +134,12 @@ class Adventure(Base):
|
||||
story_cards: Mapped[list["StoryCard"]] = relationship(
|
||||
back_populates="adventure", cascade="all, delete-orphan"
|
||||
)
|
||||
# Every action of the adventure — that is, every *branch's*. Not the story
|
||||
# being played, and re-ordering it by depth would not make it one: the
|
||||
# collection is the tree, and a path is a selection out of it. Anything
|
||||
# showing a reader a story goes through `context.history`, which goes
|
||||
# through the branch clause. What is left here is ownership and the
|
||||
# delete-orphan cascade, which are facts about the adventure.
|
||||
actions: Mapped[list["Action"]] = relationship(
|
||||
back_populates="adventure",
|
||||
cascade="all, delete-orphan",
|
||||
|
||||
@@ -1144,6 +1144,12 @@ def export_adventure(
|
||||
# Export is the one read that genuinely wants every attempt, so it asks for
|
||||
# the deferred `variants` column up front — iterating adv.actions instead
|
||||
# would lazy-load it one row at a time.
|
||||
#
|
||||
# And the one read deliberately left un-pathed: a backup wants the whole
|
||||
# adventure, not the branch its owner happens to be standing on. `index`
|
||||
# orders it because the v1 bundle is a flat list keyed on index and its
|
||||
# reader has no idea branches exist — which is exactly why SP6 replaces the
|
||||
# format rather than quietly widening this query.
|
||||
exported_actions = (
|
||||
db.query(models.Action)
|
||||
.filter(models.Action.adventure_id == adv.id)
|
||||
|
||||
+27
-6
@@ -88,15 +88,21 @@ def head_branch(db: Session, adventure: models.Adventure) -> models.Branch:
|
||||
|
||||
|
||||
def place_action(
|
||||
db: Session, adventure: models.Adventure, action: models.Action
|
||||
db: Session,
|
||||
adventure: models.Adventure,
|
||||
action: models.Action,
|
||||
branch: models.Branch | None = None,
|
||||
) -> models.Branch:
|
||||
"""Put `action` on the head branch and move the head to it.
|
||||
|
||||
`depth` follows `index` while the two coexist. They have to agree: a read
|
||||
ordering by depth and a cursor counting in index space are describing the
|
||||
same story, and SP2 swaps one for the other under everything at once.
|
||||
|
||||
`branch` is the head, already resolved, for a caller placing several nodes
|
||||
at once — see `place_new_nodes` for why that is worth a parameter.
|
||||
"""
|
||||
branch = head_branch(db, adventure)
|
||||
branch = branch or head_branch(db, adventure)
|
||||
action.branch_id = branch.id
|
||||
if action.depth is None:
|
||||
action.depth = action.index
|
||||
@@ -107,7 +113,10 @@ def place_action(
|
||||
|
||||
|
||||
def place_memory(
|
||||
db: Session, adventure: models.Adventure, memory: models.Memory
|
||||
db: Session,
|
||||
adventure: models.Adventure,
|
||||
memory: models.Memory,
|
||||
branch: models.Branch | None = None,
|
||||
) -> models.Branch:
|
||||
"""Attach a memory to the node that produced it.
|
||||
|
||||
@@ -115,7 +124,7 @@ def place_memory(
|
||||
that node's depth. A hand-written memory summarises nothing, so its depth
|
||||
stays NULL and it belongs to the adventure rather than to a path.
|
||||
"""
|
||||
branch = head_branch(db, adventure)
|
||||
branch = branch or head_branch(db, adventure)
|
||||
memory.branch_id = branch.id
|
||||
if memory.depth is None and memory.source_end is not None:
|
||||
memory.depth = memory.source_end
|
||||
@@ -135,7 +144,15 @@ def place_new_nodes(session: Session) -> None:
|
||||
Nodes whose adventure has not been inserted yet are left alone: there is no
|
||||
id to hang a branch off, and an Action needs `adventure_id` to be written
|
||||
at all, so the case does not arise from any writer we have.
|
||||
|
||||
The head is resolved once per adventure per flush, and held in `heads` for
|
||||
the length of the call. That is not just saving a dictionary lookup: the
|
||||
identity map holds *weak* references, so a branch row nobody keeps a strong
|
||||
reference to is collected between two nodes and read back from the database
|
||||
for the next one. Resolving per node turned a fixture writing two hundred
|
||||
actions in one flush into two hundred SELECTs on `branches`.
|
||||
"""
|
||||
heads: dict[int, models.Branch] = {}
|
||||
for obj in list(session.new):
|
||||
if isinstance(obj, models.Action):
|
||||
place = place_action
|
||||
@@ -146,8 +163,12 @@ def place_new_nodes(session: Session) -> None:
|
||||
if obj.branch_id is not None or obj.adventure_id is None:
|
||||
continue
|
||||
adventure = session.get(models.Adventure, obj.adventure_id)
|
||||
if adventure is not None:
|
||||
place(session, adventure, obj)
|
||||
if adventure is None:
|
||||
continue
|
||||
head = heads.get(adventure.id)
|
||||
if head is None:
|
||||
head = heads[adventure.id] = head_branch(session, adventure)
|
||||
place(session, adventure, obj, head)
|
||||
|
||||
|
||||
def refresh_head(db: Session, adventure: models.Adventure) -> None:
|
||||
|
||||
@@ -350,6 +350,28 @@ def test_a_memory_written_without_a_branch_is_placed_anyway(forked):
|
||||
assert memory.depth == 3
|
||||
|
||||
|
||||
def test_placing_a_flush_of_nodes_reads_the_branch_once(forked, emitted_sql):
|
||||
"""The guard resolves the head once per flush, not once per node.
|
||||
|
||||
The identity map holds weak references, so a branch row nobody keeps a
|
||||
strong reference to is collected between two nodes and read back for the
|
||||
next one. Writing two hundred actions in one flush was two hundred SELECTs
|
||||
on `branches` before the head was hoisted out of the loop, and nothing
|
||||
about the result would have told you.
|
||||
"""
|
||||
db, adventure, _ = forked
|
||||
emitted_sql.clear()
|
||||
for i in range(50):
|
||||
db.add(models.Action(
|
||||
adventure_id=adventure.id, index=500 + i, type="do", text=f"bulk {i}"
|
||||
))
|
||||
db.commit()
|
||||
branch_reads = [s for s in emitted_sql if s.startswith("SELECT") and "FROM branches" in s]
|
||||
assert len(branch_reads) <= 2, (
|
||||
f"{len(branch_reads)} reads of `branches` to place 50 nodes"
|
||||
)
|
||||
|
||||
|
||||
def test_an_adventure_with_no_branch_at_all_reads_as_empty(forked):
|
||||
"""The loud version of a missing branch: nothing, rather than everything.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user