Skip comments wherever a list element can start, not only at top level - #150
Skip comments wherever a list element can start, not only at top level#150MesTTo wants to merge 2 commits into
Conversation
`(foo (bar baz) ; a comment` with the closing bracket on the next line fails to parse with UnexpectedRightBracket. `sexpr`'s top-level loop treats `;` as trivia. The loop reading a list's elements skips whitespace only, so a comment there looks like the start of a child. The recursive call skips the comment, finds `)` where an expression was expected, and returns UnexpectedRightBracket. The arity had already been reserved for a child that parsed nothing. Both loops have to skip the same set, so they now share one `skip_trivia` and the two open-coded copies are gone. The list loop no longer inspects what it is looking at: skip trivia, and until the bracket, parse a child and bump the arity. A one-line `b';'` arm in the inner match fixes the symptom instead, and leaves the two loops free to drift apart again, which is what produced the bug. A `;` with no whitespace before it is still part of the symbol being read. Changing that would change how every symbol containing one parses, and is a decision about the language rather than a fix to this loop. The corpus already carries the case. Since 536c22b, `comment_before_closing_bracket.mm2` has shipped marked ;; @Skip parser bug: this shape gives UnexpectedRightBracket with the wiki block it was distilled from left unported. The `@skip` is gone, the entry pins its parsed space rather than only checking that the two engines agree, and the 59-line block is here too, because a one-liner passing says nothing about the shape it came from. It is MORK.wiki "MM2 tutorial: Reachability P2" block 12, which cannot load today. differential/run.py --slow --generated on main: both programs exit 101 (UnexpectedRightBracket) with this: 112 ok, 0 failed, 2 skipped Deleting the `b';'` arm from `skip_trivia` breaks the corpus, so the new code is exercised. cargo test passes for mork and mork-frontend.
d99c2dc to
46ffe72
Compare
MesTTo
changed the title
|
What's the impact on parsing performance? Doesn't this technically call the helper one too many times? Also, inlining. |
…parse cost
The bracket loop skips trivia to decide whether it is looking at `)`, and then the child it
recurses into skipped again on entry. The second skip always found nothing, because the first
had just left the cursor on a non-trivia byte. The version this replaces avoided that by
inspecting the byte itself before recursing, and only recursing when it was an element.
Splitting `sexpr` into the skipping entry point and `sexpr_at`, which takes the cursor already
on an element's first byte, keeps `sexpr`'s contract -- call it anywhere and it finds the next
element -- while letting the bracket loop skip exactly once per child.
Parsing, min-of-3 instructions:u over 120,000 statements, output identical:
before this commit after
no comments +3.775% +0.458%
a comment per line +3.750% +0.472%
deeply nested +3.571% +0.716%
The residue is the fix itself: recognising a comment where an element can start means testing
for `;` at each element boundary, which the loop this replaces did not do, and is exactly why
it could not read `(foo (bar baz) ; comment` with the bracket on the next line.
`#[inline]` on `skip_trivia` was measured separately and moves nothing -- 1,361,840,573 against
1,361,851,696 instructions:u, inside run-to-run spread -- so it is already being inlined and is
not carried here.
|
All three measured; you were right on the second one, and it was most of the cost. Parsing performance. min-of-3
"one too many times" — yes, exactly once too many, per element. The bracket loop skips Fixed by splitting Inlining. Measured on its own and it moves nothing: The residual 0.46% is the fix itself rather than overhead I can remove. Recognising a and why
|
2 participants
(foo (bar baz) ; a commentwith the closing bracket on the next line fails to parse withUnexpectedRightBracket.sexpr's top-level loop treats;as trivia. The loop reading a list's elements skips whitespace only, so a comment there looks like the start of a child. The recursive call skips the comment, finds)where an expression was expected, and returnsUnexpectedRightBracket. The arity had already been reserved for a child that parsed nothing.The corpus already carries the case. Since 536c22b,
differential/corpus/wiki/comment_before_closing_bracket.mm2has shipped marked;; @skip parser bug: this shape gives UnexpectedRightBracket, with the wiki block it was distilled from left unported.The fix
Both loops have to skip the same set, so they now share one
skip_triviaand the two open-coded copies are gone. The list loop no longer inspects what it is looking at: skip trivia, and until the bracket, parse a child and bump the arity.A one-line
b';'arm in the inner match fixes the symptom instead, and leaves the two loops free to drift apart again, which is what produced the bug. Say the word and I will reduce it to that; the corpus change is the same either way.A
;with no whitespace before it is still part of the symbol being read. Changing that would change how every symbol containing one parses, and is a decision about the language rather than a fix to this loop.Tests
The
@skipis gone, and the entry now pins its parsed space,(foo (bar baz)), rather than only checking that the two engines agree.reachability_p2_12.mm2is new: MORK.wiki "MM2 tutorial: Reachability P2" code block 12, the 59-line shape the one-liner was distilled from, which cannot load today. A one-liner passing says nothing about the shape it came from. It pins@steps 0and@expect-steps 0, followingunify/large_statement.mm2's precedent for a parse-only entry.Deleting the
b';'arm fromskip_triviabreaks the corpus, so the new code is exercised.cargo testpasses formorkandmork-frontend.