maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #05059
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
Follow ups
