Re: e0afb5f08b5: cleanup: further replace thd->alloc() with operator new
Hi, Nikita, Despite the commit has in the title, comments below apply to four commits: $ git log --oneline 7d5165894282^..0f6fd23e1604 0f6fd23e160 cleanup: add new(thd) to Sql_alloc and Item e0afb5f08b5 cleanup: further replace thd->alloc() with operator new 8daea2e14b2 cleanup: add operator new(size_t, const THD*) for single-object allocations 7d516589428 cleanup: add operator new[](size_t, const THD*) On Dec 06, Nikita Malyavin wrote:
diff --git a/sql/sql_class.h b/sql/sql_class.h index aff02df69ec..299c8298606 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1279,11 +1279,5 @@ class Query_arena { return state == STMT_CONVENTIONAL_EXECUTION; }
template <typename T=char> - inline T* alloc(size_t size) const - { - return (T*)alloc_root(mem_root, sizeof(T)*size); - } - - template <typename T=char> inline T* calloc(size_t size) const
Hmm, you didn't remove thd->calloc<>() I understand why, but that kind of makes the API inconsistent, perhaps it would've been cleaner to keep thd->alloc<>() and only implement new (thd) and new (arena) for Sql_alloc? May be not only for Sql_alloc. There are 1. pod types. they can be alloc'ed an calloc'ed, I think thd->alloc/thd->calloc is fine for them 2. non-pod types, can be allocated on the heap and on a mem_root. your new (thd)/delete (thd) operators are best for them. 3. non-pod types that can only be allocated on a mem_root. Sql_alloc can be used for them, if it'll disallow non-placement new. I feel like we don't need to make a distinction between 2 and 3, in that case Sql_alloc can simply go away.
{
diff --git a/plugin/feedback/feedback.cc b/plugin/feedback/feedback.cc index bc8a98c11ae..dcc2e4417e5 100644 --- a/plugin/feedback/feedback.cc +++ b/plugin/feedback/feedback.cc @@ -103,22 +103,22 @@ static COND* make_cond(THD *thd, TABLE_LIST *tables, LEX_STRING *filter) nrc->resolve_in_table_list_only(tables); nrc->select_lex= tables->select_lex;
- res= new (thd) Item_cond_or(thd); + res= new (thd) Item_cond_or(thd); if (!res) return OOM;
for (; filter->str; filter++) { - Item_field *fld= new (thd) Item_field(thd, nrc, db, table, + Item_field *fld= new (thd) Item_field(thd, nrc, db, table, field); - Item_string *pattern= new (thd) Item_string(thd, filter->str, + Item_string *pattern= new (thd) Item_string(thd, filter->str, (uint) filter->length, cs);
you might want to go through the diff and fix the indentation
- Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs); + Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs);
if (!fld || !pattern || !escape) return OOM;
- Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern, + Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern, escape, 0);
if (!like) diff --git a/sql/item.h b/sql/item.h index 631fd1b19f8..b0cf03c1435 100644 --- a/sql/item.h +++ b/sql/item.h @@ -857,8 +857,9 @@ class Item :public Value_source, static void *operator new(size_t size);
public: - static void *operator new(size_t size, MEM_ROOT *mem_root) throw () + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept { return alloc_root(mem_root, size); } + static void *operator new(size_t size, const THD *thd) noexcept; static void operator delete(void *ptr,size_t size) { TRASH_FREE(ptr, size); } static void operator delete(void *ptr, MEM_ROOT *mem_root) {}
Better inherit it from Sql_alloc
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index d86fdf5c5d6..e0adc5c3bb4 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -2007,7 +2007,7 @@ bool Item_func_interval::fix_length_and_dec(THD *thd)
if (not_null_consts) { - intervals= thd->alloc<interval_range>(rows - 1); + intervals= new(current_thd) interval_range[rows - 1];
current_thd ?
if (!intervals) return true;
diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc index 008384a6590..21364286260 100644 --- a/sql/item_subselect.cc +++ b/sql/item_subselect.cc @@ -6086,8 +6086,8 @@ bool Ordered_key::init(int col_idx)
// TIMOUR: check for mem allocation err, revert to scan
- key_columns= thd->alloc<Item_field*>(1); - compare_pred= thd->alloc<Item_func_lt*>(1); + key_columns= new(thd) Item_field*();; + compare_pred= new(thd) Item_func_lt*();;
;;
key_columns[0]= new (thd) Item_field(thd, tbl->field[col_idx]); /* Create the predicate (tmp_column[i] < outer_ref[i]). */ diff --git a/sql/opt_subselect.h b/sql/opt_subselect.h index 8b4dd1f0092..37902c6eebe 100644 --- a/sql/opt_subselect.h +++ b/sql/opt_subselect.h @@ -357,7 +357,7 @@ uint get_number_of_tables_at_top_level(JOIN *join); the tuple. */
-class SJ_TMP_TABLE : public Sql_alloc +class SJ_TMP_TABLE
why?
{ public: /* diff --git a/sql/sql_alloc.h b/sql/sql_alloc.h index f5d2d4e8b1a..e643a9f817d 100644 --- a/sql/sql_alloc.h +++ b/sql/sql_alloc.h @@ -23,17 +23,21 @@ class Sql_alloc { public: - static void *operator new(size_t size) throw () + static void *operator new(size_t size) noexcept { return thd_alloc(_current_thd(), size); }
can this be disallowed completely?
- static void *operator new[](size_t size) throw () + static void *operator new[](size_t size) noexcept { return thd_alloc(_current_thd(), size); } - static void *operator new[](size_t size, MEM_ROOT *mem_root) throw () + + static void *operator new[](size_t size, const THD *thd) noexcept; + static void *operator new(size_t size, const THD *thd) noexcept; + + static void *operator new[](size_t size, MEM_ROOT *mem_root) noexcept { return alloc_root(mem_root, size); } - static void *operator new(size_t size, MEM_ROOT *mem_root) throw() + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept { return alloc_root(mem_root, size); } static void operator delete(void *ptr, size_t size) { TRASH_FREE(ptr, size); } static void operator delete(void *, MEM_ROOT *){} diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 7847389dabc..37869da63ae 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -8831,3 +8831,46 @@ LEX_CSTRING make_string(THD *thd, const char *start_ptr, size_t length= end_ptr - start_ptr; return {strmake_root(thd->mem_root, start_ptr, length), length}; } +void* operator new(size_t size, const Query_arena *thd) noexcept +{ + return alloc_root(thd->mem_root, size); +} + +void operator delete[](void *ptr, const Query_arena *thd) noexcept +{} + +void operator delete(void *ptr, const Query_arena *thd) noexcept +{}
if you put them into sql_class.h - will they be inlined? Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei! Thanks for a fast response. I'll give you the answers to the point, and later will get to the fixes that you mentioned. On Fri, 6 Dec 2024 at 17:19, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
Despite the commit has in the title, comments below apply to four commits:
$ git log --oneline 7d5165894282^..0f6fd23e1604 0f6fd23e160 cleanup: add new(thd) to Sql_alloc and Item e0afb5f08b5 cleanup: further replace thd->alloc() with operator new 8daea2e14b2 cleanup: add operator new(size_t, const THD*) for single-object allocations 7d516589428 cleanup: add operator new[](size_t, const THD*)
On Dec 06, Nikita Malyavin wrote:
diff --git a/sql/sql_class.h b/sql/sql_class.h index aff02df69ec..299c8298606 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1279,11 +1279,5 @@ class Query_arena { return state == STMT_CONVENTIONAL_EXECUTION; }
template <typename T=char> - inline T* alloc(size_t size) const - { - return (T*)alloc_root(mem_root, sizeof(T)*size); - } - - template <typename T=char> inline T* calloc(size_t size) const
Hmm, you didn't remove thd->calloc<>()
Well, I could do that, the calloc analogue looks like new (thd) char[size] (); Note the brackets before the semicolon -- they force zero initialization for scalar types, as well as for POD types: new (thd) pod_t[size] (); *As far as I understand, it forces calling a default constructor* *of a POD type or a scalar, which is a zero initialization.* *For non-POD types, the default constructor is always called.* But I thought that maybe it can be found too odious for our developer community ☺️ So I decided to see how it goes with single THD:alloc() first:)
- static void *operator new[](size_t size) throw () + static void *operator new[](size_t size) noexcept { return thd_alloc(_current_thd(), size); } - static void *operator new[](size_t size, MEM_ROOT *mem_root) throw () + + static void *operator new[](size_t size, const THD *thd) noexcept; + static void *operator new(size_t size, const THD *thd) noexcept; + + static void *operator new[](size_t size, MEM_ROOT *mem_root) noexcept { return alloc_root(mem_root, size); } - static void *operator new(size_t size, MEM_ROOT *mem_root) throw() + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept { return alloc_root(mem_root, size); } static void operator delete(void *ptr, size_t size) { TRASH_FREE(ptr, size); } static void operator delete(void *, MEM_ROOT *){} diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 7847389dabc..37869da63ae 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -8831,3 +8831,46 @@ LEX_CSTRING make_string(THD *thd, const char *start_ptr, size_t length= end_ptr - start_ptr; return {strmake_root(thd->mem_root, start_ptr, length), length}; } +void* operator new(size_t size, const Query_arena *thd) noexcept +{ + return alloc_root(thd->mem_root, size); +} + +void operator delete[](void *ptr, const Query_arena *thd) noexcept +{} + +void operator delete(void *ptr, const Query_arena *thd) noexcept +{}
if you put them into sql_class.h - will they be inlined?
Apparently, one can't put a global operator new/delete definition into a header: then a linker finds duplicates:( I didn't find a solution for that. But I'm sure it'll be inlined with -flto and O1 or O2. -- Yours truly, Nikita Malyavin
Hi, Nikita, On Dec 06, Nikita Malyavin wrote:
template <typename T=char> - inline T* alloc(size_t size) const - { - return (T*)alloc_root(mem_root, sizeof(T)*size); - } - - template <typename T=char> inline T* calloc(size_t size) const
Hmm, you didn't remove thd->calloc<>()
Well, I could do that, the calloc analogue looks like
new (thd) char[size] ();
Note the brackets before the semicolon -- they force zero initialization for scalar types, as well as for POD types:
did you test that it works? it does require getting used to, but still I'd prefer a consistent API. Either we have both alloc and calloc or neither.
+void operator delete[](void *ptr, const Query_arena *thd) noexcept +{} + +void operator delete(void *ptr, const Query_arena *thd) noexcept +{}
if you put them into sql_class.h - will they be inlined?
Apparently, one can't put a global operator new/delete definition into a header: then a linker finds duplicates:(
even if you declare them static inline? Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
On Tue, 10 Dec 2024 at 14:04, Sergei Golubchik <serg@mariadb.org> wrote:
new (thd) char[size] ();
Note the brackets before the semicolon -- they force zero initialization for scalar types, as well as for POD types:
did you test that it works?
Yes, I did. And even better news that new (thd) char[size] {}; looks much better, and also works just the same.
Apparently, one can't put a global operator new/delete definition into
a header: then a linker finds duplicates:(
even if you declare them static inline?
you can't make global operator new static :( you actually can't do it inline (without static) either, at least, formally. But I tried: inline void* operator new(size_t size){ return malloc(size); } inline void operator delete(void *ptr) noexcept { free(ptr); }; Putting this in sql_alloc.h works for gcc, but doesn't work for msvc: warning C4595: 'operator delete': non-member operator new or delete functions may not be declared inline -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik