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

Make RustyLevelDB API async. #74

Closed
dan-da opened this issue Dec 14, 2023 · 3 comments
Closed

Make RustyLevelDB API async. #74

dan-da opened this issue Dec 14, 2023 · 3 comments

Comments

@dan-da
Copy link
Collaborator

dan-da commented Dec 14, 2023

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
Copy link
Collaborator Author

dan-da commented Dec 16, 2023

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
Copy link
Collaborator Author

dan-da commented Feb 17, 2024

Closed by fa89d04

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
Copy link
Collaborator Author

dan-da commented Feb 17, 2024

closing.

@dan-da dan-da closed this as completed Feb 17, 2024
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

No branches or pull requests

1 participant