[Lang] Attach only a cheap source header per AST node - #857
Conversation
ASTTransformer attached full source-position info to every generated IR node via get_pos_info, which builds a TextWrapper-formatted source-line-plus-caret hint for each node. That formatting ran for every AST node of every kernel compilation and dominated the Python side of a kernel build. The eager per-node path now attaches only the cheap 'File "...", line N, in fn' header; the full hint is still produced on the actual compile-error path, so compile-error messages are unchanged. This is change 2 of #804, split out on its own.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 362c37c119
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # hint (get_pos_info) here means running TextWrapper for every AST node of every kernel compilation, | ||
| # which dominates kernel build time. The full hint is still produced on the actual compile-error path | ||
| # below (get_pos_info in the except handler), so error messages are unchanged. | ||
| info = ctx.get_pos_header(node) if isinstance(node, (ast.stmt, ast.expr)) else "" |
There was a problem hiding this comment.
When a diagnostic is emitted after AST conversion, it does not re-enter this except handler; backend paths such as bounds/type-check assertions and debug overflow reporting concatenate stmt->get_tb() from the IR. Because the IR now stores only this header, those user-facing messages lose the source line/caret that get_pos_info() used to provide, so compile/runtime diagnostics emitted from transforms or codegen are no longer unchanged.
Useful? React with 👍 / 👎.
ae5dd79 to
362c37c
Compare
|
Agent wrote: Closing this: the change is not behaviour-preserving, and my claim above that "error messages are unchanged" was The per-node source info is not read only by the compile-error path. It is embedded in Reproduced on CPU against the same build: main 130 passed, this branch 64 failed. CI caught it on the Mac CPU jobs. #858 takes the same hotspot by memoising |
1 participant
This is change 2 of #804 (@duburcqa), split out on its own so it can land independently of that PR's other change.
The code is Alexis's; this PR adds only the measurement below.
What
ASTTransformerBase.__call__attaches source-position info to every generated IR node, and did so viaget_pos_info, which builds aTextWrapper-formatted source-line-plus-caret hint. That formatting therefore ranfor every stmt/expr node of every kernel compilation.
The eager per-node path now attaches only the cheap
File "...", line N, in fnheader (get_pos_header). The fullhint is still produced on the actual compile-error path -- the
excepthandler in the same function callsget_pos_info-- so compile-error messages are unchanged.Why split it out
#804 pairs this with an unrelated change to
_inside_class(replacing anO(len(sys.modules))scan with alinecachelookup). The two are independent, and on the workload below only this one matters, so separating themlets this land on its own merits.
Impact
Measured on a kernel-build-heavy workload (a qipc solver step, ~130 offloaded tasks), timing the first step after
editing one kernel. Variants were interleaved over a single unchanged build with only this file swapped between
cells, so no rebuild and no exposure to a neighbouring job on the node; each figure is one full cold-warm-edit
cycle against a fresh cache.
_inside_classchange)So this change is worth ~4.1 s of the ~4.2 s that #804 delivers here; the
_inside_classscan is0.05 s on this-10% ofworkload, within noise. #804's own suite-wide measurement attributes most of its win to this change as well
(
Scene.buildout of -12.5% combined).Testing
test_exception.py,test_syntax_errors.py,test_nested_kernel_error.pyandtest_ast_refactor.py-- the suitesthat assert on error-message text -- give 161 passed both with and without the change, on the same build.
Made with Cursor