Hi, Michael!
On Dec 03, Michael Widenius wrote:
> revision-id: 53a88b2b7032 (mariadb-10.5.2-272-g53a88b2b7032)
> parent(s): 0aab153e05be
> author: Michael Widenius <michael.widen...@gmail.com>
> committer: Michael Widenius <michael.widen...@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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
