Skip to content

[Syntax] Perf: Optimize 'root' and 'ancestorOrSelf' #2842

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

Merged
merged 1 commit into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 65 additions & 6 deletions Sources/SwiftSyntax/Syntax.swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,13 @@ public struct Syntax: SyntaxProtocol, SyntaxHashable {
}
}

private var root: Syntax {
switch info.info! {
case .root(_): return self
case .nonRoot(let info): return info.parent.root
public var root: Syntax {
return self.withUnownedSyntax {
var node = $0
while let parent = node.parent {
node = parent
}
return node.value
}
}

Expand Down Expand Up @@ -129,7 +132,10 @@ public struct Syntax: SyntaxProtocol, SyntaxHashable {
}

/// "designated" memberwise initializer of `Syntax`.
init(_ raw: RawSyntax, info: Info) {
// transparent because normal inlining is too late for eliminating ARC traffic for Info.
// FIXME: Remove @_transparent after OSSA enabled.
@_transparent
Copy link
Member

Choose a reason for hiding this comment

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

Do we need @_transparent here? It does make debugging odd because you can no longer step into the function. Isn‘t @inline(__always) enough? Same for the other @_transparent functions. https://github.com/swiftlang/swift/blob/main/docs/TransparentAttr.md for reference.

Copy link
Member Author

@rintaro rintaro Sep 9, 2024

Choose a reason for hiding this comment

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

Unfortunately normal inlining is too late for eliminating ARC operations for:

  func withValue<T>(_ body: (Syntax) -> T) ->  T {
    info._withUnsafeGuaranteedRef {
      body(Syntax(self.raw, info: $0))
    }
  }

In https://godbolt.org/z/j1o6YM4Pz Try changing @_transparent to @inline(__always) on Syntax.init, and see the difference of withValue

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that’s unfortunate. Could you add a comment with this explanation to the @_transparent attribute?

init(_ raw: RawSyntax, info: __shared Info) {
self.raw = raw
self.info = info
}
Expand Down Expand Up @@ -309,7 +315,7 @@ public struct Syntax: SyntaxProtocol, SyntaxHashable {
/// Create a ``Syntax`` node from a specialized syntax node.
// Inline always so the optimizer can optimize this to a member access on `syntax` without having to go through
// generics.
@inline(__always)
@_transparent
public init(_ syntax: __shared some SyntaxProtocol) {
self = syntax._syntaxNode
}
Expand Down Expand Up @@ -380,6 +386,59 @@ extension Syntax {
}
}

/// Temporary non-owning Syntax.
///
/// This can be used for handling Syntax node without ARC traffic.
struct UnownedSyntax {
private let raw: RawSyntax
private let info: Unmanaged<Syntax.Info>

@_transparent
init(_ node: __shared Syntax) {
self.raw = node.raw
self.info = .passUnretained(node.info.unsafelyUnwrapped)
}

/// Extract the Syntax value.
@inline(__always)
var value: Syntax {
Syntax(raw, info: info.takeUnretainedValue())
}

/// Get the parent of the Syntax value, but without retaining it.
@inline(__always)
var parent: UnownedSyntax? {
return info._withUnsafeGuaranteedRef {
switch $0.info.unsafelyUnwrapped {
case .nonRoot(let info):
return UnownedSyntax(info.parent)
case .root(_):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case .root(_):
case .root:

Copy link
Member Author

@rintaro rintaro Sep 9, 2024

Choose a reason for hiding this comment

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

Looks like we're inconsistent. var nonRootInfo uses case .root(_). I personally prefer (_) style because it's more explicit on ignoring associated values.

return nil
}
}
}

/// Temporarily use the Syntax value.
@inline(__always)
func withValue<T>(_ body: (Syntax) -> T) -> T {
info._withUnsafeGuaranteedRef {
body(Syntax(self.raw, info: $0))
}
}
}

extension SyntaxProtocol {
/// Execute the `body` with ``UnownedSyntax`` of `node`.
///
/// This guarantees the life time of the `node` during the `body` is executed.
@inline(__always)
func withUnownedSyntax<T>(_ body: (UnownedSyntax) -> T) -> T {
return withExtendedLifetime(self) {
body(UnownedSyntax(Syntax($0)))
}
}
}

/// ``SyntaxNode`` used to be a pervasive type name in SwiftSyntax that has been
/// replaced by the ``Syntax`` type.
@available(*, unavailable, message: "use 'Syntax' instead")
Expand Down
22 changes: 11 additions & 11 deletions Sources/SwiftSyntax/SyntaxProtocol.swift
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,7 @@ extension SyntaxProtocol {

/// The root of the tree in which this node resides.
public var root: Syntax {
var this = _syntaxNode
while let parent = this.parent {
this = parent
}
return this
return Syntax(self).root
}

/// Whether or not this node has a parent.
Expand All @@ -241,14 +237,18 @@ extension SyntaxProtocol {

/// Returns this node or the first ancestor that satisfies `condition`.
public func ancestorOrSelf<T>(mapping map: (Syntax) -> T?) -> T? {
var walk: Syntax? = Syntax(self)
while let unwrappedParent = walk {
if let mapped = map(unwrappedParent) {
return mapped
return self.withUnownedSyntax {
var node = $0
while true {
if let mapped = node.withValue(map) {
return mapped
}
guard let parent = node.parent else {
return nil
}
node = parent
}
walk = unwrappedParent.parent
}
return nil
}
}

Expand Down