Add a serialized variant of the Trie-router #402
Conversation
Benchmark Results |
|
Can you post a benchmark comparison, with the original |
|
@adam-fowler the comparison is between |
|
I've reduces the bloat from my benchmark comment |
|
This is failing CI |
|
|
||
| // Serialize the path constant | ||
| try trie.writeLengthPrefixed(as: Integer.self) { buffer in | ||
| buffer.writeSubstring(path) |
There was a problem hiding this comment.
Given you only ever write Strings with a prefixed length why don't you add a ByteBuffer. writeLengthPrefixedString()
There was a problem hiding this comment.
In actual fact you could fix the issue where you are throwing an error from init and provide more detailing error information.
func writeLengthPrefixedString(_ string: SubString) {
do {
try trie.writeLengthPrefixed(as: Integer.self) { buffer in
buffer.writeSubstring(path)
}
} catch {
preconditionFailure("Failed to write \"\(string)\" into BinaryTrie")
}
}There was a problem hiding this comment.
Also have you checked performance if you use a UInt16. 64 bit integer is quite a bit of overkill here
| let _token: Integer = trie.readInteger(), | ||
| let token = TokenKind(rawValue: _token), | ||
| let nextSiblingNodeIndex: UInt32 = trie.readInteger() | ||
| { |
There was a problem hiding this comment.
Maybe add a ByteBuffer.readNode() that returns a BinaryTrieNode type to improve readability
| // Serialize the node's component | ||
| switch node.key { | ||
| case .path(let path): | ||
| trie.writeInteger(TokenKind.path.rawValue) |
There was a problem hiding this comment.
Add a ByteBuffer.writeNode() to match the ByteBuffer.readNode() mentioned above
Sources/Hummingbird/Router/BinaryTrie/BinaryTrie+serialize.swift
Outdated
Show resolved
Hide resolved
| private func descendPath( | ||
| in trie: inout ByteBuffer, | ||
| index: UInt16, | ||
| parameters: inout Parameters, |
There was a problem hiding this comment.
Given you return the parameters, why is this marked inout?
Since this trie is contiguous memory, routing performance should improve. Note that
Joannis
force-pushed
the
jo/serialized-trie-router
branch
from
a28940e
to
a75ae59
Compare
| return token | ||
| } | ||
|
|
||
| mutating func readBinaryTrieNode() -> BinaryTrieNode? { |
There was a problem hiding this comment.
This function doesn't compile and it doesn't seem to be used
| return nil | ||
| } | ||
|
|
||
| component = components[components.startIndex] |
There was a problem hiding this comment.
Can we guarantee at this point that the array is non-empty?
There was a problem hiding this comment.
haha. just saw the check above, perhaps you could change this to
guard let component = components.first else { return nil }
| import NIOCore | ||
|
|
||
| public struct BinaryRouterResponder<Context: BaseRequestContext>: HTTPResponder { | ||
| let trie: BinaryTrie<EndpointResponders<Context>> |
There was a problem hiding this comment.
This seems like an odd place to add this type. Might be worthwhile adding its own file. The original RouterResponder has its own file.
| if self.options.contains(.autoGenerateHeadEndpoints) { | ||
| self.trie.forEach { node in | ||
| node.value?.autoGenerateHeadEndpoint() | ||
| } | ||
| } | ||
| return RouterResponder( | ||
| return try! .init( |
|
|
||
| // Serialize the path constant | ||
| try trie.writeLengthPrefixed(as: Integer.self) { buffer in | ||
| buffer.writeSubstring(path) |
There was a problem hiding this comment.
In actual fact you could fix the issue where you are throwing an error from init and provide more detailing error information.
func writeLengthPrefixedString(_ string: SubString) {
do {
try trie.writeLengthPrefixed(as: Integer.self) { buffer in
buffer.writeSubstring(path)
}
} catch {
preconditionFailure("Failed to write \"\(string)\" into BinaryTrie")
}
}|
|
||
| // Serialize the path constant | ||
| try trie.writeLengthPrefixed(as: Integer.self) { buffer in | ||
| buffer.writeSubstring(path) |
There was a problem hiding this comment.
Also have you checked performance if you use a UInt16. 64 bit integer is quite a bit of overkill here
Since this trie is contiguous memory, routing performance should improve. This does not implement recursive wildcards yet, and I found that those tests are missing too.
In a quick test setup, the gap between no-router and having the old trie router was ~3-4k req/sec on a baseline (with router) of 98k. That gap shrunk by roughly 1k req/sec, leading to an estimated improvement of 25% in a small routing table (PerformanceBenchmarks).
I expect, and seemed to find a more significant performance gap when as the amount of path components increased. The expectation is based on the fact that more trie nodes are created when there are more path components. By keeping the routing table contiguous, it's expected that these scenarios are more performant.
APIs tend to have more path components to organise themselves, with many backends opting for
/api/v1-type of prefixes for versioning./api/v1/users/:id/profileis not a very long route.TODO