From 36cdd5c3cdb06d8538f64c0b312ffe4672a92e75 Mon Sep 17 00:00:00 2001 From: Monty Date: Wed, 16 Sep 2020 11:23:50 +0300 Subject: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc() The problem was that when one used 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(21), which forces extra malloc/free calls 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() was 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 did not 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 - Fixed bug in String(const char *str, size_t len, CHARSET_INFO *cs) that used Binary_string((char *) str, len) instead of Binary_string(str,len). - Changed argument to String() creations and String.set() functions to use 'const char*' instead of 'char*'. This ensures that Alloced_length is not set, which gives safety against someone trying to change the original string. This also would allow us to use !Alloced_length in c_ptr() if needed. - Changed string_ptr_cmp() to use memcmp() instead of c_ptr() to avoid a possible malloc during string comparision. --- sql/sql_string.h | 63 +++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 12 deletions(-) (limited to 'sql/sql_string.h') diff --git a/sql/sql_string.h b/sql/sql_string.h index b3eca118b63..c731f6189c8 100644 --- a/sql/sql_string.h +++ b/sql/sql_string.h @@ -600,25 +600,55 @@ public: 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) and + String("hello",5) efficiently. + + We have two options here. To test for !Alloced_length or !alloced. + Using "Alloced_length" is slightly safer so that we do not read + from potentially unintialized memory (normally not dangerous but + may give warnings in valgrind), but "alloced" is safer as there + are less change to get memory loss from code that is using + String((char*), length) or String.set((char*), length) and does + not free things properly (and there is several places in the code + where this happens and it is hard to find out if any of these will call + c_ptr(). + */ + if (unlikely(!alloced && !Ptr[str_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; } + /* + One should use c_ptr() instead for most cases. 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(); } + /* + This is to be used only in the case when one cannot use c_ptr(). + The cases are: + - When one initializes String with an external buffer and length and + buffer[length] could be uninitalized when c_ptr() is called. + - When valgrind gives warnings about uninitialized memory with c_ptr(). + */ 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; } @@ -634,7 +664,16 @@ public: } inline bool alloc(size_t arg_length) { - if (arg_length < Alloced_length) + /* + Allocate if we need more space or if we don't have p_done any + allocation yet (we don't want to have Ptr to be NULL for empty strings). + + Note that if arg_length == Alloced_length then we don't allocate. + This ensures we don't do any extra allocations in protocol and String:int, + but the string will not be atomically null terminated if c_ptr() is not + called. + */ + if (arg_length <= Alloced_length && Alloced_length) return 0; return real_alloc(arg_length); } @@ -642,7 +681,7 @@ public: bool realloc_raw(size_t arg_length); bool realloc(size_t arg_length) { - if (realloc_raw(arg_length)) + if (realloc_raw(arg_length+1)) return TRUE; Ptr[arg_length]= 0; // This make other funcs shorter return FALSE; @@ -743,7 +782,7 @@ public: */ String(const char *str, size_t len, CHARSET_INFO *cs) :Charset(cs), - Binary_string((char *) str, len) + Binary_string(str, len) { } String(char *str, size_t len, CHARSET_INFO *cs) :Charset(cs), -- cgit v1.2.1