account.Address: use RWLocker

[robert-zaremba]

Description

Use sync.RWMutex instead of sync.Mutex in account cache as suggested in #8767 (comment)

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[odeke-em]

Thanks for the PR but sync.RWMutex is going to give surprising results for situations unless you have multiple readers and a single producer scenario plus overwhelming evidence that reads are slower, otherwise one is tacking on expenses that are unnecessary. Please see https://gist.github.com/odeke-em/234bc88fbaf9e439b70a618ce6674659#file-aresults-md I’d just stick to entirely using sync.Mutex unless the mutex is being held for a long time. In fact there are plans to fix sizes and cost of sync.RWMutex like golang/go#37142

…

On Wed, Mar 3, 2021 at 11:24 AM Robert Zaremba ***@***.***> wrote: @robert-zaremba <https://github.com/robert-zaremba> requested your review on: #8774 <#8774> account.Address: use RWLocker. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#8774 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3VYJIA7WRNQO6JNOQ4TTB2EHFANCNFSM4YR2RIQQ> .

[codecov]

Codecov Report

Merging #8774 (dd84aa1) into master (42919c8) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8774      +/-   ##
==========================================
+ Coverage   59.17%   59.34%   +0.17%     
==========================================
  Files         571      563       -8     
  Lines       31800    31256     -544     
==========================================
- Hits        18817    18550     -267     
+ Misses      10780    10551     -229     
+ Partials     2203     2155      -48     

Impacted Files	Coverage Δ
types/address.go	66.66% <100.00%> (+0.65%)	⬆️
x/feegrant/simulation/decoder.go	0.00% <0.00%> (-100.00%)	⬇️
x/bank/types/msgs.go	53.91% <0.00%> (-42.97%)	⬇️
x/staking/types/msg.go	23.92% <0.00%> (-31.01%)	⬇️
x/gov/client/utils/utils.go	47.05% <0.00%> (-26.48%)	⬇️
x/distribution/types/msg.go	31.86% <0.00%> (-15.68%)	⬇️
x/gov/client/rest/query.go	19.31% <0.00%> (-6.82%)	⬇️
x/upgrade/types/plan.go	86.95% <0.00%> (-5.91%)	⬇️
x/auth/tx/service.go	69.35% <0.00%> (-3.51%)	⬇️
x/gov/client/cli/query.go	69.56% <0.00%> (-1.15%)	⬇️
... and 38 more

[robert-zaremba]

Thanks for insights @odeke-em . So on direct benchmark, sync.Mutex is 50% faster than syncRWMutex?
I think here, after some time we will have mostly reads. (especially with regards to consensus addresses). So probably RWMutex will still make more sense because we will rarely do a normal lock.

[odeke-em]

This is why benchmarking code will answer those questions and quell assumptions that might not hold in practice. As I showed in that surprising benchmark, multiple readers and a single writer, yet memory and allocations were still high, an objective measure would be to check for performance before and after having written a carefully close to reality scenario for benchmarking.

[robert-zaremba]

My assumption for a real-world use case, that after the warm up we will have mostly (95+% ?) reads. @marbar3778 , @alessio - what do you think?

[tac0turtle]

yea agree. Might as well do some quick benchmarks, doesnt hurt

[alessio]

Merging master in as tests are failing.

[alessio]

The race detector is failing

[orijbot]

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=af2d7eabe3064b71bc725b7c3049ec5d

[orijbot]

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=4cbcc86fa36d99a198421ee08ec87ae6409a9b96

[alexanderbez]

It's unforunate we have to rely on singletons for the cache and mutexes :-/

[robert-zaremba]

True. Changing it would involve a big refactoring -> reducing package public functions and making everything managed by objects.

[robert-zaremba]

I made a benchmark based on the code Emanuel shared (https://gist.github.com/odeke-em/234bc88fbaf9e439b70a618ce6674659#file-aresults-md). I adjusted it to my CPU (4 cores) and here are the result:

cpu: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz
BenchmarkRWMutex-4       3968	    264872 ns/op    1393 B/op	      30 allocs/op
BenchmarkMutex-4         2596	    490315 ns/op    1163 B/op	      27 allocs/op

Also, we can expect that there will be way more reads than writes after a warmup. Decreasing writes will only bring more points to the RWMutex in the benchmark. So it make even more sense to use RWMutex.

[tac0turtle]

utACK.

Thank you for doing the benchmarking. Do you want to add the code to the repo?

[odeke-em]

@robert-zaremba thanks for letting me know.
For proper benchmarking:

• please run benchmarks between 5 to even 20 times and save each to a different file e.g. go test -run=^$ -bench=<SPECIFIC NAME or .> -count=10 > target
• always use benchstat to compare deltas, otherwise folks have to mentally do Maths

The above steps help remove noise, and give a direct comparison and summary.

Also the benchmarks I provided were specific for a comparison. Did you adapt to code that uses the cosmos-sdk and this new code in specific usage?

[robert-zaremba]

Here are aggregated stats, with multiple run.

• old = normal mutex
• new = rw mutex

name     old time/op    new time/op    delta
Mutex-4     414µs ±20%     252µs ± 2%  -39.25%  (p=0.000 n=10+9)

name     old alloc/op   new alloc/op   delta
Mutex-4    1.31kB ± 9%    1.28kB ± 3%     ~     (p=0.460 n=10+9)

name     old allocs/op  new allocs/op  delta
Mutex-4      28.8 ± 8%      28.6 ± 2%     ~     (p=0.665 n=10+9)

[robert-zaremba]

merging, the rw mutex is appropriate for this use case.

[orijbot]

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=8537baddeb5eace04b5156fe69a130ff22a3cde6

[orijbot]

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=1aa770d250184d9f890dfd4cc345f6bb