Use string based SourceLocationConverter init because it's faster. #200
Conversation
|
The tree one walks the full tree while the string one just looks for line breaks in a contiguous buffer, so the tree one is bound to be slower though I wasn't aware that it is significantly slower. Could you file a swift-syntax JIRA so we keep track of this? Not sure if there is something we can do but we should at least get a trace out of it. |
| /// - outputStream: A value conforming to `TextOutputStream` to which the formatted output will | ||
| /// be written. | ||
| /// - Throws: If an unrecoverable error occurs when formatting the code. | ||
| public func format<Output: TextOutputStream>( |
There was a problem hiding this comment.
We should keep the APIs that take a SourceFileSyntax as input; the goal here is to let someone construct a syntax tree in memory using SwiftSyntax APIs (e.g., a code generator) and then hand that off to the swift-format API to format it, without having to convert it to a string with description first and then reparse it again.
Can we have our cake and eat it too? If we make the new source argument of Context optional, then we could create the SourceLocationConverter using source if it's present, or fall back to the syntax tree if it's not. Then, we'd only be pessimizing performance of the case where we didn't have source text to begin with.
There was a problem hiding this comment.
That works - I hadn't considered a use case where we needed to support having just the syntax representation.
There was a problem hiding this comment.
I was thinking that we wouldn't change any of the public signatures—we wouldn't pass source at all into the version of the function that takes a SourceFileSyntax, because then we'd have to have a way to handle what would happen to the source locations if syntax.description and source didn't match.
I guess to keep the signatures all the same along with the cascading function calls, we'd need a private helper function that takes both syntax and source and have all the public ones delegate to that with the appropriate combinations, instead of having them delegate to a single bottom-most public API.
There was a problem hiding this comment.
That's reasonable, updated to use a private method that accepts both.
dylansturg
force-pushed
the
faster_context_parsing
branch
2 times, most recently
from
06e0161 to
32d0ed9
Compare
I filed https://bugs.swift.org/browse/SR-12972, let me know if I can provide any more helpful details. |
| /// - Parameters: | ||
| /// - syntax: The Swift syntax tree to be converted to be linted. | ||
| /// - url: A file URL denoting the filename/path that should be assumed for this syntax tree. | ||
| /// - source: The Swift source code. This can be provided to improve the linter's performance. |
There was a problem hiding this comment.
This parameter comment can be removed.
I'm not sure why, but the string based initializer for `SourceLocationConverter` is substantially faster than the syntax tree based initializer. The impact is a few hundred ms per file, so it is substantial when formatting many files. Results: - swift-protobuf 21.4 seconds before, 17.9 seconds after - 10k LOC file - 2.5 seconds before, 2.2 seconds after
dylansturg
force-pushed
the
faster_context_parsing
branch
from
32d0ed9 to
eadeff6
Compare
Use interpolated matrix values to denote Xcode and Swift versions
I'm not sure why, but the string based initializer for
SourceLocationConverteris substantially faster than the syntax tree based initializer.The impact is a few hundred ms per file, so it is substantial when formatting many files.
Results: