[Maria-developers] On bb-10.6-monty work
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
commit 26045ea1708c6551142fa4c155e708f9985b0114 Author: Monty <monty@mariadb.org> Date: Wed Aug 12 20:29:55 2020 +0300
Reduce usage of strlen()
diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc index 3c43595533f..d949687283f 100644 --- a/storage/maria/ha_maria.cc +++ b/storage/maria/ha_maria.cc @@ -407,13 +407,13 @@ static handler *maria_create_handler(handlerton *hton, }
-static void _ma_check_print(HA_CHECK *param, const char* msg_type, +static void _ma_check_print(HA_CHECK *param, const LEX_CSTRING *msg_type, const char *msgbuf)
I would also suggest passing LEX_CSTRING by value. Its size is not more than two pointers, so it can be passed by registers by most ABIs. Passing it by a pointer add a second dereference, which is costy
{ - if (msg_type == MA_CHECK_INFO) + if (msg_type == &MA_CHECK_INFO) sql_print_information("%s.%s: %s", param->db_name, param->table_name, msgbuf); - else if (msg_type == MA_CHECK_WARNING) + else if (msg_type == &MA_CHECK_WARNING) sql_print_warning("%s.%s: %s", param->db_name, param->table_name, msgbuf); else @@ -423,7 +423,7 @@ static void _ma_check_print(HA_CHECK *param, const char* msg_type,
// collect errors printed by maria_check routines
-static void _ma_check_print_msg(HA_CHECK *param, const char *msg_type, +static void _ma_check_print_msg(HA_CHECK *param, const LEX_CSTRING *msg_type, const char *fmt, va_list args) { THD *thd= (THD *) param->thd;
-- Yours truly, Nikita Malyavin
Hi! On Fri, Sep 11, 2020 at 8:11 AM Nikita Malyavin <nikitamalyavin@gmail.com> wrote: <cut>
-static void _ma_check_print(HA_CHECK *param, const char* msg_type, +static void _ma_check_print(HA_CHECK *param, const LEX_CSTRING *msg_type, const char *msgbuf)
I would also suggest passing LEX_CSTRING by value. Its size is not more than two pointers, so it can be passed by registers by most ABIs.
I greatly prefer to always pass things as pointers to ensure that one doesn't accidentally pass bug structures on the stack (a problem we have had several times). Yes, it's true that for a simple function that directly uses the LEX_CSTRING then it's not a big difference in speed or size when calling with LEX_CSTRING. However if the LEX_CSTRING is passed down to other functions that in their turn do the same then things will start to matter. Because the caller should not know how deep the called function will go, I prefer to use pointers in most cases.
Passing it by a pointer add a second dereference, which is costy
Not really as when you pass the LEX_CSTRING on the stack, you have to read and copy the pointers. It's less code to do the deference of LEX_CSTRING in the call than in all the 100+ places that may call the function.
{ - if (msg_type == MA_CHECK_INFO) + if (msg_type == &MA_CHECK_INFO) sql_print_information("%s.%s: %s", param->db_name, param->table_name, msgbuf); - else if (msg_type == MA_CHECK_WARNING) + else if (msg_type == &MA_CHECK_WARNING) sql_print_warning("%s.%s: %s", param->db_name, param->table_name, msgbuf);
Not also that the above code would not work if the LEX_CSTRING would be an object on the stack. Regards, Monty
Hi! Sorry for the delay, have been very busy with working on bug fixes and internal discussions. On Fri, Sep 11, 2020 at 7:50 AM Nikita Malyavin <nikitamalyavin@gmail.com> wrote:
Hello, Monty!
I've been asked to review your recent work regarding memory footprint optimizations.
<cut>
--- 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.
I need the constructor to ensure that no one calls set_join_tab_idx() (which can be called for any item) and possible other methods. The two other methods are a safeguard as it's not obvious when they could be called.
Besides, I see fix_char_length is not virtual at all, so reimplementing it has no efect.
Yes, I missed that. I have now deleted that one.
And why can't we do all that in Item_bool itself?
We can't fix join_tab_idx().
Note that adding new class increases a virtual method table, and it's huge already, for Item hierarchy.
Adding a new class should not increase the size of the virtual table for items. That only contains virtual functions. Add a new item with virtual methods does create a new virtual table, but it's not that huge (but it's not that small either). It's around 1654 bytes for Item's in 10.6
We could also add some other frequently used Items, like 0, 1, '', NULL, to a static memory.
We create very few item's with fixed value on the fly. Another problem with static item's is that we have functions that change them, with _set_join_tab_idx() and marker() for example, which means most items can't be static. We only use the new Item_true and Item_false to replace top level items, which are not marked. Other places are not safe. This makes it hard to use other constants like 0, 1 and NULL as static items.
commit 6ca3ab6c2c18cdbf614984cb9d5312d58e718372 <cut>
+ + +/* 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; +}
<cut>
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.
When I wrote the above I was considering doing the above with a template, but as it was only needed for two item's I decided it was not yet time to do that. Doing it for 2 instances and 5 functions would in the end make the code longer. If we would ever need to add a third flag, the yes we should definitely try to automate this.
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.
I agree that this is a good way forward, if we ever need to add more flags or we want to use the same method elsewhere.
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...
I looked at this. It's an interesting solution, but I didn't like the way the code looks. It's not obvious for a C programmer and one it's not clear how to use or debug it. Isn't there a way to instantiate templates for just the 5 functions that I added? (In other words, create resulting code that is in effect exact like what I wrote, and no other versions)? <cut>
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.
Agree that we need a good simple way to define bit fields in the future. However I will for now leave this until we have to add a new bitfield. Regards, Monty
participants (2)
-
Michael Widenius -
Nikita Malyavin