-
Notifications
You must be signed in to change notification settings - Fork 420
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
Split path strings using UTF8 #1027
Conversation
Motivation: For unary calls there can be a lot of path manipulation. Splitting in utf8 space is approximately 10x faster. Modifications: Hand craft a two part split using utf8. Result: Very small overall speed improvement (less than 1%)
For discussion on if this is worth it - makes things very slightly faster at the cost of more complex code. I'll not be upset if we decide not to do this as it ended up more complex than I originally thought it would be and the overall gains are quite small. |
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.
I'm generally on board with this, it's not that complex and has a useful performance gain. That said, it needs a little tidying up. You'll also need to run ./scripts/format.sh
and make generate-linuxmain
Co-authored-by: Cory Benfield <[email protected]> Co-authored-by: George Barnett <[email protected]>
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.
You still need to run make generate-linuxmain
. I have a couple of naming nits but looks great otherwise.
/// URI format: "/package.Servicename/MethodName" | ||
init?(requestURI: String) { | ||
var utf8View = requestURI.utf8[...] | ||
guard utf8View.splitFirst(separator: self.pathSplitDelimiter)?.count == 0 else { |
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.
nit: isEmpty
over count == 0
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.
I'm not sure you'll like my change here - I'm not sure I like it either but however you express this it's a little ugly.
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.
No notes other than @glbrntt's.
Co-authored-by: George Barnett <[email protected]>
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.
LGTM, thanks @PeterAdams-A!
Motivation:
For unary calls there can be a lot of path manipulation.
Splitting in utf8 space is approximately 10x faster.
Modifications:
Hand craft a two part split using utf8.
Result:
Very small overall speed improvement (less than 1%)