Convert MyRocks clone and associated code to use std::string_view

facebook / mysql-5.6

Facebook's branch of the Oracle MySQL database. This includes MyRocks.

http://myrocks.io

Other

2.46k stars 711 forks source link

Convert MyRocks clone and associated code to use std::string_view #1427

Closed laurynas-biveinis closed 3 months ago

laurynas-biveinis commented 5 months ago

std::string_view has several advantages over std::string char char *:

it's a non-owning view that cheaply converts from std::string and converts from char * with a cost of a strlen call (which the patch takes care to remove where possible)
is more efficient (ones less level of indirection to string contents, no SSO checking) than std::string while supporting similar (read-only) API
it's cheaper to pass std::string_view by value than std::string by const reference;
in some cases it avoids the cost of constructing new std::strings.

In converting clone code, also convert some utility functions that also called from elsewhere. In particular, convert rocksdb_datadir and rocksdb_wal_dir sysvars to be std::string_views, keeping the pointer-to-char variables only for interfacing with the MYSQL_SYSVAR macros.

facebook-github-bot commented 4 months ago

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

luqun commented 4 months ago

Looks like there are some compilation error, maybe different gcc version?

storage/rocksdb/clone/common.h:403:12: error: call to non-‘constexpr’ function ‘std::cxx11::basic_string<_CharT, _Traits, _Alloc>::operator sv_type() const [with _CharT = char; _Traits = std::char_traits; _Alloc = std::allocator; __sv_type = std::basic_string_view]’ 403 | return m_name;

storage/rocksdb/clone/donor.cc:90:12: error: call to non-‘constexpr’ function ‘std::cxx11::basic_string<_CharT, _Traits, _Alloc>::operator sv_type() const [with _CharT = char; _Traits = std::char_traits; _Alloc = std::allocator; __sv_type = std::basic_string_view]’ 90 | return m_dir;

storage/rocksdb/clone/client.cc:119:12: error: call to non-‘constexpr’ function ‘std::cxx11::basic_string<_CharT, _Traits, _Alloc>::operator sv_type() const [with _CharT = char; _Traits = std::char_traits; _Alloc = std::allocator; __sv_type = std::basic_string_view]’ 119 | return m_name;

facebook-github-bot commented 4 months ago

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis commented 4 months ago

@luqun, rebased and updated the PR, should be fixed now

facebook-github-bot commented 4 months ago

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

laurynas-biveinis commented 4 months ago

The PR passes calls data() on string_views and passes such pointers where null-terminated strings are expected. Since string_views are length-based, they are not guaranteed to be null-terminated. All string views are created from C or C++ strings here are, in fact, null terminated, but this is not a good practice to rely on that.

facebook-github-bot commented 4 months ago

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis commented 4 months ago

Removed the incorrect uses of string_view::data in the contexts where null-terminated strings were expected. The patch is smaller now and ready for review

facebook-github-bot commented 4 months ago

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

facebook-github-bot commented 3 months ago

This pull request has been merged in facebook/mysql-5.6@869b0c6511c6f1960dc17a7e96bc6b58b51106d4.