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

⚡️ perf(ctx.Range): reduce allocations #2705

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

nickajacks1
Copy link
Member

Description

strings.Split was causing extra allocations where strings.Cut can suffice. Also switch from strconv.Atoi because it causes an allocation when parsing a non-integer, which is common for Ranges.

The remaining allocation (see benchmarks) comes from the returned slice.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes
  • I tried to make my code as fast as possible with as few allocations as possible
  • For new code I have written benchmarks so that they can be analyzed and improved

Bench:

                            │  master.txt  │             branch.txt              │
                            │    sec/op    │   sec/op     vs base                │
_Ctx_Range/bytes=-700-8       197.80n ± 0%   65.06n ± 0%  -67.11% (p=0.000 n=20)
_Ctx_Range/bytes=500--8       198.10n ± 0%   63.70n ± 0%  -67.84% (p=0.000 n=20)
_Ctx_Range/bytes=500-1000-8   162.30n ± 0%   64.76n ± 0%  -60.10% (p=0.000 n=20)
_Ctx_Range/bytes=500-700-8    161.75n ± 0%   64.48n ± 0%  -60.14% (p=0.000 n=20)
geomean                        179.1n        64.50n       -63.98%

                            │ master.txt  │             branch.txt             │
                            │    B/op     │    B/op     vs base                │
_Ctx_Range/bytes=-700-8       144.00 ± 0%   16.00 ± 0%  -88.89% (p=0.000 n=20)
_Ctx_Range/bytes=500--8       144.00 ± 0%   16.00 ± 0%  -88.89% (p=0.000 n=20)
_Ctx_Range/bytes=500-1000-8    96.00 ± 0%   16.00 ± 0%  -83.33% (p=0.000 n=20)
_Ctx_Range/bytes=500-700-8     96.00 ± 0%   16.00 ± 0%  -83.33% (p=0.000 n=20)
geomean                        117.6        16.00       -86.39%

                            │ master.txt │             branch.txt             │
                            │ allocs/op  │ allocs/op   vs base                │
_Ctx_Range/bytes=-700-8       5.000 ± 0%   1.000 ± 0%  -80.00% (p=0.000 n=20)
_Ctx_Range/bytes=500--8       5.000 ± 0%   1.000 ± 0%  -80.00% (p=0.000 n=20)
_Ctx_Range/bytes=500-1000-8   4.000 ± 0%   1.000 ± 0%  -75.00% (p=0.000 n=20)
_Ctx_Range/bytes=500-700-8    4.000 ± 0%   1.000 ± 0%  -75.00% (p=0.000 n=20)
geomean                       4.472        1.000       -77.64%

@nickajacks1 nickajacks1 marked this pull request as ready for review November 6, 2023 04:14
@ReneWerner87
Copy link
Member

thanks for the optimization

unfortunately we can't do it like this
because of
image
image

maybe you can do the exchange of the cut function only for the golang version from 1.18 ? here an example
https://github.com/gofiber/fiber/blob/master/utils/convert_s2b_new.go#L1
https://github.com/gofiber/fiber/blob/master/utils/convert_s2b_old.go#L1

@efectn
Copy link
Member

efectn commented Nov 6, 2023

thanks for the optimization

unfortunately we can't do it like this because of image image

maybe you can do the exchange of the cut function only for the golang version from 1.18 ? here an example https://github.com/gofiber/fiber/blob/master/utils/convert_s2b_new.go#L1 https://github.com/gofiber/fiber/blob/master/utils/convert_s2b_old.go#L1

Or we can use strings.Index instead of strings.Cut. It might worth for performance gain on 1.17

@ReneWerner87
Copy link
Member

ok sure, lets test this

@nickajacks1
Copy link
Member Author

Sure, I'll try out strings.IndexByte

ctx_test.go Outdated
c := app.AcquireCtx(&fasthttp.RequestCtx{})
defer app.ReleaseCtx(c)

james := []struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

oops...I'll fix this too...

@gaby
Copy link
Member

gaby commented Nov 7, 2023

@nickajacks1

Error: ./ctx.go:1398:26: undefined: strings.Cut
Error: ./ctx.go:1410:40: undefined: strings.Cut
note: module requires Go 1.20

@ReneWerner87 ReneWerner87 added this to the v2 Next Release milestone Nov 7, 2023
@ReneWerner87
Copy link
Member

@nickajacks1 any progress?

@nickajacks1
Copy link
Member Author

Yes, sorry, been busy. Just need to find time to sit down and finish this. Should be ready within the next couple days.

strings.Split was causing extra allocations where using
strings.IndexByte can suffice. ALso switch from strconv.Atoi because it
causes an allocation when parsing a non-integer, which is common for
Ranges.
@nickajacks1
Copy link
Member Author

Removed strings.Cut, added more tests, updated Benchmark to include a case with multiple ranges.
New benchmarks:

                                  │  master.txt  │             branch.txt              │
                                  │    sec/op    │   sec/op     vs base                │
_Ctx_Range/bytes=-700-8             198.20n ± 1%   55.46n ± 1%  -72.02% (p=0.000 n=20)
_Ctx_Range/bytes=500--8             197.50n ± 0%   55.08n ± 0%  -72.11% (p=0.000 n=20)
_Ctx_Range/bytes=500-1000-8         160.60n ± 0%   55.93n ± 1%  -65.18% (p=0.000 n=20)
_Ctx_Range/bytes=0-700,800-1000-8   245.10n ± 0%   93.25n ± 0%  -61.95% (p=0.000 n=20)
geomean                              198.1n        63.18n       -68.11%

                                  │ master.txt  │             branch.txt             │
                                  │    B/op     │    B/op     vs base                │
_Ctx_Range/bytes=-700-8             144.00 ± 0%   16.00 ± 0%  -88.89% (p=0.000 n=20)
_Ctx_Range/bytes=500--8             144.00 ± 0%   16.00 ± 0%  -88.89% (p=0.000 n=20)
_Ctx_Range/bytes=500-1000-8          96.00 ± 0%   16.00 ± 0%  -83.33% (p=0.000 n=20)
_Ctx_Range/bytes=0-700,800-1000-8   176.00 ± 0%   48.00 ± 0%  -72.73% (p=0.000 n=20)
geomean                              136.8        21.06       -84.61%

                                  │ master.txt │             branch.txt             │
                                  │ allocs/op  │ allocs/op   vs base                │
_Ctx_Range/bytes=-700-8             5.000 ± 0%   1.000 ± 0%  -80.00% (p=0.000 n=20)
_Ctx_Range/bytes=500--8             5.000 ± 0%   1.000 ± 0%  -80.00% (p=0.000 n=20)
_Ctx_Range/bytes=500-1000-8         4.000 ± 0%   1.000 ± 0%  -75.00% (p=0.000 n=20)
_Ctx_Range/bytes=0-700,800-1000-8   6.000 ± 0%   2.000 ± 0%  -66.67% (p=0.000 n=20)
geomean                             4.949        1.189       -75.97%

@ReneWerner87 ReneWerner87 merged commit 5d888ce into gofiber:master Nov 10, 2023
20 checks passed
@nickajacks1 nickajacks1 deleted the ctx-range-perf branch November 11, 2023 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants