-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
ast: Standardize visiting order #126993
ast: Standardize visiting order #126993
Conversation
Id, attributes, inner nodes in source order if possible, tokens, span. Also always use exhaustive matching in visiting infra, and visit some missing nodes.
Changes to the size of AST and/or HIR nodes. cc @nnethercote |
@@ -39,37 +39,37 @@ ast-stats-1 - Match 72 ( 1.1%) 1 | |||
ast-stats-1 - Struct 72 ( 1.1%) 1 | |||
ast-stats-1 - Lit 144 ( 2.2%) 2 | |||
ast-stats-1 - Block 216 ( 3.3%) 3 | |||
ast-stats-1 PathSegment 720 (10.9%) 30 24 |
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.
PathSegment
hasn't changed in size. Something about the visitor must have changed, but I can't see what. Do you know?
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.
This comment is relevant, it describes how the AST node measurements aren't perfect:
rust/compiler/rustc_passes/src/hir_stats.rs
Lines 47 to 63 in 4bc39f0
/// This type measures the size of AST and HIR nodes, by implementing the AST | |
/// and HIR `Visitor` traits. But we don't measure every visited type because | |
/// that could cause double counting. | |
/// | |
/// For example, `ast::Visitor` has `visit_ident`, but `Ident`s are always | |
/// stored inline within other AST nodes, so we don't implement `visit_ident` | |
/// here. In contrast, we do implement `visit_expr` because `ast::Expr` is | |
/// always stored as `P<ast::Expr>`, and every such expression should be | |
/// measured separately. | |
/// | |
/// In general, a `visit_foo` method should be implemented here if the | |
/// corresponding `Foo` type is always stored on its own, e.g.: `P<Foo>`, | |
/// `Box<Foo>`, `Vec<Foo>`, `Box<[Foo]>`. | |
/// | |
/// There are some types in the AST and HIR tree that the visitors do not have | |
/// a `visit_*` method for, and so we cannot measure these, which is | |
/// unfortunate. |
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.
Paths in attributes are now visited.
@bors r+ rollup=never cool :) visitor boilerplate is always kinda sketch when it just uses field accesses instead of exhaustively destructuring |
☀️ Test successful - checks-actions |
Finished benchmarking commit (036b38c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 696.635s -> 697.296s (0.09%) |
Order: ID, attributes, inner nodes in source order if possible, tokens, span.
Also always use exhaustive matching in visiting infra, and visit some discovered missing nodes.
Unlike #125741 this shouldn't affect anything serious like
macro_rules
scopes.