Skip to content
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

Investigate list parsing for possible performance improvement #10847

Open
dhruvmanila opened this issue Apr 9, 2024 · 4 comments
Open

Investigate list parsing for possible performance improvement #10847

dhruvmanila opened this issue Apr 9, 2024 · 4 comments
Assignees
Labels
parser Related to the parser performance Potential performance improvement

Comments

@dhruvmanila
Copy link
Member

It seems like there could be performance improvements in list parsing. These methods are used to parse a sequence of nodes, optionally with a comma, which performs error recovery. This is only the case in the new parser.

This PR (#10786) showed a regression which got resolved (at least locally) by removing list parsing in the fast path (single assignment target).

@dhruvmanila dhruvmanila added performance Potential performance improvement parser Related to the parser labels Apr 9, 2024
@dhruvmanila dhruvmanila self-assigned this Apr 9, 2024
@MichaReiser
Copy link
Member

I wonder if that was simply because list parsing might not get inlined (our parser is now so fast that function calls could be expensive :D)

@dhruvmanila
Copy link
Member Author

I also noticed that parse_conditionl_expression_or_higher is not getting inlined anymore after #10809. It could be because there's a new branch in the function:

if self.at(TokenKind::Lambda) {
Expr::Lambda(self.parse_lambda_expr()).into()

We could create a new parse_lambda_expression_or_higher which calls into parse_conditional_expression_or_higher and update all references of the latter to use the former:

pub(super) fn parse_lambda_expression_or_higher(&mut self, allow_starred_expression: AllowStarredExpression) -> ParsedExpr {
	if self.at(TokenKind::Lambda) {
		Expr::Lambda(self.parse_lambda_expr()).into()
	} else {
		self.parse_conditional_expression_or_higher(allow_starred_expression)
	}
}

@MichaReiser
Copy link
Member

Yeah, or try to force inline by using #[inline]

@dhruvmanila
Copy link
Member Author

Yeah, or try to force inline by using #[inline]

I tried that, it didn't work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Related to the parser performance Potential performance improvement
Projects
None yet
Development

No branches or pull requests

2 participants