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

Extract common/port.h and optimize dbstats #2145

Merged
merged 3 commits into from
Mar 10, 2024
Merged

Conversation

mapleFU
Copy link
Member

@mapleFU mapleFU commented Mar 9, 2024

This patch does two things:

  1. Extract a common/port.h, which handles difference between platforms
  2. DBStats is used heavily in kvrocks. So, we need to enhance it's performance. This patch uses fetch_add and aligned it with cacheline

std::atomic<uint64_t> flush_count = 0;
std::atomic<uint64_t> keyspace_hits = 0;
std::atomic<uint64_t> keyspace_misses = 0;
struct alignas(CACHE_LINE_SIZE) DBStats {
Copy link
Member Author

Choose a reason for hiding this comment

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

Like rocksdb, an Per core array optimization can also be introduced here. Maybe I'll try it in another patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another problem is that, should we change DBStats to unique_ptr<DBStats> in Storage? @git-hulk @PragmaTwice

Copy link
Member

Choose a reason for hiding this comment

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

I feel good about both of them since the lifetime of DBStats is the same as the storage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Emm I think previously it's size is 8*8B=64B, now sizeof(DBStats) becomes 8 * 64B. Which would be a large object

@mapleFU mapleFU requested review from git-hulk and PragmaTwice March 9, 2024 17:21
git-hulk
git-hulk previously approved these changes Mar 10, 2024
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, good patch.

Comment on lines +29 to +33
#if defined(__GNUC__) && __GNUC__ < 7
constexpr size_t CACHE_LINE_SIZE = 64U;
#else
constexpr size_t CACHE_LINE_SIZE = 256U;
#endif
Copy link
Member

@PragmaTwice PragmaTwice Mar 10, 2024

Choose a reason for hiding this comment

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

I wonder why it will be changed for different GCC versions?

Copy link
Member Author

@mapleFU mapleFU Mar 10, 2024

Choose a reason for hiding this comment

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

The code is borrow from rocksdb, seems it's a bug in gcc, we can ingoring it?
facebook/rocksdb@65996dd

@mapleFU
Copy link
Member Author

mapleFU commented Mar 10, 2024

Performance for sadd(On my WSL2 Ubuntu22, with gcc11.4, CPU AMD 3800X) with default release build:

before:
SADD: 27173.91 requests per second

after:
SADD: 30211.48 requests per second

cc @git-hulk @PragmaTwice

Copy link

sonarcloud bot commented Mar 10, 2024

@mapleFU mapleFU merged commit 19d7bad into unstable Mar 10, 2024
59 checks passed
@mapleFU mapleFU deleted the optimize-db-stats branch March 10, 2024 06:43
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

Successfully merging this pull request may close these issues.

3 participants