-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Allow specialization of the RouterResponder #444
Conversation
83b9f5e
to
49fe5c8
Compare
12dc8e3
to
1f6d21b
Compare
49fe5c8
to
657aa5a
Compare
Can you get the Benchmarks and Tests working again, to indicate what we would need to make public. I guess for the tests it doesn't matter too much as you can import as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment above
# Conflicts: # Sources/Hummingbird/Router/Trie/Trie+serialize.swift
657aa5a
to
ee0dce5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue, that was inherited from the original array of structs PR
@@ -15,10 +15,12 @@ | |||
import HummingbirdCore | |||
|
|||
/// URI Path Trie Builder | |||
@_spi(Internal) public struct RouterPathTrieBuilder<Value: Sendable> { | |||
@_documentation(visibility: internal) | |||
public struct RouterPathTrieBuilder<Value: Sendable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need to be public. It is only ever used to build a router. I'm not sure why the TrieRouter serialize functions have been made @inlineable
. They don't need to be. Building a router doesn't need to be optmized., It is only the resolve functions that should be @inlineable
. By removing the inlineable on the serialize functions this can be make internal again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor compile error I fixed, but otherwise looks good
I've ran the
PerformanceTest
codebase two times, before and after this PR. Note that the PerformanceTest only uses 4 cores for HB2 - so that we have resource budget for regular OS use and thewrk
client benckmark suite.Since I made a lot of symbols inlinable, I had to make them public or internal. Public with SPI does not work with inlinable. We'll need to decide on a public API surface if we want benchmarks.
TODOs:
Before
After