Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222
Scaling: Resource: remember which names a tree holds instead of re-reading it#1222BioCam wants to merge 9 commits into
Resource: remember which names a tree holds instead of re-reading it#1222Conversation
`_check_naming_conflicts` recursed over the whole tree on every assignment, so an assignment cost the size of everything already in the tree rather than the size of what was arriving. The root now keeps the names in its tree, built the first time something asks and maintained by the two methods that change a tree's shape. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
let's remove the stuff from the Deck class in this case |
| self.children: List[Resource] = [] | ||
| # Every name in this tree, kept only by the root and only once anyone asks. A name cannot change | ||
| # while a resource is assigned, and a tree changes shape in exactly two places, so an index can | ||
| # be carried forward instead of rebuilt: see `_names_in_tree`. | ||
| self._name_index: Optional[Set[str]] = None |
There was a problem hiding this comment.
imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something
|
it would be clean to use the will/did assign child resource
|
…n `Deck` Every resource keeps a map of everything at or beneath it, by name, seeded with itself and kept in step by did-assign and did-unassign handlers it registers on itself. Those callbacks already propagate to every ancestor, so an assignment anywhere updates each map above it without walking a tree. The map holds the resources themselves, so it answers both questions a name is asked: whether it is taken, and which resource has it. `get_resource` becomes a lookup rather than a recursive search, and `Deck` no longer needs its own `_resources` dict, the two handlers that maintained it, or the `_check_naming_conflicts` override commented "overwrite for speed" - which checked only the arriving resource's own name and let a clash buried in its subtree through. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e copy in `Deck`" This reverts commit d884b2f.
…hem on demand A resource is a root until something takes it, so it can hold the map of its own tree from the moment it is made, seeded with itself. `assign_child_resource` hands what arrives to the new root and `unassign_child_resource` hands it back, which are the only two moments a root changes. Nothing is built on demand, so `_names_in_tree` and the unbuilt state it existed to guard both go. Maintained by those two methods directly rather than through the did-assign and did-unassign callbacks: those are a public notification list, and a subscriber that raises part-way, or one that deregisters a handler, would leave the map short of names the tree really holds and let a duplicate in. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`assign_child_resource` walked what was arriving twice: once to check each name against the tree, once to record what to add. `_check_naming_conflicts` now returns what it walked, so the second pass goes. The check still runs before the tree changes, so a clash leaves it untouched. Grafting a carrier of five plates onto a facility of 17 823 resources traverses the 486 arriving resources once and costs 0.12 ms; the facility's size does not enter it, since each arriving name is one lookup in the root's map. `Deck` overrides the check, so it hands back the same map until that override is removed. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under review. The reverted commits changed how the index is maintained, which is the open question in review and not settled yet.
@rickwierenga - Done.
One behaviour change I think is worth flagging: That behaviour is what prompted me to generate #1228 . |
# Conflicts: # pylabrobot/resources/resource.py
2 participants
Problem
_check_naming_conflictsenforces tree-wide name uniqueness by recursing over the whole tree, andassign_child_resourcecalls it onget_root()for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured betweenn^1.9andn^2.0- and every later assignment still walks all of it.A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:
Changes
Resource._name_indexholds the names in a tree, kept only by the root, built the first time_names_in_treeasks for it._subtree_namesreturns the names at or beneath a resource;assign_child_resourceunions the arriving ones in,unassign_child_resourcesubtracts them._check_naming_conflictswalks the arriving subtree against that set instead of recursing over both trees. It reads the index offget_root(), where it was already called from, so it still covers the whole tree.Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.
Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.
Tests
TestNameIndex(a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences.ruff format,ruff check --select I,ruff checkandmypy pylabrobot --check-untyped-defsare clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).