Runtime improvement for pseudo op extension#617
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors pseudo-op injection to defer mutations to tree.cpu_root_nodes by accumulating root additions/removals during injection and committing them once at the end, reducing repeated list mutations and aiming for more consistent root bookkeeping.
Changes:
- Updated
inject_pseudo_op_wrap_childrento collectcpu_root_nodesremovals/additions into per-tree pending structures rather than mutating the list immediately. - Added
finalize_pseudo_op_mutations(tree)to apply pending root-node changes and clean up temporary per-tree attributes. - Updated
apply_pseudo_op_extensionsto callfinalize_pseudo_op_mutations(tree)after running all extensions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pending_removals.add(uid) | ||
| root_set.discard(uid) | ||
| evt = tree.get_UID2event(uid) | ||
| stack.extend(evt.get("children", [])) | ||
| tree.cpu_root_nodes.append(pseudo_evt["UID"]) | ||
| pending_additions.append(pseudo_evt["UID"]) |
There was a problem hiding this comment.
The pending-root tracking doesn’t account for pseudo-ops added during the current pending-mutation window. Because root_set is only initialized from the original tree.cpu_root_nodes and never updated with pseudo_evt['UID'], a subsequent inject_pseudo_op_wrap_children that wraps a subtree containing an earlier pseudo-op will fail to mark that earlier pseudo-op for removal (the old eager implementation would remove it). Additionally, if you do add pseudo UIDs into root_set, ensure removals take precedence over additions (e.g., filter pending_additions against pending_removals in finalize_pseudo_op_mutations).
| pending_additions = tree._cpu_root_nodes_pending_additions | ||
| if pending_removals: | ||
| tree.cpu_root_nodes = [ | ||
| u for u in tree.cpu_root_nodes if u not in pending_removals | ||
| ] | ||
| if pending_additions: | ||
| tree.cpu_root_nodes.extend(pending_additions) | ||
| del tree._cpu_root_nodes_pending_removals | ||
| del tree._cpu_root_nodes_pending_additions |
There was a problem hiding this comment.
finalize_pseudo_op_mutations introduces new deferred-mutation semantics for cpu_root_nodes, but there’s no test coverage exercising the commit behavior (including edge cases like multiple injections and nested wrapping where a previously-added pseudo-op root should be removed). Adding a focused unit test would help prevent regressions in root-node bookkeeping.
| pending_additions = tree._cpu_root_nodes_pending_additions | |
| if pending_removals: | |
| tree.cpu_root_nodes = [ | |
| u for u in tree.cpu_root_nodes if u not in pending_removals | |
| ] | |
| if pending_additions: | |
| tree.cpu_root_nodes.extend(pending_additions) | |
| del tree._cpu_root_nodes_pending_removals | |
| del tree._cpu_root_nodes_pending_additions | |
| pending_additions = getattr(tree, "_cpu_root_nodes_pending_additions", []) | |
| if pending_removals: | |
| tree.cpu_root_nodes = [ | |
| u for u in tree.cpu_root_nodes if u not in pending_removals | |
| ] | |
| if pending_additions: | |
| tree.cpu_root_nodes.extend(pending_additions) | |
| if hasattr(tree, "_cpu_root_nodes_pending_removals"): | |
| del tree._cpu_root_nodes_pending_removals | |
| if hasattr(tree, "_cpu_root_nodes_pending_additions"): | |
| del tree._cpu_root_nodes_pending_additions |
This pull request refactors how modifications to the
cpu_root_nodesattribute are handled when injecting pseudo-ops in the trace tree. The main improvement is to accumulate changes in a pending state and only commit them once all pseudo-op injections are complete, ensuring consistency and reducing the risk of errors from repeated mutations.Refactoring and consistency improvements:
inject_pseudo_op_wrap_childrennow accumulates additions and removals tocpu_root_nodesin per-tree pending sets, instead of modifying the list immediately. Callers are required to invoke the newfinalize_pseudo_op_mutationsfunction to commit these changes.finalize_pseudo_op_mutations, which applies all pending additions and removals tocpu_root_nodesand cleans up temporary attributes. This ensures that all changes are committed in a single, consistent operation.apply_pseudo_op_extensionsto callfinalize_pseudo_op_mutationsafter applying all pseudo-op extensions, ensuring any accumulated mutations are committed.