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.

Sign up for GitHub

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

Jump to bottom

[store] bump CommitKVStoreCache limit and switch to 2Q from arc #152

Merged
merged 7 commits into from
Feb 3, 2023
Merged

[store] bump CommitKVStoreCache limit and switch to 2Q from arc #152

merged 7 commits into from
Feb 3, 2023

Conversation

BrandonWeng
Copy link
Contributor

@BrandonWeng BrandonWeng commented Feb 3, 2023 •
edited
Loading

Describe your changes and provide context

IRRC we had discussions around bumping the limit for the inter-block cache since 1k is pretty little for our volume of TXs per block. Im setting it to 100k to be the same as the BoundedCacheKv store

While I was investigating the race condition issue in the LRU cache, I saw that several repos brought up concerns around using ArcCache in their code as it's been patented by IBM (and later sold to Intel)

hashicorp/golang-lru#31
hashicorp/golang-lru#73

ipfs/kubo#6590
ipfs/go-ipfs-blockstore#20

Postgres and IPFS replaced it with 2Q, which based on the whitepaper should have the same performance as ARC.

Testing performed to validate your change

Deployed a LT cluster with these changes and saw no impact to consensus and the performance (with LT clients running) is similar
image

BrandonWeng
BrandonWeng commented Feb 3, 2023
View reviewed changes
store/cache/cache.go
@@ -28,7 +28,7 @@ type (
// CommitKVStore and below is completely irrelevant to this layer.
CommitKVStoreCache struct {
types.CommitKVStore
cache *lru.ARCCache
cache *lru.TwoQueueCache
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason for investigating ARCCache was due to this exception that caused a consensus issue

And another git issues reporting non-threadsafe behaviour hashicorp/golang-lru#71

github.com/cosmos/cosmos-sdk/baseapp.newDefaultRecoveryMiddleware.func1
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:69
github.com/cosmos/cosmos-sdk/baseapp.newRecoveryMiddleware.func1
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:39
github.com/cosmos/cosmos-sdk/baseapp.processRecovery
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:28
github.com/cosmos/cosmos-sdk/baseapp.processRecovery
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:33
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx.func1
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/baseapp.go:748
runtime.gopanic
            /usr/lib/go-1.19/src/runtime/panic.go:890
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runMsgs.func1
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/baseapp.go:877
runtime.gopanic
            /usr/lib/go-1.19/src/runtime/panic.go:884
runtime.panicmem
            /usr/lib/go-1.19/src/runtime/panic.go:260
runtime.sigpanic
            /usr/lib/go-1.19/src/runtime/signal_unix.go:835
aeshashbody
            /usr/lib/go-1.19/src/runtime/asm_amd64.s:1158
runtime.typehash
            /usr/lib/go-1.19/src/runtime/alg.go:166
runtime.nilinterhash
            /usr/lib/go-1.19/src/runtime/alg.go:130
runtime.mapdelete
            /usr/lib/go-1.19/src/runtime/map.go:718
github.com/hashicorp/golang-lru/simplelru.(*LRU).removeElement
            /root/go/pkg/mod/github.com/hashicorp/golang-lru@v0.5.5-0.20210104140557-80c98217689d/simplelru/lru.go:173
github.com/hashicorp/golang-lru/simplelru.(*LRU).RemoveOldest
            /root/go/pkg/mod/github.com/hashicorp/golang-lru@v0.5.5-0.20210104140557-80c98217689d/simplelru/lru.go:115
github.com/hashicorp/golang-lru.(*ARCCache).Add
            /root/go/pkg/mod/github.com/hashicorp/golang-lru@v0.5.5-0.20210104140557-80c98217689d/arc.go:168
github.com/cosmos/cosmos-sdk/store/cache.(*CommitKVStoreCache).Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cache/cache.go:123
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109
github.com/cosmos/cosmos-sdk/store/gaskv.(*Store).Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/gaskv/store.go:43
github.com/cosmos/cosmos-sdk/store/prefix.Store.Get
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/prefix/store.go:72
github.com/cosmos/cosmos-sdk/x/bank/keeper.BaseKeeper.GetDenomMetaData
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/x/bank/keeper/keeper.go:268
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper.Keeper.validateCreateDenom
            /home/ubuntu/sei-chain/x/tokenfactory/keeper/createdenom.go:60
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper.Keeper.CreateDenom
            /home/ubuntu/sei-chain/x/tokenfactory/keeper/createdenom.go:14
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper.msgServer.CreateDenom
            /home/ubuntu/sei-chain/x/tokenfactory/keeper/msg_server.go:26
github.com/sei-protocol/sei-chain/x/tokenfactory/types._Msg_CreateDenom_Handler.func1
            /home/ubuntu/sei-chain/x/tokenfactory/types/tx.pb.go:573
github.com/cosmos/cosmos-sdk/baseapp.(*MsgServiceRouter).RegisterService.func2.1
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/msg_service_router.go:113
github.com/sei-protocol/sei-chain/x/tokenfactory/types._Msg_CreateDenom_Handler
            /home/ubuntu/sei-chain/x/tokenfactory/types/tx.pb.go:575
recovered: runtime error: invalid memory address or nil pointer dereference
stack:
goroutine 151846271 [running]:
runtime/debug.Stack()
            /usr/lib/go-1.19/src/runtime/debug/stack.go:24 +0x65
github.com/cosmos/cosmos-sdk/baseapp.newDefaultRecoveryMiddleware.func1({0x1ca4420, 0x37cf8b0})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:71 +0x27
github.com/cosmos/cosmos-sdk/baseapp.newRecoveryMiddleware.func1({0x1ca4420?, 0x37cf8b0?})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:39 +0x30
github.com/cosmos/cosmos-sdk/baseapp.processRecovery({0x1ca4420, 0x37cf8b0}, 0xc002e7d180?)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:28 +0x37
github.com/cosmos/cosmos-sdk/baseapp.processRecovery({0x1ca4420, 0x37cf8b0}, 0x28da800?)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/recovery.go:33 +0x5e
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx.func1()
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/baseapp.go:748 +0x210
panic({0x1ca4420, 0x37cf8b0})
            /usr/lib/go-1.19/src/runtime/panic.go:890 +0x262
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runMsgs.func1()
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/baseapp.go:877 +0x7b
panic({0x1ca4420, 0x37cf8b0})
            /usr/lib/go-1.19/src/runtime/panic.go:884 +0x212
github.com/hashicorp/golang-lru/simplelru.(*LRU).removeElement(0xc000352460, 0xc079af2450?)
            /root/go/pkg/mod/github.com/hashicorp/golang-lru@v0.5.5-0.20210104140557-80c98217689d/simplelru/lru.go:173 +0xc7
github.com/hashicorp/golang-lru/simplelru.(*LRU).RemoveOldest(0xc0003d3650?)
            /root/go/pkg/mod/github.com/hashicorp/golang-lru@v0.5.5-0.20210104140557-80c98217689d/simplelru/lru.go:115 +0x3d
github.com/hashicorp/golang-lru.(*ARCCache).Add(0xc0003d3650, {0x1bf82e0, 0xc0925ebb10}, {0x1bd2aa0, 0x38c9fe0})
            /root/go/pkg/mod/github.com/hashicorp/golang-lru@v0.5.5-0.20210104140557-80c98217689d/arc.go:168 +0x422
github.com/cosmos/cosmos-sdk/store/cache.(*CommitKVStoreCache).Get(0xc000028ab0, {0xc062bc46c0, 0xb7, 0xb7})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cache/cache.go:123 +0x21e
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get(0xc0ade100c0, {0xc062bc46c0, 0xb7, 0xb7})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109 +0x14b
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get(0xc0d91b9200, {0xc062bc46c0, 0xb7, 0xb7})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109 +0x14b
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get(0xc0db6166c0, {0xc062bc46c0, 0xb7, 0xb7})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109 +0x14b
github.com/cosmos/cosmos-sdk/store/cachekv.(*Store).Get(0xc0db616cc0, {0xc062bc46c0, 0xb7, 0xb7})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/cachekv/store.go:109 +0x14b
github.com/cosmos/cosmos-sdk/store/gaskv.(*Store).Get(0xc0db6172c0, {0xc062bc46c0, 0xb7, 0xb7})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/gaskv/store.go:43 +0x71
github.com/cosmos/cosmos-sdk/store/prefix.Store.Get({{0x28d77b0, 0xc0db6172c0}, {0xc028a5a360, 0x5c, 0x60}}, {0xc028a5a3c0, 0x5b, 0x5b?})
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/store/prefix/store.go:72 +0x10c
github.com/cosmos/cosmos-sdk/x/bank/keeper.BaseKeeper.GetDenomMetaData({{{{0x28d7660, 0xc0002bf200}, {0x28b0e58, 0xc00051be60}, {0x28d9660, 0xc0001bcfc0}, 0x0}, {0x28d7660, 0xc0002bf200}, {0x28d9660, ...}, ...}, ...}, ...)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/x/bank/keeper/keeper.go:268 +0x25b
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper.Keeper.validateCreateDenom({{0x28b0e58, 0xc00051bfe0}, {{0x28d7660, 0xc0002bf200}, 0xc0001269d0, {0x28b0e58, 0xc00051bf00}, {0x28b0ea8, 0xc0001e20c0}, {0xc0002b7660, ...}, ...}, ...}, ...)
            /home/ubuntu/sei-chain/x/tokenfactory/keeper/createdenom.go:60 +0x11e
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper.Keeper.CreateDenom({{0x28b0e58, 0xc00051bfe0}, {{0x28d7660, 0xc0002bf200}, 0xc0001269d0, {0x28b0e58, 0xc00051bf00}, {0x28b0ea8, 0xc0001e20c0}, {0xc0002b7660, ...}, ...}, ...}, ...)
            /home/ubuntu/sei-chain/x/tokenfactory/keeper/createdenom.go:14 +0x9e
github.com/sei-protocol/sei-chain/x/tokenfactory/keeper.msgServer.CreateDenom({{{0x28b0e58, 0xc00051bfe0}, {{0x28d7660, 0xc0002bf200}, 0xc0001269d0, {0x28b0e58, 0xc00051bf00}, {0x28b0ea8, 0xc0001e20c0}, {0xc0002b7660, ...}, ...}, ...}}, ...)
            /home/ubuntu/sei-chain/x/tokenfactory/keeper/msg_server.go:26 +0x167
github.com/sei-protocol/sei-chain/x/tokenfactory/types._Msg_CreateDenom_Handler.func1({0x28c84d8, 0xc0d6dd8870}, {0x1eb8460?, 0xc0a52405e0})
            /home/ubuntu/sei-chain/x/tokenfactory/types/tx.pb.go:573 +0x78
github.com/cosmos/cosmos-sdk/baseapp.(*MsgServiceRouter).RegisterService.func2.1({0x28c84d8, 0xc0d6dd8810}, {0x517526?, 0x412eab?}, 0x1f18b00?, 0xc09b260d08)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/msg_service_router.go:113 +0xd2
github.com/sei-protocol/sei-chain/x/tokenfactory/types._Msg_CreateDenom_Handler({0x1ed6de0?, 0xc000a6e140}, {0x28c84d8, 0xc0d6dd8810}, 0x2618388, 0xc095549de0)
            /home/ubuntu/sei-chain/x/tokenfactory/types/tx.pb.go:575 +0x138
github.com/cosmos/cosmos-sdk/baseapp.(*MsgServiceRouter).RegisterService.func2({{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc05ab77500}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, {0x165e750d, ...}, ...}, ...}, ...)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/msg_service_router.go:126 +0x373
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runMsgs(_, {{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc05ab774c0}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, ...}, ...}, ...)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/baseapp.go:904 +0x78a
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx(_, {{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc077845b80}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, ...}, ...}, ...)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/baseapp.go:851 +0x12e5
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).DeliverTx(_, {{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc077845b80}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, ...}, ...}, ...)
            /root/go/pkg/mod/github.com/sei-protocol/sei-cosmos@v0.1.409/baseapp/abci.go:265 +0x2ee
github.com/sei-protocol/sei-chain/app.(*App).DeliverTx(_, {{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc077845b80}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, ...}, ...}, ...)
            /home/ubuntu/sei-chain/app/abci.go:48 +0x25f
github.com/sei-protocol/sei-chain/app.(*App).DeliverTxWithResult(_, {{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc077845b80}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, ...}, ...}, ...)
            /home/ubuntu/sei-chain/app/app.go:991 +0x5d
github.com/sei-protocol/sei-chain/app.(*App).ProcessTxConcurrent(_, {{0x28c84d8, 0xc033c63860}, {0x28da800, 0xc077845b80}, {{0x0, 0x0}, {0xc000814780, 0x9}, 0xbd23, ...}, ...}, ...)
            /home/ubuntu/sei-chain/app/app.go:1066 +0x518
created by github.com/sei-protocol/sei-chain/app.(*App).ProcessBlockConcurrent
            /home/ubuntu/sei-chain/app/app.go:1098 +0x3cf
: panic

BrandonWeng
BrandonWeng commented Feb 3, 2023
View reviewed changes
go.mod Outdated
@@ -144,7 +144,7 @@ replace (
github.com/gin-gonic/gin => github.com/gin-gonic/gin v1.7.0
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1

github.com/tendermint/tendermint => github.com/sei-protocol/sei-tendermint v0.1.147
github.com/tendermint/tendermint => github.com/sei-protocol/sei-tendermint v0.1.129
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ignore, just wanted to confirm the data race issue is related to a tendermint change. Which seems to be the case :(

@BrandonWeng BrandonWeng mentioned this pull request Feb 3, 2023
Merged
@BrandonWeng BrandonWeng merged commit 7333425 into main Feb 3, 2023
@BrandonWeng BrandonWeng deleted the bweng-bump-cachekv-limit branch February 3, 2023 06:02
codchen pushed a commit that referenced this pull request Feb 12, 2023
@BrandonWeng @codchen
[store] bump CommitKVStoreCache limit and switch to 2Q from arc (#152)
1d173a5 
## Describe your changes and provide context
IRRC we had discussions around bumping the limit for the inter-block
cache since 1k is pretty little for our volume of TXs per block. Im
setting it to 100k to be the same as the BoundedCacheKv store

While I was investigating the race condition issue in the LRU cache, I
saw that several repos brought up concerns around using ArcCache in
their code as it's been patented by IBM (and later sold to Intel)

hashicorp/golang-lru#31 
hashicorp/golang-lru#73 

ipfs/kubo#6590
ipfs/go-ipfs-blockstore#20

Postgres and IPFS replaced it with 2Q, which based on the whitepaper
should have the same performance as ARC.
## Testing performed to validate your change
Deployed a LT cluster with these changes and saw no impact to consensus
and the performance (with LT clients running) is similar

![image](https://user-images.githubusercontent.com/18161326/216499319-be5ae693-242f-453c-8073-414bf5f810bd.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codchen codchen codchen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BrandonWeng @codchen