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.

Sign up for GitHub

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

Jump to bottom

Optimize Peer lookups #447

Merged
merged 3 commits into from
Jan 28, 2023
Merged

Optimize Peer lookups #447

merged 3 commits into from
Jan 28, 2023

Conversation

mattrobenolt
Copy link
Contributor

I don't know if there was an intention behind lazily initializing the Peer struct on-demand within the Peer() method, but from what I can tell, Peer() ends up being called 100% of the time in every path I've seen, and within NewClient, it's ultimately called twice.

Within NewClient, we also parse the params.URL then multiple times through url.ParseRequestURI then again through url.Parse, which is rather expensive and requires at least 1 heap allocation for the url.URL struct which we discard.

This behavior changes so the URL itself is parsed 1 time, then pass the url.URL struct to both functions that need it. As well as memoize the actual Peer on our concrete Client struct.

There is a slight change in behavior, but from my understanding, this seems safe:

validateRequestURL runs through url.ParseRequestURI, and newPeerFromURL was passed through url.Parse instead. From my understanding and my testing, url.ParseRequestURI is more strict than url.Parse, but since validateRequestURL was always called first, I don't particularly see how a less strict URL could have gotten through to the subsequent call to create the Peer.

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

mattrobenolt and others added 3 commits January 27, 2023 14:21
@mattrobenolt
Optimize Peer lookups
30570f4 
I don't know if there was an intention behind lazily initializing the
Peer struct on-demand within the Peer() method, but from what I can
tell, Peer() ends up being called 100% of the time in every path I've
seen, and within NewClient, it's ultimately called twice.

Within NewClient, we also parse the params.URL then multiple times
through url.ParseRequestURI then again through url.Parse, which is
rather expensive and requires at least 1 heap allocation for the url.URL
struct which we discard.

This behavior changes so the URL itself is parsed 1 time, then pass the
url.URL struct to both functions that need it. As well as memoize the
actual Peer on our concrete Client struct.

There is a slight change in behavior, but from my understanding, this
seems safe:

validateRequestURL runs through url.ParseRequestURI, and newPeerFromURL
was passed through url.Parse instead. From my understanding and my
testing, url.ParseRequestURI is more strict than url.Parse, but since
validateRequestURL was always called first, I don't particularly see how
a less strict URL could have gotten through to the subsequent call to
create the Peer.
@akshayjshah
Inline newGrpcPeerFromURL
5e211a7
@akshayjshah
Merge branch 'main' into optimize-peer
5cc29d1
@akshayjshah
Copy link
Member

Makes sense to me. I think I had some vague thought that Peer might contain some mutable data (a map or slice or something), so we'd want to make a fresh one on each call to Peer(). Clearly, that didn't end up happening, so this is a very nice improvement.

I pushed a commit that inlines the grpc-specific NewPeer function, but that's just a taste thing - code's great. ❤️

@akshayjshah akshayjshah merged commit e0629a7 into connectrpc:main Jan 28, 2023
@mattrobenolt mattrobenolt deleted the optimize-peer branch January 28, 2023 00:17
renovate bot referenced this pull request in open-feature/flagd Feb 2, 2023
@renovate
fix(deps): update module github.com/bufbuild/connect-go to v1.5.1 (#365)
e25f452 
[![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>
mattrobenolt added a commit to mattrobenolt/connect-go that referenced this pull request Feb 18, 2023
@mattrobenolt
client: Parse the url only once per Client
41e7dd4 
This stood out on flame graphs due ultimately to the
http.NewRequestFromContext call, which ends up calling url.Parse.

I did an optimization in connectrpc#447 which similarly, and this is a natural
extension of this.

We can just parse the url string once during NewClient, validate at that
point, then tag along the parsed *url.URL everywhere else and never use
it as a string again. This value never mutates through the lifetime of a
Client and isn't publicly available on any structs.
@mattrobenolt mattrobenolt mentioned this pull request Feb 18, 2023
Merged
mattrobenolt added a commit to mattrobenolt/connect-go that referenced this pull request Feb 18, 2023
@mattrobenolt
client: Parse the url only once per Client
cde703b 
This stood out on flame graphs due ultimately to the
http.NewRequestFromContext call, which ends up calling url.Parse.

I did an optimization in connectrpc#447 which similarly, and this is a natural
extension of this.

We can just parse the url string once during NewClient, validate at that
point, then tag along the parsed *url.URL everywhere else and never use
it as a string again. This value never mutates through the lifetime of a
Client and isn't publicly available on any structs.
akshayjshah pushed a commit that referenced this pull request Mar 6, 2023
@mattrobenolt
Parse the url only once per Client (#467)
6118a20 
This stood out on flame graphs due ultimately to the
http.NewRequestFromContext call, which ends up calling url.Parse and
this happens on every single RPC call.

I did an optimization in #447 which similarly, and this is a natural
extension of this.

We can just parse the url string once during NewClient, validate at that
point, then tag along the parsed *url.URL everywhere else and never use
it as a string again. This value never mutates through the lifetime of a
Client and isn't publicly available on any structs.

Before:
<img width="1026" alt="image"
src="https://user-images.githubusercontent.com/375744/219882639-b05416ee-3a8f-4b38-9716-9d66a34eeffc.png">

After:
<img width="1177" alt="image"
src="https://user-images.githubusercontent.com/375744/220477453-c1b48350-665b-495b-a070-3efb08e53067.png">
xxgreg pushed a commit to xxgreg/connect-go that referenced this pull request Apr 14, 2023
@greg-montoux
[connectrpc#447] add WithProtoJSONOptions() expose protojson marshall…
959e8a8 
…ing options
xxgreg pushed a commit to xxgreg/connect-go that referenced this pull request Apr 14, 2023
@greg-montoux
[connectrpc#447] add WithProtoJSONOptions() expose protojson marshall…
fe23a0f 
…ing options
akshayjshah added a commit that referenced this pull request Jul 26, 2023
@mattrobenolt @akshayjshah
Optimize Peer lookups (#447)
ba3169c 
Peer() is nearly always called, often more than once. Because Peer is a returned as a value and all fields are immutable strings, it's safe to create it once and memoize it. Along the way, this PR also cuts down on repeated URL parsing.

---------

Co-authored-by: Akshay Shah <ashah@buf.build>
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
@mattrobenolt
Parse the url only once per Client (#467)
c83da00 
This stood out on flame graphs due ultimately to the
http.NewRequestFromContext call, which ends up calling url.Parse and
this happens on every single RPC call.

I did an optimization in #447 which similarly, and this is a natural
extension of this.

We can just parse the url string once during NewClient, validate at that
point, then tag along the parsed *url.URL everywhere else and never use
it as a string again. This value never mutates through the lifetime of a
Client and isn't publicly available on any structs.

Before:
<img width="1026" alt="image"
src="https://user-images.githubusercontent.com/375744/219882639-b05416ee-3a8f-4b38-9716-9d66a34eeffc.png">

After:
<img width="1177" alt="image"
src="https://user-images.githubusercontent.com/375744/220477453-c1b48350-665b-495b-a070-3efb08e53067.png">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshayjshah akshayjshah akshayjshah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mattrobenolt @akshayjshah