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

♻️ Refactor: use c.app.getString instead of string(...) #2489

Merged
merged 1 commit into from Jun 1, 2023
Merged

♻️ Refactor: use c.app.getString instead of string(...) #2489

merged 1 commit into from Jun 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 1, 2023

Description

I noticed 2 functions (those being GetReqHeaders and GetRespHeaders) use string(...) instead of c.app.getString to convert a byte array to a string

c.Request().Header.VisitAll(func(k, v []byte) {
	headers[string(k)] = c.app.getString(v)
})

c.Response().Header.VisitAll(func(k, v []byte) {
	headers[string(k)] = c.app.getString(v)
})

Using c.app.getString is around 16-19% faster, and allocates less. (See benchmark spreadsheets below. Both c.app.getString, and string(...) was tested on both functions, 100 times.)

Benchmark result for using string(...) on GetReqHeaders: GetReqHeaders.xlsx
(Average 1,756,668 Iterations, 700 ns/op, 357 B/op, 5 allocs/op)

Benchmark result for using c.app.getString on GetReqHeaders: GetReqHeadersNew.xlsx
(Average 2,164,354 Iterations, 567 ns/op, 336 B/op, 2 allocs/op)

Benchmark result for using string(...) on GetRespHeaders: GetRespHeaders.xlsx
(Average 1,823,697 Iterations, 678 ns/op, 357 B/op, 5 allocs/op)

Benchmark result for using c.app.getString on GetRespHeaders: GetRespHeadersNew.xlsx
(Average 2,176,443 Iterations, 564 ns/op, 336 B/op, 2 allocs/op)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have performed a self-review of my own code
  • 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

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

@ReneWerner87
Copy link
Member

thx, good found

@ReneWerner87 ReneWerner87 merged commit 4c12938 into gofiber:master Jun 1, 2023
@sadfun
Copy link
Contributor

sadfun commented Jun 22, 2023

Hmmm. @ReneWerner87, func (h *RequestHeader) VisitAll(f func(key, value []byte)) literally says:

// f must not retain references to key and/or value after returning.
// Copy key and/or value contents before returning if you need retaining them.

But by calling c.app.getString, which is unsafe.String(unsafe.SliceData(b), len(b)) by default, we are making reference to key's byte slice 💀

@ReneWerner87
Copy link
Member

yes i know, that's why the instructions for copying are also in our documentation and methods when they are used

so that one makes a copy for the value, if one would like to use these data over the request/response expiration further

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.

2 participants