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

[SR-12972] SourceLocationConverter initializer that accepts a SourceFileSyntax is slower than the String version #410

Closed
swift-ci opened this issue Jun 10, 2020 · 3 comments
Labels
bug Something isn't working

Comments

@swift-ci
Copy link
Contributor

Previous ID SR-12972
Radar rdar://problem/64230275
Original Reporter dylansturg (JIRA User)
Type Bug

Attachment: Download

Additional Detail from JIRA
Votes 0
Component/s SwiftSyntax
Labels Bug
Assignee None
Priority Medium

md5: 4656fb002ec2b67ed48c8aa22d56eebf

Issue Description:

SourceLocationConverter has 2 initializers, one uses a String and the other uses a SourceFileSyntax. Based on my testing, the String based initializer is noticeably faster than using a SourceFileSyntax.

For a test case, I selected a relatively large file from swift-syntax (~ 10k LOC). The initializer that accepts a string takes ~ 2ms while the other initializer takes ~ 70 ms.

@akyrtzi
Copy link
Member

akyrtzi commented Jun 11, 2020

@swift-ci

@beccadax
Copy link
Contributor

@swift-ci create

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 9, 2022
@ahoppen
Copy link
Collaborator

ahoppen commented Oct 18, 2022

I was able to reproduce this to some degree with the following test:

import SwiftSyntax
import SwiftParser
import Foundation

let str = try! String(contentsOfFile: "/Users/alex/Downloads/SR-12972/SyntaxBuilders.swift")

let tree = try! Parser.parse(source: str)

// Warm up caches
for _ in 0..<10 {
  _ = SourceLocationConverter(file: "", source: str)
}
for _ in 0..<10 {
  _ = SourceLocationConverter(file: "", tree: tree)
}

do {
  let start = Date()
  for _ in 0..<100 {
    _ = SourceLocationConverter(file: "", source: str)
  }
  print("String based initializer took: \(Date().timeIntervalSince(start)/100 * 1000) ms")
}

do {
  let start = Date()
  for _ in 0..<100 {
    _ = SourceLocationConverter(file: "", tree: tree)
  }
  print("Tree-based initializer took: \(Date().timeIntervalSince(start)/100 * 1000) ms")
}

The output was as follows:

String based initializer took: 11.692179441452026 ms
Tree-based initializer took: 35.53478002548218 ms

This matches my expectations because the tree needs to first be converted into a source string before SourceLocationConverter can build up its line table. I suspect the original discrepancy was larger because the reporter was using a debug build of SwiftSyntax.

I’m closing this as correct behavior. If the performance of SourceLocationConverter is a bottleneck for someone in release builds, we can re-consider creating an optimized initializer for SourceLocationConverter that doesn’t convert the tree into a string first.

@ahoppen ahoppen closed this as completed Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants