-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Prevents Table Cache to open same files more times #6707
Conversation
@pdillinger @siying please take a look and comment |
util/mutexlock.h
Outdated
} | ||
|
||
private: | ||
std::vector<T> locks_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put those locks in different cache lines to prevent false sharing? We do something like this in other places:
rocksdb/monitoring/statistics.h
Line 83 in fdf882d
struct ALIGN_AS(CACHE_LINE_SIZE) StatisticsData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review if I got that correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I forgot destructor. Sorry...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to test this in unit test?
db/table_cache.cc
Outdated
@@ -62,14 +62,17 @@ void AppendVarint64(IterKey* key, uint64_t v) { | |||
|
|||
} // namespace | |||
|
|||
#define LOAD_CONCURENCY 128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be a const instead of a macro? The memory is dynamically allocated anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can. I was waiting for feedback if you want it in options (I would not put it there). And I saw other macros as well. But yes can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
I have unit test in Java. Unfortunately I did not get check points framework. But if you require I will try to learn it. |
Test might be a little bit hard to write though. If you run Linux, can you try to run "make crash_test" and see whether it can pass"? (it takes several hours). |
I was testing in debugger to stop threads at correct place and it works as expected. Java multithread test I attached to issue also works as expected after fix. I have deployed build on our stress test and will start test you have mentioned. I will keep you posted. |
I have run the make crash_test. It was running many hours as you have said. Basically it looks to me it was running in some cycle some tool. So it was repeating some command. All of them looks like this (hopefully it is ok):
|
Our stress test does not show any deviation in response times and NO_FILES_OPENS looks beautiful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…nt requests. Fixes (facebook#6699)
a011dc9
to
c315442
Compare
@koldat has updated the pull request. Re-import the pull request |
Fixed reviews and squashed / rebased. Please let me know if you want to do further modifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request has been merged in 6ee66cf. |
If anyone interested, I have posted LRU concurrent performance analysis here: https://groups.google.com/forum/#!topic/rocksdb/1_uo3Pr6DiE |
In highly concurrent requests table cache opens same file more times which lowers purpose of max_open_files. Fixes (#6699)