ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Investigate list parsing for possible performance improvement

Open dhruvmanila opened this issue 1 year ago • 4 comments
trafficstars

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 (https://github.com/astral-sh/ruff/pull/10786) showed a regression which got resolved (at least locally) by removing list parsing in the fast path (single assignment target).

dhruvmanila avatar Apr 09 '24 11:04 dhruvmanila

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)

MichaReiser avatar Apr 09 '24 12:04 MichaReiser

I also noticed that parse_conditionl_expression_or_higher is not getting inlined anymore after https://github.com/astral-sh/ruff/pull/10809. It could be because there's a new branch in the function:

https://github.com/astral-sh/ruff/blob/22d70b0f3325f42f735f7836299fe8dd8c1dd5b4/crates/ruff_python_parser/src/parser/expression.rs#L211-L212

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)
	}
}

dhruvmanila avatar Apr 09 '24 13:04 dhruvmanila

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

MichaReiser avatar Apr 09 '24 13:04 MichaReiser

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

I tried that, it didn't work

dhruvmanila avatar Apr 09 '24 16:04 dhruvmanila