Skip to content
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

Merged
merged 9 commits into from
Nov 5, 2020
Merged

Split path strings using UTF8 #1027

merged 9 commits into from
Nov 5, 2020

Conversation

PeterAdams-A
Copy link
Contributor

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%)

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%)
@PeterAdams-A
Copy link
Contributor Author

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.

Copy link
Collaborator

@glbrntt glbrntt left a 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

Sources/GRPC/GRPCServerRequestRoutingHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCServerRequestRoutingHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCServerRequestRoutingHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCServerRequestRoutingHandler.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCServerRequestRoutingHandler.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@glbrntt glbrntt left a 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.

Sources/GRPC/GRPCServerRequestRoutingHandler.swift Outdated Show resolved Hide resolved
/// URI format: "/package.Servicename/MethodName"
init?(requestURI: String) {
var utf8View = requestURI.utf8[...]
guard utf8View.splitFirst(separator: self.pathSplitDelimiter)?.count == 0 else {
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

@Lukasa Lukasa left a 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.

@PeterAdams-A PeterAdams-A requested a review from glbrntt November 4, 2020 09:15
@PeterAdams-A PeterAdams-A requested a review from glbrntt November 4, 2020 14:12
Copy link
Collaborator

@glbrntt glbrntt left a 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!

@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Nov 4, 2020
@glbrntt glbrntt merged commit d43f258 into grpc:main Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants