maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Cut
> > 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".
>
> (also, "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
>
> s/where/was/
>
> >   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

Will fix the typos. Thanks!

> > diff --git a/sql/item.cc b/sql/item.cc
> > index f6f3e2720fa..a8a43c6266a 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -4703,7 +4703,7 @@ Item *Item_param::value_clone_item(THD *thd)
> >      return 0; // Should create Item_decimal. See MDEV-11361.
> >    case STRING_RESULT:
> >      return new (mem_root) Item_string(thd, name,
> > -                                      Lex_cstring(value.m_string.c_ptr_quick(),
> > +                                      Lex_cstring(value.m_string.ptr(),
>
> Hmm, you said that LEX_CSTRING::str should always be \0-terminated.
> If that's the case, then c_ptr() would be correct here, not ptr()
>
> >                                                    value.m_string.length()),
> >                                        value.m_string.charset(),
> >                                        collation.derivation,

Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes
build these
up I do not want to depend on that for generic usage. For usage to
printf() than we
should trust that the developer knows what he is doing.

However Item_string() will only use the exact length and never call
printf, so here this is safe.

> > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
<cut>

> > -  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));
>
> These two changes contradict each other. Either part_name must be
> \0-terminated, and then you don't need to specify a length in printf.
> Or it is not \0-terminated and you must specify a length.

part_name is not null terminated and that is why I had to add .* and length.
What is the contradiction ?

> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index f2a0f55aec8..95a57017c53 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -118,9 +121,14 @@ bool Binary_string::realloc_raw(size_t alloc_length)
> >    return FALSE;
> >  }
> >
> > +
> >  bool String::set_int(longlong num, bool unsigned_flag, CHARSET_INFO *cs)
> >  {
> > -  uint l=20*cs->mbmaxlen+1;
> > +  /*
> > +    This allocates a few bytes extra in the unlikely case that cs->mb_maxlen
> > +    > 1, but we can live with that
>
> dunno, it seems that utf8 is the *likely* case and it's getting more and
> more likely with the time,

Not for integers, as we often use binary strings for these.

> > @@ -1254,21 +1262,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,
>
> you don't need an if() because, as you wrote yourself "my_realloc()
> can't fail" here.

Yes, this should be true, but one never knows with allocation libraries.
There is implementation of realloc that makes a copy and deletes the old one.
However I am not sure what happens if there is no place to make a copy.
Better safe than sorry.

>      {
> > diff --git a/sql/sql_string.h b/sql/sql_string.h
> > index b3eca118b63..ba0cff68fb4 100644
> > --- a/sql/sql_string.h
> > +++ b/sql/sql_string.h
> > @@ -600,25 +600,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]))
> > +      return Ptr;
>
> No, this is not good. 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:
>
>   if (!Alloced_length && !Ptr[str_length])
>     return Ptr;

Good point.> > +    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();
>
> 1. why not to remove it now?
> 2. it's strange that you write "use c_ptr() instead" but you actually
>    use c_ptr_safe() instead.

What I meant is that the developer should instead use c_ptr().
I will make the message more clear.
I decided to call c_ptr_safe() here as this is the most safe
version to use.  In practice the new c_ptr() should be safe
for 99% of our code.


> >    }
> >    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);
> >      return Ptr;
> >    }
>
>
> could you write a comment, explaining when one should use c_ptr() and
> when - c_ptr_safe() ?

Yes, try to.  Will not be easy as most cases is now handled by c_ptr().

Regards,
Monty