Port incremental parsing ability to `CodeBlockItem` #1685

StevenWong12 · 2023-05-21T08:59:14Z

I want to make this pr as a start effort to port incremental parsing ability to SwiftParser.

StevenWong12 · 2023-06-03T07:47:29Z

To prevent incremental parse from missing the legal parts of re-used nodes ahead, I made following changes:

Add a property firstPotentialTokenChoices to SyntaxKind to represent the potential introducer for each SyntaxKind.
Add a property syntaxLayout to SyntaxKind to represent the layout of nodes in the form of SyntaxKind.
Add a property nextPotentialTokenChoices to RawSyntax to compute the next potential token of a certain node when it is initialized, which is essentially the next expected optional child of a node, and will be located by the last non-nil child. This mechanism is supported by syntaxLayout and firstPotentialTokenChoices.
Judge whether a node can be reused according to whether nextPotentialTokenChoices contains lookahead.currentToken.

This is still WIP but I am open to feedback.

StevenWong12 · 2023-06-04T07:32:32Z

I think this pr is ready for review.

I will squash the commits when there aren't any big problems with the design.

Sources/SwiftParser/IncrementalParseTransition.swift

ahoppen · 2023-06-20T18:32:19Z

Sources/SwiftParser/IncrementalParseTransition.swift

+  fileprivate var edits: ConcurrentEdits?
+  fileprivate var reusedDelegate: IncrementalParseReusedNodeDelegate?
+
+  public var furthestOffset: Int?


Split IncrementalParseTransition into two types as discussed offline.

I add a IncrementalParseNodeAffectRangeCollector to collect this and make it as a trigger to enable incremental parse.

Sources/SwiftParser/IncrementalParseTransition.swift

ahoppen · 2023-06-20T18:38:59Z

Sources/SwiftParser/Lexer/LexemeSequence.swift

+    func getOffsetToStart(_ token: Lexer.Lexeme) -> Int {
+      return self.sourceBufferStart.distance(to: token.cursor)
+    }


Not part of this PR: Can we use getOffsetToStart instead of offset(of:).

ahoppen · 2023-06-20T18:40:30Z

Sources/SwiftParser/Lookahead.swift

@@ -27,22 +27,30 @@ extension Parser {
    /// i.e. how far it looked ahead.
    var tokensConsumed: Int = 0

+    var parseTransition: IncrementalParseTransition?


As discussed offline, we could store furthestOffset in a standalone struct and pass that over here as an UnsafeMutablePointer.

Since we have to record the offset in both Parser.peek() and Lookahead.consumeAnyToken(), I make this pointer a member of LexemeSequence.

Sources/SwiftParser/Lookahead.swift

Sources/SwiftParser/Parser.swift

ahoppen · 2023-06-20T18:44:18Z

Sources/swift-parser-cli/Commands/PerformanceTest.swift

-          _ = Parser.parse(source: buf.bindMemory(to: UInt8.self))
+          previousTreeDict[file] = Parser.parse(source: buf.bindMemory(to: UInt8.self), parseTransition: parseTransition)


I would add a flag to decide whether we want to do incremental parsing on subsequent iterations.

I've added a flag about this, but I'm not quite sure whether the code is still a bit hacky.

Sources/SwiftParser/Parser.swift

Sources/SwiftParser/IncrementalParseTransition.swift

Sources/swift-parser-cli/Commands/PerformanceTest.swift

Tests/SwiftParserTest/IncrementalParsingTests.swift

Sources/swift-parser-cli/Commands/PerformanceTest.swift

Tests/SwiftParserTest/IncrementalParsingTests.swift

StevenWong12 · 2023-07-03T03:57:00Z

I add two Parser.parse methods which accept IncrementalParseTransition and return (tree: SourceFileSyntax, nodeAffectRangeCollector: IncrementalParseNodeAffectRangeCollector) as the incremental parse entries. Then we could use the nodeAffectRangeCollector returned by last parse to initialize IncrementalParseTransition. What do you think about it?

ahoppen

This looks very close now. I mostly have comments about naming and doc comments.

Sources/SwiftParser/IncrementalParseTransition.swift

CodeGeneration/Sources/generate-swiftsyntax/templates/swiftparser/ParserEntryFile.swift

ahoppen · 2023-07-06T13:45:30Z

Sources/SwiftParser/IncrementalParseTransition.swift

@@ -20,26 +20,39 @@
 /// This is also used for testing purposes to ensure incremental reparsing
 /// worked as expected.
 public protocol IncrementalParseReusedNodeDelegate {


Definitely not part of this PR but I think we should consider change IncrementalParseReusedNodeDelegate to just be a callback function on IncrementalParseTransition, i.e. removing this protocol altogether. That’s more swifty than the Objective-C delegate pattern.

Sources/SwiftParser/IncrementalParseTransition.swift

ahoppen · 2023-07-06T14:15:11Z

Sources/SwiftParser/Parser.swift

@@ -237,6 +262,7 @@ public struct Parser {
 extension Parser {
  /// Retrieves the token following the current token without consuming it.
  func peek() -> Lexer.Lexeme {
+    lexemes.recordFurthestOffset()


Just something that crossed my mind: Can’t we just call recordFurthestOffset from LexemeSequences.advance and LexemeSequence.peek? That way we can’t forget to call it from any Parser or Lookahead functions that interact with LexemeSequence. Or is there a performance regression associated with that?

Ah, I didn't do that because I was trap into some weird bugs and fail the tests then. And it turned out that it was because I called recordNextTokenInLookaheadTracker in the wrong time🥹.

Or is there a performance regression associated with that?

About the performance, see my comments below.

Sources/SwiftParser/Parser.swift

Sources/swift-parser-cli/Commands/PerformanceTest.swift

StevenWong12 · 2023-07-08T15:59:51Z

I re-run some performance tests at two points.

About the SyntaxCursor, we actually get many benefits from it (obvious in 100-iterations run) and I've been mis-using it before.

About calling recordNextTokenInLookaheadTracker in LexemeSequence. There aren't very much difference between calling in LexemeSequence and Lookahead.comsumeAnyToken/Parser.peek in terms of performance. And the difference is too small to be caught by instruments. I think calling them in LexemeSequence is more clear.

ahoppen

Even better performance, I love it.

I think the only non-trivial question that remains is who’s responsible for calling recordNextTokenInLookaheadTracker, everything else is super minor now.

Sources/SwiftParser/generated/Parser+Entry.swift

Sources/SwiftParser/Parser.swift

Sources/SwiftParser/Lookahead.swift

Sources/SwiftParser/Parser.swift

Tests/SwiftParserTest/Assertions.swift

ahoppen

One last comment to make a method private, otherwise this is looking great now 🕺🏽💃🏽.

Sources/SwiftParser/Lexer/LexemeSequence.swift

Use `LookaheadTracker` to track the furthest offset we reached and use `LookaheadRanges` to record the offset as the source range where might affect the parse of a node. And we could use that information to judge whether a node can be reused

ahoppen · 2023-07-10T14:41:34Z

🚀

@swift-ci Please test

ahoppen

🚀

ahoppen · 2023-07-11T05:03:14Z

swiftlang/swift-stress-tester#242

@swift-ci Please test macOS

ahoppen · 2023-07-11T05:03:20Z

swiftlang/swift-stress-tester#242

@swift-ci Please test Windows

StevenWong12 · 2023-07-11T07:43:00Z

Oops, the same problem as before. Maybe we need to re-trigger again🥲

ahoppen · 2023-07-11T09:55:33Z

swiftlang/swift-stress-tester#242

@swift-ci Please test macOS

ahoppen · 2023-07-11T13:25:39Z

swiftlang/swift-stress-tester#242

@swift-ci Please test macOS

StevenWong12 mentioned this pull request May 31, 2023

Add utils for incremental parse test #1712

Merged

StevenWong12 force-pushed the enable_incremental_parsing branch 2 times, most recently from 69f7cf2 to 7a19dca Compare June 3, 2023 07:33

StevenWong12 marked this pull request as ready for review June 4, 2023 07:32

StevenWong12 requested a review from ahoppen as a code owner June 4, 2023 07:32

StevenWong12 force-pushed the enable_incremental_parsing branch from 3ffcc72 to 0055ebe Compare June 11, 2023 15:23

StevenWong12 force-pushed the enable_incremental_parsing branch from 70b5a43 to 33f1a29 Compare June 19, 2023 09:13

ahoppen reviewed Jun 20, 2023

View reviewed changes

StevenWong12 force-pushed the enable_incremental_parsing branch from b0cf0d5 to 92d7ceb Compare June 25, 2023 13:01

StevenWong12 commented Jun 26, 2023

View reviewed changes

Sources/SwiftParser/Parser.swift Outdated Show resolved Hide resolved

ahoppen reviewed Jun 26, 2023

View reviewed changes

StevenWong12 force-pushed the enable_incremental_parsing branch from 92d7ceb to accde01 Compare June 26, 2023 16:13

ahoppen reviewed Jun 26, 2023

View reviewed changes

Sources/swift-parser-cli/Commands/PerformanceTest.swift Outdated Show resolved Hide resolved

Sources/swift-parser-cli/Commands/PerformanceTest.swift Outdated Show resolved Hide resolved

ahoppen reviewed Jun 26, 2023

View reviewed changes

Sources/swift-parser-cli/Commands/PerformanceTest.swift Outdated Show resolved Hide resolved

Sources/swift-parser-cli/Commands/PerformanceTest.swift Outdated Show resolved Hide resolved

ahoppen reviewed Jun 26, 2023

View reviewed changes

Sources/swift-parser-cli/Commands/PerformanceTest.swift Outdated Show resolved Hide resolved

Tests/SwiftParserTest/IncrementalParsingTests.swift Outdated Show resolved Hide resolved

StevenWong12 force-pushed the enable_incremental_parsing branch from 5edfc14 to 82c73e5 Compare July 3, 2023 03:51

This was referenced Jul 3, 2023

Port incremental parse ability to sourcekit-lsp swiftlang/sourcekit-lsp#761

Merged

Sync api change in IncrementalParseTransition swiftlang/swift-stress-tester#242

Merged

ahoppen reviewed Jul 6, 2023

View reviewed changes

StevenWong12 force-pushed the enable_incremental_parsing branch 2 times, most recently from bc9be97 to 05ea6d0 Compare July 8, 2023 15:47

ahoppen reviewed Jul 10, 2023

View reviewed changes

StevenWong12 force-pushed the enable_incremental_parsing branch from 05ea6d0 to fdcd570 Compare July 10, 2023 13:50

ahoppen reviewed Jul 10, 2023

View reviewed changes

Sources/SwiftParser/Lexer/LexemeSequence.swift Outdated Show resolved Hide resolved

Port incremental parse to CodeBlockItem

1e6406d

Use `LookaheadTracker` to track the furthest offset we reached and use `LookaheadRanges` to record the offset as the source range where might affect the parse of a node. And we could use that information to judge whether a node can be reused

StevenWong12 force-pushed the enable_incremental_parsing branch from fdcd570 to 1e6406d Compare July 10, 2023 14:40

ahoppen approved these changes Jul 10, 2023

View reviewed changes

ahoppen merged commit 6507f71 into swiftlang:main Jul 11, 2023

StevenWong12 mentioned this pull request Jul 15, 2023

Re-Enable Incremental Parsing Tests #1045

Closed

		_ = Parser.parse(source: buf.bindMemory(to: UInt8.self))
		previousTreeDict[file] = Parser.parse(source: buf.bindMemory(to: UInt8.self), parseTransition: parseTransition)

Port incremental parsing ability to CodeBlockItem #1685

Port incremental parsing ability to CodeBlockItem #1685

Uh oh!

Conversation

StevenWong12 commented May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StevenWong12 commented Jun 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StevenWong12 commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StevenWong12 commented Jul 3, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StevenWong12 Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StevenWong12 commented Jul 8, 2023

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Port incremental parsing ability to `CodeBlockItem` #1685

Port incremental parsing ability to `CodeBlockItem` #1685

StevenWong12 commented May 21, 2023 •

edited

Loading

StevenWong12 commented Jun 3, 2023 •

edited

Loading

StevenWong12 commented Jun 4, 2023 •

edited

Loading

StevenWong12 Jul 8, 2023 •

edited

Loading