Skip to content

#883 regressed performance by ~7% #894

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

Closed
ahoppen opened this issue Dec 10, 2024 · 4 comments · Fixed by #901 or #913
Closed

#883 regressed performance by ~7% #894

ahoppen opened this issue Dec 10, 2024 · 4 comments · Fixed by #901 or #913

Comments

@ahoppen
Copy link
Member

ahoppen commented Dec 10, 2024

#883 regressed swift-format’s performance by 7-8%. To reproduce run

cd /tmp
git clone https://github.com/apple/swift-syntax.git
cd swift-syntax
git checkout -f 509.0.0

cd path/to/swift-format
swift build -c release
.build/release/swift-format lint --measure-instructions --recursive /tmp/swift-syntax

And compare the instruction count between swift-format with and without #883.

@ahoppen
Copy link
Member Author

ahoppen commented Dec 10, 2024

Synced to Apple’s issue tracker as rdar://141233035

@macshome
Copy link
Contributor

In case anyone was wondering, here is the difference.

Before #883: Instructions executed: 67167400341
With #883: Instructions executed: 71260352299

@macshome
Copy link
Contributor

macshome commented Dec 13, 2024

Changing the code to:

let lines = text.count { $0 == "\n" }
    lineNumber += lines
    guard lines > 1, let range = text.range(of: "\n", options: .backwards) else {
      // Handle case where no newline exists
      column += text.count
      return
    }
    let lastLine = text[range.upperBound...]
    column = lastLine.count

Lowers the instructions executed to: 68378386615

@macshome
Copy link
Contributor

macshome commented Dec 13, 2024

After playing around in Godbolt it seems that lastIndex(of:_) uses a few less instructions per-loop than range(of:_). Changing the code to:

 let lines = text.count { $0 == "\n" }
    lineNumber += lines
    guard lines > 1, let lastNewlineIndex = text.lastIndex(of: "\n") else {
      // Handle case where no newline exists
      column += text.count
      return
    }
    let lastLine = text[text.index(after: lastNewlineIndex)...]
    column = lastLine.count

Further lowers the instruction executed count to: 68259258073

That's only about ~ 1.5% difference from before the changes.

macshome added a commit to macshome/swift-format that referenced this issue Dec 13, 2024
Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower.
macshome added a commit to macshome/swift-format that referenced this issue Dec 17, 2024
…yPrinter for swiftlang#894

Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower.

Using a lazy filter as `count(where:_)` isn't avaliable < Swift 6.0
macshome added a commit to macshome/swift-format that referenced this issue Dec 17, 2024
…mance Optimized the PrettyPrinter for swiftlang#894

Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower.

Using a lazy filter as `count(where:_)` isn't avaliable < Swift 6.0.

Evaluating the text in reverse to shave a few more instructions off.
macshome added a commit to macshome/swift-format that referenced this issue Dec 18, 2024
…mance PrettyPrinterPerformance Optimized the PrettyPrinter for swiftlang#894

Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower.

Using a lazy filter as `count(where:_)` isn't avaliable < Swift 6.0.

One forward loop and using the UTF8 view makes this faster than the original code pre-swiftlang#883.
ahoppen added a commit that referenced this issue Dec 19, 2024
PrettyPrinterPerformance Optimized the PrettyPrinter for #894
bnbarham added a commit that referenced this issue Dec 20, 2024
…mance

Revert "PrettyPrinterPerformance Optimized the PrettyPrinter for #894"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants