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

Minimize allocations parsing Content-Type #444

Merged

Conversation

mattrobenolt
Copy link
Contributor

@mattrobenolt mattrobenolt commented Jan 26, 2023

For context, this function is run on every request within ServeHTTP, and the mime parsing and now lowercasing, is a marginal, but non trivial amount of memory allocations.

The expensive bit is hitting the mime.FormatMediaType path, when we already have a canonical form.

This removes the most expensive part over arguable the most common cases where there are no additional parameters on the Content-Type.

$ go test -bench '^BenchmarkCanonicalizeContentType$' -run '^$' .
goos: darwin
goarch: arm64
pkg: github.com/bufbuild/connect-go
BenchmarkCanonicalizeContentType/simple-10         	92344741	        12.85 ns/op	       0 B/op	       0 allocs/op
BenchmarkCanonicalizeContentType/with_charset-10   	 1744219	       693.8 ns/op	     424 B/op	       6 allocs/op
BenchmarkCanonicalizeContentType/with_other_param-10         	 1969113	       614.4 ns/op	     424 B/op	       6 allocs/op
PASS
ok  	github.com/bufbuild/connect-go	5.800s

Before submitting your PR: Please read through the contribution guide at https://github.com/bufbuild/connect-go/blob/main/.github/CONTRIBUTING.md

For context, this function is run on every request within ServeHTTP, and
the mime parsing and now lowercasing, is a marginal, but non trivial
amount of memory allocations.

The expensive bit is hitting the mime.FormatMediaType path, when we
already have a canonical form.

This removes the most expensive part over arguable the most common cases
where there are no additional parameters on the Content-Type.

```
$ go test -v -bench '^BenchmarkCanonicalizeContentType$' -run '^$' .
goos: darwin
goarch: arm64
pkg: github.com/bufbuild/connect-go
BenchmarkCanonicalizeContentType
BenchmarkCanonicalizeContentType/simple
BenchmarkCanonicalizeContentType/simple-10         	 7160896	       157.4 ns/op	      48 B/op	       1 allocs/op
BenchmarkCanonicalizeContentType/with_charset
BenchmarkCanonicalizeContentType/with_charset-10   	 1780041	       674.4 ns/op	     424 B/op	       6 allocs/op
BenchmarkCanonicalizeContentType/with_other_param
BenchmarkCanonicalizeContentType/with_other_param-10         	 2029819	       592.6 ns/op	     424 B/op	       6 allocs/op
PASS
ok  	github.com/bufbuild/connect-go	5.129s
```
@mattrobenolt mattrobenolt force-pushed the optimize-canonicalize-content-type branch from 3dc9b60 to 3810312 Compare January 26, 2023 19:35
@mattrobenolt
Copy link
Contributor Author

As a follow up, maybe a micro-optimization, but it might be worth having a switch statement to check for the known types to avoid even going through the mime.ParseMediaType path.

I only think this is a worthwhile optimization to make because for a connect client and a connect server, which in our case is all the time, we know the types it sent within a set of known types. So it seems pretty valid IMO to have a fast path for known client <> server hot path.

@mattrobenolt
Copy link
Contributor Author

mattrobenolt commented Jan 26, 2023

Gross, but kinda what I came up with:

diff --git a/protocol.go b/protocol.go
index 3fd4369..2ba638f 100644
--- a/protocol.go
+++ b/protocol.go
@@ -316,6 +316,10 @@ func flushResponseWriter(w http.ResponseWriter) {
 }
 
 func canonicalizeContentType(ct string) string {
+	switch ct {
+	case "application/connect+proto", "application/connect+json", "application/json", "application/proto", "application/grpc", "application/grpc-web", "application/grpc+json", "application/grpc+proto", "application/grpc-web+json", "application/json; charset=utf-8", "application/json; charset=UTF-8":
+		return ct
+	}
 	base, params, err := mime.ParseMediaType(ct)
 	if err != nil {
 		return ct

I'm not sure if there's a simpler way to build this list, but this makes this function effectively free when paired with a connect-go client.

In go1.19+, I believe this switch gets optimized to a jump table as well.

With this, we go from ~150ns on the hot path (with the current proposed patch) down to ~2ns and 0 allocations.

For precedent, stdlib sorta does a similar optimization for common known header values during the canonicalization.

https://cs.opensource.google/go/go/+/refs/tags/go1.19.5:src/net/textproto/reader.go;l=732-777;drc=89de906aa20b18f801b1c44abc03f298ae850954

We can do a similar approach with a map and a lazy init on it to populate, but I think overall it's a worthwhile optimization to make.

protocol_test.go Outdated Show resolved Hide resolved
@akshayjshah
Copy link
Member

@mattrobenolt Spiritually, I'm on board with being more even aggressive here - as long as we're in here optimizing, let's create a fast path that avoids all use of mime. The hard-coded switch you're suggesting isn't totally out of the question, but I'd like to avoid baking so much knowledge of the protocol(s) and encodings into this function. I think we can do nearly as well if we leverage a little knowledge of the mime-type grammar.

Let me push a commit here and see what you think.

@mattrobenolt
Copy link
Contributor Author

I like this. Pretty solid middleground.

@akshayjshah
Copy link
Member

@mattrobenolt LMK how this looks to you - with the commits I've pushed, the fast path is down to zero allocs and ~10ns/op. I think this is an okay middle ground - it gets a little into the weeds of what a valid content-type is, but it doesn't directly couple the canonicalizeContentType function to gRPC or Connect protocol details.

@akshayjshah
Copy link
Member

😆 OK you're faster than me

@akshayjshah akshayjshah changed the title Optimize canonicalizeContentType Minimize allocations parsing Content-Type Jan 27, 2023
@mattrobenolt
Copy link
Contributor Author

@akshayjshah I updated the benchmark in the description with the new hot path. Not quite down to 2ns, but I'll take 12ns. :)

@akshayjshah akshayjshah merged commit e488bce into connectrpc:main Jan 27, 2023
@mattrobenolt mattrobenolt deleted the optimize-canonicalize-content-type branch January 28, 2023 00:09
renovate bot referenced this pull request in open-feature/flagd Feb 2, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/bufbuild/connect-go](https://github.com/bufbuild/connect-go)
| require | patch | `v1.5.0` -> `v1.5.1` |

---

### Release Notes

<details>
<summary>bufbuild/connect-go</summary>

###
[`v1.5.1`](https://github.com/bufbuild/connect-go/releases/tag/v1.5.1)

[Compare
Source](https://github.com/bufbuild/connect-go/compare/v1.5.0...v1.5.1)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

Thanks to [@&#8203;mattrobenolt](https://github.com/mattrobenolt),
v1.5.1 exclusively contains performance improvements. There should be no
other user-visible behavior changes.

##### Bugfixes

- Minimize allocations writing User-Agent header by
[@&#8203;mattrobenolt](https://github.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/446](https://github.com/bufbuild/connect-go/pull/446)
- Minimize allocations parsing Content-Type by
[@&#8203;mattrobenolt](https://github.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/444](https://github.com/bufbuild/connect-go/pull/444)
- Optimize header access by
[@&#8203;mattrobenolt](https://github.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/445](https://github.com/bufbuild/connect-go/pull/445)
- Optimize Peer lookups by
[@&#8203;mattrobenolt](https://github.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/447](https://github.com/bufbuild/connect-go/pull/447)

**Full Changelog**:
bufbuild/connect-go@v1.5.0...v1.5.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/open-feature/flagd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMTkuMCIsInVwZGF0ZWRJblZlciI6IjM0LjExOS4wIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
akshayjshah added a commit that referenced this pull request Jul 26, 2023
For context, this function is run on every request within ServeHTTP, and
the mime parsing and now lowercasing, is a marginal, but non trivial
amount of memory allocations.

The expensive bit is hitting the mime.FormatMediaType path, when we
already have a canonical form.

This removes the most expensive part over arguable the most common cases
where there are no additional parameters on the Content-Type.

```
$ go test -bench '^BenchmarkCanonicalizeContentType$' -run '^$' .
goos: darwin
goarch: arm64
pkg: github.com/bufbuild/connect-go
BenchmarkCanonicalizeContentType/simple-10         	92344741	        12.85 ns/op	       0 B/op	       0 allocs/op
BenchmarkCanonicalizeContentType/with_charset-10   	 1744219	       693.8 ns/op	     424 B/op	       6 allocs/op
BenchmarkCanonicalizeContentType/with_other_param-10         	 1969113	       614.4 ns/op	     424 B/op	       6 allocs/op
PASS
ok  	github.com/bufbuild/connect-go	5.800s
```

---------

Co-authored-by: Akshay Shah <akshay@akshayshah.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants