Hi, Michael! On Dec 03, Michael Widenius wrote:

revision-id: 53a88b2b7032 (mariadb-10.5.2-272-g53a88b2b7032) parent(s): 0aab153e05be author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2020-09-24 17:26:40 +0300 message:

Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

The proble was that hen using String::alloc() to allocate a string,

"hen" ? after a minute of staring I realized that you probably meant "when". (and "problem")

the String ensures that there is space for an extra NULL byte in the buffer and if not, reallocates the string. This is a problem with the String::set_int() that calls alloc(21), which forces an extra malloc/free to happen.

- We do not anymore re-allocate String if alloc() is called with the Allocated_length. This reduces number of malloc() allocations, especially one big re-allocation in Protocol::send_result_Set_metadata() for almost every query that produced a result to the connnected client. - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE This can now be done as alloc() doesn't increase buffers if new length is not bigger than old one. - c_ptr() is redesigned to be safer (but a bit longer) than before. - Remove wrong usage of c_ptr_quick() c_ptr_quick() where used in many cases to get the pointer to the used buffer, even when it didn't need to be \0 terminated. In this case ptr() is a better substitute. Another problem with c_ptr_quick() is that it didn't guarantee that the string would be \0 terminated. - item_val_str(), an API function not used currently by the server, now always returns a null terminated string (before it didn't always do that). - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old mixed usage of performance keys caused assert's when String buffers where shrunk. - Binary_string::shrink() is simplifed

diff --git a/sql/item.cc b/sql/item.cc index 4a26cb6c140c..328fe96018a5 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -4574,7 +4574,7 @@ const String *Item_param::value_query_val_str(THD *thd, String *str) const break; } DBUG_ASSERT(str->length() <= typelen); - buf= str->c_ptr_quick(); + buf= (char*) str->ptr(); ptr= buf + str->length(); *ptr++= '\''; ptr+= (uint) my_TIME_to_str(&value.time, ptr, decimals);

this is just wrong. One cannot simply write data at the String's end pointer. One must alloc() the space first.

diff --git a/sql/partition_info.cc b/sql/partition_info.cc index 8ad14ca260c4..ce72a7af051d 100644 --- a/sql/partition_info.cc +++ b/sql/partition_info.cc @@ -126,7 +126,7 @@ partition_info *partition_info::get_clone(THD *thd) /** Mark named [sub]partition to be used/locked.

- @param part_name Partition name to match. + @param part_name Partition name to match. Must be \0 terminated! @param length Partition name length.

@return Success if partition found @@ -172,9 +172,9 @@ bool partition_info::add_named_partition(const char *part_name, size_t length) else bitmap_set_bit(&read_partitions, part_def->part_id); } - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s", + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s", part_def->part_id, part_def->is_subpart, - part_name)); + length, part_name));

why did you specify a length if the name "must be \0 terminated!" ? (and I agree that it must be)

DBUG_RETURN(false); }

diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 633d969b3dec..2b603380aa6c 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -404,12 +404,16 @@ static int item_value_type(struct st_mysql_value *value) static const char *item_val_str(struct st_mysql_value *value, char *buffer, int *length) { + size_t org_length= *length; - String str(buffer, *length, system_charset_info), *res; + String str(buffer, org_length, system_charset_info), *res; if (!(res= ((st_item_value_holder*)value)->item->val_str(&str))) return NULL; *length= res->length(); - if (res->c_ptr_quick() == buffer) + if (res->ptr() == buffer && res->length() < org_length) + { + buffer[res->length()]= 0; return buffer; + }

Okay. This looked like the *only* correct usage of c_ptr_quick(). Agree, better to remove c_ptr_quick().

/* Lets be nice and create a temporary string since the diff --git a/sql/sql_string.cc b/sql/sql_string.cc index e2defba434dd..8ab156d0e556 100644 --- a/sql/sql_string.cc +++ b/sql/sql_string.cc @@ -1251,21 +1259,16 @@ bool String::append_semi_hex(const char *s, uint len, CHARSET_INFO *cs) // Shrink the buffer, but only if it is allocated on the heap. void Binary_string::shrink(size_t arg_length) { - if (!is_alloced()) - return; - if (ALIGN_SIZE(arg_length + 1) < Alloced_length) + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length) + { + char *new_ptr; + /* my_realloc() can't fail as new buffer is less than the original one */ + if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length, + MYF(thread_specific ? + MY_THREAD_SPECIFIC : 0))))

why do you need an if() if "my_realloc() can't fail" ? (and it really cannot, you're right)

{ - char* new_ptr; - if (!(new_ptr = (char*)my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length, - MYF(thread_specific ? MY_THREAD_SPECIFIC : 0)))) - { - Alloced_length = 0; - real_alloc(arg_length); - } - else - { - Ptr = new_ptr; - Alloced_length = (uint32)arg_length; - } + Ptr= new_ptr; + Alloced_length= (uint32) arg_length; } + } } diff --git a/sql/sql_string.h b/sql/sql_string.h index 2ef817ea0adc..01d5211fe850 100644 --- a/sql/sql_string.h +++ b/sql/sql_string.h @@ -599,25 +599,34 @@ class Binary_string: public Static_binary_string

inline char *c_ptr() { - DBUG_ASSERT(!alloced || !Ptr || !Alloced_length || - (Alloced_length >= (str_length + 1))); - - if (!Ptr || Ptr[str_length]) // Should be safe - (void) realloc(str_length); + if (unlikely(!Ptr)) + return (char*) ""; + /* + Here we assume that any buffer used to initalize String has + an end \0 or have at least an accessable character at end. + This is to handle the case of String("Hello",5) efficently. + */ + if (unlikely(!alloced && !Ptr[str_length]))

No, this is wrong. Note the difference between String a("Hello", 5) and char hello[5]; String a(buf, 5); Your assumption should only apply to the first case, not to the second. In the first case alloced==Alloced_length==0, in the second case only alloced==0 and Alloced_length==5. So in the if() above you need to look at Alloced_length.

+ return Ptr; + if (str_length < Alloced_length) + { + Ptr[str_length]=0; + return Ptr; + } + (void) realloc(str_length+1); /* This will add end \0 */ return Ptr; } + /* Use c_ptr() instead. This will be deleted soon, kept for compatiblity */ inline char *c_ptr_quick() { - if (Ptr && str_length < Alloced_length) - Ptr[str_length]=0; - return Ptr; + return c_ptr_safe(); } inline char *c_ptr_safe() { if (Ptr && str_length < Alloced_length) Ptr[str_length]=0; else - (void) realloc(str_length); + (void) realloc(str_length + 1);

what's the difference between c_ptr_safe and c_ptr, again? they're very similar, so they'd need a good comment explaining when to use what.

return Ptr; }

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org