Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: apply rw mutex to cachekv #204

Merged
merged 3 commits into from
May 26, 2021

Conversation

wetcod
Copy link
Contributor

@wetcod wetcod commented May 24, 2021

Description

When checkTx is executed concurrently, lock contention occurs on Read in cachekv.Store.
So modify cachekv.Store to use RWLock.
Also, since set cache in Read, concurrent-safe cache is required. So I use sync.Map.

-->

closes: https://github.com/line/lbm/issues/1187


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

@wetcod wetcod force-pushed the sangyeop/v2/apply-rw-mutex-to-cache-kv branch 2 times, most recently from 8dc9ffa to 6113434 Compare May 24, 2021 06:16
@wetcod wetcod self-assigned this May 24, 2021
@wetcod wetcod force-pushed the sangyeop/v2/apply-rw-mutex-to-cache-kv branch from 6113434 to e901359 Compare May 24, 2021 06:17
@codecov
Copy link

codecov bot commented May 24, 2021

Codecov Report

Merging #204 (824f748) into v2/develop (4bf9d52) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           v2/develop     #204      +/-   ##
==============================================
+ Coverage       53.48%   53.49%   +0.01%     
==============================================
  Files             652      652              
  Lines           47341    47344       +3     
==============================================
+ Hits            25318    25325       +7     
+ Misses          19170    19166       -4     
  Partials         2853     2853              
Impacted Files Coverage Δ
store/cachekv/store.go 91.34% <100.00%> (+4.21%) ⬆️

@wetcod wetcod force-pushed the sangyeop/v2/apply-rw-mutex-to-cache-kv branch from e901359 to 163bb34 Compare May 24, 2021 07:07
Copy link

@egonspace egonspace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Contributor

@iproudhon iproudhon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think sync.Map is necessary as it's accessed with lock held.

@wetcod
Copy link
Contributor Author

wetcod commented May 25, 2021

I don't think sync.Map is necessary as it's accessed with lock held.

@iproudhon I think sync.Map is needed because concurrent set occurs in https://github.com/line/lbm-sdk/pull/204/files#diff-9d8fdb334ee27219ad1f8351ab43222e3450cbf889f2599f97bf6957743aa69fR67

@whylee259
Copy link
Contributor

whylee259 commented May 25, 2021

I think we should separate the read-only cache from the Store.cache.
If the unmodified values are staged to Store.cache through Get(), seems performance will be degraded because unnecessary writes are performed during Write().

@whylee259
Copy link
Contributor

I think we should separate the read-only cache from the Store.cache.
If the unmodified values are staged to Store.cache through Get(), seems performance will be degraded because unnecessary writes are performed during Write().

Ah, it checks whether the item is dirty.
https://github.com/line/lbm-sdk/blob/dfef88dea841e983218f2f6f8875244e8d6b0c57/store/cachekv/store.go#L103

@wetcod
Copy link
Contributor Author

wetcod commented May 25, 2021

Ah, it checks whether the item is dirty.

Yes, I think Store already writes only dirty elements in Write()

@iproudhon
Copy link
Contributor

As it has over 66% update, sync.Map is the right choice. With that, I don't think we need RWLock. Instead, we can just use Mutex to protect setCacheValue, Write and iterator. Get() doesn't need lock protection.

@wetcod
Copy link
Contributor Author

wetcod commented May 26, 2021

As it has over 66% update, sync.Map is the right choice. With that, I don't think we need RWLock. Instead, we can just use Mutex to protect setCacheValue, Write and iterator. Get() doesn't need lock protection.

@iproudhon When Rlock is removed, Get and Write are executed at the same time.
Then parent.Get and parent.Set can be called at the same time.
Since CacheKV is the only KVStore that has concurrency protection with lock, then it may cause problems.

@iproudhon
Copy link
Contributor

Ok, I got it.

@wetcod wetcod merged commit e2e9a46 into v2/develop May 26, 2021
@wetcod wetcod deleted the sangyeop/v2/apply-rw-mutex-to-cache-kv branch May 26, 2021 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants