-
Notifications
You must be signed in to change notification settings - Fork 495
PS-9647: MySQL Perf Improvements (deque) #5611
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
Conversation
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
@@ -7049,11 +7049,13 @@ static uint get_semi_join_select_list_index(Item_field *item_field) { | |||
if (emb_sj_nest && emb_sj_nest->is_sj_or_aj_nest()) { | |||
const mem_root_deque<Item *> &items = | |||
emb_sj_nest->nested_join->sj_inner_exprs; | |||
for (size_t i = 0; i < items.size(); i++) { | |||
const Item *sel_item = items[i]; | |||
size_t i = 0; |
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.
variable name i
is too short, expected at least 2 characters
sql/sql_parse.cc
Outdated
@@ -1304,7 +1304,9 @@ void free_items(Item *item) { | |||
*/ | |||
void cleanup_items(Item *item) { | |||
DBUG_TRACE; | |||
for (; item; item = item->next_free) item->cleanup(); | |||
for (; item; item = item->next_free) { |
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.
implicit conversion Item *
-> bool
for (; item; item = item->next_free) { | |
for (; item != nullptr; item = item->next_free) { |
*/ | ||
template <class Element_type> | ||
template <class T> | ||
class mem_root_deque { |
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.
And more generic question...
Why do we need this class at all? How is it different from
template <class T>
using mem_root_deque = std::list<T, Mem_root_allocator<T>> list_;
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.
for pop_back/pop_front we need a wrapper, because for std::list, if the list is empty, the behavior is undefined.
Also, there is no indexing operator in std::list
https://perconadev.atlassian.net/browse/PS-9647 This patch uses list instead of deque for memory efficiency reasons, primarily to optimize operations like bulk insert
47b61c9
to
7509e14
Compare
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.
LGTM
PS-9647: MySQL Perf Improvements (deque)
https://perconadev.atlassian.net/browse/PS-9647
This patch uses list instead of deque for memory efficiency reasons, primarily to optimize operations like bulk insert