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

Atomic based limiter #15

Merged
merged 2 commits into from
Dec 5, 2019
Merged

Atomic based limiter #15

merged 2 commits into from
Dec 5, 2019

Conversation

storozhukBM
Copy link
Contributor

@storozhukBM storozhukBM commented Oct 23, 2018

Hi,
I've made your limiter implementation atomic based. It is a little bit harder but in generally faster and has no goroutine scalability issues that any mutex based implementation have.
Checkout this relation between RateLimiter latency and count of goroutines:
limiter_scalability
Original implementation moved to mutexbased.go file, so you can run benchmarks and play with other tests. But I'd suggest to leave only one implementation after all.

[Side note] This wired latency spike in mutex based implementation is a direct illustration of mutex falling into starvation mode.

Full benchmarking results here

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2018

CLA assistant check
All committers have signed the CLA.

@storozhukBM
Copy link
Contributor Author

Hi @kriskowal,
Sorry for bothering you, but maybe you'll have some time to look at this changes.

@siennathesane
Copy link

@kriskowal any thoughts here?

@kriskowal
Copy link
Contributor

This looks like a pretty fantastic change. Can you move the prior implementation into an internal package so we don’t grow the API surface?

Sorry for the extraordinary delay.

@storozhukBM
Copy link
Contributor Author

Hi @kriskowal
There was no easy way to move mutexLimiter into internal package because it depends on some types from ratelimit package like Clock, Limiter, Option...
So I decided to make newMutexBased private, which is basically has the same results, and we don’t grow the API surface.

@kriskowal
Copy link
Contributor

Looks good. I’ll try to land, changelog, release this week.

@kriskowal kriskowal merged commit ecb970a into uber-go:master Dec 5, 2019
noah8713 pushed a commit to noah8713/ratelimit that referenced this pull request Aug 4, 2020
noah8713 pushed a commit to noah8713/ratelimit that referenced this pull request Aug 4, 2020
noah8713 pushed a commit to noah8713/ratelimit that referenced this pull request Aug 4, 2020
@peakle
Copy link

peakle commented Nov 21, 2021

@storozhukBM where did you create this awesome benchmark visualization?

@storozhukBM
Copy link
Contributor Author

@peakle
Exported benchmark results to csv and configured graph in Numbers(spreadsheets software that goes together with MacOS)

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.

None yet

5 participants