List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:May 13 2011 6:24pm
Subject:Re: bzr commit into mysql-trunk branch (tor.didriksen:3083) Bug#12552221
View as plain text  
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
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