perf: avoid unnecessary copy operation #3376
Conversation
fukua95
changed the title
fukua95
changed the title
| c.mu.RLock() | ||
| if !c.closed { | ||
| list = make([]*ringShard, len(c.shards.list)) | ||
| copy(list, c.shards.list) | ||
| } | ||
| c.mu.RUnlock() | ||
| defer c.mu.RUnlock() | ||
|
|
||
| return list | ||
| if c.closed { | ||
| return nil | ||
| } | ||
| return c.shards.list |
There was a problem hiding this comment.
Although the current use is to just call the Client and not change the slice, I do think that returning a copy is safer since this is public and a developer can get the slice and alter it. WDYT, @fukua95 ?
There was a problem hiding this comment.
Scratch the comment above, I do agree that this should be safe if documented correctly. The sharding is not exposed to the outside. Maybe document it and mark your findings that this may trigger data race.
There was a problem hiding this comment.
Hello @ndyakov , thanks your review. Are you meaning that we should add a comment in the List() method to warn that exposing c.shards.list externally may risk causing a data race?
There was a problem hiding this comment.
Correct, and also would advice to add a one-liners on the three places where the List is used, just to note that the received slice is not a copy but the actual slice.
There was a problem hiding this comment.
Thank you, I've updated it.
fukua95
force-pushed
the
optimize/reduce-copy
branch
from
1ad81f8 to
8cd300f
Compare
ring.goring.goIn struct
Ring, the size of shards can be up to 100 (#2190 (comment)), in this case, a copy operation is called for each heartbeat, and approximately 3.1 KB data are copied. This is a waste of CPU resources. So avoiding copy operations yields benefits.The pr #2931 reported a race here, but there actually isn't one. Because every time we get the slice
c.shards.list, we don't change slice itself, we only usec.shards.list[i].Clientwhich is concurrency-safe.