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@mariadb.org> 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:
commit 6ca3ab6c2c18cdbf614984cb9d5312d58e718372 Author: Monty <monty@mariadb.org> 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
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. 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-bitf... 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