From: Dmitry Lenev
Date: February 14 2011 1:12pm
Subject: Re: bzr commit into mysql-5.5 branch (jon.hauglid:3324)
 Bug#11752069
List-Archive: http://lists.mysql.com/commits/131184
Message-Id: <20110214131257.GC1903@bandersnatch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hello Jon Olav!

My comments about new version of your patch are mostly cosmetic:

* Jon Olav Hauglid <jon.hauglid@stripped> [11/02/14 12:32]:
> #At file:///export/home/x/mysql-5.5-bug43152/ based on revid:georgi.kodinov@stripped
> 
>  3324 Jon Olav Hauglid	2011-02-14
>       Bug #11752069 (former bug 43152)
>       Assertion `bitmap_is_set_all(&table->s->all_set)' failed in
>       handler::ha_reset

...

>      @ include/my_bit.h
>         Removed now unused function.

Maybe it makes sense to mention in the file comment that you've also
introduced new function?

...

> === modified file 'mysys/my_bitmap.c'
> --- a/mysys/my_bitmap.c	2011-01-11 09:07:37 +0000
> +++ b/mysys/my_bitmap.c	2011-02-14 09:28:11 +0000
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 2000 MySQL AB, 2008-2009 Sun Microsystems, Inc
> +/* Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved.
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -92,6 +92,46 @@ static inline void bitmap_unlock(MY_BITM
>      mysql_mutex_unlock(map->mutex);
>  }
>  
> +static inline uint get_first_set(uint32 value, uint word_pos)
> +{

AFAIR our coding style says that functions should be separated with
two empty lines.

> +  uchar *byte_ptr;
> +  uint byte_pos, bit_pos;
> +  if (value)
> +  {

Hmm.. Actually, I think, it is better to have this if-statement
outside this function, in the caller. See more below.

> +    byte_ptr= (uchar*)&value;
> +    for (byte_pos=0; byte_pos < 4; byte_pos++, byte_ptr++)
> +    {
> +      if (*byte_ptr)
> +      {
> +        for (bit_pos=0; bit_pos < 8; bit_pos++)

Note that "bit_pos < 8" condition is redundant here.
Please consider removing it.

> +          if (*byte_ptr & (1 << bit_pos))
> +            return (word_pos*32) + (byte_pos*8) + bit_pos;
> +      }
> +    }
> +  }
> +  return MY_BIT_NONE;
> +}
> +
> +static inline uint get_first(uint32 value, uint word_pos)
> +{

Similar comment about two empty lines here.

> +  uchar *byte_ptr;
> +  uint byte_pos, bit_pos;
> +  if (value != 0xFFFFFFFF)
> +  {

Similar comment about this if-statement as well.

> +    byte_ptr= (uchar*)&value;
> +    for (byte_pos=0; byte_pos < 4; byte_pos++, byte_ptr++)
> +    {
> +      if (*byte_ptr != 0xFF)
> +      {
> +        for (bit_pos=0; bit_pos < 8; bit_pos++)

And similar comment about  "bit_pos < 8" condition here.

> +          if (!(*byte_ptr & (1 << bit_pos)))
> +            return (word_pos*32) + (byte_pos*8) + bit_pos;
> +      }
> +    }
> +  }
> +  return MY_BIT_NONE;
> +}
> +
>  
>  my_bool bitmap_init(MY_BITMAP *map, my_bitmap_map *buf, uint n_bits,
>  		    my_bool thread_safe __attribute__((unused)))
> @@ -259,28 +299,38 @@ void bitmap_set_prefix(MY_BITMAP *map, u
>  
>  my_bool bitmap_is_prefix(const MY_BITMAP *map, uint prefix_size)
>  {
> -  uint prefix_bits= prefix_size & 0x7, res;
> -  uchar *m= (uchar*)map->bitmap;
> -  uchar *end_prefix= m+prefix_size/8;
> -  uchar *end;
> -  DBUG_ASSERT(m && prefix_size <= map->n_bits);
> -  end= m+no_bytes_in_map(map);
> -
> -  while (m < end_prefix)
> -    if (*m++ != 0xff)
> -      return 0;
> -
> -  *map->last_word_ptr&= ~map->last_word_mask; /*Clear bits*/
> -  res= 0;
> -  if (prefix_bits && *m++ != (1 << prefix_bits)-1)
> -    goto ret;
> -
> -  while (m < end)
> -    if (*m++ != 0)
> -      goto ret;
> -  res= 1;
> -ret:
> -  return res; 
> +  uint prefix_bits= prefix_size % 32;
> +  my_bitmap_map *word_ptr= map->bitmap, last_word;
> +  my_bitmap_map *end_prefix= word_ptr + prefix_size / 32;
> +  DBUG_ASSERT(word_ptr && prefix_size <= map->n_bits);
> +
> +  /* 1: Words that should be filled with 1 */
> +  while (word_ptr < end_prefix)
> +    if (*word_ptr++ != 0xFFFFFFFF)
> +      return FALSE;
> +
> +  last_word= *map->last_word_ptr & ~map->last_word_mask;
> +
> +  /* 2: Word which contains the end of the prefix (if any) */
> +  if (prefix_bits)
> +  {
> +    if (word_ptr == map->last_word_ptr)
> +      return uint4korr((uchar*)&last_word) == (uint32)((1 << prefix_bits) - 1);
> +    else if (uint4korr((uchar*)word_ptr) != (uint32)((1 << prefix_bits) - 1))
> +      return FALSE;
> +    word_ptr++;
> +  }
> +
> +  /* 3: Words that should be filled with 0 */
> +  while (word_ptr < map->last_word_ptr)
> +    if (*word_ptr++ != 0)
> +      return FALSE;
> +
> +  /*
> +    word_ptr will be larger than map->last_word_ptr if
> +    prefix_size equals map->n_bits and the bitmap is filled with 1.
> +  */
> +  return word_ptr > map->last_word_ptr || last_word == 0;
>  }

Hmm... OK. But I think it is also worth to mention in the above comment
that word_ptr > map->last_word_ptr is possible only when prefix % 32 == 0
(after you have added a shortcut for case when word_ptr == last_word_ptr).

>  
>  

...

> @@ -478,65 +538,39 @@ void bitmap_copy(MY_BITMAP *map, const M
>  
>  uint bitmap_get_first_set(const MY_BITMAP *map)
>  {
> -  uchar *byte_ptr;
> -  uint i,j,k;
> +  uint word_pos, bit_pos;
>    my_bitmap_map *data_ptr, *end= map->last_word_ptr;
>  
>    DBUG_ASSERT(map->bitmap);
>    data_ptr= map->bitmap;
> -  *map->last_word_ptr &= ~map->last_word_mask;
>  
> -  for (i=0; data_ptr <= end; data_ptr++, i++)
> +  for (word_pos=0; data_ptr < end; data_ptr++, word_pos++)
>    {
> -    if (*data_ptr)
> -    {
> -      byte_ptr= (uchar*)data_ptr;
> -      for (j=0; ; j++, byte_ptr++)
> -      {
> -        if (*byte_ptr)
> -        {
> -          for (k=0; ; k++)
> -          {
> -            if (*byte_ptr & (1 << k))
> -              return (i*32) + (j*8) + k;
> -          }
> -        }
> -      }
> -    }
> +    bit_pos= get_first_set(*data_ptr, word_pos);
> +    if (bit_pos != MY_BIT_NONE)
> +      return bit_pos;

As I have said above, IMO, it makes sense to move "if (*data_ptr)"
out of get_first_set() call. This will make the above "if (bit_pos)"
unnecessary.

>    }
> -  return MY_BIT_NONE;
> +
> +  return get_first_set(*map->last_word_ptr & ~map->last_word_mask, word_pos);
>  }
>  
>  
>  uint bitmap_get_first(const MY_BITMAP *map)
>  {
> -  uchar *byte_ptr;
> -  uint i,j,k;
> +  uint word_pos, bit_pos;
>    my_bitmap_map *data_ptr, *end= map->last_word_ptr;
>  
>    DBUG_ASSERT(map->bitmap);
>    data_ptr= map->bitmap;
> -  *map->last_word_ptr|= map->last_word_mask;
>  
> -  for (i=0; data_ptr <= end; data_ptr++, i++)
> +  for (word_pos=0; data_ptr < end; data_ptr++, word_pos++)
>    {
> -    if (*data_ptr != 0xFFFFFFFF)
> -    {
> -      byte_ptr= (uchar*)data_ptr;
> -      for (j=0; ; j++, byte_ptr++)
> -      {
> -        if (*byte_ptr != 0xFF)
> -        {
> -          for (k=0; ; k++)
> -          {
> -            if (!(*byte_ptr & (1 << k)))
> -              return (i*32) + (j*8) + k;
> -          }
> -        }
> -      }
> -    }
> +    bit_pos= get_first(*data_ptr, word_pos);
> +    if (bit_pos != MY_BIT_NONE)
> +      return bit_pos;

Same comment here.

>    }
> -  return MY_BIT_NONE;
> +
> +  return get_first(*map->last_word_ptr | map->last_word_mask, word_pos);
>  }

I think it is OK to push this patch after considering the above comments
and getting approval from the second reviewer.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification