Repository navigation
Share postfix parsing and reduce expression copies - #29480
Conversation
charliermarsh
added
performance
Memory usage reportMemory usage unchanged ✅ |
|
|
Merging this PR will improve performance by 6.32%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | parser[large/dataset.py] |
6.8 ms | 6.3 ms | +7.78% |
| ⚡ | Simulation | parser[pydantic/types.py] |
3 ms | 2.8 ms | +6.32% |
| ⚡ | Simulation | parser[numpy/ctypeslib.py] |
1.3 ms | 1.3 ms | +5.97% |
| ⚡ | Simulation | parser[unicode/pypinyin.py] |
516.5 µs | 490.7 µs | +5.24% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing charlie/ruff-shared-postfix-atom (7b27b5d) with main (0a977a8)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
cd6f40b to
9743630
Compare
| /// | ||
| /// See: <https://docs.python.org/3/reference/expressions.html#atoms> | ||
| fn parse_atom(&mut self, context: ExpressionContext) -> ParsedExpr { | ||
| fn parse_atom(&mut self, context: ExpressionContext, is_parenthesized: &mut bool) -> Expr { |
There was a problem hiding this comment.
I always find &mut arguments hard to understand and try to avoid them if i can. But it's probably fine, given that this is local.
An alternative is to widen parse_atom and make it into parse_primary_expression (primary expression is a Python grammar). This way, is_parenthesized could remain entirely local to parse_primary_expression.
## Summary We parse atoms and their postfix chains together in `parse_primary_expression`, keeping grouping-parenthesis metadata local and removing the `&mut bool` output parameter. Implements [Micha's suggestion](#29480 (comment)) on top of #29480 so CodSpeed compares the two implementations directly. [CodSpeed reports no performance changes](https://github.com/astral-sh/ruff/pull/29507/checks?check_run_id=118282844884): 244 benchmarks unchanged and 10 skipped.
This branch was successfully deployed
2 participants
Summary
We skip prefix-operator dispatch for identifiers and parse atoms together with their postfix chains. Keeping this work in
parse_primary_expressionmakes the grouping-parenthesis flag local and avoids passing an intermediateParsedExprbetween atom and postfix parsing.