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

Mutex contention from shared_ptr reference count inc/dec #330

Closed
mdcallag opened this issue Oct 2, 2016 · 6 comments
Closed

Mutex contention from shared_ptr reference count inc/dec #330

mdcallag opened this issue Oct 2, 2016 · 6 comments
Assignees
Labels

Comments

@mdcallag
Copy link

@mdcallag mdcallag commented Oct 2, 2016

Using builds from Aug 5 and Sep 27 I see many PMP dogpiles like this. The problem is that m_pk_descr is passed by value so the ref count for the shared pointer is highly contended when many threads run.

This is a big problem for sysbench read-write using a command line like:

sysbench --test=tests/db/oltp.lua --db-driver=mysql --mysql-engine-trx=yes --mysql-user=root --mysql-password=pw --mysql-host=127.0.0.1 --mysql-db=test --mysql-table-engine=rocksdb --oltp-table-size=100000 --oltp-tables-count=8 --num-threads=50 --max-requests=0 --max-time=60 --oltp-read-only=off --oltp-index-updates=0 --oltp-non-index-updates=0 --oltp-delete-inserts=1 run
#0  0x00000000006ce828 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (this=0x7f979daf0400) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr_base.h:146
#1  0x0000000000cac3d2 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (this=0x7f98e8599028, __in_chrg=<optimized out>) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr_base.h:666
#2  0x0000000000cac3d2 in std::__shared_ptr<myrocks::Rdb_key_def const, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (this=0x7f98e8599020, __in_chrg=<optimized out>) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr_base.h:914
#3  0x0000000000cac3d2 in std::shared_ptr<myrocks::Rdb_key_def const>::~shared_ptr() (this=0x7f98e8599020, __in_chrg=<optimized out>) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr.h:93
#4  0x0000000000cac3d2 in myrocks::ha_rocksdb::secondary_index_read(int, unsigned char*) (this=this@entry=0x7f9743692e10, keyno=<optimized out>, buf=buf@entry=0x7f973f9ff410 "\377\237G") at /home/mcallaghan/mysql.aug5/5.6/storage/rocksdb/ha_rocksdb.cc:5743
#5  0x0000000000cb0e69 in myrocks::ha_rocksdb::index_next_with_direction(unsigned char*, bool) (move_forward=true, buf=0x7f973f9ff410 "\377\237G", this=0x7f9743692e10) at /home/mcallaghan/mysql.aug5/5.6/storage/rocksdb/ha_rocksdb.cc:6463
#6  0x0000000000cb0e69 in myrocks::ha_rocksdb::index_next(unsigned char*) (this=0x7f9743692e10, buf=0x7f973f9ff410 "\377\237G") at /home/mcallaghan/mysql.aug5/5.6/storage/rocksdb/ha_rocksdb.cc:6415
#7  0x000000000082653a in join_read_next(READ_RECORD*) (info=0x7f97345ac220) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:2588
#8  0x000000000082a4f1 in sub_select(JOIN*, st_join_table*, bool) (join=0x7f973b81c0d8, join_tab=0x7f97345ac190, end_of_records=<optimized out>) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:1297
#9  0x00000000008285fb in do_select (join=0x7f973b81c0d8) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:950
#10 0x00000000008285fb in JOIN::exec() (this=0x7f973b81c0d8) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:207
#11 0x00000000008704a5 in mysql_execute_select (free_join=true, select_lex=0x7f98e19ae8f0, thd=0x7f98e19ab000) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_select.cc:1133
#12 0x00000000008704a5 in mysql_select(THD*, TABLE_LIST*, unsigned int, List<Item>&, Item*, SQL_I_List<st_order>*, SQL_I_List<st_order>*, Item*, unsigned long long, select_result*, st_select_lex_unit*, st_select_lex*) (thd=thd@entry=0x7f98e19ab000, tables=0x7f973b81b480, wild_num=0, fields=..., conds=0x7f973b81bce8, order=order@entry=0x7f98e19aeab8, group=0x7f98e19ae9f0, having=0x0, select_options=2148797184, result=0x7f973b81c0b0, unit=0x7f98e19ae298, select_lex=0x7f98e19ae8f0) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_select.cc:1254
#13 0x0000000000870cee in handle_select(THD*, select_result*, unsigned long) (thd=thd@entry=0x7f98e19ab000, result=result@entry=0x7f973b81c0b0, setup_tables_done_option=setup_tables_done_option@entry=0) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_select.cc:126
#14 0x00000000005d3675 in execute_sqlcom_select(THD*, TABLE_LIST*, ulonglong*) (thd=thd@entry=0x7f98e19ab000, all_tables=<optimized out>, last_timer=last_timer@entry=0x7f98e859a7b8) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_parse.cc:6042
#15 0x000000000084e8a7 in mysql_execute_command(THD*, unsigned long long*, unsigned long long*) (thd=thd@entry=0x7f98e19ab000, statement_start_time=statement_start_time@entry=0x7f98e859a678, post_parse=post_parse@entry=0x7f98e859a7b8) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_parse.cc:3346
#16 0x00000000008550a0 in mysql_parse(THD*, char*, unsigned int, Parser_state*, unsigned long long*, char*, std::string const&, char*) (thd=thd@entry=0x7f98e19ab000, rawbuf=<optimized out>, length=<optimized out>, parser_state=parser_state@entry=0x7f98e859af40, last_timer=last_timer@entry=0x7f98e859a7b8, async_commit=async_commit@entry=0x7f98e859a7b6 "", db=..., exit_admission_control=0x7f98e859a7b7 "") at /home/mcallaghan/mysql.aug5/5.6/sql/sql_parse.cc:7674
#17 0x00000000008569b4 in dispatch_command(enum_server_command, THD*, char*, unsigned int) (command=COM_QUERY, thd=0x7f98e19ab000, packet=0x7f98e19b0001 "", packet_length=<optimized out>) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_parse.cc:1691
#18 0x0000000000858649 in do_command(THD*) (thd=<optimized out>) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_parse.cc:1133
#19 0x000000000081aa5a in do_handle_one_connection(THD*) (thd_arg=<optimized out>) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_connect.cc:1108
#20 0x000000000081acb9 in handle_one_connection(void*) (arg=<optimized out>) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_connect.cc:929
#21 0x00007f98f61d87f1 in start_thread () at /usr/local/fbcode/gcc-4.9-glibc-2.20/lib/libpthread.so.0
#22 0x00007f98f455746d in clone () at /usr/local/fbcode/gcc-4.9-glibc-2.20/lib/libc.so.6

One example where m_pk_descr is passed by value. This shows up in PMP output and the line numbers are from the Aug 5 build. See lines 5742 and 5743.

 5722 int ha_rocksdb::secondary_index_read(const int keyno, uchar *buf)
 5723 {
 5724   DBUG_ASSERT(buf != nullptr);
 5725   DBUG_ASSERT(table != nullptr);
 5726
 5727   stats.rows_requested++;
 5728
 5729   /* Use STATUS_NOT_FOUND when record not found or some error occurred */
 5730   table->status= STATUS_NOT_FOUND;
 5731
 5732   if (m_scan_it->Valid())
 5733   {
 5734     rocksdb::Slice key= m_scan_it->key();
 5735
 5736     /* Check if we've ran out of records of this index */
 5737     if (m_key_descr_arr[keyno]->covers_key(key))
 5738     {
 5739       int rc;
 5740
 5741       //TODO: We could here check if we have ran out of range we're scanning
 5742       uint size= m_key_descr_arr[keyno]->get_primary_key_tuple(
 5743           table, m_pk_descr, &key, m_pk_packed_tuple);
 5744       if (size == RDB_INVALID_KEY_LEN)
 5745       {
 5746         return HA_ERR_INTERNAL_ERROR;
 5747       }
@mdcallag

This comment has been minimized.

Copy link
Author

@mdcallag mdcallag commented Oct 2, 2016

And it is also a problem for sysbench read-only using this command line

sysbench --test=tests/db/oltp.lua --db-driver=mysql --mysql-engine-trx=yes --mysql-user=root --mysql-password=pw --mysql-host=127.0.0.1 --mysql-db=test --mysql-table-engine=rocksdb --oltp-table-size=100000 --oltp-tables-count=8 --num-threads=50 --max-requests=0 --max-time=60 --oltp-read-only=on run
@mdcallag

This comment has been minimized.

Copy link
Author

@mdcallag mdcallag commented Oct 2, 2016

Another example from the Aug 5 build

#0  0x00000000006ce828 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (this=0x7f979d9e9d00) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr_base.h:146
#1  0x0000000000cac52c in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (this=0x7f98e79ca028, __in_chrg=<optimized out>) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr_base.h:666
#2  0x0000000000cac52c in std::__shared_ptr<myrocks::Rdb_key_def const, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (this=0x7f98e79ca020, __in_chrg=<optimized out>) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr_base.h:914
#3  0x0000000000cac52c in std::shared_ptr<myrocks::Rdb_key_def const>::~shared_ptr() (this=0x7f98e79ca020, __in_chrg=<optimized out>) at /home/engshare/fbcode/third-party2/libgcc/4.9.x/gcc-4.9-glibc-2.20/024dbc3/include/c++/4.9.x/bits/shared_ptr.h:93
#4  0x0000000000cac52c in myrocks::ha_rocksdb::secondary_index_read(int, unsigned char*) (this=this@entry=0x7f973aa85610, keyno=<optimized out>, buf=buf@entry=0x7f97e4c5d810 "\377\310\003") at /home/mcallaghan/mysql.aug5/5.6/storage/rocksdb/ha_rocksdb.cc:5755
#5  0x0000000000cb0e69 in myrocks::ha_rocksdb::index_next_with_direction(unsigned char*, bool) (move_forward=true, buf=0x7f97e4c5d810 "\377\310\003", this=0x7f973aa85610) at /home/mcallaghan/mysql.aug5/5.6/storage/rocksdb/ha_rocksdb.cc:6463
#6  0x0000000000cb0e69 in myrocks::ha_rocksdb::index_next(unsigned char*) (this=0x7f973aa85610, buf=0x7f97e4c5d810 "\377\310\003") at /home/mcallaghan/mysql.aug5/5.6/storage/rocksdb/ha_rocksdb.cc:6415
#7  0x000000000082653a in join_read_next(READ_RECORD*) (info=0x7f973a78f220) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:2588
#8  0x000000000082a4f1 in sub_select(JOIN*, st_join_table*, bool) (join=0x7f973a61c0d8, join_tab=0x7f973a78f190, end_of_records=<optimized out>) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:1297
#9  0x00000000008285fb in do_select (join=0x7f973a61c0d8) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:950
#10 0x00000000008285fb in JOIN::exec() (this=0x7f973a61c0d8) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_executor.cc:207
#11 0x00000000008704a5 in mysql_execute_select (free_join=true, select_lex=0x7f98e1aa58f0, thd=0x7f98e1aa2000) at /home/mcallaghan/mysql.aug5/5.6/sql/sql_select.cc:1133

 5741       //TODO: We could here check if we have ran out of range we're scanning
 5742       uint size= m_key_descr_arr[keyno]->get_primary_key_tuple(
 5743           table, m_pk_descr, &key, m_pk_packed_tuple);
 5744       if (size == RDB_INVALID_KEY_LEN)
 5745       {
 5746         return HA_ERR_INTERNAL_ERROR;
 5747       }
 5748
 5749       m_last_rowkey.copy((const char*)m_pk_packed_tuple, size, &my_charset_bin);
 5750
 5751       if (m_keyread_only && m_lock_rows == RDB_LOCK_NONE &&
 5752           !has_hidden_pk(table))
 5753       {
 5754         rc= try_keyonly_read_from_sk(buf, m_key_descr_arr[keyno],
 5755                                      key, m_scan_it->value(), size);
 5756       }
 5757       else
 5758       {
 5759         rc= get_row_by_rowid(buf, m_pk_packed_tuple, size);
 5760       }
@mdcallag

This comment has been minimized.

Copy link
Author

@mdcallag mdcallag commented Oct 2, 2016

My C++ is rusty, but the problem appears to be from derefencing m_key_descr_arr on line 5754 above.

  /* Array of index descriptors */
  std::shared_ptr<Rdb_key_def> *m_key_descr_arr;
@spetrunia

This comment has been minimized.

Copy link
Collaborator

@spetrunia spetrunia commented Oct 3, 2016

My understanding of the issue: the cause is that m_key_descr is defined as shared_ptr<Rdb_key_def>:

  /* Array of index descriptors */
  std::shared_ptr<Rdb_key_def> *m_key_descr_arr;

while try_keyonly_read_from_sk's parameter is a reference to shared_ptr<const Rdb_key_def>:

int ha_rocksdb::try_keyonly_read_from_sk(uchar* buf,
    const std::shared_ptr<const Rdb_key_def>& kd,
    const rocksdb::Slice& key,

Different underlying types cause a new std::shared_ptr object to be created. when a new shared pointer is added, it increments shared_ptr's counter under the hood.

@jkedgar jkedgar self-assigned this Oct 3, 2016
@jkedgar

This comment has been minimized.

Copy link
Contributor

@jkedgar jkedgar commented Oct 3, 2016

Okay - that is really frustrating! I want try_key_only_read_from_sk() and get_primary_key_tuple() to have a pointer that is marked as const so that they can't change the data, but doing so (while using shared_ptrs causes performance problems. Changing the functions to accept a non-const pointer is probably the only solution, but that seems counter-productive. :(

@jkedgar

This comment has been minimized.

Copy link
Contributor

@jkedgar jkedgar commented Oct 3, 2016

It turns out the original code was on the wrong track. We should never pass a shared_ptr<> into a function unless we are intending for the function to take ownership. I changed the code to pass in a const reference to the original object instead.
https://reviews.facebook.net/D64605

jkedgar added a commit that referenced this issue Oct 10, 2016
Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
@jkedgar jkedgar closed this Oct 11, 2016
georgelorchpercona added a commit to georgelorchpercona/percona-server that referenced this issue Jan 25, 2017
Upstream commit ID: d77735ab8e1dc358e4381a3405d16095bce715ac

Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
georgelorchpercona added a commit to georgelorchpercona/percona-server that referenced this issue Jan 25, 2017
Upstream commit ID: d77735ab8e1dc358e4381a3405d16095bce715ac

Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
georgelorchpercona added a commit to georgelorchpercona/percona-server that referenced this issue Jan 30, 2017
Upstream commit ID: d77735ab8e1dc358e4381a3405d16095bce715ac

Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
georgelorchpercona added a commit to georgelorchpercona/percona-server that referenced this issue Jan 30, 2017
Upstream commit ID: d77735ab8e1dc358e4381a3405d16095bce715ac

Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
hermanlee added a commit that referenced this issue Jan 31, 2017
Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
georgelorchpercona added a commit to georgelorchpercona/percona-server that referenced this issue Feb 1, 2017
Upstream commit ID: d77735ab8e1dc358e4381a3405d16095bce715ac

Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
VitaliyLi pushed a commit to VitaliyLi/mysql-5.6 that referenced this issue Feb 9, 2017
Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
gunnarku added a commit to facebook/mysql-8.0 that referenced this issue Jul 21, 2017
Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
gunnarku added a commit to facebook/mysql-8.0 that referenced this issue Jul 25, 2017
Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (facebook/mysql-5.6#330)

Test Plan: MTR and ran through the code under the debugger to make sure that a shared_ptr was not being constructed/destructed as before

Reviewers: yoshinorim, spetrunia, MarkCallaghan, hermanlee4

Reviewed By: hermanlee4

Subscribers: mung, vasilep, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D64605
facebook-github-bot added a commit that referenced this issue Dec 23, 2019
Summary:
We were passing around references to shared_ptr<Rdb_key_def> and sometimes using shared_ptr<const Rdb_key_def>.  Whenever we passed one into the other a new shared_ptr was temporarily constructed and then destructed.  This is the wrong way to do this.  We should only be passing the shared_ptr object when we want to take ownership.

As is often the case, fully understanding the issue leads to cleaner/simpler code.

Fixes Issue 330 (#330)

Differential Revision: https://reviews.facebook.net/D64605

fbshipit-source-id: 271569d985f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.