fix: prioritise requests in order by allocating memory/workers upfront #163
Conversation
aws/s3/s3_concurrent.go
Outdated
| } | ||
| cm.memoryPool = mp | ||
| cm.memoryPool = memoryPool{channel: mp} | ||
| cm.memoryTotalSize = memoryTotalSize |
There was a problem hiding this comment.
Based on the math above, memoryTotalSize = memoryChunkSize * maxWorkers = maxBytes.
So can this be simplified as cm.memoryTotalSize = maxBytes ? Or there's something else?
There was a problem hiding this comment.
There's potential rounding on the division, and I want memoryTotalSize to be an exact multiple of memoryChunkSize, but yes an equivalent that's better is:
cm.memoryTotalSize = memoryChunkSize * maxWorkers
aws/s3/s3_concurrent.go
Outdated
| var totalMemory int64 = 0 | ||
| for _, o := range objects { | ||
| var memoryForObject int64 = 0 | ||
| for memoryForObject < o.Size { |
There was a problem hiding this comment.
Since memoryChungSize is fixed, is there an easier math so we don't need a loop?
chunks := math.Ceil(float64(o.Size) / float64(cm.memoryChungSize))
totalMemoy += cm.memoryChunkSize * chunks
There was a problem hiding this comment.
woops I didn't see your code suggestion. Mine is a bit longer but no floats.
aws/s3/s3_concurrent.go
Outdated
| securedMemory += <-cm.memoryPool | ||
| for _, o := range objects { | ||
| var securedMemory int64 = 0 | ||
| for securedMemory < o.Size { |
There was a problem hiding this comment.
Same as the comment below. (Sorry I commented the code below first)
There was a problem hiding this comment.
I updated the below bit, but in this case still need a for loop to receive from the channel, so wasn't much point.
… front I found under load testing with many large requests, the memory and workers were allocated too thinly across all accepted requests, which resulted in the whole system locking up as no request could get enough to continue
|
great work! |
I found under load testing with many large requests, the memory and workers were allocated too thinly across all accepted requests, which resulted in the whole system locking up as no request could get enough to continue.
Getting memory and workers from the channel is now controlled by a mutex, but returning them remains concurrent. This ensures a requests are allocated what they need to continue in the order that they arrive.
Reference: https://github.com/GeoNet/tickets/issues/14787
Production Changes
The following production changes are required to deploy these changes:
Review
Check the box that applies to this code review. If necessary please seek help with adding a checklist guide for the reviewer.
When assigning the code review please consider the expertise needed to review the changes.
Code Review Guide
Insert check list here if needed.