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: Set async_io default value to yes #2308

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

xiaobiaozhao
Copy link
Contributor

async_io had ready, I think kvrocks should use async_io as default value.

@git-hulk
Copy link
Member

@xiaobiaozhao You need to turn the default value of rocksdb.async_io to true as well. Another quick question: is the rocksdb community marking this feature as stable?

@xiaobiaozhao
Copy link
Contributor Author

The feature has been unlabeled for experimentation, but rocksdb has not enabled it by default.

Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM, we need to highlight this change in the release log.

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

This is LGTM. Seems for Scan it depends on advanced fs api and could decrease latency on some disk. I don't know on some fast NVMe disk, would this helps a lot? Also maybe it doesn't works well on some Linux kernel without io_uring?

Anyway this is ok

@xiaobiaozhao
Copy link
Contributor Author

This is LGTM. Seems for Scan it depends on advanced fs api and could decrease latency on some disk. I don't know on some fast NVMe disk, would this helps a lot? Also maybe it doesn't works well on some Linux kernel without io_uring?

Anyway this is ok

Do you mean on older kernels? I just test on ubuntu 20/22/24 , it works well.

@mapleFU
Copy link
Member

mapleFU commented May 27, 2024

Do you mean on older kernels? I just test on ubuntu 20/22/24 , it works well.

On legacy os it uses "Read" rather than ReadAsync api

@mapleFU
Copy link
Member

mapleFU commented May 27, 2024

I've check the code of async_io:

document:

  // If async_io is enabled, RocksDB will prefetch some of data asynchronously.
  // RocksDB apply it if reads are sequential and its internal automatic
  // prefetching.
  bool async_io = false;

For rocksdb, related macro is

  • WITH_LIBURING ROCKSDB_IOURING_PRESENT: io_uring related
  • USE_COROUTINES USE_FOLLY: MultiGet related, it will using coroutine to schedule issued task

I didn't check do we enabled this.

For fs io, a code below would be triggered:

inline bool CheckFSFeatureSupport(FileSystem* fs, FSSupportedOps feat) {
  int64_t supported_ops = 0;
  fs->SupportedOps(supported_ops);
  if (supported_ops & (1ULL << feat)) {
    return true;
  }
  return false;
}

If fs doesn't supports async (.i.e no io_uring ) I guess this might making sense in MultiGet.

The ForwardIterator will also check the async feature is support.

I also checked the code for iterator, BlockPrefetcher might be issued io for this, so, without io_uring, scan might be optimized(with larger io prefetch-depth)? I didn't run the test, maybe it work ( or maybe not )

@PragmaTwice
Copy link
Member

Yeah we need to check the implementation for its behavior.

In docker image, seems we don't install liburing at all. Not sure if the option has effect.

@xiaobiaozhao
Copy link
Contributor Author

Yeah we need to check the implementation for its behavior.

In docker image, seems we don't install liburing at all. Not sure if the option has effect.

here is a test report

#1215

Copy link

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
11.9% Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

@xiaobiaozhao xiaobiaozhao merged commit 8fc2dc4 into apache:unstable Jun 4, 2024
32 of 33 checks passed
@PragmaTwice
Copy link
Member

## 8.1.0 (03/18/2023)
### Behavior changes
* Compaction output file cutting logic now considers range tombstone start keys. For example, SST partitioner now may receive ParitionRequest for range tombstone start keys.
* If the async_io ReadOption is specified for MultiGet or NewIterator on a platform that doesn't support IO uring, the option is ignored and synchronous IO is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants