Make SyntaxCollections conform to BidirectionalCollection #149
Conversation
|
@swift-ci Please test |
|
@harlanhaskins You mean in terms of binary size?
|
Sources/SwiftSyntax/Syntax.swift
Outdated
| @@ -118,7 +121,10 @@ extension _SyntaxBase { | |||
| guard let parent = self.parent else { | |||
| return nil | |||
| } | |||
| for absoluteRaw in PresentRawSyntaxNextSiblings(self) { | |||
| let siblings = PresentRawSyntaxChildren(self.parent!) | |||
| let selfIndex = SyntaxChildrenIndex(self.data.absoluteRaw.info) | |||
There was a problem hiding this comment.
Should selfIndex be a property on Syntax/_SyntaxBase for convenience?
There was a problem hiding this comment.
Sounds reasonable. That way the index can also be retrieved externally and library users are able to efficiently slice the collection.
| let startIndex: SyntaxChildrenIndex | ||
| let endIndex: SyntaxChildrenIndex |
There was a problem hiding this comment.
I think the RawSyntaxChildren struct size will be smaller if you just store the given AbsoluteRawSyntax and make the existing fields calculated properties. Calculating the startIndex/endIndex should be cheap.
There was a problem hiding this comment.
My idea was that recomputing the endIndex might have an ever so slight performance impact that occurs every time the collection is iterated which might happen multiple times.
Increasing the size of the collection by 32 bytes means we only need to allocate 32 more bytes on the stack once which should not make a noticeable performance difference since we are already allocating the space for the rest of the struct anyway.
Anyway, there should not be a noticable performance difference with either implementation, I believe.
There was a problem hiding this comment.
I'd suggest to measure both ways. For example, the visitation performance regression could be because RawSyntaxChildren is constructed all the time for every index iteration access for a SyntaxCollection, while if RawSyntaxChildren is just a wrapper of the existing SyntaxCollection's AbsoluteRawSyntax then the optimizer may do a better job to remove overhead.
| let startIndex: SyntaxChildrenIndex | ||
| let endIndex: SyntaxChildrenIndex |
There was a problem hiding this comment.
startIndex does some amount of traversing so probably ok to store as field but endIndex can be a calculated property.
There was a problem hiding this comment.
Right now PresentRawSyntaxChildren.endIndex is initialized like this though:self.endIndex = allChildren.endIndex
It's not avoiding any calculation, it is storing the same value twice unnecessarily.
| private var presentRawChildren: PresentRawSyntaxChildren { | ||
| return PresentRawSyntaxChildren(self.data.absoluteRaw) | ||
| } |
There was a problem hiding this comment.
SyntaxCollection types are essentially arrays of nodes, they should not be containing nil children (e.g. note that the implementation of SyntaxCollection.count is based on this invariant), we should use RawSyntaxChildren here since that one has more efficient index iteration.
They could also get a distance(from:to:) implementation.
| self.indexInTree = indexInTree | ||
| } | ||
| } | ||
|
|
||
| /// Provides a stable and unique identity for `Syntax` nodes. | ||
| public struct SyntaxIdentifier: Hashable { |
There was a problem hiding this comment.
Not related to the overall PR but it occurred to me that this could use the Identifiable protocol.
There was a problem hiding this comment.
I thought the same and then forgot about it. Will do so in a follow-up PR.
ahoppen
force-pushed
the
syntax-collections
branch
from
3f5fc46 to
4eb99be
Compare
|
@akyrtzi Thanks for the feedback. Incorporated some of your suggestions and commented on others. I just checked visitor performance as well (running an empty visitor 50 times on
|
|
Due to the performance problems, I’m holding merging this back until we figured out what causes the degression and how to fix this. |
ahoppen
force-pushed
the
syntax-collections
branch
3 times, most recently
from
8e8089a to
6259a4f
Compare
ahoppen
force-pushed
the
syntax-collections
branch
from
6259a4f to
f8ef071
Compare
|
I have pretty much completely rewritten the PR to use custom iterators for forward iteration to speed up the common case. That way we still have the old |
|
@swift-ci Please test |
|
Build failed |
|
swiftlang/swift-stress-tester#111 @swift-ci Please test |
Add a group around member types used in return clauses.
This makes
SyntaxCollectionsconform toBidirectionalCollection, allowing multiple traversals of the collections. It also happens to clean the code inSyntaxChildrenup a little.