Re: [Maria-developers] 099596296ce: Avoid mallocs when using LONGLONG_BUFFER_SIZE
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 <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> 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<LONGLONG_BUFFER_SIZE> buf; + StringBuffer<LONGLONG_BUFFER_SIZE+1> buf; val_str(&buf, &buf); return to + sort_field->pack_sort_string(to, &buf, 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<LONGLONG_BUFFER_SIZE> buf; + StringBuffer<LONGLONG_BUFFER_SIZE+1> buf; // my_charset_bin is good enough for numbers buf.set_int(value, unsigned_flag, &my_charset_bin); str->append(buf);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Fri, Sep 11, 2020 at 1:29 PM Sergei Golubchik <serg@mariadb.org> 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
participants (2)
-
Michael Widenius -
Sergei Golubchik