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 writing User-Agent header #446

Merged
merged 2 commits into from
Jan 27, 2023

Conversation

mattrobenolt
Copy link
Contributor

The fmt.Sprintf stood out again in my profiling as a relatively hot path, and through the lifetime of an application, this value will not change. So we can generate this header value at init() time and avoid the extra memory allocation as well on each request.

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

@mattrobenolt
Copy link
Contributor Author

I'm not sure if you'd want to exclude these linter failures since this pattern is already used elsewhere, and doing this any other way would also require a global variable. (with a sync.Once)

@mattrobenolt mattrobenolt force-pushed the optimize-user-agent branch 2 times, most recently from 2b04189 to fcbb808 Compare January 26, 2023 21:43
The fmt.Sprintf stood out again in my profiling as a relatively hot
path, and through the lifetime of an application, this value will not
change. So we can generate this header value at init() time and avoid
the extra memory allocation as well on each request.
@bufdev
Copy link
Member

bufdev commented Jan 27, 2023

Also just a comment, we/I hate force pushes on PRs because it overwrites review context...I only mention it as I assume you're going to be a long-time contributor and figured worth getting on same page :-)

@mattrobenolt
Copy link
Contributor Author

Also just a comment, we/I hate force pushes on PRs because it overwrites review context

👍

Would you typically then squash merge? As long as it's squashed, that's fine with me.

@pkwarren
Copy link
Contributor

pkwarren commented Jan 27, 2023

Would you typically then squash merge? As long as it's squashed, that's fine with me.

Yep - that's the only option on most/all of our repos.

@pkwarren pkwarren requested a review from akshayjshah January 27, 2023 04:38
Copy link
Contributor

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will wait for @akshayjshah/@bufdev to merge.

Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'd prefer to stop here - we're eating a slice allocation every time we write this header, but I don't want a global []string here. (I say this because long ago I tried to optimize away all the slice allocations, but the safety tradeoffs weren't worth it for a Buf-supported library.)

@akshayjshah akshayjshah changed the title Optimize user agent creation Minimize allocations writing User-Agent header Jan 27, 2023
@akshayjshah akshayjshah merged commit 4c2c9a9 into connectrpc:main Jan 27, 2023
@mattrobenolt mattrobenolt deleted the optimize-user-agent 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://togithub.com/bufbuild/connect-go)
| require | patch | `v1.5.0` -> `v1.5.1` |

---

### Release Notes

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

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

[Compare
Source](https://togithub.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://togithub.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://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/446](https://togithub.com/bufbuild/connect-go/pull/446)
- Minimize allocations parsing Content-Type by
[@&#8203;mattrobenolt](https://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/444](https://togithub.com/bufbuild/connect-go/pull/444)
- Optimize header access by
[@&#8203;mattrobenolt](https://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/445](https://togithub.com/bufbuild/connect-go/pull/445)
- Optimize Peer lookups by
[@&#8203;mattrobenolt](https://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/447](https://togithub.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 pushed a commit that referenced this pull request Jul 26, 2023
The fmt.Sprintf stood out again in my profiling as a relatively hot
path, and through the lifetime of an application, this value will not
change. So we can generate this header value at init() time and avoid
the extra memory allocation as well on each request.

**Before submitting your PR:** Please read through the contribution
guide at
https://github.com/bufbuild/connect-go/blob/main/.github/CONTRIBUTING.md
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.

4 participants