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

Bump github.com/valyala/fasthttp from 1.32.0 to 1.33.0 #1744

Merged
merged 1 commit into from Feb 3, 2022

Conversation

ReneWerner87
Copy link
Member

@ReneWerner87 ReneWerner87 commented Feb 3, 2022

Please provide enough information so that others can review your pull request:

Explain the details for making this change. What existing problem does the pull request solve?

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 Author

ReneWerner87 commented Feb 3, 2022

hi @erikdubbelboer,
we have found a performance decrease for the AppendBody method in our benchmarks, maybe this is interesting for you

image

| `Benchmark_Ctx_Write`
| `64.5` ns/op\t      71 B/op\t       0 allocs/op  <- 1.33
| `25.5` ns/op\t      69 B/op\t       0 allocs/op  <- 1.32

https://github.com/gofiber/fiber/runs/5048671292?check_suite_focus=true

fiber/ctx.go

Lines 1316 to 1319 in d85ae2b

func (c *Ctx) Write(p []byte) (int, error) {
c.fasthttp.Response.AppendBody(p)
return len(p), nil
}

https://github.com/valyala/fasthttp/blob/9d665e00df6d44682b64a4af9cfa3aa051f63090/http.go#L505-L508

@erikdubbelboer
Copy link

erikdubbelboer commented Feb 3, 2022

I don't see how that is possible, there wasn't any change that touched that code. Are you sure it wasn't just a hiccup in the benchmark? Is it reproducible if you re-run the benchmark?

@ReneWerner87
Copy link
Member Author

ReneWerner87 commented Feb 3, 2022

ok I will check it again manually

@ReneWerner87
Copy link
Member Author

ReneWerner87 commented Feb 3, 2022

@erikdubbelboer
was my mistake, thought our task is very stable and i saw a similar problem in another action

sorry for that -> was a false alarm

@ReneWerner87 ReneWerner87 merged commit 4d3cc6c into master Feb 3, 2022
27 checks passed
@ReneWerner87 ReneWerner87 deleted the fasthttp-1-33-0 branch Mar 25, 2022
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

3 participants