-
Notifications
You must be signed in to change notification settings - Fork 617
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
Issue 595 watchbackend #629
Conversation
This pull request addresses #595. |
Here is some detail around memory performance comparing the pull request to the current 1.5.11 build. In the image below, the yellow line is the current 1.5.11 binary. The green line is a build with switching next in main.watchBackend to a bytes.Buffer. The fabio processes were both started at the same time and allow to run for approximately 10 hours. As you can see the 1.5.11 build continues to increase in memory, while the instance running the pull request build is relatively flat. If we look at the pprofs taken from the 1.5.11 build we see the following:
In the 1.5.11 build, the "next := svccfg + "\n" + mancfg" is over 2 GB in size after only 10 hours of running. This matches our behavior where our production nodes with 64GB or memory are exhausted in a few days. In the pull request build we see the following from the pprof:
The "next" buffer is only 4.5MB in size, which matches the current size of our routes from consul. |
@murphymj25 LGTM! Thank you for all you effort debugging and providing a solution. |
@murphymj25 would you mind re-basing this and resolving the conflicts created by merging #614? |
Yep, I can take care of that. Thanks! |
Not sure what exactly happened here. When I compare you fork against current master it looks pretty clean (only some minor differences) but this PR is a bit crazy. |
b0bbcd3
to
c9705ca
Compare
@leprechau Yeah, i don't think my rebase was correct at that time. Take a look now. |
@murphymj25 thank you! Looks better. |
@murphymj25 and thank you for that example link. That made me remember something I had forgotten. String concatenation with |
@leprechau No Problem! Glad i could help get this merged in. |
Could maybe also have used the new |
I tested with the strings.builder, but didn't see a way to read directly from the buffer. When i had to convert it to a string in the route/parse_new.go to read it, I was seeing the memory leak at that point. I think it would be a great idea to replace the remaining concatenations with strings.Builder(). That's something we were debating on tackling as well. |
No description provided.