-
Notifications
You must be signed in to change notification settings - Fork 251
Merge main into next #2534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge main into next #2534
Conversation
* fix: bypass decorator retrieval in release mode Gate all decorator retrieval calls behind `in_debug_mode` checks, ensuring zero overhead when debugging is disabled. Processor changes: - before_enter/after_exit decorator loops - decorators_for_op in basic block execution FastProcessor changes: - execute_before_enter_decorators early return - execute_after_exit_decorators early return - decorators_for_op in basic block execution Includes spy tests to verify retrieval is bypassed. * fix(core): strip decorators while maintaining valid CSR structure Update strip_decorators() to create an empty but valid CSR structure instead of calling clear(), which removed the structure entirely and caused panics when accessing decorator information after stripping. - Add DebugInfo::empty_for_nodes(num_nodes) to create valid empty CSR - Update from_components to accept empty structures - Add edge case tests for empty forest, idempotency, multiple node types * fix(processor): gate error context decorator access with in_debug_mode This fix resolves a 20x performance degradation in release mode when decorators were present in the MastForest but not being executed. Root Cause: The err_ctx! macro was unconditionally calling node.get_assembly_op(), which traversed the CSR decorator storage on every operation (522,059 times in blake3 benchmark), even when in_debug_mode was false. This caused execution time to increase from 191ms to 3,884ms. Solution: Modified the error context creation pipeline to accept and respect the in_debug_mode flag: - Updated err_ctx! macro to require in_debug_mode parameter - Updated ErrorContextImpl::new() and new_with_op_idx() to accept in_debug_mode - Modified precalc_label_and_source_file() to return early when !in_debug_mode, avoiding expensive decorator traversal - Updated all err_ctx!() call sites in Process and FastProcessor to pass in_debug_mode flag Performance Impact: - Before: Op execution time 3,884ms (7,439ns/op) - After: Op execution time 191ms (366ns/op) - Improvement: 20.3x faster When in_debug_mode is false, decorators (including AsmOp decorators used for error context) are no longer accessed, even if present in the MastForest. * chore: Changelog
7bdf5b5 to
ae25ef5
Compare
This merge brings the decorator bypass optimization from main (#2529) into next. The changes adapt the decorator bypass optimization to next's architecture: - No changes to legacy Process (removed in next) - All changes apply to FastProcessor only - Decorator retrieval gated behind in_debug_mode checks - Error context creation respects in_debug_mode flag Performance impact: ~10% overall speedup, 99.7% reduction in trace execution time. Conflicts resolved using solutions from huitseeker/decorator-bypass-on-next rebase work.
ae25ef5 to
2112b5e
Compare
plafer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you!
A couple of comments for the future (which I also mentioned in other places):
- We may want to simplify
execute_op_batch()method to passnode_idinstead ofbasic_blockas a parameter, and more generally, think about how to simplify the parameters passed into this method. - Ideally, with some future refactorings, stripping decorators would result in full removal of the
DebugInfostruct (and we should probably renamestrip_decorators()intoclear_debug_info()or something similar).
|
Ah - I should have merged this w/o squashing. Sorry! Will make a commit into |
|
This PR merges
mainintonext, bringing the decorator bypass optimization from #2529 into the next development branch.What's Being Merged
The decorator bypass optimization that was merged to main via #2529, which includes:
in_debug_modechecks