Details
-
Bug
-
Resolution: Fixed
-
Critical
-
6.6.2
-
Untriaged
-
1
-
Unknown
Description
I have run across a couple places in code that are problematically using value receivers instead of pointer receivers for methods. Usually this is not what we want, because it will make a (shallow) copy of the entire calling object and then invoke the method on it, instead of just invoking the method on the existing object. I assume there are probably more of these scattered around our code as I have not been seeking them out specifically, so we should do a scrub.
There are at least two problems with value receivers:
1. Performance: memory/CPU/garbage collection hit of making the unwanted copy of the calling object. For example, in indexer.go the following method creates a copy of the entire indexer object before invoking the method on that copy:
This example was fixed by MB-36746:
indexer.go
|
|
func (idx indexer) newIndexInstMsg(m common.IndexInstMap) *MsgUpdateInstMap {
|
return &MsgUpdateInstMap{indexInstMap: m, stats: idx.stats.Clone(), |
rollbackTimes: idx.keyspaceIdRollbackTimes}
|
}
|
2. Correctness: for cases of atomic pointer loads and stores inside Holder objects, if the Get method uses a value receiver, then a non-atomic copy of the Holder object is made upon which the Get method is invoked. The non-atomic copy may catch the pointer mid-update, resulting in a garbage pointer in the copied object, and then the Get method will atomically load that already-garbage pointer:
This example was fixed by MB-36746:
indexer/stats_manager.go
|
|
func (h IndexerStatsHolder) Get() *IndexerStats {
|
return (*IndexerStats)(atomic.LoadPointer(&h.ptr)) |
}
|
These should instead use pointer receivers (idx *indexer) and (h *IndexerStatsHolder), respectively.
I will fix the above two examples as part of MB-36746, but we need to do a more thorough scrub.
Attachments
Issue Links
- is a backport of
-
MB-43037 Scrub code for value receivers where pointer receivers should be used
- Closed