On Fri, May 13, 2011 at 8:24 PM, Davi Arnaut <davi.arnaut@stripped> wrote:
> 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.
OK
>
>
> + 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.
OK
>
>
> + 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.
We use alloc_root() all over the place, and I see no alignment arguments
anywhere.
We must assume that alloc_root() has the same poperty as malloc():
return a pointer to the allocated memory, which is suitably aligned for any
kind of variable
>
>
> + 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.
OK
>
>
> + 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?
>
>
Sorry for being lazy here. I'll write a proper CTOR.
> Regards,
>
> Davi
>
thanks -- didrik
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>
>
