maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hello, Monty!

I've been asked to review your recent work regarding memory footprint
optimizations.

I went through all the work done, and mostly liked the changes done. So
here are only two commits i'd like to highlight

> commit bf8bd0573440dc39490eb795b5455735dd63132b
> Author: Monty <monty@xxxxxxxxxxx>
> Date:   Tue Jul 14 18:36:05 2020 +0300
>
>     MDEV-23001 Precreate static Item_bool() to simplify code
>
>     The following changes where done:
>     - Create global Item: Item_false and Item_true
>     - Replace all creation if 'FALSE' and 'TRUE' top level items used for
>       WHERE/HAVING/ON clauses to use Item_false and Item_true.
>
>     The benefit are:
>     - Less and faster code
>     - No test needed if we where able to create the new item.
>     - Fixed possible errors if 'new' would have failed for the Item_bool's
>
> diff --git a/sql/item.h b/sql/item.h
> index c4fbf8f9c0a..4cdee637415 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -4236,6 +4239,23 @@ class Item_bool :public Item_int
>  };
>
>
> +class Item_bool_static :public Item_bool
> +{
> +public:
> +  Item_bool_static(const char *str_arg, longlong i):
> +    Item_bool(NULL, str_arg, i) {};
> +
> +  /* Create dummy functions for things that may change the static item */
> +  void fix_length_and_charset(uint32 max_char_length_arg, CHARSET_INFO
*cs)
> +  {}
> +  void fix_char_length(size_t max_char_length_arg)
> +  {}
> +  void set_join_tab_idx(uint join_tab_idx_arg)
> +  { DBUG_ASSERT(0); }
> +};
> +
> +extern const Item_bool_static Item_false, Item_true;
> +
>  class Item_uint :public Item_int
>  {
>  public:



Do we really need an additional class for static Item_bool's?
I know that we have constructors hidden for Item, so maybe just
unhide it in Item_bool, and that's it.

Besides, I see fix_char_length is not virtual at all, so reimplementing it
has no efect.
And why can't we do all that in Item_bool itself?

Note that adding new class increases a virtual method table, and it's huge
already, for Item hierarchy.

We could also add some other frequently used Items, like 0, 1, '', NULL, to
a static memory.


> commit 6ca3ab6c2c18cdbf614984cb9d5312d58e718372
> Author: Monty <monty@xxxxxxxxxxx>
> Date:   Wed Sep 2 03:13:32 2020 +0300
>
>     Split item->flags into base_flags and with_flags
>
>     This was done to simplify copying of with_* flags
>
>     Other things:
>     - Changed Flags to C++ enums, which enables gdb to print
>       out bit values for the flags. This also enables compiler
>       errors if one tries to manipulate a non existing bit in
>       a variable.
>     - Added set_maybe_null() as a shortcut as setting the
>       MAYBE_NULL flags was used in a LOT of places.
>     - Renamed PARAM flag to SP_VAR to ensure it's not confused with
persistent
>       statement parameters.
>
> diff --git a/sql/item.h b/sql/item.h
> index 09618bbb52c..9a794d96982 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -717,28 +717,94 @@ class Item_const
>
>  #define STOP_PTR ((void *) 1)
>
> -/* Bits used in Item.flags */
> -/* If item may be null */
> -#define ITEM_FLAG_MAYBE_NULL_SHIFT 0
> -#define ITEM_FLAG_MAYBE_NULL  (1<<ITEM_FLAG_MAYBE_NULL_SHIFT)
> -/* If used in GROUP BY list of a query with ROLLUP */
> -#define ITEM_FLAG_IN_ROLLUP       (1<<1)
> -/* If Item contains an SP parameter */
> -#define ITEM_FLAG_WITH_PARAM  (1<<2)
> -/* If item contains a window func */
> -#define ITEM_FLAG_WITH_WINDOW_FUNC (1<<3)
> -/* True if any item except Item_sum contains a field. Set during
parsing. */
> -#define ITEM_FLAG_WITH_FIELD (1<<4)
> -/* If item was fixed with fix_fields */
> -#define ITEM_FLAG_FIXED (1<<5)
> -/* Indicates that name of this Item autogenerated or set by user */
> -#define ITEM_FLAG_IS_AUTOGENERATED_NAME (1 << 6)
> -/* Indicates that this item is in CYCLE clause of WITH */
> -#define ITEM_FLAG_IS_IN_WITH_CYCLE (1<<7)
> -/* True if item contains a sum func */
> -#define ITEM_FLAG_WITH_SUM_FUNC (1<< 8)
> -/* True if item containts a sub query */
> -#define ITEM_FLAG_WITH_SUBQUERY (1<< 9)
> +
> +/* Base flags (including IN) for an item */
> +
> +
> +typedef uint8 item_flags_t;
> +
> +enum class item_base_t : item_flags_t
> +{
> +  NONE=                  0,
> +#define ITEM_FLAGS_MAYBE_NULL_SHIFT 0 // Must match MAYBE_NULL
> +  MAYBE_NULL=            (1<<0),   // May be NULL.
> +  IN_ROLLUP=             (1<<1),   // Appears in GROUP BY list
> +                                   // of a query with ROLLUP.
> +  FIXED=                 (1<<2),   // Was fixed with fix_fields().
> +  IS_AUTOGENERATED_NAME= (1<<3),   // The name if this Item was
> +                                   // generated or set by the user.
> +  IS_IN_WITH_CYCLE=      (1<<4)    // This item is in CYCLE clause
> +                                   // of WITH.
> +};
> +
> +
> +/* Flags that tells us what kind of items the item contains */
> +
> +enum class item_with_t : item_flags_t
> +{
> +  NONE=             0,
> +  SP_VAR=      (1<<0), // If Item contains a stored procedure variable
> +  WINDOW_FUNC= (1<<1), // If item contains a window func
> +  FIELD=       (1<<2), // If any item except Item_sum contains a field.
> +  SUM_FUNC=    (1<<3), // If item contains a sum func
> +  SUBQUERY=    (1<<4)  // If item containts a sub query
> +};
> +
> +
> +/* Make operations in item_base_t and item_with_t work like 'int' */
> +static inline item_base_t operator&(const item_base_t a, const
item_base_t b)
> +{
> +  return (item_base_t) (((item_flags_t) a) & ((item_flags_t) b));
> +}
> +
> +static inline item_base_t & operator&=(item_base_t &a, item_base_t b)
> +{
> +  a= (item_base_t) (((item_flags_t) a) & (item_flags_t) b);
> +  return a;
> +}
> +
> +static inline item_base_t operator|(const item_base_t a, const
item_base_t b)
> +{
> +  return (item_base_t) (((item_flags_t) a) | ((item_flags_t) b));
> +}
> +
> +static inline item_base_t & operator|=(item_base_t &a, item_base_t b)
> +{
> +  a= (item_base_t) (((item_flags_t) a) | (item_flags_t) b);
> +  return a;
> +}
> +
> +static inline item_base_t operator~(const item_base_t a)
> +{
> +  return (item_base_t) ~(item_flags_t) a;
> +}
> +
> +static inline item_with_t operator&(const item_with_t a, const
item_with_t b)
> +{
> +  return (item_with_t) (((item_flags_t) a) & ((item_flags_t) b));
> +}
> +
> +static inline item_with_t & operator&=(item_with_t &a, item_with_t b)
> +{
> +  a= (item_with_t) (((item_flags_t) a) & (item_flags_t) b);
> +  return a;
> +}
> +
> +static inline item_with_t operator|(const item_with_t a, const
item_with_t b)
> +{
> +  return (item_with_t) (((item_flags_t) a) | ((item_flags_t) b));
> +}
> +
> +static inline item_with_t & operator|=(item_with_t &a, item_with_t b)
> +{
> +  a= (item_with_t) (((item_flags_t) a) | (item_flags_t) b);
> +  return a;
> +}
> +
> +static inline item_with_t operator~(const item_with_t a)
> +{
> +  return (item_with_t) ~(item_flags_t) a;
> +}
>
>
>  class Item :public Value_source,
> @@ -932,8 +998,8 @@ class Item :public Value_source,
>    const char *orig_name;
>
>    /* All common bool variables for an Item is stored here */
> -  typedef uint16 item_flags_t;
> -  item_flags_t flags;
> +  item_base_t base_flags;
> +  item_with_t with_flags;
>
>     /* Marker is used in some functions to temporary mark an item */
>    int16 marker;


Nice experimentation! I wanted to suggest using constexprs inside Item, but
this is even better, because you cant freely add a bit that not in the enum.

Though I think it is too bloaty to specify all the operators each time,
so I'd prefer to see duplications metaprogrammed out somehow.

There is no sane way in C++ (even in modern one) to specify traits, which
extend
the class functionality (see Golang's traits), so I'd suggest to use good
old
preprocessor way, like following:

#define T item_base_t
#include "setup_bitwise_operators.inc"

#define T item_with_t
#include "setup_bitwise_operators.inc"

and then in setup_bitwise_operators.inc:
static inline T operator&(const T a, const T b)
{
  return (T) (((item_flags_t) a) & ((item_flags_t) b));
}

And so on.

Another variant I've just googled is to make a template operator for all
types,
but restrict its applicatory type set with std::enable_if<T>:
https://www.justsoftwaresolutions.co.uk/cplusplus/using-enum-classes-as-bitfields.html

So we would only need to add a structure:
template<>
struct enable_bitmask_operators<item_base_t>{
    static constexpr bool enable=true;
};

To work.


Thereby, the first approach is:
+ Good old way
+ Works fast
- not a C++ way, though we work in a C++-only file

The second approach is:
+ modern
+ C++ way
- may compile slow
- may produce uncomfortable errors when compiler tries to deduce a
function/operator to apply

Up to you whic way to use, but I think it should be done, since we have
many other flag sets,
which are likely to be prettified with the same style.


-- 
Yours truly,
Nikita Malyavin