Render managed centrals in parallel #1434
Conversation
| index := i | ||
| g.Go(func() error { | ||
| var err error | ||
| managedDinosaurList.Items[index], err = h.presenter.PresentManagedCentral(centralRequests[index]) |
There was a problem hiding this comment.
I see multiple challenges with this approach:
1. Race conditions:
This could lead to a race condition when to goroutines try to allocate the same address at the same time. The index is AFAIK not guaranteed to allocated the memory in an order.
You can still do this in two different ways:
- We now the sice of the slice ahead of time, allocating the memory in the
makestate should do the trick to address the indexes according to their address - My favorite is using a channel, send the results to a channel and iterate over it, every result is appended to the result slice. This prevents shared memory access.
2. Excessive go routine creation
A limit is necessary for go routines to not create an unlimited amount of them.
3. Go routine stacking and cancellation:
When a go routine is blocked it is never cancelled or timed out. Fleetshard-Sync will timeout and continuously try to get Centrals. An additional call to GetAll triggers again a Go routine for every active Central.
There was a problem hiding this comment.
This could lead to a race condition when to goroutines try to allocate the same address at the same time. The index is AFAIK not guaranteed to allocated the memory in an order.
I don't think that's true, the index is declared within the loop. It will only be scoped to the goroutine.
We now the sice of the slice ahead of time, allocating the memory in the make state should do the trick to address the indexes according to their address
That's already the case:
managedDinosaurList.Items = make([]private.ManagedCentral, len(centralRequests))
Go routine stacking and cancellation:
If the context is canceled, the errgroup will stop
There was a problem hiding this comment.
I added a max parallelism of 50
There was a problem hiding this comment.
I don't think that's true, the index is declared within the loop. It will only be scoped to the goroutine.
The memory allocation in the slice is not bound to the index. But this would only apply if you hadn't allocated the memory beforehand, I've not seen that 👍
If the context is canceled, the errgroup will stop
Uh, interesting I did not know that. TIL 🥳
| index := i | ||
| g.Go(func() error { | ||
| var err error | ||
| managedDinosaurList.Items[index], err = h.presenter.PresentManagedCentral(centralRequests[index]) |
There was a problem hiding this comment.
I don't think that's true, the index is declared within the loop. It will only be scoped to the goroutine.
The memory allocation in the slice is not bound to the index. But this would only apply if you hadn't allocated the memory beforehand, I've not seen that 👍
If the context is canceled, the errgroup will stop
Uh, interesting I did not know that. TIL 🥳
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ludydoo, SimonBaeumer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.