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

caddyhttp: Optimize logs using zap's WithLazy() #6590

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

AlliBalliBaba
Copy link
Contributor

After #6541 I realized that using zap's WithLazy() for the error logger was still much more efficient even if we are cloning the request. Originally I wanted to try using this originalRequest, which is made for logging but just gives a shallow copy without request headers. Instead, using request.Clone() gives a deep copy that just omits the request body.

This makes a simple request about 7% more efficient:

:80 {
    respond "Hello World"
}

wrk -t 4 -c 200 -d 60 http://localhost (20 cores Docker Ubuntu WSL)
With() ~205.000 RPS
WithLazy() ~219.000 RPS

Flamegraph With()
torch-caddy

Flamegraph WithLazy() and Copy()
torch-caddy2

Note that this will make the case where an error is logged about 1% slower since we are doing an additional request.Clone()

@francislavoie
Copy link
Member

We could possible do the same in reverseproxy.go and fastcgi.go?

@mholt mholt added the optimization 📉 Performance or cost improvements label Sep 24, 2024
@mholt
Copy link
Member

mholt commented Sep 24, 2024

That's very interesting -- thanks for submitting this PR!

I second Francis' question, but I think this LGTM either way.

@AlliBalliBaba
Copy link
Contributor Author

I can look into reverseproxy.go and fastcgi.go if there's similar potential for improvement. Probably something for another PR

@mholt
Copy link
Member

mholt commented Sep 25, 2024

Okay, doing it in increments sounds reasonable.

I'll merge this, but before I do, why does cloning the request make it faster? That seems counterintuitive. (I wonder if we should comment next to that line, either a link to this PR or a brief explanation)

@AlliBalliBaba
Copy link
Contributor Author

Cloning the request is faster than json encoding the request and cloning the logger. But yeah it seems counterintuitive, I'll add a comment.

@francislavoie francislavoie changed the title Performance: Uses zap's WithLazy() for the error logger caddyhttp: Optimize logs using zap's WithLazy() Sep 26, 2024
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thank you! Looking forward to seeing if this works well in any other places.

@mholt mholt merged commit 22c98ea into caddyserver:master Sep 26, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization 📉 Performance or cost improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants