[MDEV-31340] Remove MY_COLLATION_HANDLER::strcasecmp() Created: 2023-05-25 Updated: 2024-01-19 |
|
| Status: | In Review |
| Project: | MariaDB Server |
| Component/s: | Character Sets |
| Fix Version/s: | 11.5 |
| Type: | Task | Priority: | Critical |
| Reporter: | Alexander Barkov | Assignee: | Sergei Golubchik |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
| Comments |
| Comment by Alexander Barkov [ 2023-11-30 ] | ||||||||||||||||||||||||||||||||||
|
Hello serg, Please review a patch for this task: https://github.com/MariaDB/server/commit/02b85f1cf9bbdaf4c0fb65ab4c259eb1d8446e1c Thanks. | ||||||||||||||||||||||||||||||||||
| Comment by Alexander Barkov [ 2023-12-28 ] | ||||||||||||||||||||||||||||||||||
|
Hello serg, This is a new patch version: Replied to your review comments by email. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2024-01-02 ] | ||||||||||||||||||||||||||||||||||
|
I reviewed the InnoDB changes, except those related to For dict_table_schema_check(), it could be better to modify the type of dict_col_meta_t::name and the declarations of table_stats_schema and index_stats_schema. The constants innobase_index_reserve_name_ls, FTS_DOC_ID_COL_NAME_ls and FTS_DOC_ID_INDEX_NAME_ls could be renamed to less obfuscated ones GEN_CLUST_INDEX_ls, FTS_DOC_ID_ls and FTS_DOC_ID_INDEX_ls. Why are those constant defined as static const in header files? Could they be static constexpr instead? Or could the constants be declared in the header file, and defined only once in one compilation unit? Have you measured the code size and performance impact of defining dict_table_get_col_name_ls() and similar functions inline? As far as I understand, it could be better to emit function calls than to duplicate code that includes loops or conditional branches. Should the following be removed from client/mysql.cc?
Should the functions my_strcasecmp_8bit() and my_strcasecmp_latin1() continue to be defined? | ||||||||||||||||||||||||||||||||||
| Comment by Alexander Barkov [ 2024-01-09 ] | ||||||||||||||||||||||||||||||||||
|
Hi marko! Thanks for your review!
True, the mem_pressure part won't be in the final version.
Done. It looks better. Thanks for this suggestion.
This rename does not seem to relate to the current task. Can we do it in a separate patch if needed?
I changed them to static constexpr.
No I have not. dict_table_get_col_name_ls() does not seem to have conditional branches.
cmp_database() is used in the code. Why remove it?
Yes, latin1 case insensitive comparison is still used in the code. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2024-01-09 ] | ||||||||||||||||||||||||||||||||||
|
Right, the function dict_table_get_col_name_ls() may not directly expand to any conditional branches (depending on the build parameters), but its execution will include them, for example strlen(). In fact, dict_table_get_col_name_ls() is duplicating some effort by invoking strlen() one more time, after dict_col_t::name() had already computed it for the returned name. On my system, even when I enable -O3 build, I observe that dict_table_has_column() is actually invoking dict_table_get_col_name_ls() via a call instruction and not expanding it inline. I think that it would be better to declare and define dict_table_get_col_name_ls() and dict_table_get_v_col_name_ls() without inline. The functions dict_col_t::name() and dict_table_get_v_col_name() had better be refactored, something like this:
The get_name() part is currently duplicated in both functions. You would write get_name_ls() in such a way that the duplicate call to strlen() would be avoided. Having less inline code defined in common header files should improve the compilation time. Since this is infrequently called code, any runtime speed advantage of inlining should not matter. Reduced code size could lead to faster running and easier-to-debug code. | ||||||||||||||||||||||||||||||||||
| Comment by Alexander Barkov [ 2024-01-16 ] | ||||||||||||||||||||||||||||||||||
|
Hi marko, thanks for your suggestions. | ||||||||||||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2024-01-16 ] | ||||||||||||||||||||||||||||||||||
|
Thank you, bar, the InnoDB changes look good to me. For any new or rewritten functions, such as dict_table_t::get_name_from_z_list() or dict_col_t::name(), I think that it is best to use the common formatting style instead of the archaic InnoDB one. | ||||||||||||||||||||||||||||||||||
| Comment by Alexander Barkov [ 2024-01-19 ] | ||||||||||||||||||||||||||||||||||
|
Hi serg, Please find the latest patch here: https://github.com/MariaDB/server/commit/be66e975ecf15ae696d2e645da4aaf1a06cf0082 It addresses your and Marko's comments. Thanks. |