-
-
Notifications
You must be signed in to change notification settings - Fork 493
Re-enable Joop Leo right-recursion optimization (fixes #397) #1602
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
Open
erezsh
wants to merge
5
commits into
master
Choose a base branch
from
fix_joop_leo
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+187
−41
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8b2013e
Re-enable Joop Leo right-recursion optimization (fixes #397)
erezsh a0f980f
Simplify SymbolNode.load_paths: single pass with unified node_cache
erezsh 7f367ff
Add tests for more coverage
erezsh 583bac0
Remove three unreachable defensive branches in Joop Leo completer
erezsh 25347a5
Address PR review comments on Joop Leo re-enable
erezsh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ | |
| from ..utils import logger, OrderedSet, dedup_list | ||
| from .grammar_analysis import GrammarAnalyzer | ||
| from ..grammar import NonTerminal | ||
| from .earley_common import Item | ||
| from .earley_common import Item, TransitiveItem | ||
| from .earley_forest import ForestSumVisitor, SymbolNode, StableSymbolNode, TokenNode, ForestToParseTree | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -75,7 +75,7 @@ def __init__(self, lexer_conf: 'LexerConf', parser_conf: 'ParserConf', term_matc | |
| self.term_matcher = term_matcher | ||
|
|
||
|
|
||
| def predict_and_complete(self, i, to_scan, columns, transitives, node_cache): | ||
| def predict_and_complete(self, i, to_scan, columns, transitives, node_cache, start_symbol=None): | ||
| """The core Earley Predictor and Completer. | ||
|
|
||
| At each stage of the input, we handling any completed items (things | ||
|
|
@@ -86,6 +86,70 @@ def predict_and_complete(self, i, to_scan, columns, transitives, node_cache): | |
| # Held Completions (H in E.Scotts paper). | ||
| held_completions = {} | ||
|
|
||
| def is_quasi_complete(item): | ||
| quasi = item.advance() | ||
| while not quasi.is_complete: | ||
| if quasi.expect not in self.NULLABLE: | ||
| return False | ||
| if quasi.rule.origin == start_symbol and quasi.expect == start_symbol: | ||
| return False | ||
| quasi = quasi.advance() | ||
| return True | ||
|
|
||
| def create_leo_transitives(origin, start): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original implementation of this function contains code comments that would probably be helpful to retain. |
||
| visited = set() | ||
| to_create = [] | ||
| trule = None | ||
| previous = None | ||
|
|
||
| ### Recursively walk backwards through the Earley sets until we find the | ||
| # first transitive candidate. If this is done continuously, we shouldn't | ||
| # have to walk more than 1 hop. | ||
| while True: | ||
| if origin in transitives[start]: | ||
| previous = trule = transitives[start][origin] | ||
| break | ||
|
|
||
| if not self.FIRST[origin]: | ||
| break | ||
|
|
||
| if origin in self.NULLABLE: | ||
| break | ||
|
|
||
| candidates = [c for c in columns[start] if c.expect is not None and c.expect == origin] | ||
| if len(candidates) != 1: | ||
| break | ||
| originator = candidates[0] | ||
|
|
||
| if originator in visited: | ||
| break | ||
|
|
||
| visited.add(originator) | ||
| if not is_quasi_complete(originator): | ||
| break | ||
|
|
||
| trule = originator.advance() | ||
| if originator.start != start: | ||
| visited.clear() | ||
|
|
||
| to_create.append((origin, start, originator)) | ||
| origin = originator.rule.origin | ||
| start = originator.start | ||
|
|
||
| # If a suitable Transitive candidate is not found, bail. | ||
| if trule is None: | ||
| return | ||
|
|
||
| ### Now walk forwards and create Transitive Items in each set we walked through; | ||
| # and link each transitive item to the next set forwards. | ||
| while to_create: | ||
| origin, start, originator = to_create.pop() | ||
| if previous is not None: | ||
| titem = previous.next_titem = TransitiveItem(origin, trule, originator, previous.column) | ||
| else: | ||
| titem = TransitiveItem(origin, trule, originator, start) | ||
| previous = transitives[start][origin] = titem | ||
|
|
||
| column = columns[i] | ||
| # R (items) = Ei (column.items) | ||
| items = deque(column) | ||
|
|
@@ -99,24 +163,23 @@ def predict_and_complete(self, i, to_scan, columns, transitives, node_cache): | |
| item.node = node_cache[label] if label in node_cache else node_cache.setdefault(label, self.SymbolNode(*label)) | ||
| item.node.add_family(item.s, item.rule, item.start, None, None) | ||
|
|
||
| # create_leo_transitives(item.rule.origin, item.start) | ||
| create_leo_transitives(item.rule.origin, item.start) | ||
|
|
||
| ###R Joop Leo right recursion Completer | ||
| if item.rule.origin in transitives[item.start]: | ||
| transitive = transitives[item.start][item.s] | ||
| if transitive.previous in transitives[transitive.column]: | ||
| root_transitive = transitives[transitive.column][transitive.previous] | ||
| else: | ||
| root_transitive = transitive | ||
| transitive = transitives[item.start][item.rule.origin] | ||
| # Every titem is inserted into transitives[col][previous], | ||
| # so this lookup always finds the chain's root (or transitive | ||
| # itself when it is the root). | ||
| root_transitive = transitives[transitive.column][transitive.previous] | ||
|
|
||
| new_item = Item(transitive.rule, transitive.ptr, transitive.start) | ||
| label = (root_transitive.s, root_transitive.start, i) | ||
| new_item.node = node_cache[label] if label in node_cache else node_cache.setdefault(label, self.SymbolNode(*label)) | ||
| new_item.node.add_path(root_transitive, item.node) | ||
| if new_item.expect in self.TERMINALS: | ||
| # Add (B :: aC.B, h, y) to Q | ||
| to_scan.add(new_item) | ||
| elif new_item not in column: | ||
| # new_item.expect cannot be a terminal: is_quasi_complete | ||
| # required the post-recursion suffix to be all-nullable. | ||
| if new_item not in column: | ||
| # Add (B :: aC.B, h, y) to Ei and R | ||
| column.add(new_item) | ||
| items.append(new_item) | ||
|
|
@@ -127,8 +190,7 @@ def predict_and_complete(self, i, to_scan, columns, transitives, node_cache): | |
| # any predictions that result, that themselves require empty. Avoids | ||
| # infinite recursion on empty symbols. | ||
| # held_completions is 'H' in E.Scott's paper. | ||
| is_empty_item = item.start == i | ||
| if is_empty_item: | ||
| if item.start == i: | ||
| held_completions[item.rule.origin] = item.node | ||
|
|
||
| originators = [originator for originator in columns[item.start] if originator.expect is not None and originator.expect == item.s] | ||
|
|
@@ -169,22 +231,6 @@ def predict_and_complete(self, i, to_scan, columns, transitives, node_cache): | |
|
|
||
| def _parse(self, lexer, columns, to_scan, start_symbol=None): | ||
|
|
||
| def is_quasi_complete(item): | ||
| if item.is_complete: | ||
| return True | ||
|
|
||
| quasi = item.advance() | ||
| while not quasi.is_complete: | ||
| if quasi.expect not in self.NULLABLE: | ||
| return False | ||
| if quasi.rule.origin == start_symbol and quasi.expect == start_symbol: | ||
| return False | ||
| quasi = quasi.advance() | ||
| return True | ||
|
|
||
| # def create_leo_transitives(origin, start): | ||
| # ... # removed at commit 4c1cfb2faf24e8f8bff7112627a00b94d261b420 | ||
|
|
||
| def scan(i, token, to_scan): | ||
| """The core Earley Scanner. | ||
|
|
||
|
|
@@ -246,15 +292,15 @@ def scan(i, token, to_scan): | |
| i = 0 | ||
| node_cache = {} | ||
| for token in lexer.lex(expects): | ||
| self.predict_and_complete(i, to_scan, columns, transitives, node_cache) | ||
| self.predict_and_complete(i, to_scan, columns, transitives, node_cache, start_symbol) | ||
|
|
||
| to_scan, node_cache = scan(i, token, to_scan) | ||
| i += 1 | ||
|
|
||
| expects.clear() | ||
| expects |= {i.expect for i in to_scan} | ||
|
|
||
| self.predict_and_complete(i, to_scan, columns, transitives, node_cache) | ||
| self.predict_and_complete(i, to_scan, columns, transitives, node_cache, start_symbol) | ||
|
|
||
| ## Column is now the final column in the parse. | ||
| assert i == len(columns)-1 | ||
|
|
||
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement isn't covered by the test cases. So, either we are missing a test case that exercises this branch, or
start_symbolisn't actually needed here.(There are a couple other lines in the Joop Leo code that aren't covered, but this one is the most interesting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though one of the new test cases executes this branch, all the tests pass if this branch is removed, so I'm still not convinced it is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I guess I'll have to go a bit deeper into it myself, so I don't keep generating slop :)