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