Hi Tor,
On 5/13/11 10:44 AM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/trunk-dynarray/ based on
> revid:bjorn.munch@stripped
>
> 3083 Tor Didriksen 2011-05-13
> Bug#12552221 - MEMORY LEAK IN UPDATE_REF_AND_KEYS
>
> Allocate array in memroot rather than heap.
> Fixes the leak, and also yields a 1-2% performance gain with sysbench if you
> have lots of cpus/threads.
Looks good. Some minor suggestions below.
> === added file 'sql/mem_root_array.h'
> --- a/sql/mem_root_array.h 1970-01-01 00:00:00 +0000
> +++ b/sql/mem_root_array.h 2011-05-13 13:44:14 +0000
[...]
> +
> + // Returns a pointer to the first element in the array.
> + Element_type *begin() { return&m_array[0]; }
> +
> + // Returns an pointer to the past-the-end element in the array.
an -> a.
> + Element_type *end() { return&m_array[size()]; }
> +
> + // Erases all of the elements.
> + void clear()
> + {
> + if (!empty())
> + chop(0);
> + }
> +
> + /*
> + Chops the tail off the array, erasing all tail elements.
> + @param pos Index of first element to erase.
> + */
> + void chop(const size_t pos)
> + {
> + DBUG_ASSERT(pos < m_size);
> + if (pos >= m_size)
> + return;
Hum, no need? We do not protect other members for non-debug cases.
> + if (!has_trivial_destructor)
> + {
> + for (size_t ix= pos; ix< m_size; ++ix)
> + {
> + Element_type *p=&m_array[ix];
> + p->~Element_type(); // Destroy discarded element.
> + }
> + }
> + m_size= pos;
> + }
> +
> + /*
> + Reserves space for array elements.
> + Copies over existing elements, in case we are re-expanding the array.
> +
> + @param n number of elements.
> + @retval true if out-of-memory, false otherwise.
> + */
> + bool reserve(size_t n)
> + {
> + if (n<= m_capacity)
> + return false;
> +
> + void *mem= alloc_root(m_root, n * element_size());
Hum, what about alignment? Should we care? If not, just add a note.
> + if (!mem)
> + return true;
> + Element_type *array= static_cast<Element_type*>(mem);
> +
> + // Copy all the existing elements into the new array.
Could you add a @remark about this in top-level doxygen comment of the
class? It's something that deserves extra attention from developer using
this.
> + for (size_t ix= 0; ix< m_size; ++ix)
> + {
> + Element_type *new_p=&array[ix];
> + Element_type *old_p=&m_array[ix];
> + new (new_p) Element_type(*old_p); // Copy into new location.
> + if (!has_trivial_destructor)
> + old_p->~Element_type(); // Destroy the old element.
> + }
> +
> + // Forget the old array.
> + m_array= array;
> + m_capacity= n;
> + return false;
> + }
> +
[...]
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h 2011-05-02 11:51:41 +0000
> +++ b/sql/sql_select.h 2011-05-13 13:44:14 +0000
> @@ -30,6 +30,7 @@
> #include "records.h" /* READ_RECORD */
> #include "opt_range.h" /* SQL_SELECT, QUICK_SELECT_I */
>
> +#include "mem_root_array.h"
>
> /* Values in optimize */
> #define KEY_OPTIMIZE_EXISTS 1
> @@ -37,12 +38,12 @@
>
> /**
> Information about usage of an index to satisfy an equality condition.
> -
> - @note such objects are stored in DYNAMIC_ARRAY which uses sizeof(), so keep
> - this class as POD as possible.
> */
> class Key_use {
> public:
> + // We need the default constructor for unit testing.
> + Key_use() { memset(this, 0, sizeof(*this)); }
I'm always a bit worried about the consequences of a memset like this.
Could you add a note that generally this should be mostly a POD?
Regards,
Davi
