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 <[email protected]> > 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 <[email protected]> > 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
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
