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

templater: relax operator precedence rule to reduce possibility of large reparse #3449

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Apr 4, 2024

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

…rge reparse

After upgrading pest from 2.7.8 to 2.7.9, I noticed CLI tests got significantly
slow (something around 40sec -> 60sec on my laptop.) I suspect this would be
caused by detailed error state tracking introduced in 2.7.9, but it's also true
that our template grammar exercises such code path.

My understanding is that PEG is basically a top down parsing with unlimited
lookahead. Before this change, the default commit_summary template would be
parsed as follows:
 1. parse the outermost separate(..) as "term"
 2. "concat" rule can't continue, so
 3. reparse the whole string as "expression"
Because this pattern is not uncommon, I think it's better to rewrite the
grammar to avoid large retry.

With this patch, our tests runs within ~50sec under debug build. It appears to
save a few milliseconds in release build, but my development environment isn't
quiet enough to say the difference is significant.
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find!

@yuja yuja merged commit 32afea1 into jj-vcs:main Apr 4, 2024
16 checks passed
@yuja yuja deleted the push-uuxuxklosnuk branch April 4, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants