ring.DoBatch with goroutine pool #431

colega · 2023-11-13T16:18:20Z

This adds an option to ring.DoBatch to run the callbacks through a function provided that could make use of a goroutine pool (with concurrency.ReusableGoroutinesPool as an implementation).

This should help in Mimir's distributors to avoid extra stack allocation on every new goroutine created to perform a push to an ingester.

Since there was a recent change to DoBatch that added an isClientError param, I made a more generic version DoBatchWithOptions. I also made the cleanup optional, since I see it's
not so common to pass it down.

With this change, I also marked as deprecated the default ring.DoBatch version.

I did two benchmark versions, showing some imrovement in CPU time on the pooled version:

$ benchstat -col /go bench.txt
goos: linux
goarch: amd64
pkg: github.com/grafana/dskit/ring
cpu: 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz
                 │   default    │  concurrency.NonBlockingWorkerPool   │
                 │    sec/op    │    sec/op     vs base                │
Batch10x100-16     69.04µ ±  0%   64.53µ ±  0%   -6.53% (p=0.000 n=20)
Batch100x100-16    131.6µ ±  1%   123.8µ ±  1%   -5.94% (p=0.000 n=20)
Batch100x1000-16   2.088m ± 30%   1.146m ± 39%  -45.12% (p=0.001 n=20)
geomean            266.7µ         209.2µ        -21.57%

This adds an option to `ring.DoBatch` to run the callbacks through a function provided that could make use of a goroutine pool (with concurrency.ReusableGoroutinesPool as an implementation). This should help in Mimir's distributors to avoid extra stack allocation on every new goroutine created to perform a push to an ingester. Since there was a recent change to `DoBatch` that added an `isClientError` param, I made a more generic version `DoBatchWithOptions`. I also made the cleanup optional, since I see it's not so common to pass it down. I did two benchmark versions, showing some imrovement in CPU time on the pooled version: $ benchstat -col /go bench.txt goos: linux goarch: amd64 pkg: github.com/grafana/dskit/ring cpu: 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz │ default │ concurrency.NonBlockingWorkerPool │ │ sec/op │ sec/op vs base │ Batch10x100-16 69.04µ ± 0% 64.53µ ± 0% -6.53% (p=0.000 n=20) Batch100x100-16 131.6µ ± 1% 123.8µ ± 1% -5.94% (p=0.000 n=20) Batch100x1000-16 2.088m ± 30% 1.146m ± 39% -45.12% (p=0.001 n=20) geomean 266.7µ 209.2µ -21.57% Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

duricanikolic · 2023-11-13T16:46:19Z

Could you please update CHANGELOG.md and write the improtant changes this PR brings:

ring.DoBatchWithOptions() has been added.
ring.DoBatch() and ring.DoBatchWithClientError() have been deprecated.

ring/batch.go

duricanikolic · 2023-11-13T16:44:35Z

ring/batch.go

 //
 // Not implemented as a method on Ring, so we can test separately.
-func DoBatchWithClientError(ctx context.Context, op Operation, r ReadRing, keys []uint32, callback func(InstanceDesc, []int) error, cleanup func(), isClientError func(error) bool) error {
+func DoBatchWithOptions(ctx context.Context, op Operation, r ReadRing, keys []uint32, callback func(InstanceDesc, []int) error, o DoBatchOptions) error {


Did you remove DoBatchWithClientError()?
I would put it back and possibly mark it as deprecated, since it will now be part of r264.

We don't do server in dskit. We're already very generous at leaving the DoBatch with the old signature: I didn't change it just to make the update path easier for codebases with tangled dependencies.

I don't see any need to leave DoBatchWithClientError here.

Then we should replace DoBatchWithClientError with DoBatchWithOptions in CHANGELOG too, and mention both PRs there.
The usage of DoBatchWithClientError should be removed from Mimir once the vendored dskit is updated.

Changelog updated. We'll update Mimir once this is vendored there, of course.

duricanikolic · 2023-11-13T16:48:49Z

concurrency/worker.go

+// NewReusableGoroutinesPool creates a new worker pool with the given size.
+// These workers will run the workloads passed through Go() calls.
+// If all workers are busy, Go() will spawn a new goroutine to run the workload.
+func NewReusableGoroutinesPool(size int) *ReusableGoroutinesPool {


Do we plan to add a CLI flag for specifying the pool size in mimir?

Yes, there will be a CLI flag just like there's one in the dskit/server for grpc workers.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega · 2023-11-14T08:54:22Z

I have updated the CHANGELOG.md, however since we don't do releases, and changelog is sorted by section type (CHANGE/FEATURE/ENHANCEMENT/BUGFIX) instead of the date, I think it's quite useless.

duricanikolic

Thank you for addressing my comments.
LGTM

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

CHANGELOG.md

ring/ring_test.go

concurrency/worker_test.go

Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>

krajorama

Not an expert on goroutines, but LGTM

colega force-pushed the do-batch-with-goroutine-pool branch from 023a0a4 to e1385b9 Compare November 13, 2023 16:21

colega marked this pull request as ready for review November 13, 2023 16:21

duricanikolic reviewed Nov 13, 2023

View reviewed changes

s/setDefaults/replaceZeroValuesWithDefaults/

420bf24

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

krajorama self-requested a review November 14, 2023 08:50

Update CHANGELOG.md

d3f6168

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

duricanikolic approved these changes Nov 14, 2023

View reviewed changes

Merge branch 'main' into do-batch-with-goroutine-pool

41d999b

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

krajorama requested changes Nov 14, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ring/ring_test.go Outdated Show resolved Hide resolved

concurrency/worker_test.go Show resolved Hide resolved

colega and others added 2 commits November 14, 2023 13:14

Update CHANGELOG.md

969e7c3

Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>

Update ring/ring_test.go

21806ab

Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>

krajorama approved these changes Nov 14, 2023

View reviewed changes

colega merged commit f58e02b into main Nov 14, 2023
3 checks passed

colega deleted the do-batch-with-goroutine-pool branch November 14, 2023 14:07

colega mentioned this pull request Nov 14, 2023

Distributor pushes to ingesters through a goroutines pool grafana/mimir#6660

Merged

3 tasks

colega mentioned this pull request Nov 29, 2023

Reduce stack allocations by reusing goroutines when pushing series from the distributors to the ingesters grafana/mimir#6776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ring.DoBatch with goroutine pool #431

ring.DoBatch with goroutine pool #431

colega commented Nov 13, 2023 •

edited

duricanikolic commented Nov 13, 2023

duricanikolic Nov 13, 2023

colega Nov 13, 2023

duricanikolic Nov 13, 2023

colega Nov 14, 2023

duricanikolic Nov 13, 2023

colega Nov 13, 2023

colega commented Nov 14, 2023

duricanikolic left a comment

krajorama left a comment

ring.DoBatch with goroutine pool #431

ring.DoBatch with goroutine pool #431

Conversation

colega commented Nov 13, 2023 • edited

duricanikolic commented Nov 13, 2023

duricanikolic Nov 13, 2023

Choose a reason for hiding this comment

colega Nov 13, 2023

Choose a reason for hiding this comment

duricanikolic Nov 13, 2023

Choose a reason for hiding this comment

colega Nov 14, 2023

Choose a reason for hiding this comment

duricanikolic Nov 13, 2023

Choose a reason for hiding this comment

colega Nov 13, 2023

Choose a reason for hiding this comment

colega commented Nov 14, 2023

duricanikolic left a comment

Choose a reason for hiding this comment

krajorama left a comment

Choose a reason for hiding this comment

colega commented Nov 13, 2023 •

edited