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

Core-local statistics #2258

Closed
wants to merge 5 commits into from
Closed

Core-local statistics #2258

wants to merge 5 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented May 6, 2017

This diff changes StatisticsImpl from a thread-local approach to a core-local one. The goal is to perform faster aggregations, particularly for applications that have many threads. There should be no behavior change.

Test Plan:

  • sysbench with 4k concurrent transaction threads: stats aggregations 6.3x faster compared to thread-local (thread-local: 28ms, core-local: 4.4ms)
  • db_bench readwhilewriting, 32 threads, 100M keys, on SSD: stats updates 1.2x slower compared to thread-local
thread-local:

readwhilewriting : 3.072 micros/op 325566 ops/sec; 22.8 MB/s (631365 of 1000000 found)

+ 0.96% 0.11% db_bench.master db_bench.master [.] rocksdb::StatisticsImpl::measureTime
+ 0.89% 0.32% db_bench.master db_bench.master [.] rocksdb::StatisticsImpl::recordTick

core-local:

readwhilewriting : 3.050 micros/op 327826 ops/sec; 22.9 MB/s (631365 of 1000000 found)

+ 1.14% 0.38% db_bench.core-l db_bench.core-local.getcpu.bitshift [.] rocksdb::StatisticsImpl::recordTick
+ 1.09% 0.17% db_bench.core-l db_bench.core-local.getcpu.bitshift [.] rocksdb::StatisticsImpl::measureTime
  • db_bench readrandom, 32 threads, 100M keys, in-memory: stats updates 1.2x slower compared to thread-local
thread-local:

+ 0.88% 0.07% db_bench.master db_bench.master [.] rocksdb::StatisticsImpl::measureTime
0.55% 0.18% db_bench.master db_bench.master [.] rocksdb::StatisticsImpl::recordTick

readrandom : 2.049 micros/op 487965 ops/sec; 34.7 MB/s (1000000 of 1000000 found)

core-local:

+ 0.97% 0.13% db_bench.core-l db_bench.core-local.getcpu.bitshift [.] rocksdb::StatisticsImpl::measureTime
+ 0.74% 0.24% db_bench.core-l db_bench.core-local.getcpu.bitshift [.] rocksdb::StatisticsImpl::recordTick

readrandom : 2.086 micros/op 479325 ops/sec; 34.1 MB/s (1000000 of 1000000 found)

@facebook-github-bot
Copy link
Contributor

@ajkr updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@siying
Copy link
Contributor

siying commented May 12, 2017

CLANG Analyze failure with:

internal_repo_rocksdb/repo/monitoring/statistics.h:71:9: error: no matching function for call to 'atomic_init'
std::atomic_init(&tickers_[i], static_cast<uint64_t>(0));
^~~~~~~~~~~~~~~~

@facebook-github-bot
Copy link
Contributor

@ajkr updated the pull request - view changes - changes since last import

@ajkr
Copy link
Contributor Author

ajkr commented May 12, 2017

Thanks, @siying, I was confused about the array initialization but fixed it now.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr
Copy link
Contributor Author

ajkr commented May 22, 2017

can somebody review in time for the June release?

__declspec(align(64)) struct StatisticsData {
std::atomic_uint_fast64_t tickers_[INTERNAL_TICKER_ENUM_MAX] = {{0}};
HistogramImpl histograms_[INTERNAL_HISTOGRAM_ENUM_MAX];
} __attribute__((__aligned__(64)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure it works in Windows. Looks like it is failing.

@facebook-github-bot
Copy link
Contributor

@ajkr updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

3 participants