Repository navigation
[ty] Reduce Expr size to 64 bytes - #27591
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 96.96%. The percentage of expected errors that received a diagnostic held steady at 92.96%. The number of fully passing files held steady at 106/133. |
Memory usage reportSummary
Significant changesClick to expand detailed breakdownflake8
trio
sphinx
prefect
|
|
|
Merging this PR will improve performance by 5.62%
Performance Changes
Tip Curious why this is faster? Use the CodSpeed MCP and ask your agent. Comparing 
Footnotes
|
|
Can we split the optimization into separate PRs, or are all of those needed to reduce the size? I also think that this will soon matter less, once we have proper eviction for parsed module. So I'd suggest we focus on the perf improvement (which is nice!) |
|
I think the CodSpeed "perf improvement" is all memory though, right? :D |
559c623 to
9e37e7a
Compare
charliermarsh
changed the title
charliermarsh
added
performance
charliermarsh
changed the title
|
(Carved out the other changes but then decided they weren't worth it so this just reduces expression size.) |
charliermarsh
changed the title
charliermarsh
changed the title
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I'd prefer if we split the changes unrelated to reducing the size of Expr into their own PR. I want to see the memory usage/perf results for those separately to get a better sense on if the error recovery trade offs are justified.
| func: Box::new(func), | ||
| arguments, | ||
| range: self.node_range(start), | ||
| range_start: start, |
There was a problem hiding this comment.
Can we add some debug assertions here and in other places in the parser, asserting that self.node_range(start) == arguments.end()
| #[cfg_attr(feature = "get-size", derive(get_size2::GetSize))] | ||
| pub struct Alias { | ||
| pub range: TextRange, | ||
| pub range_end: TextSize, |
There was a problem hiding this comment.
We should document the assumption that name.start() == alias.start() here (somewhere). Because it has implications on the parsers error recovery, that make me a feel a little uneasy (just something more that we have to think about in the already complex recovery). We should document the same for other nodes where we now apply this optimization.
Specifically, the parser is not allowed to perform any error recovery between alias.start() and parsing name. Because any bumped token between start and name (even if only for error recovery), would violate the name.start() == alias.start() constraints.
Given this, I'd prefer if we only make this optimization for nodes where there's a very clear benefit. E.g. reducing Expr to 64 bytes seems meaningful. I'm not convinced the tradeoff for Alias are given (also, changing Alias doesn't fit the PR description, so I'd suggest we move this optimization out of this PR).
MichaReiser
added
parser
## Summary This PR reduces the size of `Expr` from 72 to 64 bytes by storing only the start offset on `ExprCall` (64 to 56 bytes) and boxing the uncommon implicitly concatenated string representation in `ExprStringLiteral` (64 to 48 bytes). Both changes are required to reduce the size of the containing `Expr` enum. `ExprCall` derives its end offset from its argument list; parser and error-recovery paths assert that their ranges share the same end. --------- Co-authored-by: Charlie Marsh <charliemarsh@openai.com>
3 participants
Summary
This PR reduces the size of
Exprfrom 72 to 64 bytes by storing only the start offset onExprCall(64 to 56 bytes) and boxing the uncommon implicitly concatenated string representation inExprStringLiteral(64 to 48 bytes). Both changes are required to reduce the size of the containingExprenum.ExprCallderives its end offset from its argument list; parser and error-recovery paths assert that their ranges share the same end.