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

Reduce short living tasks produced by memcached implementation and move them to goroutines pool #251

Merged
merged 12 commits into from
May 28, 2021

Conversation

storozhukBM
Copy link
Contributor

@storozhukBM storozhukBM commented May 3, 2021

During investigation on #250 I made a quick look at pprof results didn't show any noticeable hot spots in the github.com/envoyproxy/ratelimit or github.com/bradfitz/gomemcache
but a lot of samples were in runtime.schedule, runtime.findrunnable, runtime.futex. So I decided to reduce the potential load on the scheduler by moving the continuous spawn of new short-lived goroutines to a pool that can automatically expand to a necessary size, stabilize and stop creating new goroutines if the load remains stable.

This simple change reduced overall CPU consumption on 32 cores machine by 20% in the same setup that is described in the issue #250

mattklein123
mattklein123 previously approved these changes May 25, 2021
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@mattklein123
Copy link
Member

Oops looks like a real CI issue, please take a look.

/wait

@storozhukBM
Copy link
Contributor Author

@mattklein123
The issue on CI is caused by the missing https://golang.org/pkg/time/#Ticker.Reset method that was added only in Go 1.15, but this project target version is Go 1.14

The question is: can we consider bumping the target version to the recent 1.16? If not, I'll change the implementation to work on Go 1.14

@storozhukBM storozhukBM requested a review from mattklein123 May 26, 2021 08:11
@mattklein123
Copy link
Member

I would be fine with bumping the Go version, please go for it!

/wait

@storozhukBM
Copy link
Contributor Author

@mattklein123 I bumped Go version to 1.16, please take a look

@mattklein123
Copy link
Member

I think there might be another fix required due to the Go update. Sorry. :(

/wait

storozhukBM and others added 10 commits May 28, 2021 10:16
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
…yproxy#246)

The underlying memcache client library allows this to be configured,
and currently defaults to a value of 2, see:

https://github.com/bradfitz/gomemcache/blob/master/memcache/memcache.go#L72
https://github.com/bradfitz/gomemcache/blob/master/memcache/memcache.go#L145
https://github.com/bradfitz/gomemcache/blob/master/memcache/memcache.go#L239

This allows this value to be configured by a new environmet variable:

MEMCACHE_MAX_IDLE_CONNS

which defaults to -1 meaning the default from the library will apply
(which is the current behaviour).

Signed-off-by: Peter Marsh <pete.d.marsh@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Signed-off-by: devincd <505259926@qq.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Signed-off-by: Peter Marsh <pete.d.marsh@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
…oyproxy#252)

- servers listen addresses are configurable via environment variable
- matches port configurability providing *HOST environment variables

Fixes envoyproxy#245

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
User deferred barrier.signal() so panic definitely occurs before
we continue on in test.

Config reload uses recover() and increments config load counter, tests
were failing to see config load error counter increment.

Fixes: envoyproxy#256

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
This allows MEMCAHE_SRV to be specified as an SRV record from
which multiple memcache hosts can be resolved. For example:

    MEMCACHE_SRV=_memcache._tcp.mylovelydomain.com

This can be used instead of MEMCACHE_HOST_PORT.

This will then be resolved and whatever set of servers it represents
will be used as the set of memcache servers to connect to. At this
stage neither priority or weight is supported, though weight could
be fairly straightforwardly in future.

The SRV can be polled periodically for new servers by setting the
following env var (with 0 meaning "never check"):

    MEMCACHE_SRV_REFRESH=600s # supports standard go durations

Signed-off-by: Peter Marsh <pete.d.marsh@gmail.com>
Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
@storozhukBM
Copy link
Contributor Author

@mattklein123
The issue with Go 1.16 upgrade is caused by this change https://go-review.googlesource.com/c/go/+/231379

So x509 (c *Certificate) VerifyHostname(h string) now fails on legacy certificates, but even if we fix the certificate in our tests, this will continue failing on some certificates of other envoyproxy/ratelimit users.

This behavior can be temporarily disabled by GODEBUG=x509ignoreCN=0 ENV variable, but all users of ratelimiter will need to pass this variable while, they are updating their certificates.

I think such a change is definitely beyond the scope of this ticket, so I moved back Go 1.14 and changed the goroutine pool to work on Go 1.14

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit c0cdd75 into envoyproxy:main May 28, 2021
zdmytriv pushed a commit to verygoodsecurity/ratelimit that referenced this pull request Aug 2, 2021
…ve them to goroutines pool (envoyproxy#251)

Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
timcovar pushed a commit to goatapp/ratelimit that referenced this pull request Jan 16, 2024
…ve them to goroutines pool (envoyproxy#251)

Signed-off-by: bstorozhuk <storozhuk.b.m@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants