Neptune-Crypto / neptune-core

anonymous peer-to-peer cash
Apache License 2.0
25 stars 7 forks source link

Make RustyLevelDB API async. #74

Closed dan-da closed 7 months ago

dan-da commented 9 months ago

LevelDB provides a synchronous API.

RustyLevelDB is a thin wrapper around LevelDB API. It also provides sync methods.

In neptune-core, we have async API like get_block(), get_block_header(), etc that appear to be async. However internally they are calling these blocking DB APIs.

This creates a hidden source of blocking/contention that may not be obvious until operating under load, and suddenly performance becomes very bad one day. See this description.

note: The RustyLevelDb is presently wrapped in a tokio lock, which requires await to acquire the lock. This gives an appearance of async, however the actual DB calls such as get(), set(), delete() remain blocking calls and may potentially block the calling task for database disk I/O.

note: I'm in the process of removing this tokio lock, which is no longer necessary because the C++ leveldb does its own locking. While refactoring, this issue made itself clear.

A simple solution could be to make the RustyLevelDb methods async. Internally, they would wrap the DB calls inside spawn_blocking. This enables the RustyLevelDb method to await the DB operation (and return a Future immediately).

In general, issuing a blocking call or performing a lot of compute in a future without yielding is problematic, as it may prevent the executor from driving other futures forward. This function runs the provided closure on a thread dedicated to blocking operations.

dan-da commented 9 months ago

I've created a RustyLevelDbAsync struct that provides async versions of all methods in RustyLevelDB, even new(), which creates/opens a DB.

For now this lives in neptune-core, on a branch in my forked repo. But that's probably not where it belongs, because...

This async issue also affects twenty_first::storage. The LevelDB wrapper in there and all the StorageVec, DbtSchema stuff is synchronous, and will need to be made async.

Currently, twenty-first does not have any tokio dep, which is nice, and I've avoided adding one, which is why I placed the tokio Atomic sync wrappers in neptune-core::util_types::sync instead of twenty_first::sync.

So, in order to make the DB access fully async, we will have to either add a tokio dep in twenty-first or split the storage module out into its own crate. I recommend/prefer the latter, and I think this async issue is a good reason to do it.

A further thought: we should even consider publishing the RustyLevelDbAsync API I made as its own crate, perhaps rs-leveldb-async or rs-leveldb-tokio. This would seem to be a generally useful thing. nevermind, I'm not happy with the iterator yet. It may always require an external spawn_blocking wrapper.

dan-da commented 7 months ago

Closed by fa89d0414f6d95cd7407c67e15e00cdc06b9c121

note however that the LevelDB iterator(s) remain sync, so any iterator callers need to do their own async wrapping using eg spawn_blocking() or block_in_place().

dan-da commented 7 months ago

closing.