Skip to content
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

[6.0] Performance improvements for SyntaxRewriter #2742

Merged
merged 12 commits into from
Jul 19, 2024

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Jul 18, 2024

Cherry-pick #2726 #2728 #2729 #2741 into release/6.0

  • Explanation: Several performance optimizations mainly for BaiscFormat performance:
    • Factor-out SyntaxVisitor's Syntax.Info reusing mechanism into SyntaxNodeFactory and adopt it in SyntaxRewriter
    • Optimize SyntaxRewriter's new layout creation logic using manually-allocated UnsafeMutableBufferPointer
    • Mark the parameter of Syntax/SyntaxProtocol casting initializers __shared to save retain/release operations
    • Turn some static computed properties into stored constants Specifically, OperatorTable.standardOperators and SyntaxProtocol.structure so that the values are cached.
  • Scope: Syntax performance
  • Risk: Low-Mid. This introduces some UnsafeMutablePointer operations. But tested with ASAN
  • Testing: Passes the current test suite. PR tested with ASAN and LSAN
  • Issue: Part of rdar://130478685
  • Reviewer: Alex Hoppen (@ahoppen)

rintaro added 12 commits July 18, 2024 16:17
Similar treatment as 'SyntaxVisitor'. Reuse `Syntax.Info` when it's safe
to do so (i.e. uniquely referenced)

(cherry picked from commit 6be8e8d)
Factor out 'Syntax.Info' reusing into 'SyntaxNodeFactory' and
'SyntaxInfoRepository' as the backing storage.
The underlying storage now uses a fixed length 'ManagedBuffer' to avoid
'ContiguousArray' overhead.

(cherry picked from commit b6898d1)
Use manually allocated 'UnsafeBufferPointer' to avoid 'ContiguousArray'
overhead. Pre-initialize the buffer with the "old" layout at once, and
update each element only when updated.

(cherry picked from commit e664473)
This was a generic function only for casting the result at the end.
Hoist the casting part to the caller and make 'visitChilden()'
non-generic.

(cherry picked from commit e2d4423)
Simpify the iteration code.
Also, stop counting 'childIndex' as it can be retrieved from the
absolute info.

(cherry picked from commit b67e899)
Use 'SyntaxNodeFactory' in 'SyntaxVisitor' too. Thanks to the simplified
implementation, it improves the performance a bit.

(cherry picked from commit 7298fe8)
There's no performance reason to return 'nil'

(cherry picked from commit ef8bb05)
Workaround for Windows command line length limitation

(cherry picked from commit ec96b4d)
Initializer parameters are "+1" in caller site, but these casting
initializers only uses the ._syntaxNode and don't store the parameter itself.

(cherry picked from commit 99b670f)
Similar to `SyntaxProtocol.init()` initializers.

(cherry picked from commit c5526ac)
Returnning array literal from computed property allocates and
initializes the array buffer every time it's called, and they are
deallocated after use.
Make them stored properties so they stay in memory.

(cherry picked from commit 92dbbf8)
Building standard operator table is not trivial. Make is an immutable
stored property so it stays in memory once it's initialized.

(cherry picked from commit fad2743)
@rintaro
Copy link
Member Author

rintaro commented Jul 18, 2024

@swift-ci Please test

@rintaro rintaro merged commit 5d4b04d into swiftlang:release/6.0 Jul 19, 2024
3 checks passed
SimplyDanny added a commit to SimplyDanny/swift-syntax that referenced this pull request Aug 12, 2024
The changes introduced by swiftlang#2742 in SyntaxRewriter require at least Swift 5.8.
ahoppen pushed a commit that referenced this pull request Aug 12, 2024
The changes introduced by #2742 in SyntaxRewriter require at least Swift 5.8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants