parser rewrite #9989
Closed
parser rewrite #9989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
new-cli
in place of inherited() — not quite useful yet since we don't distinguish plain and inheritFrom attr kinds so far.
this also has the effect of sorting let bindings lexicographically rather than by symbol creation order as was previously done, giving a better canonicalization in the process.
for plain inherits this is really just a stylistic choice, but for inherit-from it actually fixes an exponential size increase problem during expr printing (as may happen during assertion failure reporting, on during duplicate attr detection in the parser)
tomberek
added
the
idea approved
fricklerhandwerk
added
the
significant
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-123/39775/1 |
desugaring inherit-from to syntactic duplication of the source expr also duplicates side effects of the source expr (such as trace calls) and expensive computations (such as derivationStrict).
it's no longer widely used and has a rather confusing meaning now that inherit-from is handled very differently.
the parser modifies its inputs, which means that sharing them between the error context reporting system and the parser itself can confuse the reporting system. usually this led to early truncation of error context reports which, while not dangerous, can be quite confusing.
we already normalize attr order to lexicographic, doing the same for formals makes sense. doubly so because the order of formals would otherwise depend on the context of the expression, which is not quite as useful as one might expect.
previously we reported the error at the beginning of the binding block (for plain inherits) or the beginning of the attr list (for inherit-from), effectively hiding where exactly the error happened. this also carries over to runtime positions of attributes in sets as reported by unsafeGetAttrPos. we're not worried about this changing observable eval behavior because it *is* marked unsafe, and the new behavior is much more useful.
the parser treats a plain \r as a newline, error reports do not. this can lead to interesting divergences if anything makes use of this feature, with error reports pointing to wrong locations in the input (or even outside the input altogether).
this needs a string comparison because there seems to be no other way to get that information out of bison. usually the location info is going to be correct (pointing at a bad token), but since EOF isn't a token as such it'll be wrong in that this case. this hasn't shown up much so far because a single line ending *is* a token, so any file formatted in the usual manner (ie, ending in a line ending) would have its EOF position reported correctly.
we now keep not a table of all positions, but a table of all origins and their sizes. position indices are now direct pointers into the virtual concatenation of all parsed contents. this slightly reduces memory usage and time spent in the parser, at the cost of not being able to report positions if the total input size exceeds 4GiB. this limit is not unique to nix though, rustc and clang also limit their input to 4GiB (although at least clang refuses to process inputs that are larger, we will not). this new 4GiB limit probably will not cause any problems for quite a while, all of nixpkgs together is less than 100MiB in size and already needs over 700MiB of memory and multiple seconds just to parse. 4GiB worth of input will easily take multiple minutes and over 30GiB of memory without even evaluating anything. if problems *do* arise we can probably recover the old table-based system by adding some tracking to Pos::Origin (or increasing the size of PosIdx outright), but for time being this looks like more complexity than it's worth. since we now need to read the entire input again to determine the line/column of a position we'll make unsafeGetAttrPos slightly lazy: mostly the set it returns is only used to determine the file of origin of an attribute, not its exact location. the thunks do not add measurable runtime overhead. notably this change is necessary to allow changing the parser since apparently nothing supports nix's very idiosyncratic line ending choice of "anything goes", making it very hard to calculate line/column positions in the parser (while byte offsets are very easy).
this doesn't help much yet since the state objects themselves also leak all memory they are given.
now that destructors are hooked up we want to give the C skeleton a chance to actually run them. since bison does not call destructors on values that have been passed to semantic actions even when the action causes an abort we will also have to do some manual deleting. partially reverts e8d9de9.
storing a pointer only adds an unnecessary indirection and memory allocation.
almost all places where Exprs are passed as pointers expect the pointers to be non-null. pass them as references instead to encode this constraint in types.
with the prepatory work done this mostly means turning plain pointers into unique_ptrs, with all the associated churn that necessitates.
this gives about 20% performance improvements on pure parsing. obviously it'll be less on full eval, but depending on how much parsing has to be done (eg whether nixpkgs haskell modules are included or not) it ranges anywhere from 4% to 10% in our tests. this has been tested with thousands of core hours of fuzz testing to ensure that the ASTs produced by the new parser are exactly the same as the ones produced by the old parser. error messages will change (sometimes a lot) and are currently not perfect, but we'd rather leave that open for improvement than having this work rot forever.
|
Discussed during the Nix maintainers meeting on 2024-02-28. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1 |
|
🎉 All dependencies have been resolved ! |
|
initial high-level look:
please rebase now that #9874 is merged, would make review easier. |
|
unfortunately we do not have the energy to push the parser rewrite forward at this time. that's not to say we don't want to still see it happen (we do!), but for time being it'll have be on hold from our side. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-06-05-nix-team-meeting-minutes-150/46583/1 |
|
I assume with NixOS/nixpkgs#323845 someone else will have to pick up this right? |
|
i think this code was merged to lix |
|
Nix hasn't stood still in the meanwhile.
This would have to be measured again, since #11145 "improves parsing performance by ~6%, maybe more." #11072 has also repaid at least a bit of tech debt. I do like the idea of using a parser library instead, because it could indeed be a cleaner solution, but the impact is not as significant as it would have been around 2.20 or what the merge base of this is. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
the current parser is not really maintainable, slow, leaks memory all over the place, has very surprising short-range syntax interactions (as demonstrated by its large number of syntactic corner cases), uses a C skeleton and thus prohibits use of C++ features, the list just goes on and on. this exchanges the parser for one that is much faster, much more extensible, and reusable.
we have retrofitted some memory management into the old parser that is mostly (but not completely) correct since this was necessary to allow fuzzing of the old parser. we've run fuzzers over both parser for a couple thousands core hours to make sure that all valid inputs of the old parser are also valid inputs of the new one and produce the same AST, and that the new one doesn't accept anything the old one would reject (excluding bison implementation-defined errors like GLR stack exhaustion). unfortunately we can't share the test dataset because the fuzzers have managed to gobble up bits of private data and we will not be reviewing 50MB of junk to remove all of that again.
depends on #9874
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.