Re: b16319bdd0f: MDEV-35469 Heap tables are calling mallocs to often
Hi, Monty,
commit b16319bdd0f Author: Michael Widenius <monty@mariadb.org> AuthorDate: Thu Nov 21 12:28:57 2024 +0200 Commit: Michael Widenius <monty@mariadb.org> CommitDate: Fri Dec 27 13:53:37 2024 +0200
diff --git a/storage/heap/hp_block.c b/storage/heap/hp_block.c index 324efc8b4af..44bca363746 100644 --- a/storage/heap/hp_block.c +++ b/storage/heap/hp_block.c @@ -75,9 +75,11 @@ int hp_get_new_block(HP_SHARE *info, HP_BLOCK *block, size_t *alloc_length) Next time we allocate data for X rows. When level 1 is full, we allocate data for HP_PTRS_IN_NOD at level 2 and 1 + X rows at level 0. */ *alloc_length= (sizeof(HP_PTRS) * ((i == block->levels) ? i : i - 1) + (ulonglong)block->records_in_block * block->recbuffer); + /* Alloc in blocks of powers of 2 */ + *alloc_length= MY_MAX(*alloc_length, block->alloc_size);
What will happen to block->alloc_size - *alloc_length bytes? they'll be unused?
if (!(root=(HP_PTRS*) my_malloc(hp_key_memory_HP_PTRS, *alloc_length, MYF(MY_WME | (info->internal ? diff --git a/storage/heap/hp_create.c b/storage/heap/hp_create.c index c07a1e968c4..29812f08107 100644 --- a/storage/heap/hp_create.c +++ b/storage/heap/hp_create.c @@ -15,11 +15,23 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */
#include "heapdef.h" +#include <my_bit.h>
static int keys_compare(void *heap_rb, const void *key1, const void *key2); static void init_block(HP_BLOCK *block,uint reclength,ulong min_records, ulong max_records);
+ +/* + In how many parts are we going to do allocations of memory and indexes + If we assigne 1M to the heap table memory, we will allocate roughly + (1M/16) bytes per allocaiton +*/ +int heap_allocation_parts= 16;
static. technically, it could be `static const`, But I presume the compiler can figure out the `const` part if the variable is static. And e.g. use >> 4. Otherwise it has to assume it can be modified in some other file.
+ +/* min block allocation */ +ulong heap_min_allocation_block= 16384;
same. static.
+ /* Create a heap table */
int heap_create(const char *name, HP_CREATE_INFO *create_info, @@ -170,7 +182,8 @@ int heap_create(const char *name, HP_CREATE_INFO *create_info, share->keydef= (HP_KEYDEF*) (share + 1); share->key_stat_version= 1; keyseg= (HA_KEYSEG*) (share->keydef + keys); - init_block(&share->block, visible_offset + 1, min_records, max_records); + init_block(&share->block, hp_memory_needed_per_row(visible_offset + 1),
visible_offset is already MY_MAX(reclength, sizeof (char*)). perhaps you don't need visible_offset and should use hp_memory_needed_per_row(reclength) here?
+ min_records, max_records); /* Fix keys */ memcpy(share->keydef, keydef, (size_t) (sizeof(keydef[0]) * keys)); for (i= 0, keyinfo= share->keydef; i < keys; i++, keyinfo++) @@ -266,44 +279,90 @@ static int keys_compare(void *heap_rb_, const void *key1_, heap_rb->search_flag, not_used); }
+ +/* + Calculate length needed for storing one row +*/ + +size_t hp_memory_needed_per_row(size_t reclength) +{ + /* Data needed for storing record + pointer to records */ + reclength= MY_MAX(reclength, sizeof(char*) + 1); + reclength= MY_ALIGN(reclength, sizeof(char*)); + return reclength; +} + +/* + Calculate the number of rows that fits into a given memory size +*/ + +ha_rows hp_rows_in_memory(size_t reclength, size_t index_size, + size_t memory_limit) +{ + /* We need one extra byte for the delete flag */ + reclength= hp_memory_needed_per_row(reclength+1);
you have +1 for the delete flag in hp_memory_needed_per_row()
+ if ((memory_limit - sizeof(HP_PTRS)) < (index_size + reclength)) + return 0; /* Wrong arguments */
Why do you need this if()? If its condition is true the expression below will return 0 naturally, you don't need a separate check for that.
+ return (ha_rows) ((memory_limit - sizeof(HP_PTRS)) / + (index_size + reclength)); +} + + static void init_block(HP_BLOCK *block, uint reclength, ulong min_records, ulong max_records) { diff --git a/storage/heap/hp_write.c b/storage/heap/hp_write.c index 5469784c8c1..cb079eac757 100644 --- a/storage/heap/hp_write.c +++ b/storage/heap/hp_write.c @@ -145,6 +145,8 @@ static uchar *next_free_record_pos(HP_SHARE *info) DBUG_PRINT("exit",("Used old position: %p", pos)); DBUG_RETURN(pos); } + if (!(block_pos=(info->records % info->block.records_in_block))) + {
why?
if ((info->records > info->max_records && info->max_records) || (info->data_length + info->index_length >= info->max_table_size)) {
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi! On Fri, Dec 27, 2024 at 8:12 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Monty,
commit b16319bdd0f Author: Michael Widenius <monty@mariadb.org> AuthorDate: Thu Nov 21 12:28:57 2024 +0200 Commit: Michael Widenius <monty@mariadb.org> CommitDate: Fri Dec 27 13:53:37 2024 +0200
diff --git a/storage/heap/hp_block.c b/storage/heap/hp_block.c index 324efc8b4af..44bca363746 100644 --- a/storage/heap/hp_block.c +++ b/storage/heap/hp_block.c @@ -75,9 +75,11 @@ int hp_get_new_block(HP_SHARE *info, HP_BLOCK *block, size_t *alloc_length) Next time we allocate data for X rows. When level 1 is full, we allocate data for HP_PTRS_IN_NOD at level 2 and 1 + X rows at level 0. */ *alloc_length= (sizeof(HP_PTRS) * ((i == block->levels) ? i : i - 1) + (ulonglong)block->records_in_block * block->recbuffer); + /* Alloc in blocks of powers of 2 */ + *alloc_length= MY_MAX(*alloc_length, block->alloc_size);
What will happen to block->alloc_size - *alloc_length bytes? they'll be unused?
Yes. It is better to have that small part unused than have malloc() use it for other purposes, which would cause fragmentation. The algorithm used to calculate row position assumes we have the same amount of rows in each segment. Could be fixed, but better to not do that now. <cut>
+/* + In how many parts are we going to do allocations of memory and indexes + If we assigne 1M to the heap table memory, we will allocate roughly + (1M/16) bytes per allocaiton +*/ +int heap_allocation_parts= 16;
static. technically, it could be `static const`, But I presume the compiler can figure out the `const` part if the variable is static. And e.g. use >> 4. Otherwise it has to assume it can be modified in some other file.
I wanted to first talk with you if we should make this user configurable. Will make it static const for now. <cut>
/* Create a heap table */
int heap_create(const char *name, HP_CREATE_INFO *create_info, @@ -170,7 +182,8 @@ int heap_create(const char *name, HP_CREATE_INFO *create_info, share->keydef= (HP_KEYDEF*) (share + 1); share->key_stat_version= 1; keyseg= (HA_KEYSEG*) (share->keydef + keys); - init_block(&share->block, visible_offset + 1, min_records, max_records); + init_block(&share->block, hp_memory_needed_per_row(visible_offset + 1),
visible_offset is already MY_MAX(reclength, sizeof (char*)). perhaps you don't need visible_offset and should use hp_memory_needed_per_row(reclength) here?
reclength is the number of bytes that is needed to store the record visible_offset is the number of bytes needed for storing either record/delete_link + delete marker. I can change hp_memory_needed_per_row() to work on reclength instead of visible_offset, however I still need to calculate visible_offset in heap_create() to know where to put the delete marker.
+ min_records, max_records); /* Fix keys */ memcpy(share->keydef, keydef, (size_t) (sizeof(keydef[0]) * keys)); for (i= 0, keyinfo= share->keydef; i < keys; i++, keyinfo++) @@ -266,44 +279,90 @@ static int keys_compare(void *heap_rb_, const void *key1_, heap_rb->search_flag, not_used); }
+ +/* + Calculate length needed for storing one row +*/ + +size_t hp_memory_needed_per_row(size_t reclength) +{ + /* Data needed for storing record + pointer to records */ + reclength= MY_MAX(reclength, sizeof(char*) + 1); + reclength= MY_ALIGN(reclength, sizeof(char*)); + return reclength; +} + +/* + Calculate the number of rows that fits into a given memory size +*/ + +ha_rows hp_rows_in_memory(size_t reclength, size_t index_size, + size_t memory_limit) +{ + /* We need one extra byte for the delete flag */ + reclength= hp_memory_needed_per_row(reclength+1);
you have +1 for the delete flag in hp_memory_needed_per_row()
The old hp_memory_needed_per_row() was assuming it got the row including the delete flag. The +1 was for (char*), it did not affect row length. The new version is: size_t hp_memory_needed_per_row(size_t reclength) { /* Data needed for storing record + pointer to records */ reclength= MY_MAX(reclength, sizeof(char*)); /* The + 1 below is for the delete marker at the end of record*/ reclength= MY_ALIGN(reclength+1, sizeof(char*)); return reclength; } With this I can remove the +1 in hp_rows_in_memory
+ if ((memory_limit - sizeof(HP_PTRS)) < (index_size + reclength)) + return 0; /* Wrong arguments */
Why do you need this if()? If its condition is true the expression below will return 0 naturally, you don't need a separate check for that.
+ return (ha_rows) ((memory_limit - sizeof(HP_PTRS)) / + (index_size + reclength));
If memory_limit < sizeof(HP_PTRS) and index_size + reclength is very small, the above would return a negative value. Assume memory_limit= 512, sizeof(HP_PTRS) = 1024, index_size=0, reclength=10 Better to have an explicit check against bad values. I have changed the test to be: if ((memory_limit < (index_size + reclength + sizeof(HP_PTR))) to avoid problems with small memory limits and unsigned arithmetic.
--- a/storage/heap/hp_write.c +++ b/storage/heap/hp_write.c @@ -145,6 +145,8 @@ static uchar *next_free_record_pos(HP_SHARE *info) DBUG_PRINT("exit",("Used old position: %p", pos)); DBUG_RETURN(pos); } + if (!(block_pos=(info->records % info->block.records_in_block))) + {
why?
This means that there is no more room in the current record pool and it's time to allocate more records. Assume block.records_in_block is 100 This means we have to allocate new records when info->records is 0, 100, 200, 300 ... Regards, Monty
Hi, Michael, On Dec 30, Michael Widenius wrote:
On Fri, Dec 27, 2024 at 8:12 PM Sergei Golubchik wrote:
commit b16319bdd0f Author: Michael Widenius <monty@mariadb.org> AuthorDate: Thu Nov 21 12:28:57 2024 +0200 Commit: Michael Widenius <monty@mariadb.org> CommitDate: Fri Dec 27 13:53:37 2024 +0200
diff --git a/storage/heap/hp_block.c b/storage/heap/hp_block.c index 324efc8b4af..44bca363746 100644 --- a/storage/heap/hp_block.c +++ b/storage/heap/hp_block.c @@ -75,9 +75,11 @@ int hp_get_new_block(HP_SHARE *info, HP_BLOCK *block, size_t *alloc_length) Next time we allocate data for X rows. When level 1 is full, we allocate data for HP_PTRS_IN_NOD at level 2 and 1 + X rows at level 0. */ *alloc_length= (sizeof(HP_PTRS) * ((i == block->levels) ? i : i - 1) + (ulonglong)block->records_in_block * block->recbuffer); + /* Alloc in blocks of powers of 2 */ + *alloc_length= MY_MAX(*alloc_length, block->alloc_size);
What will happen to block->alloc_size - *alloc_length bytes? they'll be unused?
Yes. It is better to have that small part unused than have malloc() use it for other purposes, which would cause fragmentation. The algorithm used to calculate row position assumes we have the same amount of rows in each segment. Could be fixed, but better to not do that now.
I've added some stats printing here, they show that your change can increase the memory usage by 30% and more: 13328 -> 16352 (+3024) 22.69 % 12488 -> 16352 (+3864) 30.94 % 12304 -> 16352 (+4048) 32.90 % not the total usage of course, but it can allocate up 30% larger buffers in some cases. Over the course of whole mtr run it loses 1.4M, that is 1.5%. Not much. Ok. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Michael Widenius -
Sergei Golubchik