Replace the autolabel debounce with a burst-served label cache and idle refresh - #6
charlesangus wants to merge 4 commits into
Conversation
…le refresh Profiling with real X input against 3k-10k node scripts showed the cost that matters is not building a label but handing Nuke a *changed* label string: every change costs a main-loop stall proportional to the script size (~70 ms at 3k nodes, ~200 ms at 10k). The 100 ms debounce made slider drags 120-260 ms per pointer event on those scripts (stock: 15-25 ms) and, once the stall exceeded its window, rebuilt on every move anyway. It also dropped trailing edits (two edits 50 ms apart left the first value on screen). Nuke asks for a label on a real knob change (one or two requests) or in a whole-script pass (every node back to back, after a viewer input change or any knob change followed by a frame step). The new scheme: - bursts of requests are answered from a per-node cache, so whole-script passes cost microseconds per node instead of a build - a changed label string is never returned while the user interacts; the previous text is shown and the node is marked stale - once label traffic has been quiet for >= 0.4 s (5x the measured stall), stale nodes get a dope_sheet change-and-revert (one relabel, no undo entry, no visible change), which releases the new text - frame-dependent nodes (keys, expressions, TCL labels) are cached per frame and refreshed the same way after a scrub - no knobChanged hook: dragging a 3k-node selection fires it 123k times; onDestroy (once per deleted node) keeps a reused name from inheriting the old label Measured in one Nuke 17 session with the implementations interleaved (ms per pointer event, 10 002 nodes): slider drag 13-20 vs stock 13-23 (was 154-266); timeline scrub 22-32 vs 139-186; whole-script pass 171 ms vs 700 ms; node drags unchanged. Config and preference saves now invalidate the cache and refresh every label, so changes show immediately. The temporary profiling instrumentation is removed; the harness lives in .profiling (ignored).
charlesangus
left a comment
There was a problem hiding this comment.
Reviewed by Codex (codex: available — 5h 19%, 7d 38%, cap 90%, plan plus) via cat-pm. Found one high-, three medium-, and four low-severity issues.
| node_label_value = nuke.value("this.label", "") | ||
| with contextlib.suppress(RuntimeError): | ||
| node_label_value = nuke.tcl("subst", node_label_value) | ||
| self.node_label_value = node_label_value or "" |
There was a problem hiding this comment.
high — The value saved for frame-dependency detection is the result after nuke.tcl("subst", ...). Substitution normally removes the [ syntax, so label knobs such as [frame] are cached as frame-independent. During bursts, affected nodes can then keep returning an old frame's label without ever being marked stale.
Suggested fix: Preserve the raw label-knob string before substitution and make _frame_dependent() test that raw value. Add a test using a raw [frame] label.
There was a problem hiding this comment.
Fixed in 1527d28: label_readout_creator now keeps the raw knob value (node_label_raw) before nuke.tcl("subst", ...) and _frame_dependent tests that. Test test_tcl_in_the_label_knob_is_cached_per_frame runs the real build with a [frame] label and asserts the entry is cached with a frame.
| if node is None: | ||
| continue | ||
| knob = node.knob("dope_sheet") | ||
| if knob is None: |
There was a problem hiding this comment.
medium — A stale node without dope_sheet is silently skipped after _refresh_stale_labels() has already removed it from _stale. Its old _shown value therefore remains, and subsequent changed builds repeat the same hold/skip cycle, so the displayed label can remain permanently stale.
Suggested fix: Do not defer changed strings for nodes that cannot be poked, and retain skipped burst entries until a natural request can rebuild and show them. Alternatively, implement a verified safe fallback poke knob.
There was a problem hiding this comment.
Fixed in 1527d28, slightly more broadly than suggested: a node without dope_sheet is never served from the cache and never has a changed string held back (_pokeable), so it is rebuilt and shown on every request. Viewers/backdrops are a handful per script, so the cost is negligible and there is no path that can leave them stale.
| now = time.perf_counter() | ||
| self._note_stall(now) | ||
| in_burst = self._track_burst(now) | ||
| full_name = nuke.thisNode().fullName() |
There was a problem hiding this comment.
medium — The cache is keyed only by the mutable fullName(), while cleanup occurs only on destruction. Renaming a node or enclosing group leaves the old keys behind. If that name is reused—or a group is renamed back—a large burst can serve the obsolete entry and, because bursts over 200 are not marked stale, leave the wrong label indefinitely.
Suggested fix: Track each node's previous full name by stable node identity. On every callback, migrate or evict entries when its full name changes; clear that identity mapping on destruction and invalidation.
There was a problem hiding this comment.
Fixed in 1527d28 without identity tracking: an onCreate callback (fires once per node, unlike knobChanged) evicts every cache entry under the created node's fullName(), so a name freed by a rename or delete cannot hand a new node an obsolete label. A renamed node itself is relabelled by Nuke on rename and rebuilds under its new name.
| labelmaker_config.reload_composed_config() | ||
| labelmaker.autolabeller_singleton.config = labelmaker_config.composed_config_singleton | ||
| labelmaker.autolabeller_singleton.set_enabled(self.labelmaker_enabled_checkbox.isChecked()) | ||
| if self.labelmaker_enabled_checkbox.isChecked(): |
There was a problem hiding this comment.
medium — Disabling Labelmaker unregisters the callback but deliberately skips any relabel poke. Since redraws do not cause label requests, nodes can continue displaying the last Labelmaker strings indefinitely instead of immediately reverting to Nuke's default autolabel.
Suggested fix: On the enabled-to-disabled transition, unregister first and poke all nodes without adding _forced entries, so Nuke rebuilds them through its default autolabel.
There was a problem hiding this comment.
Fixed in 1527d28: set_enabled(False) unregisters and then pokes every node without forcing, so Nuke rebuilds them through its stock autolabel; set_enabled(True) registers and refreshes every label. The dialog's extra refresh_all_labels() call is gone.
| full_name = nuke.thisNode().fullName() | ||
| cached = self._content.get(full_name) | ||
| if in_burst and cached is not None and full_name not in self._forced: | ||
| # a whole-script pass: nothing about this node changed |
There was a problem hiding this comment.
low — The comment is false for bursts of 9–200 requests: this branch also handles genuine multi-node edits where the node may have changed. It contradicts the later burst-close logic and the stated design.
Suggested fix: Explain that both candidate whole-script passes and multi-node edits are initially served from cache, with classification deferred until the burst closes.
There was a problem hiding this comment.
Reworded in 1527d28: the branch serves from the cache for both whole-script passes and genuine multi-node edits, which are only told apart in _close_burst.
| names = ["Grade{}".format(i) for i in range(20)] | ||
| for name in names: | ||
| request(labeller, clock, name, "old") | ||
| whole_script_pass(labeller, clock, names) # 20 nodes changed by a script |
There was a problem hiding this comment.
low — The comment says the 20 nodes changed, but their configured build text remains "old". Consequently test_small_burst_is_a_real_edit_and_gets_refreshed checks only stale-set classification and never verifies that changed content is ultimately rebuilt and shown.
Suggested fix: Change every node's pending text before the burst, fire the refresh, simulate the resulting callbacks, and assert that each new value is returned.
There was a problem hiding this comment.
Fixed in 1527d28: every node's text changes before the burst, the pass is asserted to still show the old text, the burst closes, the refresh fires, and the simulated relabel requests are asserted to return the new text.
| knob = labeller.nodes["Grade1"]["dope_sheet"] | ||
| clock.now += 1.0 | ||
| labeller._refresh_timer.fire() | ||
| assert knob.value() is False # flipped and flipped back |
There was a problem hiding this comment.
low — Several added comments merely narrate adjacent setup or assertions (for example lines 123, 135, 139, 145, 160, 180, 197, and 213), contrary to the requested comment policy.
Suggested fix: Remove the narrating comments. Where context is genuinely needed, explain a hidden constraint—for example, that the stub cannot emit Nuke's automatic relabel callback and the test must simulate it.
There was a problem hiding this comment.
Removed in 1527d28. One comment kept, rewritten to state the non-obvious constraint: the stub cannot emit Nuke's own relabel request, so the tests simulate it with a lone request.
| # Intermediate Markdown generated from the README for the User Guide PDF | ||
| docs/.build/ | ||
|
|
||
| # Local profiling harness (see .profiling/README.md) |
There was a problem hiding this comment.
low — The new comment directs readers to .profiling/README.md, but that file is absent and the entire directory is ignored. The related source comment also refers to unavailable .profiling results.
Suggested fix: Remove the dangling references or commit the profiling methodology/results in a tracked documentation location.
There was a problem hiding this comment.
Fixed in 1527d28: both references now point at the profiling-harness branch, which holds the harness, raw results and write-up.
…e reuse, disable path - _frame_dependent tested the label knob after TCL substitution, so a [frame] label was cached as frame-independent; use the raw knob value - nodes without a dope_sheet knob (Viewer, Backdrop) cannot be poked, so a held-back string would never be released: rebuild and show them on every request instead of caching or holding - onCreate evicts cache entries under a created node's name, so a name freed by a rename or delete cannot hand a new node the old label - set_enabled(False) pokes every node so Nuke rebuilds them with its own autolabel instead of leaving Labelmaker's strings on screen; set_enabled(True) refreshes every label - stronger burst test, narrating test comments removed, dangling .profiling references point at the profiling-harness branch
|
Review round closed: all 8 Codex findings addressed in 1527d28 (replies on each thread). Re-verified in Nuke 17.0v3 on the 3 002-node script with stock and this branch interleaved per gesture: slider drag 3.4–4.6 ms/tick vs stock 2.6–4.3, whole-script pass 79 ms vs 204 ms, scrub 4.3–4.9 vs 35–46 ms/tick, node drags equal, no tracebacks, final values correct after every drag. ruff clean, pytest 84 passed. |
… bursts A burst of label requests served from the cache was classified by size (> 200 = whole-script pass, left as served); a tool setting a knob on 1000 selected nodes produces the same burst, so 784 of them kept the old label indefinitely. Any classifier has that failure mode (a frame/viewer-change heuristic was tried and dropped: a bulk edit that also steps the frame is misclassified the same way). Now every label answered from the cache during a burst is re-composed in the background once label traffic is quiet: nuke.runIn() gives the label code its node context, the compose is read-only (the indicators knob write from Foundry's set_indicators only happens on a real request), 15 ms slices on a 0 ms timer back off while traffic resumes, and only labels whose text differs are poked. Labels that were frame-dependent at their last build are verified first and released as soon as that phase ends, since a frame step is what changes them. LABEL_BURST_GENUINE_MAX and the per-frame cache entries are gone. Measured with the harness on profiling-harness (5400f88), §11 of RESULTS-2026-09-13.md: correct on bulk edits of 100/1000/all nodes, with a frame change or viewer connect in the same callback, with deletes, renames, undo, set_enabled(False) or scriptClear before the refresh; slider drags stay at stock latency (16–20 ms/tick at 10k). Cost: ~0.4 s (3k) / 1.6 s (10k) of background compute after a whole-script pass on the profiling box, and frame-dependent readouts refresh ~1–2.5 s after a frame step instead of inside stock's UI freeze.
Summary
Replaces the 100 ms autolabel debounce (94e313b) with a label cache designed around how Nuke actually drives the autolabel, measured on 3k–10k node scripts with real X input.
What the profiling showed (Nuke 17.0v3; details on the
profiling-harnessbranch,.profiling/RESULTS-2026-09-13.md):knobChangedfires per node per pointer move when dragging a selection (123k times for one drag of 3k nodes), so it is not used.The new scheme (
AutolabelReplacement.create_autolabeland helpers):dope_sheetchange-and-revert underUndo.disable()— exactly one relabel, no undo entry, no visible change — which releases the new text.fullName();onDestroy(once per deleted node) stops a reused name inheriting the old label.Measured, same Nuke session, implementations interleaved (ms per pointer event, 10 002 nodes): slider drag 13–20 vs stock 13–23 (was 154–266); timeline scrub 22–32 vs 139–186; whole-script pass 171 ms vs 700 ms; node drags unchanged.
Notes for review
dope_sheetpoke is a workaround for the lack of any API to re-request one node's label; verified on 17.0v3 only. If it ever stops triggering a relabel the fallback is a stale label until Nuke's next natural request.profiling-harness.Testing
ruff check .,pytest tests/(77 passed; 15 new intests/test_label_cache.py)