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

Use strings.Builder for gRPC percent methods #531

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

emcfarlane
Copy link
Contributor

Both grpcPercentEncode and grpcPercentDecode need to allocate a string and can use strings.Builder to own the allocation. Small optimization with Grow to give a hint to the size.

goos: darwin
goarch: arm64
pkg: github.com/bufbuild/connect-go
                      │ current.txt  │                new.txt                │
                      │    sec/op    │    sec/op     vs base                 │
GRPCPercentEncoding-8   408.6n ± ∞ ¹   371.7n ± ∞ ¹        ~ (p=1.000 n=1) ²
GRPCPercentDecoding-8   79.59n ± ∞ ¹   65.24n ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                 180.3n         155.7n        -13.65%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                      │ current.txt │               new.txt                │
                      │    B/op     │    B/op      vs base                 │
GRPCPercentEncoding-8   51.00 ± ∞ ¹   64.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
GRPCPercentDecoding-8   16.00 ± ∞ ¹   32.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                 28.57         45.25        +58.42%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                      │ current.txt │               new.txt                │
                      │  allocs/op  │  allocs/op   vs base                 │
GRPCPercentEncoding-8   7.000 ± ∞ ¹   2.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
GRPCPercentDecoding-8   1.000 ± ∞ ¹   1.000 ± ∞ ¹        ~ (p=1.000 n=1) ³
geomean                 2.646         1.414        -46.55%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05
³ all samples are equal

@emcfarlane emcfarlane self-assigned this Jun 26, 2023
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.

Works for me :)

@emcfarlane emcfarlane merged commit a93cd05 into main Jun 26, 2023
@emcfarlane emcfarlane deleted the emcfarlane/grpc-encoders branch June 26, 2023 15:12
renovate bot referenced this pull request in open-feature/flagd Jun 27, 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 | minor | `v1.8.0` -> `v1.9.0` |

---

### Release Notes

<details>
<summary>bufbuild/connect-go (github.com/bufbuild/connect-go)</summary>

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

[Compare
Source](https://togithub.com/bufbuild/connect-go/compare/v1.8.0...v1.9.0)

#### What's Changed

Along with a few new features and bugfixes, this release includes a
variety of internal performance improvements.

[v1.8.0] BenchmarkConnect/unary-12 8415 1305116 ns/op 14449031 B/op 254
allocs/op
[v1.9.0] BenchmarkConnect/unary-12 10443 1151366 ns/op 6024079 B/op 236
allocs/op

##### Enhancements

- Allow clients to set Host in headers by
[@&#8203;emcfarlane](https://togithub.com/emcfarlane) in
[https://github.com/bufbuild/connect-go/pull/522](https://togithub.com/bufbuild/connect-go/pull/522)
- Allow Connect clients to configure max HTTP GET size by
[@&#8203;jhump](https://togithub.com/jhump) in
[https://github.com/bufbuild/connect-go/pull/529](https://togithub.com/bufbuild/connect-go/pull/529)
- Reduce allocations in HTTP routing by
[@&#8203;mattrobenolt](https://togithub.com/mattrobenolt) in
[https://github.com/bufbuild/connect-go/pull/519](https://togithub.com/bufbuild/connect-go/pull/519)
- Cache mapping of methods to protocol handlers by
[@&#8203;emcfarlane](https://togithub.com/emcfarlane) in
[https://github.com/bufbuild/connect-go/pull/525](https://togithub.com/bufbuild/connect-go/pull/525)
- Improve performance of percent encoding by
[@&#8203;emcfarlane](https://togithub.com/emcfarlane) in
[https://github.com/bufbuild/connect-go/pull/531](https://togithub.com/bufbuild/connect-go/pull/531)
- Reduce marshaling allocations with MarshalAppend by
[@&#8203;emcfarlane](https://togithub.com/emcfarlane) in
[https://github.com/bufbuild/connect-go/pull/503](https://togithub.com/bufbuild/connect-go/pull/503)

##### Bugfixes

- Discard unknown JSON fields by default by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/518](https://togithub.com/bufbuild/connect-go/pull/518)
- Canonicalize Connect streaming trailer names by
[@&#8203;jchadwick-buf](https://togithub.com/jchadwick-buf) in
[https://github.com/bufbuild/connect-go/pull/528](https://togithub.com/bufbuild/connect-go/pull/528)

##### Other changes

- Tighten internal lint checks by
[@&#8203;zchee](https://togithub.com/zchee) in
[https://github.com/bufbuild/connect-go/pull/520](https://togithub.com/bufbuild/connect-go/pull/520)
- Simplify code when pooled buffers aren't returned by
[@&#8203;pkwarren](https://togithub.com/pkwarren) in
[https://github.com/bufbuild/connect-go/pull/532](https://togithub.com/bufbuild/connect-go/pull/532)

#### New Contributors

- [@&#8203;zchee](https://togithub.com/zchee) made their first
contribution in
[https://github.com/bufbuild/connect-go/pull/520](https://togithub.com/bufbuild/connect-go/pull/520)
- [@&#8203;emcfarlane](https://togithub.com/emcfarlane) made their first
contribution in
[https://github.com/bufbuild/connect-go/pull/522](https://togithub.com/bufbuild/connect-go/pull/522)

**Full Changelog**:
bufbuild/connect-go@v1.8.0...v1.9.0

</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://developer.mend.io/github/open-feature/flagd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNDEuMyIsInVwZGF0ZWRJblZlciI6IjM1LjE0MS4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
Both `grpcPercentEncode` and `grpcPercentDecode` need to allocate a
string and can use `strings.Builder` to own the allocation. Small
optimization with `Grow` to give a hint to the size.

```
goos: darwin
goarch: arm64
pkg: github.com/bufbuild/connect-go
                      │ current.txt  │                new.txt                │
                      │    sec/op    │    sec/op     vs base                 │
GRPCPercentEncoding-8   408.6n ± ∞ ¹   371.7n ± ∞ ¹        ~ (p=1.000 n=1) ²
GRPCPercentDecoding-8   79.59n ± ∞ ¹   65.24n ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                 180.3n         155.7n        -13.65%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                      │ current.txt │               new.txt                │
                      │    B/op     │    B/op      vs base                 │
GRPCPercentEncoding-8   51.00 ± ∞ ¹   64.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
GRPCPercentDecoding-8   16.00 ± ∞ ¹   32.00 ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean                 28.57         45.25        +58.42%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                      │ current.txt │               new.txt                │
                      │  allocs/op  │  allocs/op   vs base                 │
GRPCPercentEncoding-8   7.000 ± ∞ ¹   2.000 ± ∞ ¹        ~ (p=1.000 n=1) ²
GRPCPercentDecoding-8   1.000 ± ∞ ¹   1.000 ± ∞ ¹        ~ (p=1.000 n=1) ³
geomean                 2.646         1.414        -46.55%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05
³ all samples are equal
```
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