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

ASTs for identical versions of reformatted file are parsed repeatedly during bisection #213

Closed
akaihola opened this issue Sep 30, 2021 · 1 comment · Fixed by #214
Closed
Labels
performance Speed or memory usage improvement
Milestone

Comments

@akaihola
Copy link
Owner

akaihola commented Sep 30, 2021

If AST verification fails, Darker starts to bisect in order to find the smallest possible number of extra diff context lines which preserves the AST. As noted by @rogalski #205, on each iteration of the loop in which AST verification fails,

During bisect we will be hitting cases where changing context_lines will not include new chunks in reformatted file. I believe caching state of intermediate comparisons (e.g. reformatted file -> comparison result dict) also makes a lot of sense.

Parsing identical versions of the reformatted file repeatedly is of course redundant. We could get rid of redundant calls and improve performance e.g. by wrapping the Black code invocation into a function decorated with @lru_cache.

Note that if we decide to reimplement AST verification partially as suggested in #211 and #212 (both closed by merged #214), we'd probably be caching results of black.parsing.parse_ast(dst_ast). Thus implementing the caching should only be done after those changes are either made or rejected.

The downside of re-implementing AST verification in Darker is that we wouldn't automatically benefit from any possible future refinements in Black to that code.

@akaihola akaihola added the performance Speed or memory usage improvement label Sep 30, 2021
@akaihola akaihola added this to the 1.4.0 milestone Sep 30, 2021
@akaihola akaihola linked a pull request Oct 31, 2021 that will close this issue
@akaihola
Copy link
Owner Author

#214 implemented the suggested caching mechanism as part of the ASTVerifier.is_equivalent_to_baseline() method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Speed or memory usage improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant