MDEV-35190 HASH_SEARCH duplicates effort before HASH_INSERT or HASH_DELETE - #3593
MDEV-35190 HASH_SEARCH duplicates effort before HASH_INSERT or HASH_DELETE#3593dr-m wants to merge 3 commits into
Conversation
ha_storage_put_memlim(): Invoke my_crc32c() to "fold", and traverse the hash table only once. fold_lock(): Remove some redundant conditions and use my_crc32c() instead of ut_fold_ulint_pair(). trx_i_s_cache_t::add(): Replaces add_lock_to_cache(), search_innodb_locks(), and locks_row_eq_lock(). Avoid duplicated traversal of the hash table.
|
|
…ELETE The HASH_ macros are unnecessarily obfuscating the logic, so we had better replace them. hash_cell_t::remove(): Implement most of the HASH_DELETE logic. This is based on buf_pool_t::page_hash_table::remove(). xb_filter_hash_free(): Avoid any hash table lookup; just traverse the hash bucket chains and free each element. rm_if_not_found(): Make use of find_filter_in_hashtable(). dict_sys_t::acquire_temporary_table(), dict_sys_t::find_table(): Define non-inline to avoid unnecessary code duplication. dict_sys_t::add(dict_table_t *table), dict_table_rename_in_cache(): Look for duplicate while finding the insert position. dict_table_change_id_in_cache(): Merged to the only caller row_discard_tablespace(). hash_insert(): Helper function of dict_sys_t::resize(). fil_space_t::create(): Look for a duplicate (and crash if found) when searching for the insert position. lock_rec_discard(): Take the hash array cell as a parameter to avoid a duplicated lookup. lock_rec_free_all_from_discard_page(): Remove a parameter.
ut_fold_ull(): Use an identity mapping unless SIZEOF_SIZE_T<8
mariadb-DebarunBanerjee
left a comment
mariadb-DebarunBanerjee
left a comment
There was a problem hiding this comment.
The changes look functionally correct to me and the improvement points are good.
One not so good observation is that we are losing our grip on hash table abstraction with hash operations being directly coded when one is using a hash table. I think the key improvement points can be incorporated with a better hash table abstraction.
@dr-m What is your thought on it ? I am ok to approve the patch as it is correct functionally and unlikely to cause performance impact. I would love to see a better abstraction for the hash table though considering maintenance and future improvement.
| after= reinterpret_cast<dict_table_t**> | ||
| (&(table->is_temporary() ? temp_id_hash : table_id_hash). | ||
| cell_get(ut_fold_ull(table->id))->node); | ||
| for (; *after; after = &(*after)->id_hash) |
There was a problem hiding this comment.
If I am correct, it would mean that we don't apply any hash function for 8 byte table ID and use it directly. I don't know if the adhoc hashing function was doing better than that but do you think it could possibly impact the distribution ?
There was a problem hiding this comment.
Your comment is about f2fd2b8, which replaces ut_fold_ull() with an identity mapping on 64-bit systems. As far as I understand hashing, the main thing is that the slot in the hash table array is chosen by a modulus over the size of the hash table, which had better be a prime number. The ut_fold_ull() function was unnecessarily losing some entropy on 64-bit systems and potentially causing some premature loss of entropy, that is, causing some hash collision that could have been unavoidable. Any collision should be rather unlikely, because neither table or index identifiers are likely to be larger than 1U<<32. The main win of refactoring ut_fold_ull() should be to reduce the size of the code and therefore to get a little performance improvement.
Related to this, there is also the function ut_hash_ulint(), which is performing an unnecessary-looking exclusive-or operation before calculating a modulus. Based on my reading of hash algorithms and of mysql/mysql-server@b11a175 we had better get rid of that function as well. While it does not seem to lose any entropy, it is an unnecessary obfuscation.
There was a problem hiding this comment.
Part of the comment is addressed by b4ab3e7 in #3627. It adds find() and search() member functions, which will be used for searching the hash table. While that will introduce one or two levels of extra function calls to non-optimized builds, for CMAKE_BUILD_TYPE=RelWithDebInfo I checked that there is no overhead; in fact, buf_buddy_block_free() (inline as part of buf_buddy_free_low()) got a little better when compiled with GCC 14.2.0.
06082b9 to
ccb6cd8
Compare
3 participants
Description
The
HASH_macros are unnecessarily obfuscating the logic, so we had better replace them.hash_cell_t::remove(): Implement most of theHASH_DELETElogic. This is based onbuf_pool_t::page_hash_table::remove().xb_filter_hash_free(): Avoid any hash table lookup; just traverse the hash bucket chains and free each element.rm_if_not_found(): Make use offind_filter_in_hashtable().dict_sys_t::acquire_temporary_table(),dict_sys_t::find_table(): Define non-inline to avoid unnecessary code duplication.dict_sys_t::add(dict_table_t *table),dict_table_rename_in_cache(): Look for duplicate while finding the insert position.dict_table_change_id_in_cache(): Merged to the only callerrow_discard_tablespace().hash_insert(): Helper function ofdict_sys_t::resize().fil_space_t::create(): Look for a duplicate (and crash if found) when searching for the insert position.lock_rec_discard(): Take the hash array cell as a parameter to avoid a duplicated lookup.lock_rec_free_all_from_discard_page(): Remove a parameter.Release Notes
Some InnoDB hash table operations were simplified to slightly improve performance.
How can this PR be tested?
This low level code should be well covered by the regression test suite. Some additional performance and stress testing would be helpful.
Basing the PR against the correct MariaDB version
mainbranch.This depends on #3584 which currently targets 10.6.
PR quality check