Re: [Maria-developers] 099596296ce: Avoid mallocs when using LONGLONG_BUFFER_SIZE

[Michael Widenius]

Hi!

On Fri, Sep 11, 2020 at 1:29 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> This is a good catch!
>
> But note that LONGLONG_BUFFER_SIZE already reserves +1 for '\0', it's
> defined as
>
> /* Max length needed for a buffer to hold a longlong or ulonglong + end \0 */
> #define LONGLONG_BUFFER_SIZE 21
>
> The problem is that String::set_int doesn't use LONGLONG_BUFFER_SIZE, it uses
> 20*cs->mbmaxlen+1
>
> This +1 is supposed to reserve one byte for '\0'. Which is incorrect,
> because alloc() does it too, so a better fix would be to remove +1 from
> String::set_int().

This is part of the problem.  However if String::set_int would use
LONGLONG_BUFFER_SIZE
for allocation (which it should instead of 20) we still would have a problem.

I think it's right that any allocation for a String buffer should
include space for the extra end null,
as we are in may cases uses c_ptr() for these buffers.

alloc() does reserve place for an extra \0.  However the problem is
that realloc tests
if requested_buffer_length >= allocated_length and if not does an allocation.
In effect this means that the buffer, with current code, needs to be 2
bigger than
the 'size-for-string-data'

The fix I intend to do later in in 10.6 is to change the >= to > in realloc()
This will fix the problem and we can use LONGLONG_BUFFER_SIZE everywhere
without having a second malloc.  This will however require some other changes
and testing that I don't have time to do now, so I prefer to keep
things as it's now
until the above code is done.

Regards,
Monty

___
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

Re: [Maria-developers] 099596296ce: Avoid mallocs when using LONGLONG_BUFFER_SIZE

[Sergei Golubchik]

Hi, Michael!

This is a good catch!

But note that LONGLONG_BUFFER_SIZE already reserves +1 for '\0', it's
defined as

/* Max length needed for a buffer to hold a longlong or ulonglong + end \0 */
#define LONGLONG_BUFFER_SIZE 21

The problem is that String::set_int doesn't use LONGLONG_BUFFER_SIZE, it uses
20*cs->mbmaxlen+1

This +1 is supposed to reserve one byte for '\0'. Which is incorrect,
because alloc() does it too, so a better fix would be to remove +1 from
String::set_int().

On Sep 08, Michael Widenius wrote:
> revision-id: 099596296ce (mariadb-10.5.2-272-g099596296ce)
> parent(s): 2de0c74f94c
> author: Michael Widenius 
> committer: Michael Widenius 
> timestamp: 2020-09-03 15:17:56 +0300
> message:
> 
> Avoid mallocs when using LONGLONG_BUFFER_SIZE
> 
> When using String::alloc() to allocate a string, 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(LONGLONG_BUFFER_SIZE), which forces an extra malloc/free to
> happen.
> 
> Fixed by using LONGLONG_BUFFER_SIZE+1 for StringBuffer's.
> 
> diff --git a/sql/field.cc b/sql/field.cc
> index ab7254e483d..2be4da8d4a8 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -1085,7 +1085,7 @@ Field_longstr::make_packed_sort_key_part(uchar *buff,
>  uchar*
>  Field_longstr::pack_sort_string(uchar *to, const SORT_FIELD_ATTR *sort_field)
>  {
> -  StringBuffer buf;
> +  StringBuffer buf;
>val_str(, );
>return to + sort_field->pack_sort_string(to, , field_charset());
>  }
> diff --git a/sql/item.cc b/sql/item.cc
> index 4a26cb6c140..0d947749e01 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -3612,7 +3612,7 @@ String *Item_int::val_str(String *str)
>  
>  void Item_int::print(String *str, enum_query_type query_type)
>  {
> -  StringBuffer buf;
> +  StringBuffer buf;
>// my_charset_bin is good enough for numbers
>buf.set_int(value, unsigned_flag, _charset_bin);
>str->append(buf);
> 
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