limiting: Make SemaphoreLimiter.Acquire() zero-alloc #8

Merged

fyrchik merged 1 commit from fyrchik/frostfs-qos:limiter-noalloc into master

2025-02-26 16:18:44 +00:00

fyrchik commented

2025-02-26 10:18:49 +00:00

Owner

Previously, Acquire on exising key did 1 allocation because
func() { sem.Release() } was a closure capturing different variables.

Signed-off-by: Evgenii Stratonikov e.stratonikov@yadro.com

Previously, `Acquire` on exising key did 1 allocation because `func() { sem.Release() }` was a closure capturing different variables. Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>

fyrchik added 1 commit 2025-02-26 10:18:49 +00:00

limiting: Make SemaphoreLimiter.Acquire() zero-alloc

DCO action / DCO (pull_request) Failing after 24s

Details

Tests and linters / Run gofumpt (pull_request) Successful in 29s

Details

Tests and linters / Tests (pull_request) Successful in 42s

Details

Vulncheck / Vulncheck (pull_request) Successful in 41s

Details

Tests and linters / Tests with -race (pull_request) Failing after 45s

Details

Tests and linters / Staticcheck (pull_request) Successful in 1m1s

Details

Tests and linters / Lint (pull_request) Successful in 1m40s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 1m42s

Details

Tests and linters / gopls check (pull_request) Successful in 1m56s

Details

9721fcbac0

Previously, `Acquire` on exising key did 1 allocation because
`func() { sem.Release() }` was a closure capturing different variables.

Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>

fyrchik force-pushed limiter-noalloc from 9721fcbac0 to e02209a99a

2025-02-26 10:19:06 +00:00

Compare

requested reviews from dstepanov-yadro, a-savchuk, storage-core-committers, storage-core-developers

2025-02-26 10:19:19 +00:00

fyrchik commented

2025-02-26 10:28:38 +00:00

Author

Owner

I will remove or skip the test after approval.
It seems -race introduces some problems, but normal tests pass, as they should.
The motivation is not only because allocations, but also readability.

func TestLimiterZeroAlloc(t *testing.T) {
	const key = "key"

	s, err := limiting.NewSemaphoreLimiter([]limiting.KeyLimit{{Keys: []string{key}, Limit: 1}})
	require.NoError(t, err)

	testAllocs := func(t *testing.T, key string) float64 {
		return testing.AllocsPerRun(100, func() {
			release, ok := s.Acquire(key)
			require.True(t, ok)
			release()
		})
	}
	t.Run("existing key", func(t *testing.T) {
		require.Zero(t, testAllocs(t, key))
	})
	t.Run("missing key", func(t *testing.T) {
		require.Zero(t, testAllocs(t, "missing "+key))
	})
}

I will remove or skip the test after approval. It seems `-race` introduces some problems, but normal tests pass, as they should. The motivation is not only because allocations, but also readability. ```go func TestLimiterZeroAlloc(t *testing.T) { const key = "key" s, err := limiting.NewSemaphoreLimiter([]limiting.KeyLimit{{Keys: []string{key}, Limit: 1}}) require.NoError(t, err) testAllocs := func(t *testing.T, key string) float64 { return testing.AllocsPerRun(100, func() { release, ok := s.Acquire(key) require.True(t, ok) release() }) } t.Run("existing key", func(t *testing.T) { require.Zero(t, testAllocs(t, key)) }) t.Run("missing key", func(t *testing.T) { require.Zero(t, testAllocs(t, "missing "+key)) }) } ```