[Maria-developers] review of MDEV-4011 per thread memory counting and usage
Hi. Here it is:
=== modified file 'client/completion_hash.cc' --- client/completion_hash.cc 2011-06-30 15:46:53 +0000 +++ client/completion_hash.cc 2013-01-15 21:00:33 +0000 @@ -49,7 +49,7 @@ int completion_hash_init(HashTable *ht, ht->initialized = 0; return FAILURE; } - init_alloc_root(&ht->mem_root, 8192, 0); + init_alloc_root(&ht->mem_root, 8192, 0, 0);
very often you write 0 or MY_THREAD_SPECIFIC instead of MYF(0) and MYF(MY_THREAD_SPECIFIC)
ht->pHashFunction = hashpjw; ht->nTableSize = nSize; ht->initialized = 1; === modified file 'include/my_sys.h' --- include/my_sys.h 2012-12-23 21:37:11 +0000 +++ include/my_sys.h 2013-01-15 21:00:33 +0000 @@ -86,6 +86,8 @@ typedef struct my_aio_result { #define MY_SYNC 4096 /* my_copy(): sync dst file */ #define MY_SYNC_DIR 32768 /* my_create/delete/rename: sync directory */ #define MY_SYNC_FILESIZE 65536 /* my_sync(): safe sync when file is extended */ +#define MY_THREAD_SPECIFIC 0x10000 /* my_malloc(): thread specific */ +#define MY_THREAD_MOVE 0x20000 /* realloc(); Memory can move */
may be, better to remove this, as it's unused? it's easier to add it later (just a couple of lines of code), than to keep the dead code around.
#define MY_CHECK_ERROR 1 /* Params to my_end; Check open-close */ #define MY_GIVE_INFO 2 /* Give time info about process*/ @@ -148,6 +150,42 @@ typedef struct my_aio_result { /* Extra length needed for filename if one calls my_create_backup_name */ #define MY_BACKUP_NAME_EXTRA_LENGTH 17
+/* If we have our own safemalloc (for debugging) */ +#if defined(SAFEMALLOC) +#define MALLOC_SIZE_AND_FLAG(p,b) sf_malloc_usable_size(p,b) +#define MALLOC_PREFIX_SIZE 0 +#define MALLOC_STORE_SIZE(a,b,c,d) +#define MALLOC_FIX_POINTER_FOR_FREE(a) a +#define SAFEMALLOC_REPORT_MEMORY(X) sf_report_leaked_memory(X) +void sf_report_leaked_memory(my_thread_id id); +extern my_thread_id (*sf_malloc_dbug_id)(void); +#else +/* + * We use double as prefix size as this guarantees the correct + * alignment on all platforms and will optimize things for + * memcpy(), memcmp() etc. + */ +#define MALLOC_PREFIX_SIZE (sizeof(double)) +#define MALLOC_SIZE(p) (*(size_t*) ((char*)(p) - MALLOC_PREFIX_SIZE)) +#define MALLOC_STORE_SIZE(p, type_of_p, size, flag) \ +{\ + *(size_t*) p= (size) | (flag); \ + (p)= (type_of_p) (((char*) (p)) + MALLOC_PREFIX_SIZE); \ +} +static inline size_t malloc_size_and_flag(void *p, myf *flags) +{ + size_t size= MALLOC_SIZE(p); + *flags= (size & 1); + return size & ~ (ulonglong) 1; +} +#define MALLOC_SIZE_AND_FLAG(p,b) malloc_size_and_flag(p, b); +#define MALLOC_FIX_POINTER_FOR_FREE(p) (((char*) (p)) - MALLOC_PREFIX_SIZE) +#define SAFEMALLOC_REPORT_MEMORY(X) do {} while(0) +#endif /* SAFEMALLOC */ + +typedef void (*MALLOC_SIZE_CB) (long long size, myf my_flags); +extern void set_malloc_size_cb(MALLOC_SIZE_CB func);
I don't think you need many of these defines here. my_sys.h is the interface header. MALLOC_STORE_SIZE and MALLOC_FIX_POINTER_FOR_FREE (for example, I didn't check the others) are only used in my_malloc.c - they should not be used elsewhere, and thus, I think, they should be defined in my_malloc.c And, frankly speaking, I wouldn't bother doing two different ways of storing the size and the flags. my_malloc can always allocate one sizeof(double) more and store its data there. Who cares that in the debug builds safemalloc duplicates that...
/* defines when allocating data */ extern void *my_malloc(size_t Size,myf MyFlags); extern void *my_multi_malloc(myf MyFlags, ...); @@ -323,6 +361,7 @@ typedef struct st_dynamic_array uint elements,max_element; uint alloc_increment; uint size_of_element; + myf malloc_flags;
hm. you've preserved the MEM_ROOT structure for compatibiity reasons, but you've changed DYNAMIC_ARRAY. Why? I'd expect you either to change everything or to change nothing. Besides, what's the point of preserving MEM_ROOT, if you change prototypes of functions that work with it?
} DYNAMIC_ARRAY;
typedef struct st_my_tmpdir @@ -768,16 +807,13 @@ extern my_bool real_open_cached_file(IO_ extern void close_cached_file(IO_CACHE *cache); File create_temp_file(char *to, const char *dir, const char *pfx, int mode, myf MyFlags); -#define my_init_dynamic_array(A,B,C,D) init_dynamic_array2(A,B,NULL,C,D) -#define my_init_dynamic_array_ci(A,B,C,D) init_dynamic_array2(A,B,NULL,C,D) -#define my_init_dynamic_array2(A,B,C,D,E) init_dynamic_array2(A,B,C,D,E) -#define my_init_dynamic_array2_ci(A,B,C,D,E) init_dynamic_array2(A,B,C,D,E) +#define my_init_dynamic_array(A,B,C,D,E) init_dynamic_array2(A,B,NULL,C,D,E) +#define my_init_dynamic_array_ci(A,B,C,D,E) init_dynamic_array2(A,B,NULL,C,D,E) +#define my_init_dynamic_array2(A,B,C,D,E,F) init_dynamic_array2(A,B,C,D,E,F) +#define my_init_dynamic_array2_ci(A,B,C,D,E,F) init_dynamic_array2(A,B,C,D,E,F)
as you're changing function prototypes anyway, you can remove the _ci functions and all other "preserve the ABI" garbage,
extern my_bool init_dynamic_array2(DYNAMIC_ARRAY *array, uint element_size, void *init_buffer, uint init_alloc, - uint alloc_increment); -/* init_dynamic_array() function is deprecated */ -extern my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size, - uint init_alloc, uint alloc_increment); + uint alloc_increment, myf my_flags); extern my_bool insert_dynamic(DYNAMIC_ARRAY *array, const uchar * element); extern uchar *alloc_dynamic(DYNAMIC_ARRAY *array); extern uchar *pop_dynamic(DYNAMIC_ARRAY*); @@ -829,7 +865,8 @@ extern void my_free_lock(void *ptr); #define ALLOC_ROOT_MIN_BLOCK_SIZE (MALLOC_OVERHEAD + sizeof(USED_MEM) + 8) #define clear_alloc_root(A) do { (A)->free= (A)->used= (A)->pre_alloc= 0; (A)->min_malloc=0;} while(0) extern void init_alloc_root(MEM_ROOT *mem_root, size_t block_size, - size_t pre_alloc_size); + size_t pre_alloc_size, myf my_flags); +extern void alloc_root_set_min_malloc(MEM_ROOT *mem_root, size_t min_malloc);
if you'd store the flag in the lowest bit of the block_size or if you wouldn't try to keep the old MEM_ROOT structure, this function wouldn't be needed
extern void *alloc_root(MEM_ROOT *mem_root, size_t Size); extern void *multi_alloc_root(MEM_ROOT *mem_root, ...); extern void free_root(MEM_ROOT *root, myf MyFLAGS); === modified file 'include/my_tree.h' --- include/my_tree.h 2011-11-03 18:17:05 +0000 +++ include/my_tree.h 2013-01-15 21:00:33 +0000 @@ -68,13 +68,15 @@ typedef struct st_tree { MEM_ROOT mem_root; my_bool with_delete; tree_element_free free; + myf malloc_flags; uint flag; } TREE;
/* Functions on whole tree */ void init_tree(TREE *tree, size_t default_alloc_size, size_t memory_limit, int size, qsort_cmp2 compare, my_bool with_delete, - tree_element_free free_element, void *custom_arg); + tree_element_free free_element, void *custom_arg, + myf my_flags_for_malloc);
I'd just call it my_flags. it doesn't matter what tree is doing with them. and there may be other flags too. E.g. you could've removed 'with_delete' and added MY_TREE_WITH_DELETE
void delete_tree(TREE*); void reset_tree(TREE*);
=== modified file 'include/mysql.h' --- include/mysql.h 2012-11-20 14:24:39 +0000 +++ include/mysql.h 2013-01-15 21:00:33 +0000 @@ -167,7 +167,7 @@ enum mysql_option MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH, MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, MYSQL_PLUGIN_DIR, MYSQL_DEFAULT_AUTH, - MYSQL_PROGRESS_CALLBACK, + MYSQL_PROGRESS_CALLBACK, MYSQL_THREAD_SPECIFIC_MALLOC,
I don't think you need this on the client side. (the only use case is federated, and I don't think you need to extend the client API specifically for it, federated can set whatever it needs directliy).
/* MariaDB options */ MYSQL_OPT_NONBLOCK=6000 }; === modified file 'mysys/array.c' --- mysys/array.c 2012-01-13 14:50:02 +0000 +++ mysys/array.c 2013-01-15 21:00:33 +0000 @@ -62,17 +64,19 @@ my_bool init_dynamic_array2(DYNAMIC_ARRA should not throw an error */ if (init_alloc && - !(array->buffer= (uchar*) my_malloc(element_size*init_alloc, MYF(0)))) + !(array->buffer= (uchar*) my_malloc(element_size*init_alloc, + MYF(my_flags)))) array->max_element=0; DBUG_RETURN(FALSE); }
my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size, - uint init_alloc, uint alloc_increment) + uint init_alloc, uint alloc_increment, + myf my_flags) { /* placeholder to preserve ABI */ return my_init_dynamic_array_ci(array, element_size, init_alloc,
You can remove this, as you're changing the ABI anyway
- alloc_increment); + alloc_increment, my_flags); } /* Insert element at the end of array. Allocate memory if needed. === modified file 'mysys/my_alloc.c' --- mysys/my_alloc.c 2012-01-13 14:50:02 +0000 +++ mysys/my_alloc.c 2013-01-15 21:00:33 +0000 @@ -41,16 +48,20 @@ Altough error can happen during execution of this function if pre_alloc_size is non-0 it won't be reported. Instead it will be reported as error in first alloc_root() on this memory root. + + We don't want to change the api for MEMROOT. + Becasue of this, we store in min_malloc of my_thread_specific is set. */
void init_alloc_root(MEM_ROOT *mem_root, size_t block_size, - size_t pre_alloc_size __attribute__((unused))) + size_t pre_alloc_size __attribute__((unused)), + myf my_flags) { DBUG_ENTER("init_alloc_root"); DBUG_PRINT("enter",("root: 0x%lx", (long) mem_root));
mem_root->free= mem_root->used= mem_root->pre_alloc= 0; - mem_root->min_malloc= 32; + mem_root->min_malloc= 32 | (test(my_flags & MY_THREAD_SPECIFIC) << 16);
it'd be safer to put it in the lowest bit. min_malloc or - particularly - block size, are always even numbers, the lowest bit is always 0. or to add a new field to the MEM_ROOT, as compatibility is already broken anyway.
mem_root->block_size= block_size - ALLOC_ROOT_MIN_BLOCK_SIZE; mem_root->error_handler= 0; mem_root->block_num= 4; /* We shift this with >>2 */ === modified file 'mysys/my_malloc.c' --- mysys/my_malloc.c 2012-01-13 14:50:02 +0000 +++ mysys/my_malloc.c 2013-01-15 21:00:33 +0000 @@ -18,6 +18,31 @@ #include "mysys_err.h" #include <m_string.h>
+MALLOC_SIZE_CB malloc_size_cb_func= NULL; + +/** + Inform appliction that memory usage has changed
application
+ + @param size Size of memory segment allocated or freed + @param flag 1 if thread specific (allocated by MY_THREAD_SPECIFIC), + 0 if system specific. + + The type os size is long long, to be able to handle negative numbers to + decrement the memory usage +*/ + +static void update_malloc_size(long long size, myf my_flags) +{ + if (malloc_size_cb_func) + malloc_size_cb_func(size, my_flags); +} + +void set_malloc_size_cb(MALLOC_SIZE_CB func) +{ + malloc_size_cb_func= func; +} + + /** Allocate a sized block of memory.
=== modified file 'mysys/mysys_priv.h' --- mysys/mysys_priv.h 2011-12-12 21:58:24 +0000 +++ mysys/mysys_priv.h 2013-01-15 21:00:33 +0000 @@ -70,12 +70,13 @@ extern PSI_file_key key_file_charset, ke #endif /* HAVE_PSI_INTERFACE */
#ifdef SAFEMALLOC -void *sf_malloc(size_t size); -void *sf_realloc(void *ptr, size_t size); +void *sf_malloc(size_t size, myf my_flags); +void *sf_realloc(void *ptr, size_t size, myf my_flags); void sf_free(void *ptr); +size_t sf_malloc_usable_size(void *ptr, myf *my_flags);
this is used nowhere. dead code again.
#else -#define sf_malloc(X) malloc(X) -#define sf_realloc(X,Y) realloc(X,Y) +#define sf_malloc(X,Y) malloc(X) +#define sf_realloc(X,Y,Z) realloc(X,Y) #define sf_free(X) free(X) #endif
=== modified file 'mysys/safemalloc.c' --- mysys/safemalloc.c 2012-04-18 01:29:26 +0000 +++ mysys/safemalloc.c 2013-01-15 21:00:33 +0000 @@ -79,11 +81,21 @@ static int bad_ptr(const char *where, vo static void free_memory(void *ptr); static void sf_terminate();
+/* Setup default call to get a thread id for the memory */ + +my_thread_id default_sf_malloc_dbug_id(void) +{ + return my_thread_dbug_id(); +} + +my_thread_id (*sf_malloc_dbug_id)(void)= default_sf_malloc_dbug_id;
Why do you need different functions for getting the thread id?
+ + /** allocates memory */
-void *sf_malloc(size_t size) +void *sf_malloc(size_t size, myf my_flags) { struct st_irem *irem; uchar *data; === modified file 'sql/event_scheduler.cc' --- sql/event_scheduler.cc 2012-09-27 23:06:56 +0000 +++ sql/event_scheduler.cc 2013-01-15 21:00:33 +0000 @@ -182,12 +180,15 @@ deinit_event_thread(THD *thd) void pre_init_event_thread(THD* thd) { + THD *orig_thd= current_thd; DBUG_ENTER("pre_init_event_thread"); + + my_pthread_setspecific_ptr(THR_THD, thd);
I'd rather see set_current_thd(thd) here: #define set_current_thd(X) my_pthread_setspecific_ptr(THR_THD, (X))
thd->client_capabilities= 0; thd->security_ctx->master_access= 0; thd->security_ctx->db_access= 0; thd->security_ctx->host_or_ip= (char*)my_localhost; - my_net_init(&thd->net, NULL); + my_net_init(&thd->net, NULL, MY_THREAD_SPECIFIC); thd->security_ctx->set_user((char*)"event_scheduler"); thd->net.read_timeout= slave_net_timeout; thd->variables.option_bits|= OPTION_AUTO_IS_NULL; === modified file 'sql/handler.cc' --- sql/handler.cc 2012-12-17 00:49:19 +0000 +++ sql/handler.cc 2013-01-15 21:00:33 +0000 @@ -5395,6 +5395,7 @@ fl_log_iterator_buffer_init(struct handl iterator->buffer= buff; iterator->next= &fl_log_iterator_next; iterator->destroy= &fl_log_iterator_destroy; + my_dir_end(dirp);
Why did we never see memory leaks because of that?
return HA_ITERATOR_OK; }
=== modified file 'sql/mysqld.cc' --- sql/mysqld.cc 2013-01-09 03:34:33 +0000 +++ sql/mysqld.cc 2013-01-15 21:00:33 +0000 @@ -2606,9 +2607,10 @@ bool one_thread_per_connection_end(THD *
/* It's safe to broadcast outside a lock (COND... is not deleted here) */ DBUG_PRINT("signal", ("Broadcasting COND_thread_count")); + mysql_cond_broadcast(&COND_thread_count); + DBUG_LEAVE; // Must match DBUG_ENTER() my_thread_end(); - mysql_cond_broadcast(&COND_thread_count);
why?
pthread_exit(0); return 0; // Avoid compiler warnings @@ -7024,6 +7096,7 @@ SHOW_VAR status_vars[]= { {"Key", (char*) &show_default_keycache, SHOW_FUNC}, {"Last_query_cost", (char*) offsetof(STATUS_VAR, last_query_cost), SHOW_DOUBLE_STATUS}, {"Max_used_connections", (char*) &max_used_connections, SHOW_LONG}, + {"Memory_used", (char*) &total_memory_used, SHOW_LONGLONG},
May be this should rather be: SHOW SESSION STATUS -> thd->memory_used SHOW GLOBAL STATUS -> total_memory_used (which means that total_memory_used/memory_used should be in the status_var struct)
{"Not_flushed_delayed_rows", (char*) &delayed_rows_in_use, SHOW_LONG_NOFLUSH}, {"Open_files", (char*) &my_file_opened, SHOW_LONG_NOFLUSH}, {"Open_streams", (char*) &my_stream_opened, SHOW_LONG_NOFLUSH}, === modified file 'sql/net_serv.cc' --- sql/net_serv.cc 2012-02-28 17:53:05 +0000 +++ sql/net_serv.cc 2013-01-15 21:00:33 +0000 @@ -139,6 +140,7 @@ my_bool my_net_init(NET *net, Vio* vio) net->net_skip_rest_factor= 0; net->last_errno=0; net->unused= 0; + net->thread_specific_malloc= test(my_flags & MY_THREAD_SPECIFIC);
why do you need that in NET ?
if (vio != 0) /* If real connection */ { === modified file 'sql/sql_analyse.h' --- sql/sql_analyse.h 2011-11-03 18:17:05 +0000 +++ sql/sql_analyse.h 2013-01-15 21:00:33 +0000 @@ -121,7 +121,8 @@ class field_str :public field_info must_be_blob(0), was_zero_fill(0), was_maybe_zerofill(0), can_be_still_num(1) { init_tree(&tree, 0, 0, sizeof(String), (qsort_cmp2) sortcmp2, - 0, (tree_element_free) free_string, NULL); }; + 0, (tree_element_free) free_string, NULL, + MY_THREAD_SPECIFIC); };
Now, when I'm thinking of it, I got the idea that it's better to have MY_THREAD_SPECIFIC as a default, and add MY_GLOBAL_ALLOC instead to mark global allocations explicitly. It's more future-proof. When one copies init_dynamic_array() from somewhere and zeros out unknown or unneeded parameters, the memory will be per-thread by default. And if that's wrong, the test suite will find it, you have an assert for this. To mark memory global as an explicit action, one needs to think about it. Currently, the memory is global by default, and if one doesn't think,# copy-pastes, and leaves new allocation global, it will not be noticed, the memory will simply be unaccounted for.
void add(); void get_opt_type(String*, ha_rows); === modified file 'sql/sql_error.cc' --- sql/sql_error.cc 2012-01-13 14:50:02 +0000 +++ sql/sql_error.cc 2013-01-15 21:00:33 +0000 @@ -457,25 +457,37 @@ Diagnostics_area::disable_status() m_status= DA_DISABLED; }
-Warning_info::Warning_info(ulonglong warn_id_arg, bool allow_unlimited_warnings) +Warning_info::Warning_info(ulonglong warn_id_arg, + bool allow_unlimited_warnings, bool initialize)
I'd rather add a special constructor for Warning_info that THD would use. Because there's only one place where you need initialize=false, and all other "normal" uses, should not suffer (be changed) because of that.
:m_statement_warn_count(0), m_current_row_for_warning(1), m_warn_id(warn_id_arg), m_allow_unlimited_warnings(allow_unlimited_warnings), m_read_only(FALSE) { - /* Initialize sub structures */ - init_sql_alloc(&m_warn_root, WARN_ALLOC_BLOCK_SIZE, WARN_ALLOC_PREALLOC_SIZE); m_warn_list.empty(); bzero((char*) m_warn_count, sizeof(m_warn_count)); + if (initialize) + init(); }
Regards, Sergei
Hi!
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
Sergei> Hi. Sergei> Here it is:
=== modified file 'client/completion_hash.cc' --- client/completion_hash.cc 2011-06-30 15:46:53 +0000 +++ client/completion_hash.cc 2013-01-15 21:00:33 +0000 @@ -49,7 +49,7 @@ int completion_hash_init(HashTable *ht, ht-> initialized = 0; return FAILURE; } - init_alloc_root(&ht->mem_root, 8192, 0); + init_alloc_root(&ht->mem_root, 8192, 0, 0);
Sergei> very often you write 0 or MY_THREAD_SPECIFIC instead of Sergei> MYF(0) and MYF(MY_THREAD_SPECIFIC) Yes. MYF() was important in C, but not that critical anymore in C++ I can change all MY_THREAD_SPECIFIC to MYF(MY_THREAD_SPECIFIC) if you want.
=== modified file 'include/my_sys.h' --- include/my_sys.h 2012-12-23 21:37:11 +0000 +++ include/my_sys.h 2013-01-15 21:00:33 +0000 @@ -86,6 +86,8 @@ typedef struct my_aio_result { #define MY_SYNC 4096 /* my_copy(): sync dst file */ #define MY_SYNC_DIR 32768 /* my_create/delete/rename: sync directory */ #define MY_SYNC_FILESIZE 65536 /* my_sync(): safe sync when file is extended */ +#define MY_THREAD_SPECIFIC 0x10000 /* my_malloc(): thread specific */ +#define MY_THREAD_MOVE 0x20000 /* realloc(); Memory can move */
Sergei> may be, better to remove this, as it's unused? Sergei> it's easier to add it later (just a couple of lines of code), Sergei> than to keep the dead code around. It's hard for someone else to know the removed lines of code if they ever need to allocate memory for another thread. I rather keep that there for now.
#define MY_CHECK_ERROR 1 /* Params to my_end; Check open-close */ #define MY_GIVE_INFO 2 /* Give time info about process*/ @@ -148,6 +150,42 @@ typedef struct my_aio_result { /* Extra length needed for filename if one calls my_create_backup_name */ #define MY_BACKUP_NAME_EXTRA_LENGTH 17
+/* If we have our own safemalloc (for debugging) */ +#if defined(SAFEMALLOC) +#define MALLOC_SIZE_AND_FLAG(p,b) sf_malloc_usable_size(p,b) +#define MALLOC_PREFIX_SIZE 0 +#define MALLOC_STORE_SIZE(a,b,c,d) +#define MALLOC_FIX_POINTER_FOR_FREE(a) a +#define SAFEMALLOC_REPORT_MEMORY(X) sf_report_leaked_memory(X) +void sf_report_leaked_memory(my_thread_id id); +extern my_thread_id (*sf_malloc_dbug_id)(void); +#else +/* + * We use double as prefix size as this guarantees the correct + * alignment on all platforms and will optimize things for + * memcpy(), memcmp() etc. + */ +#define MALLOC_PREFIX_SIZE (sizeof(double)) +#define MALLOC_SIZE(p) (*(size_t*) ((char*)(p) - MALLOC_PREFIX_SIZE)) +#define MALLOC_STORE_SIZE(p, type_of_p, size, flag) \ +{\ + *(size_t*) p= (size) | (flag); \ + (p)= (type_of_p) (((char*) (p)) + MALLOC_PREFIX_SIZE); \ +} +static inline size_t malloc_size_and_flag(void *p, myf *flags) +{ + size_t size= MALLOC_SIZE(p); + *flags= (size & 1); + return size & ~ (ulonglong) 1; +} +#define MALLOC_SIZE_AND_FLAG(p,b) malloc_size_and_flag(p, b); +#define MALLOC_FIX_POINTER_FOR_FREE(p) (((char*) (p)) - MALLOC_PREFIX_SIZE) +#define SAFEMALLOC_REPORT_MEMORY(X) do {} while(0) +#endif /* SAFEMALLOC */ + +typedef void (*MALLOC_SIZE_CB) (long long size, myf my_flags); +extern void set_malloc_size_cb(MALLOC_SIZE_CB func);
Sergei> I don't think you need many of these defines here. my_sys.h is the interface Sergei> header. MALLOC_STORE_SIZE and MALLOC_FIX_POINTER_FOR_FREE (for example, I Sergei> didn't check the others) are only used in my_malloc.c - they should not be Sergei> used elsewhere, and thus, I think, they should be defined in my_malloc.c Originally here we had definitions for usage of the different malloc function and to get size for the memory blocks. I agree that most of the above can now be moved to my_malloc.c. I will do that. Sergei> And, frankly speaking, I wouldn't bother doing two different ways of Sergei> storing the size and the flags. my_malloc can always allocate one Sergei> sizeof(double) more and store its data there. Who cares that in the debug Sergei> builds safemalloc duplicates that... I prefer to do it right. As this is just a small set of defines, it's not hard to keep both around.
/* defines when allocating data */ extern void *my_malloc(size_t Size,myf MyFlags); extern void *my_multi_malloc(myf MyFlags, ...); @@ -323,6 +361,7 @@ typedef struct st_dynamic_array uint elements,max_element; uint alloc_increment; uint size_of_element; + myf malloc_flags;
Sergei> hm. you've preserved the MEM_ROOT structure for compatibiity reasons, Sergei> but you've changed DYNAMIC_ARRAY. Why? Sergei> I'd expect you either to change everything or to change nothing. MEMROOT is used the public client library, so I didn't want to change it. I spent a lot of time encoding the memory in other variables just to not break compatibility with 5.5 or 5.6 dynamic_array is not used in the structs, so there I did things correctly. Sergei> Besides, what's the point of preserving MEM_ROOT, if you Sergei> change prototypes of functions that work with it? Becasue a MySQL client doesn't directly access MEMROOT; They do hower allocate the MYSQL struct on stack that includes MEMROOT.
} DYNAMIC_ARRAY;
typedef struct st_my_tmpdir @@ -768,16 +807,13 @@ extern my_bool real_open_cached_file(IO_ extern void close_cached_file(IO_CACHE *cache); File create_temp_file(char *to, const char *dir, const char *pfx, int mode, myf MyFlags); -#define my_init_dynamic_array(A,B,C,D) init_dynamic_array2(A,B,NULL,C,D) -#define my_init_dynamic_array_ci(A,B,C,D) init_dynamic_array2(A,B,NULL,C,D) -#define my_init_dynamic_array2(A,B,C,D,E) init_dynamic_array2(A,B,C,D,E) -#define my_init_dynamic_array2_ci(A,B,C,D,E) init_dynamic_array2(A,B,C,D,E) +#define my_init_dynamic_array(A,B,C,D,E) init_dynamic_array2(A,B,NULL,C,D,E) +#define my_init_dynamic_array_ci(A,B,C,D,E) init_dynamic_array2(A,B,NULL,C,D,E) +#define my_init_dynamic_array2(A,B,C,D,E,F) init_dynamic_array2(A,B,C,D,E,F) +#define my_init_dynamic_array2_ci(A,B,C,D,E,F) init_dynamic_array2(A,B,C,D,E,F)
Sergei> as you're changing function prototypes anyway, Sergei> you can remove the _ci functions and all other "preserve the ABI" garbage, Will do.
extern my_bool init_dynamic_array2(DYNAMIC_ARRAY *array, uint element_size, void *init_buffer, uint init_alloc, - uint alloc_increment); -/* init_dynamic_array() function is deprecated */ -extern my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size, - uint init_alloc, uint alloc_increment); + uint alloc_increment, myf my_flags); extern my_bool insert_dynamic(DYNAMIC_ARRAY *array, const uchar * element); extern uchar *alloc_dynamic(DYNAMIC_ARRAY *array); extern uchar *pop_dynamic(DYNAMIC_ARRAY*); @@ -829,7 +865,8 @@ extern void my_free_lock(void *ptr); #define ALLOC_ROOT_MIN_BLOCK_SIZE (MALLOC_OVERHEAD + sizeof(USED_MEM) + 8) #define clear_alloc_root(A) do { (A)->free= (A)->used= (A)->pre_alloc= 0; (A)->min_malloc=0;} while(0) extern void init_alloc_root(MEM_ROOT *mem_root, size_t block_size, - size_t pre_alloc_size); + size_t pre_alloc_size, myf my_flags); +extern void alloc_root_set_min_malloc(MEM_ROOT *mem_root, size_t min_malloc);
Sergei> if you'd store the flag in the lowest bit of the block_size Sergei> or if you wouldn't try to keep the old MEM_ROOT structure, Sergei> this function wouldn't be needed I would still need a function to change the block size, but I agree that this is not likely to happen. I will change that.
extern void *alloc_root(MEM_ROOT *mem_root, size_t Size); extern void *multi_alloc_root(MEM_ROOT *mem_root, ...); extern void free_root(MEM_ROOT *root, myf MyFLAGS); === modified file 'include/my_tree.h' --- include/my_tree.h 2011-11-03 18:17:05 +0000 +++ include/my_tree.h 2013-01-15 21:00:33 +0000 @@ -68,13 +68,15 @@ typedef struct st_tree { MEM_ROOT mem_root; my_bool with_delete; tree_element_free free; + myf malloc_flags; uint flag; } TREE;
/* Functions on whole tree */ void init_tree(TREE *tree, size_t default_alloc_size, size_t memory_limit, int size, qsort_cmp2 compare, my_bool with_delete, - tree_element_free free_element, void *custom_arg); + tree_element_free free_element, void *custom_arg, + myf my_flags_for_malloc);
Sergei> I'd just call it my_flags. it doesn't matter what tree is doing with them. Sergei> and there may be other flags too. E.g. you could've removed 'with_delete' Sergei> and added MY_TREE_WITH_DELETE Will do (both).
void delete_tree(TREE*); void reset_tree(TREE*);
=== modified file 'include/mysql.h' --- include/mysql.h 2012-11-20 14:24:39 +0000 +++ include/mysql.h 2013-01-15 21:00:33 +0000 @@ -167,7 +167,7 @@ enum mysql_option MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH, MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, MYSQL_PLUGIN_DIR, MYSQL_DEFAULT_AUTH, - MYSQL_PROGRESS_CALLBACK, + MYSQL_PROGRESS_CALLBACK, MYSQL_THREAD_SPECIFIC_MALLOC,
Sergei> I don't think you need this on the client side. Sergei> (the only use case is federated, and I don't think you need to extend Sergei> the client API specifically for it, federated can set whatever it needs Sergei> directliy). I think it's better to use a clear interface for that. CONNECTDB will need to also use this.
/* MariaDB options */ MYSQL_OPT_NONBLOCK=6000 }; === modified file 'mysys/array.c' --- mysys/array.c 2012-01-13 14:50:02 +0000 +++ mysys/array.c 2013-01-15 21:00:33 +0000 @@ -62,17 +64,19 @@ my_bool init_dynamic_array2(DYNAMIC_ARRA should not throw an error */ if (init_alloc && - !(array->buffer= (uchar*) my_malloc(element_size*init_alloc, MYF(0)))) + !(array->buffer= (uchar*) my_malloc(element_size*init_alloc, + MYF(my_flags)))) array-> max_element=0; DBUG_RETURN(FALSE); }
my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size, - uint init_alloc, uint alloc_increment) + uint init_alloc, uint alloc_increment, + myf my_flags) { /* placeholder to preserve ABI */ return my_init_dynamic_array_ci(array, element_size, init_alloc,
Sergei> You can remove this, as you're changing the ABI anyway Will do.
- alloc_increment); + alloc_increment, my_flags); } /* Insert element at the end of array. Allocate memory if needed. === modified file 'mysys/my_alloc.c' --- mysys/my_alloc.c 2012-01-13 14:50:02 +0000 +++ mysys/my_alloc.c 2013-01-15 21:00:33 +0000 @@ -41,16 +48,20 @@ Altough error can happen during execution of this function if pre_alloc_size is non-0 it won't be reported. Instead it will be reported as error in first alloc_root() on this memory root. + + We don't want to change the api for MEMROOT. + Becasue of this, we store in min_malloc of my_thread_specific is set. */
void init_alloc_root(MEM_ROOT *mem_root, size_t block_size, - size_t pre_alloc_size __attribute__((unused))) + size_t pre_alloc_size __attribute__((unused)), + myf my_flags) { DBUG_ENTER("init_alloc_root"); DBUG_PRINT("enter",("root: 0x%lx", (long) mem_root));
mem_root-> free= mem_root->used= mem_root->pre_alloc= 0;
- mem_root->min_malloc= 32; + mem_root->min_malloc= 32 | (test(my_flags & MY_THREAD_SPECIFIC) << 16);
Sergei> it'd be safer to put it in the lowest bit. min_malloc or - particularly - Sergei> block size, are always even numbers, the lowest bit is always 0. Will put in block size, as agreed earlier. Sergei> or to add a new field to the MEM_ROOT, as compatibility is already Sergei> broken anyway. Can't add things to MEM_ROOT as it would break client struct compatiblity. mem_root-> block_size= block_size - ALLOC_ROOT_MIN_BLOCK_SIZE; mem_root-> error_handler= 0; mem_root-> block_num= 4; /* We shift this with >>2 */
=== modified file 'mysys/my_malloc.c' --- mysys/my_malloc.c 2012-01-13 14:50:02 +0000 +++ mysys/my_malloc.c 2013-01-15 21:00:33 +0000 @@ -18,6 +18,31 @@ #include "mysys_err.h" #include <m_string.h>
+MALLOC_SIZE_CB malloc_size_cb_func= NULL; + +/** + Inform appliction that memory usage has changed
Sergei> application ok.
=== modified file 'mysys/mysys_priv.h' --- mysys/mysys_priv.h 2011-12-12 21:58:24 +0000 +++ mysys/mysys_priv.h 2013-01-15 21:00:33 +0000 @@ -70,12 +70,13 @@ extern PSI_file_key key_file_charset, ke #endif /* HAVE_PSI_INTERFACE */
#ifdef SAFEMALLOC -void *sf_malloc(size_t size); -void *sf_realloc(void *ptr, size_t size); +void *sf_malloc(size_t size, myf my_flags); +void *sf_realloc(void *ptr, size_t size, myf my_flags); void sf_free(void *ptr); +size_t sf_malloc_usable_size(void *ptr, myf *my_flags);
Sergei> this is used nowhere. dead code again. sf_malloc_usable_size() is used in my_malloc.c trough the MALLOC_SIZE_AND_FLAG macro. <cut>
=== modified file 'mysys/safemalloc.c' --- mysys/safemalloc.c 2012-04-18 01:29:26 +0000 +++ mysys/safemalloc.c 2013-01-15 21:00:33 +0000 @@ -79,11 +81,21 @@ static int bad_ptr(const char *where, vo static void free_memory(void *ptr); static void sf_terminate();
+/* Setup default call to get a thread id for the memory */ + +my_thread_id default_sf_malloc_dbug_id(void) +{ + return my_thread_dbug_id(); +} + +my_thread_id (*sf_malloc_dbug_id)(void)= default_sf_malloc_dbug_id;
Sergei> Why do you need different functions for getting the thread id? Because in mysqld we want to count memory per THD, not per thread. One thread can allocate the THD and another one can use it. <cut>
=== modified file 'sql/event_scheduler.cc' --- sql/event_scheduler.cc 2012-09-27 23:06:56 +0000 +++ sql/event_scheduler.cc 2013-01-15 21:00:33 +0000 @@ -182,12 +180,15 @@ deinit_event_thread(THD *thd) void pre_init_event_thread(THD* thd) { + THD *orig_thd= current_thd; DBUG_ENTER("pre_init_event_thread"); + + my_pthread_setspecific_ptr(THR_THD, thd);
Sergei> I'd rather see set_current_thd(thd) here: Sergei> #define set_current_thd(X) my_pthread_setspecific_ptr(THR_THD, (X)) I can add that. <cut>
=== modified file 'sql/handler.cc' --- sql/handler.cc 2012-12-17 00:49:19 +0000 +++ sql/handler.cc 2013-01-15 21:00:33 +0000 @@ -5395,6 +5395,7 @@ fl_log_iterator_buffer_init(struct handl iterator-> buffer= buff; iterator-> next= &fl_log_iterator_next; iterator-> destroy= &fl_log_iterator_destroy; + my_dir_end(dirp);
Sergei> Why did we never see memory leaks because of that? This is test code that you only get when you compile with TRANS_LOG_MGM_EXAMPLE_CODE.
return HA_ITERATOR_OK; }
=== modified file 'sql/mysqld.cc' --- sql/mysqld.cc 2013-01-09 03:34:33 +0000 +++ sql/mysqld.cc 2013-01-15 21:00:33 +0000 @@ -2606,9 +2607,10 @@ bool one_thread_per_connection_end(THD *
/* It's safe to broadcast outside a lock (COND... is not deleted here) */ DBUG_PRINT("signal", ("Broadcasting COND_thread_count")); + mysql_cond_broadcast(&COND_thread_count); + DBUG_LEAVE; // Must match DBUG_ENTER() my_thread_end(); - mysql_cond_broadcast(&COND_thread_count);
Sergei> why? You can't all mysql_cond_broadcast(&COND_thread_count) after my_thread_end(). This causes problems with safemutex that needs mysys_var to work. my_thread_end() is the last function that a thread should call.
pthread_exit(0); return 0; // Avoid compiler warnings @@ -7024,6 +7096,7 @@ SHOW_VAR status_vars[]= { {"Key", (char*) &show_default_keycache, SHOW_FUNC}, {"Last_query_cost", (char*) offsetof(STATUS_VAR, last_query_cost), SHOW_DOUBLE_STATUS}, {"Max_used_connections", (char*) &max_used_connections, SHOW_LONG}, + {"Memory_used", (char*) &total_memory_used, SHOW_LONGLONG},
Sergei> May be this should rather be: Sergei> SHOW SESSION STATUS -> thd->memory_used Sergei> SHOW GLOBAL STATUS -> total_memory_used Sergei> (which means that total_memory_used/memory_used should be in the status_var struct) That's a good idea. I can easily fix that.
{"Not_flushed_delayed_rows", (char*) &delayed_rows_in_use, SHOW_LONG_NOFLUSH}, {"Open_files", (char*) &my_file_opened, SHOW_LONG_NOFLUSH}, {"Open_streams", (char*) &my_stream_opened, SHOW_LONG_NOFLUSH}, === modified file 'sql/net_serv.cc' --- sql/net_serv.cc 2012-02-28 17:53:05 +0000 +++ sql/net_serv.cc 2013-01-15 21:00:33 +0000 @@ -139,6 +140,7 @@ my_bool my_net_init(NET *net, Vio* vio) net-> net_skip_rest_factor= 0; net-> last_errno=0; net-> unused= 0; + net->thread_specific_malloc= test(my_flags & MY_THREAD_SPECIFIC);
Sergei> why do you need that in NET ? As NET is almost always thread specific, except in a few cases. I need to be able to remember the original flag for future mallocs done by NET.
=== modified file 'sql/sql_analyse.h' --- sql/sql_analyse.h 2011-11-03 18:17:05 +0000 +++ sql/sql_analyse.h 2013-01-15 21:00:33 +0000 @@ -121,7 +121,8 @@ class field_str :public field_info must_be_blob(0), was_zero_fill(0), was_maybe_zerofill(0), can_be_still_num(1) { init_tree(&tree, 0, 0, sizeof(String), (qsort_cmp2) sortcmp2, - 0, (tree_element_free) free_string, NULL); }; + 0, (tree_element_free) free_string, NULL, + MY_THREAD_SPECIFIC); };
Sergei> Now, when I'm thinking of it, I got the idea that it's better to have Sergei> MY_THREAD_SPECIFIC as a default, and add MY_GLOBAL_ALLOC instead to mark Sergei> global allocations explicitly. It's more future-proof. This would cause more problems: - There are many more global mallocs than thread specific. - Allocating things globally will never cause a problem; You can allocate that in one thread and free it in another. - Using MY_THREAD_SPECIFIC will lead to crashes if you do things wrongly. In other words, it doesn't matter much if you forget MY_THREAD_SPECIFIC. If it would be the other way around, you would likely get a lot of crashes both in current code an if you do a wrong merge from MySQL and forget to add MY_GLOBAL_ALLOC. Sergei> When one copies Sergei> init_dynamic_array() from somewhere and zeros out unknown or unneeded Sergei> parameters, the memory will be per-thread by default. And if that's Sergei> wrong, the test suite will find it, you have an assert for this. Sergei> To mark memory global as an explicit action, one needs to think about it. It's not clear when memory can be marked as MY_THREAD_SPECIFIC. I spent a LOT of time finding all the safe places. Doing it the other way around would cause people to work much more as they would get crashes they can't explain. Also finding the missing malloc is not trivial as valgrind is of no help here. Sergei> Currently, the memory is global by default, and if one doesn't think,# Sergei> copy-pastes, and leaves new allocation global, it will not be noticed, Sergei> the memory will simply be unaccounted for. It will be in the global pool, not in the thread pool. This is not optimal, but also not fatal. The other way around would cause a lot of grief from developers that doesn't understand why things crashes...
void add(); void get_opt_type(String*, ha_rows); === modified file 'sql/sql_error.cc' --- sql/sql_error.cc 2012-01-13 14:50:02 +0000 +++ sql/sql_error.cc 2013-01-15 21:00:33 +0000 @@ -457,25 +457,37 @@ Diagnostics_area::disable_status() m_status= DA_DISABLED; }
-Warning_info::Warning_info(ulonglong warn_id_arg, bool allow_unlimited_warnings) +Warning_info::Warning_info(ulonglong warn_id_arg, + bool allow_unlimited_warnings, bool initialize)
Sergei> I'd rather add a special constructor for Warning_info Sergei> that THD would use. Because there's only one place where you need Sergei> initialize=false, and all other "normal" uses, should not suffer (be Sergei> changed) because of that. I can do that. thanks for the idea. Regards, Monty
Hi, Michael! On Jan 18, Michael Widenius wrote:
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
=== modified file 'client/completion_hash.cc' --- client/completion_hash.cc 2011-06-30 15:46:53 +0000 +++ client/completion_hash.cc 2013-01-15 21:00:33 +0000 @@ -49,7 +49,7 @@ int completion_hash_init(HashTable *ht, ht-> initialized = 0; return FAILURE; } - init_alloc_root(&ht->mem_root, 8192, 0); + init_alloc_root(&ht->mem_root, 8192, 0, 0);
very often you write 0 or MY_THREAD_SPECIFIC instead of MYF(0) and MYF(MY_THREAD_SPECIFIC)
Yes. MYF() was important in C, but not that critical anymore in C++ I can change all MY_THREAD_SPECIFIC to MYF(MY_THREAD_SPECIFIC) if you want.
Why was it important in C? I never could find a reason why it might be needed because of a language or a specific compiler requirements. I always saw it as a visual help to attribute the last 0 in the function argument list to mysys flags. Like, by convention many functions accept 'flags' argument, and one can easily see that this function takes flags, because the flag is always MYF(0) or MYF(something). This reason is true both for C and C++.
=== modified file 'include/mysql.h' --- include/mysql.h 2012-11-20 14:24:39 +0000 +++ include/mysql.h 2013-01-15 21:00:33 +0000 @@ -167,7 +167,7 @@ enum mysql_option MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH, MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, MYSQL_PLUGIN_DIR, MYSQL_DEFAULT_AUTH, - MYSQL_PROGRESS_CALLBACK, + MYSQL_PROGRESS_CALLBACK, MYSQL_THREAD_SPECIFIC_MALLOC,
I don't think you need this on the client side.
(the only use case is federated, and I don't think you need to extend the client API specifically for it, federated can set whatever it needs directliy).
I think it's better to use a clear interface for that. CONNECTDB will need to also use this.
Yes, but unlike all other client options - which are truly client options - this one is only for "clients linked into the server". Which is a bit special use case, and I'd suggest to have a special interface for that, instead of making it every client aware of it.
=== modified file 'sql/sql_error.cc' --- sql/sql_error.cc 2012-01-13 14:50:02 +0000 +++ sql/sql_error.cc 2013-01-15 21:00:33 +0000 @@ -457,25 +457,37 @@ Diagnostics_area::disable_status() m_status= DA_DISABLED; }
-Warning_info::Warning_info(ulonglong warn_id_arg, bool allow_unlimited_warnings) +Warning_info::Warning_info(ulonglong warn_id_arg, + bool allow_unlimited_warnings, bool initialize)
I'd rather add a special constructor for Warning_info that THD would use. Because there's only one place where you need initialize=false, and all other "normal" uses, should not suffer (be changed) because of that.
I can do that. thanks for the idea.
Later you said on irc:
hm, don't like how much code I have to repeat just to add an creator function for Warning_info::Warning_info(ulonglong warn_id_arg, bool allow_unlimited_warnings, bool initialize) don't really know how to do this nicely. have to leave this for now. Don't want to duplicate a lot of code as this is a risk for errors
Ok. I don't quite understand it, but please push either way. And I'll see if I could do with with little code duplication. Regards, Sergei
participants (2)
-
Michael Widenius -
Sergei Golubchik