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

v3(enhancement): remove utils.Trim* because stdlib has same performance in go1.19 #2030

Merged
merged 17 commits into from
Aug 20, 2022
Merged

v3(enhancement): remove utils.Trim* because stdlib has same performance in go1.19 #2030

merged 17 commits into from
Aug 20, 2022

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Aug 19, 2022 •
edited
Loading

This pr remove Trim, TrimRight and TrimLeft.

Stdlib functions have save performance now.

@trim21 trim21 marked this pull request as ready for review August 19, 2022 14:47
@trim21 trim21 mentioned this pull request Aug 19, 2022
Merged
@ReneWerner87
Copy link
Member

can you check my last comments from the old one ?
https://github.com/gofiber/fiber/blob/v3-beta/utils/bytes.go#L59

@trim21 trim21 changed the title v3(tests): use a table driven test for utils.EqualFold v3(enhancement): convert more utils to generic byteSeq function. Aug 19, 2022
@trim21 trim21 marked this pull request as draft August 19, 2022 15:02
@trim21 trim21 marked this pull request as ready for review August 19, 2022 15:12
efectn
efectn reviewed Aug 19, 2022
View reviewed changes
Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

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

can you update benchmarks on README.md

@trim21
Copy link
Contributor Author

trim21 commented Aug 19, 2022

do you considering using https://github.com/stretchr/testify instead of current AssertEqual 🤔

@trim21
Copy link
Contributor Author

trim21 commented Aug 19, 2022

can you update benchmarks on README.md

done

@trim21
Copy link
Contributor Author

trim21 commented Aug 19, 2022 •
edited
Loading

I just keep the old tests and benchmark as bytes inputs

trim21
trim21 commented Aug 19, 2022
View reviewed changes
ctx.go
@@ -1245,7 +1245,7 @@ func (c *DefaultCtx) WriteString(s string) (int, error) {
// XHR returns a Boolean property, that is true, if the request's X-Requested-With header field is XMLHttpRequest,
// indicating that the request was issued by a client library (such as jQuery).
func (c *DefaultCtx) XHR() bool {
return utils.EqualFoldBytes(utils.UnsafeBytes(c.Get(HeaderXRequestedWith)), []byte("xmlhttprequest"))
return utils.EqualFold(c.Get(HeaderXRequestedWith), "xmlhttprequest")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we use EqualFoldBytes instead of EqualFold in the first place?

@trim21
Copy link
Contributor Author

trim21 commented Aug 20, 2022 •
edited
Loading

I saw the updated benchmark result, I think we can remove these Trim* functions now 🤔

go 1.19

Benchmark_TrimRightBytes/fiber-16               100000000               10.66 ns/op            8 B/op          1 allocs/op
Benchmark_TrimRightBytes/default-16             100000000               10.84 ns/op            8 B/op          1 allocs/op

Benchmark_TrimLeftBytes/fiber-16                100000000               10.72 ns/op            8 B/op          1 allocs/op
Benchmark_TrimLeftBytes/default-16              100000000               11.29 ns/op            8 B/op          1 allocs/op

Benchmark_TrimBytes/fiber-16                    74933964                15.43 ns/op           16 B/op          1 allocs/op
Benchmark_TrimBytes/default-16                  74931624                15.86 ns/op           16 B/op          1 allocs/op

Benchmark_TrimRight/fiber-16                    484963700                2.471 ns/op           0 B/op          0 allocs/op
Benchmark_TrimRight/default-16                  454740096                2.614 ns/op           0 B/op          0 allocs/op

Benchmark_TrimLeft/fiber-16                     488699834                2.451 ns/op           0 B/op          0 allocs/op
Benchmark_TrimLeft/default-16                   423590457                2.835 ns/op           0 B/op          0 allocs/op

Benchmark_Trim/fiber-16                         296457481                4.023 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default-16                       322653328                3.804 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default.trimspace-16             248519290                4.811 ns/op           0 B/op          0 allocs/op

1.18 , our version is a little bit faster.

goos: windows
goarch: amd64
pkg: github.com/gofiber/fiber/v3/utils
cpu: AMD Ryzen 7 5800X 8-Core Processor
Benchmark_TrimRightBytes/fiber-16               100000000               10.49 ns/op            8 B/op          1 allocs/op
Benchmark_TrimRightBytes/default-16             99912576                11.57 ns/op            8 B/op          1 allocs/op

Benchmark_TrimLeftBytes/fiber-16                100000000               10.42 ns/op            8 B/op          1 allocs/op
Benchmark_TrimLeftBytes/default-16              99905920                11.55 ns/op            8 B/op          1 allocs/op

Benchmark_TrimBytes/fiber-16                    70524762                16.44 ns/op           16 B/op          1 allocs/op
Benchmark_TrimBytes/default-16                  70530980                16.72 ns/op           16 B/op          1 allocs/op

Benchmark_TrimRight/fiber-16                    865098051                1.398 ns/op           0 B/op          0 allocs/op
Benchmark_TrimRight/default-16                  487361299                2.470 ns/op           0 B/op          0 allocs/op

Benchmark_TrimLeft/fiber-16                     760135455                1.569 ns/op           0 B/op          0 allocs/op
Benchmark_TrimLeft/default-16                   419346405                2.895 ns/op           0 B/op          0 allocs/op

Benchmark_Trim/fiber-16                         309708110                3.745 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default-16                       302284438                4.009 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default.trimspace-16             250067830                4.784 ns/op           0 B/op          0 allocs/op
PASS
ok      github.com/gofiber/fiber/v3/utils       17.530s

@trim21 trim21 requested a review from efectn August 20, 2022 03:32
@efectn
Copy link
Member

efectn commented Aug 20, 2022

I saw the updated benchmark result, I think we can remove these Trim* functions now 🤔

go 1.19

Benchmark_TrimRightBytes/fiber-16               100000000               10.66 ns/op            8 B/op          1 allocs/op
Benchmark_TrimRightBytes/default-16             100000000               10.84 ns/op            8 B/op          1 allocs/op

Benchmark_TrimLeftBytes/fiber-16                100000000               10.72 ns/op            8 B/op          1 allocs/op
Benchmark_TrimLeftBytes/default-16              100000000               11.29 ns/op            8 B/op          1 allocs/op

Benchmark_TrimBytes/fiber-16                    74933964                15.43 ns/op           16 B/op          1 allocs/op
Benchmark_TrimBytes/default-16                  74931624                15.86 ns/op           16 B/op          1 allocs/op

Benchmark_TrimRight/fiber-16                    484963700                2.471 ns/op           0 B/op          0 allocs/op
Benchmark_TrimRight/default-16                  454740096                2.614 ns/op           0 B/op          0 allocs/op

Benchmark_TrimLeft/fiber-16                     488699834                2.451 ns/op           0 B/op          0 allocs/op
Benchmark_TrimLeft/default-16                   423590457                2.835 ns/op           0 B/op          0 allocs/op

Benchmark_Trim/fiber-16                         296457481                4.023 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default-16                       322653328                3.804 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default.trimspace-16             248519290                4.811 ns/op           0 B/op          0 allocs/op

1.18 , our version is a little bit faster.

goos: windows
goarch: amd64
pkg: github.com/gofiber/fiber/v3/utils
cpu: AMD Ryzen 7 5800X 8-Core Processor
Benchmark_TrimRightBytes/fiber-16               100000000               10.49 ns/op            8 B/op          1 allocs/op
Benchmark_TrimRightBytes/default-16             99912576                11.57 ns/op            8 B/op          1 allocs/op

Benchmark_TrimLeftBytes/fiber-16                100000000               10.42 ns/op            8 B/op          1 allocs/op
Benchmark_TrimLeftBytes/default-16              99905920                11.55 ns/op            8 B/op          1 allocs/op

Benchmark_TrimBytes/fiber-16                    70524762                16.44 ns/op           16 B/op          1 allocs/op
Benchmark_TrimBytes/default-16                  70530980                16.72 ns/op           16 B/op          1 allocs/op

Benchmark_TrimRight/fiber-16                    865098051                1.398 ns/op           0 B/op          0 allocs/op
Benchmark_TrimRight/default-16                  487361299                2.470 ns/op           0 B/op          0 allocs/op

Benchmark_TrimLeft/fiber-16                     760135455                1.569 ns/op           0 B/op          0 allocs/op
Benchmark_TrimLeft/default-16                   419346405                2.895 ns/op           0 B/op          0 allocs/op

Benchmark_Trim/fiber-16                         309708110                3.745 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default-16                       302284438                4.009 ns/op           0 B/op          0 allocs/op
Benchmark_Trim/default.trimspace-16             250067830                4.784 ns/op           0 B/op          0 allocs/op
PASS
ok      github.com/gofiber/fiber/v3/utils       17.530s

I agree

@trim21 trim21 changed the title v3(enhancement): convert more utils to generic byteSeq function. v3(enhancement): remove utils.Trim* because stdlib has save performance now Aug 20, 2022
@trim21 trim21 changed the title v3(enhancement): remove utils.Trim* because stdlib has save performance now v3(enhancement): remove utils.Trim* because stdlib has same performance now Aug 20, 2022
@trim21 trim21 changed the title v3(enhancement): remove utils.Trim* because stdlib has same performance now v3(enhancement): remove utils.Trim* because stdlib has same performance in go1.19 Aug 20, 2022
@trim21 trim21 merged commit b161f80 into gofiber:v3-beta Aug 20, 2022
@trim21 trim21 deleted the v3-util-genetic-byteSeq branch August 20, 2022 05:52
@efectn efectn added this to the v3 milestone Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efectn efectn Awaiting requested review from efectn

Assignees
No one assigned
Labels
🧹 Updates v3
Projects
v3
Status: Done
Milestone
v3
Development

Successfully merging this pull request may close these issues.
3 participants
@trim21 @ReneWerner87 @efectn