List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:May 16 2011 11:49am
Subject:Re: bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221
View as plain text  
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
>
>

Thread
bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221Tor Didriksen13 May
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221Davi Arnaut13 May
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221Tor Didriksen16 May
      • Re: bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221Davi Arnaut16 May
        • Re: bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221Tor Didriksen18 May