Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview Lexer API cleanup: the salsa-tracked Reviewed by Cursor Bugbot for commit dcae7ca. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on orizi).
crates/cairo-lang-parser/src/lexer.rs line 257 at r1 (raw file):
} pub fn match_terminal<'a>(&mut self, db: &'a dyn Database) -> LexerTerminal<'a> {
why pub?
crates/cairo-lang-parser/src/parser.rs line 168 at r1 (raw file):
diagnostics: &'mt mut DiagnosticsBuilder<'a, ParserDiagnostic<'a>>, ) -> Self { let mut parser = Parser {
Can we remove tokenize all now?
… of cloning all tokens. `Parser::new` called the salsa-cached `tokenize_all` and deep-cloned the whole resulting `Deque<LexerTerminal>` into the parser on every parse (each terminal owns two trivia vectors). That full per-file token buffer is unnecessary: the parse is already memoized by `file_syntax_data`. Have the parser drive the `Lexer` directly, pulling terminals lazily into a small lookahead `VecDeque` (refilled on demand via `ensure_next_k_exists`), rather than consuming a clone of the cached token list. `tokenize_all` is left as-is (now used only by the lexer tests). -7.35% of cairo-to-diagnostics heap allocations on corelib; parser/lexer golden tests and corelib diagnostics unchanged.
081644c to
dcae7ca
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on eytan-starkware).
crates/cairo-lang-parser/src/lexer.rs line 257 at r1 (raw file):
Previously, eytan-starkware wrote…
why pub?
since it is used instead of the tokenize_all.
crates/cairo-lang-parser/src/parser.rs line 168 at r1 (raw file):
Previously, eytan-starkware wrote…
Can we remove tokenize all now?
moving it to the test - will see what else.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on orizi).
Summary
Replaces the pre-tokenized
Deque<LexerTerminal>inParserwith a lazily-drivenLexerinstance. Instead of callingtokenize_allupfront to lex the entire file before parsing begins, the parser now pulls tokens from the lexer on demand via a smallVecDeque<LexerTerminal>lookahead window (current_terminals). The window is kept filled to at least two terminals (fornext_terminal/next_next_terminal) and refilled to three before eachadvance()call. Aneofflag tracks when the lexer has emittedTerminalEndOfFileso no further pulls are attempted.Lexer::newandLexer::match_terminalare madepubto support this.Type of change
Please check one:
Why is this change needed?
Previously, the parser eagerly lexed the entire source file into a
Dequebefore any parsing work began. This meant the full token stream was allocated in memory upfront. By switching to lazy lexing, tokens are produced only as the parser consumes them, reducing peak memory usage and avoiding unnecessary work for early-exit or partial-parse scenarios.What was the behavior or documentation before?
Parser::newcalledtokenize_all, which lexed the entire input into aDeque<LexerTerminal>before returning. The parser then consumed from that pre-filled deque.What is the behavior or documentation after?
Parser::newconstructs aLexerdirectly and pre-fills only the first two terminals needed for lookahead. Subsequent terminals are pulled from the lexer one at a time as parsing advances, with the lookahead window never growing beyond a small constant size.Related issue or discussion (if any)
Additional context
The
cairo_lang_utils::deque::Dequeimport is replaced bystd::collections::VecDeque, and thetokenize_allfunction is no longer used by the parser.