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

Make parser LALR, conflict-free #11145

Merged
merged 4 commits into from
Aug 4, 2024
Merged

Conversation

rhendric
Copy link
Member

Motivation

This improves parsing performance by ~6%, maybe more.

The only reason GLR parsing was needed was to resolve a reduce-reduce conflict that I've factored away (along with the one shift-reduce conflict).

Context

Below is a plot of the time required to parse every Nix file in Nixpkgs, at each commit in this PR (100 runs each, median times are marked):

Violin plot showing benchmark results

Note that benchmarks are based on nix-instantiate --parse, which includes the cost of showing expressions and sending them to /dev/null, so improvement may be more significant than presented.

I recommend reading this PR commit-by-commit. Each commit is hopefully self-explanatory, but the first three are only there so that the final GLR -> LALR refactoring goes smoothly. As you can see, they have no impact on parsing time.

In the first commit, I took the liberty of using Bison's named references feature for the rules I refactored, because it makes the refactoring less accident-prone and easier to read. I can back that out if it isn't house style, but I highly recommend it for anything but the simplest Bison grammars.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@rhendric rhendric requested a review from edolstra as a code owner July 21, 2024 02:20
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jul 21, 2024
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Tested and reviewed with @djacu and @JeremiahSecrist

Ran through tests and saw a perf improvement. Also ran into a segfault as stated here: #11141 (comment)

master is in a broken state at the moment, so let's defer this for the moment

Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Deferring this change until after the next release. Otherwise, all changes look good. Only remaining question is to understand the intended behavior of:

#define YYLTYPE_IS_TRIVIAL 1

Would also like a rebase to re-run against the new CI regression checks.

@Mic92
Copy link
Member

Mic92 commented Jul 22, 2024

Good job!

@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jul 23, 2024
@tomberek tomberek self-assigned this Jul 31, 2024
@tomberek tomberek merged commit ea1f87e into NixOS:master Aug 4, 2024
12 checks passed
@roberth roberth mentioned this pull request Aug 22, 2024
@rhendric rhendric deleted the rhendric/parser-lalr branch September 12, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants