Skip to content

[Perf] Improve SyntaxRewriter visitation performance #2726

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jul 18, 2024
Merged

[Perf] Improve SyntaxRewriter visitation performance #2726

merged 8 commits into from
Jul 18, 2024

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Jul 12, 2024 •
edited
Loading

  • Introduce 'SyntaxNodeFactory'. This is an improved replacement of "Syntax.Info reusing mechanism" in SyntaxVisitor. Adopt it in both SyntaxVisitor and SyntaxRewriter
  • Improve new layout creation:
    • Use a manually allocated 'UnsafeBufferPointer' to avoid 'Array' related overhead
    • Pre-initialize the buffer with old layout at once, then only update each rewritten child element. This is simpler and faster than appending each element.

@rintaro
Copy link
Member Author

rintaro commented Jul 12, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jul 15, 2024

Too many changes in this PR. I will split this into several PRs.

@rintaro
[SyntaxRewriter] Optimize SyntaxRewriter visitation
6be8e8d 
Similar treatment as 'SyntaxVisitor'. Reuse `Syntax.Info` when it's safe
to do so (i.e. uniquely referenced)
@rintaro rintaro force-pushed the perf-rewriter branch 4 times, most recently from 7cbc241 to 5602b5c Compare July 15, 2024 18:35
@rintaro
Copy link
Member Author

rintaro commented Jul 15, 2024

@swift-ci Please test

rintaro
rintaro commented Jul 15, 2024
View reviewed changes
CodeGeneration/Sources/generate-swift-syntax/templates/swiftsyntax/SyntaxRewriterFile.swift
let rewritten = dispatchVisit(Syntax(absoluteRaw, parent: syntaxNode))
if rewritten.id != info.nodeId {
dispatchVisit(&childNode)
if childNode.raw.id != child.id {
Copy link
Member Author

@rintaro rintaro Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously it compared SyntaxIdentifier which takes the entire tree into account.
Now it compares RawSyntax.ID because we only care the RawSyntax here.

I.e. returning node.detached is NOT considered "rewritten"

rintaro added 5 commits July 15, 2024 14:00
@rintaro
[SyntaxRewriter] Introduce SyntaxNodeFactory
b6898d1 
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.
@rintaro
[SyntaxRewriter] Improve new layout node creation
e664473 
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.
@rintaro
[SyntaxRewriter] Make 'visitChildren()' a non-generic mehod
e2d4423 
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.
@rintaro
[SyntaxRewriter] Iterate RawSyntaxChildren using pattern matching
b67e899 
Simpify the iteration code.
Also, stop counting 'childIndex' as it can be retrieved from the
absolute info.
@rintaro
[SyntaxVisitor] Adopt SyntaxNodeFactory
7298fe8 
Use 'SyntaxNodeFactory' in 'SyntaxVisitor' too. Thanks to the simplified
implementation, it improves the performance a bit.
@rintaro
Copy link
Member Author

rintaro commented Jul 15, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jul 15, 2024

@swift-ci Please test Windows

1 similar comment
@rintaro
Copy link
Member Author

rintaro commented Jul 15, 2024

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Jul 16, 2024

@swift-ci Please clean test Windows

@rintaro
Copy link
Member Author

rintaro commented Jul 16, 2024

@swift-ci Please test

1 similar comment
@rintaro
Copy link
Member Author

rintaro commented Jul 16, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jul 16, 2024

@swift-ci Please test Linux

ahoppen
ahoppen reviewed Jul 16, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a couple minor comments.

Sources/SwiftSyntax/generated/SyntaxRewriter.swift
var rewrittens: ContiguousArray<Syntax> = []
// Keep 'SyntaxArena' of rewritten nodes alive until they are wrapped
// with 'Syntax'
var rewrittens: ContiguousArray<RetainedSyntaxArena> = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the performance characteristics of ContiguousArray vs Array here? I would expect Array to be faster because it doesn’t need to move all elements if it runs out of space. And I don’t think we need the contiguous property here anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I haven't tried Array here. interesting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array always uses contiguous storage unless it's backed by NSArray.
Although there's no particular reason to use ContiguousArray, its implementation is simpler than Array. So let's keep this as-is

@rintaro
Copy link
Member Author

rintaro commented Jul 16, 2024

@swift-ci Please test Linux

@rintaro
Copy link
Member Author

rintaro commented Jul 18, 2024

@swift-ci Please test

rintaro added 2 commits July 18, 2024 10:51
@rintaro
[SyntaxRewriter] Return the node as-is
ef8bb05 
There's no performance reason to return 'nil'
@rintaro
[CMake] Split 'touch' workaround into two commands
ec96b4d 
Workaround for Windows command line length limitation
@rintaro
Copy link
Member Author

rintaro commented Jul 18, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Jul 18, 2024

@swift-ci Please test Windows

@rintaro rintaro merged commit 62a352d into swiftlang:main Jul 18, 2024
3 checks passed
@rintaro rintaro mentioned this pull request Jul 18, 2024
Merged
@ahoppen ahoppen mentioned this pull request Aug 12, 2024
Closed
@SimplyDanny SimplyDanny mentioned this pull request Aug 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

@DougGregor DougGregor Awaiting requested review from DougGregor

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rintaro @bnbarham @ahoppen