maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

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@xxxxxxxxx> 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