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
<nikitamalya...@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-bitfields.html
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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
