Significantly improve prefix trie performance and memory usage #109
Conversation
The previous prefix trie made a new `Node` for each character along the input string. In addition to being wasteful, this would introduce many more hops to match a given string. Instead, store common `Substring`s in the `Node`s, and use a sorted array/binary search through children to avoid extra time spend hashing and storing in hashed collections. Doing this reduces the number of nodes in the trie for the driver's options by an order of magnitude -- from 6601 nodes to 660. (This is actually a port of LLVM's Trie, but cleaned up and made more idiomatic in Swift)
harlanhaskins
force-pushed
the
trie-trie-again
branch
from
bb0a222 to
a184a67
Compare
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
|
It shaves off 3MB of memory, too. But that's not really that much. |
|
Woo this go into Swift-argument-parser too? |
|
I tried to work it in, but the swift-argument-parser doesn’t do the kind of ‘best prefix’ matching we need to do in the driver. I was planning to follow up with @natecook1000 about this specifically! |
| @@ -53,34 +53,23 @@ final class PrefixTrieTests: XCTestCase { | |||
|
|
|||
| trie["garbage"] = nil | |||
| XCTAssertNil(trie["garbage"]) | |||
| XCTAssertEqual(trie.nodeCount, 0) | |||
| XCTAssertEqual(trie.nodeCount, 2) | |||
| // Removing a node leaves the entry in the trie | |||
|
|
|||
| trie["12345"] = 12345 // 5 nodes | |||
| trie["12367"] = 12367 // 3 common nodes, 2 new nodes | |||
There was a problem hiding this comment.
Should probably update these comments.
|
|
||
| /// There's a common part of the string, but neither the label nor the | ||
| /// query string were matched entirely. | ||
| case haveCommonPart(Int) |
There was a problem hiding this comment.
nit: label the parameter length or document what the int means.
| /// match made. | ||
| func query<S: StringProtocol>(_ s: S) -> QueryResult { | ||
| let stringCount = s.count | ||
| let labelCount = label.count |
There was a problem hiding this comment.
nit: I think it would be easier to follow if you kept consistent naming.
query, queryCount, queryIndex
label, labelCount, labelIndex
| } | ||
| case .dontMatch: | ||
| fatalError("Impossible because we found a child with id \(id)") | ||
| default: |
There was a problem hiding this comment.
Explicitly declare the last 2 cases, so the compiler catches if more are added.
| /// Each node is identified in the child list by the first character in its | ||
| /// label. | ||
| var id: Character { | ||
| label.first! |
There was a problem hiding this comment.
I personally have always liked label[0] to avoid the implicitly unwrapped optional, but do you know if there are any performance differences between the 2? Maybe the optimize the same, I'll try it out.
There was a problem hiding this comment.
In this case, the label is a Substring which doesn’t have an int subscript.
There was a problem hiding this comment.
Ah, I guess you could use https://developer.apple.com/documentation/swift/substring/2894052-subscript
But label[label.startIndex] looks worse to me than label.first!.
Also, I tried tested it out and at least for an [Int] they are the same after optimizations.
|
I’ll get these in a follow-up, thanks @cltnschlosser! |
The previous prefix trie made a new
Nodefor each character along theinput string. In addition to being wasteful, this would introduce many
more hops to match a given string.
Instead, store common
Substrings in theNodes, and use a sortedarray/binary search through children to avoid extra time spend hashing
and storing in hashed collections.
Doing this reduces the number of nodes in the trie for the driver's
options by an order of magnitude -- from 6601 nodes to 660.
(This is actually a port of LLVM's Trie, but cleaned up and made more
idiomatic in Swift)