The Wayback Machine - https://web.archive.org/web/20200225005731/https://github.com/caddyserver/caddy/issues/2681
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

v2: Optimize with compiler's inliner #2681

Open
mholt opened this issue Jul 18, 2019 · 3 comments
Open

v2: Optimize with compiler's inliner #2681

mholt opened this issue Jul 18, 2019 · 3 comments
Milestone

Comments

@mholt
Copy link
Member

@mholt mholt commented Jul 18, 2019

Go over the Caddy 2 code base at some point and see where we can take advantage of inlining for performance improvements. See: https://blog.filippo.io/efficient-go-apis-with-the-inliner/

@mholt mholt added this to the 2.0 milestone Jul 18, 2019
@mholt mholt added the optimization label Jul 19, 2019
@dominikbraun dominikbraun mentioned this issue Jul 22, 2019
4 of 4 tasks complete
@dominikbraun

This comment has been minimized.

Copy link

@dominikbraun dominikbraun commented Jul 22, 2019

The most basic approach to do this is to find all functions that return a reference - since these functions can be optimized if the referenced value has been allocated inside it.

So I've written a small tool called refreturn that finds functions returning a reference. For the Caddy v2 branch, the output is:

$ refreturn .
caddy.go:211:1: GoModule
modules/caddyhttp/encode/encode.go:95:1: openResponseWriter
modules/caddyhttp/fileserver/browse.go:135:1: browseWriteJSON
modules/caddyhttp/fileserver/browse.go:141:1: browseWriteHTML
modules/caddyhttp/fileserver/staticfiles.go:178:1: openFile
modules/caddyhttp/responsewriter.go:39:1: Hijack
modules/caddyhttp/responsewriter.go:156:1: Buffer
modules/caddyhttp/reverseproxy/healthchecker.go:80:1: NewHealthCheckWorker
modules/caddyhttp/reverseproxy/upstream.go:283:1: roundRobin
modules/caddyhttp/reverseproxy/upstream.go:305:1: random
modules/caddyhttp/reverseproxy/upstream.go:355:1: newUpstream
modules/caddyhttp/reverseproxy/upstream.go:430:1: newReverseProxy
modules/caddyhttp/reverseproxy/reverseproxy.go:116:1: NewSingleHostReverseProxy
modules/caddyhttp/reverseproxy/reverseproxy.go:196:1: ServeHTTP
modules/caddyhttp/staticresp_test.go:60:1: fakeRequest
modules/caddytls/connpolicy.go:37:1: TLSConfig

I've optimized openResponseWriter() for example and the escape analysis shows that it seems to work quite well. Before the optimization:

modules/caddyhttp/encode/encode.go:102:3: &responseWriter literal escapes to heap

And after (the output above disappeared):

modules/caddyhttp/encode/encode.go:84:29: inlining call to (*Encode).openResponseWriter

However, we may discuss which function calls should be optimized this way. For instance, browseWriteJSON calls new(bytes.Buffer) and I'm not sure if this is a intentional heap allocation or if it can be optimized away.

My progress can be found in #2687.

@mholt

This comment has been minimized.

Copy link
Member Author

@mholt mholt commented Jul 22, 2019

@dominikbraun Excellent!! What a neat tool, thank you for sharing! I saw your PR and will be following it closely and participating in it.

Functions that should definitely be optimized would be those in the hot paths: TLS handshakes, HTTP requests, etc. Maybe functions in the configuration (re)loading phase as well, but it depends how frequent config reloads are. For now, those can be a lower priority (if we have to choose). Functions used only in tests or which are only called once need not be optimized, I suppose, since the extra indirection would be harder to read.

Of the functions you listed, I think these would benefit most from the optimization:

modules/caddyhttp/encode/encode.go:95:1: openResponseWriter
modules/caddyhttp/fileserver/browse.go:135:1: browseWriteJSON
modules/caddyhttp/fileserver/browse.go:141:1: browseWriteHTML
modules/caddyhttp/fileserver/staticfiles.go:178:1: openFile
modules/caddyhttp/reverseproxy/reverseproxy.go:196:1: ServeHTTP

The ones I removed either can't be changed (because they satisfy interface requirements) or they aren't in a hot path, or they act as a Getter of an existing value.

As for the browseWriteJSON/HTML functions, the new(bytes.Buffer) can definitely be optimized as well, perhaps we should be pooling those.

It would be really neat to benchmark these changes. How would you feel about writing Benchmark* test functions to see how the speedup is with the inlining?

@caddyserver caddyserver deleted a comment Nov 12, 2019
@caddyserver caddyserver deleted a comment from Charlotte008 Nov 13, 2019
@vorsprung

This comment has been minimized.

Copy link

@vorsprung vorsprung commented Nov 22, 2019

This issue shouldn't be listed as "good first issue" as all the low hanging fruit are already picked!

See commit 4950ce4 by Dominik Braun on August 8 ( over 3 months ago at time of this mention )

3 out of the 4 hot paths mentioned by @mholt are done

@mholt mholt removed the good first issue label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.