v2: Performance improvements to Replacer implementation (placeholders)#2674
v2: Performance improvements to Replacer implementation (placeholders)#2674
Conversation
|
Thanks. Going to merge this then, but more testing is needed. @aligator If you still want to help with placeholders, we could use some good unit tests ( If you want other ways to contribute, we also need help building out the reverse proxy. There's a lot of good stuff to do there (again, intermediate level -- some network/backend Go experience is definitely useful here). Just let me know. |
|
@mholt Yes, I am definitely interested in helping you. But I am not very experienced with go yet so maybe some lighter things first. Tell me If I can help you somewhere else. (I am also registered in the caddy forum as aligator) |
|
Sounds good to me! Feel free to make a PR; incremental reviews are often easier than a big one at the end. |
1. What does this change do, exactly?
Improves performance of placeholders in Caddy 2. Placeholders are the equivalent of variables in other web servers (but better). They allow you to refer to dynamic values in static configuration.
I was lazy with the initial implementation because I wanted to get it done quickly.
The old implementation pre-generated all possible values and then lazily called
strings.ReplaceAll()in a loop. Of course this produces LOTS of unnecessary allocations. Now, we simply use functions which return the requested value, or false if the value is not recognized by that function; and instead of a blanketstrings.ReplaceAll()for every possible placeholder, we parse the input string and look for the specific placeholders we need. By usingstrings.Builder, the reconstruction of the string is very efficient.Note that this optimization affects ALL usage of placeholders, but the tests I performed were using them in a response body.
Using this sample config:
Here are some high-level load tests on my 8-core Macbook Pro:
2. Please link to the relevant issues.
Closes #2673
3. Which documentation changes (if any) need to be made because of this PR?
None.