tm-db
tm-db copied to clipboard
tendermint
Could we remove `mtx` from `prefixdb`?
I'm investigating how to increase concurrency in terms of cosmos-sdk app. To my understanding, almost db access paths from cosmos store to db backend are serialized by mtx. I think it's not a good idea for performance. As a bottom layer of db access path, I'd like to remove mtx from prefixdb. (Please note that cosmos/iavlStore depends on this prefixdb). prefixdb wraps another tm-db instance and this another tm-db is already concurrency safe by CONTRACT. So, intuitionally, I think we could remove mtx from prefixdb. HDYT?
Queries in the SDK are routed through Tendermint which holds a global mutex across the ABCI connection, this is the main bottleneck of the SDK outside the underlying storage issues with IAVL. In the near future the SDK and Tendermint will stop using this repo and house their own db interface implementations internally. This will lend itself to better performance. We are also working on making SDK state access concurrent with the recent storage ADR.
This all being the case, I'm not sure if it is worth doing this work as it is still unclear if we will do another release with this repo.
Even though this repo might be not used in the future, I'd like to discuss why mtx is needed for prefixdb and whether it's possible to remove it. Please could you share your thought?
Quickly reading the code, the reason we would need a mutex is that we can't assume the underlying database is concurrently safe. This is an overall problem with this repo. Abstracting DBs means we can make fewer assumptions of the underlying db.
AFAIK, the all db implementation should be concurrency safe. The description also shows it.
https://github.com/tendermint/tm-db/blob/53ed3dbaa3eb4c7693ca0aaa2deff093e4675b24/types.go#L16
hi @jinsankim , I have the same issue now. Did you make any progress afterwards?
@ilovers line/tm-db#20
Hey seems this change would be acceptable. Would you want to see about upstreaming it?