From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CB433445320 for ; Wed, 15 Jul 2020 13:02:15 +0300 (MSK) References: <1594221263-6228-1-git-send-email-alyapunov@tarantool.org> <1594221263-6228-15-git-send-email-alyapunov@tarantool.org> From: Aleksandr Lyapunov Message-ID: <811edf37-2708-0c08-6f0c-4acf79b88b8e@tarantool.org> Date: Wed, 15 Jul 2020 13:02:14 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH 14/16] tx: indexes List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Hi, thanks for your review! On 15.07.2020 02:50, Vladislav Shpilevoy wrote: > > 1. Missing whitespace afrer =. fixed > >> #endif /* #ifndef OLD_GOOD_BITSET */ >> + uint32_t iid = iterator->index->def->iid; >> + struct txn *txn = in_txn(); >> + bool is_rw = txn != NULL; >> + *ret = txm_tuple_clarify(txn, tuple, iid, 0, is_rw); > 2. Some of these values you don't need to load in the cycle. They don't > change. > > * in_txn() can be called out of the cycle just once; > * is_rw can be calculated only once; > * iid does not change; > * struct memtx_bitset_index *index does not change; > > The same applies to rtree changes. Actually that is not a problem for modern compilers not to make the same thing several times. For example https://godbolt.org/z/9zvnn5 So it's not a performance issue. I make those variables as aliases for readability. I could move them out of loop if you insist but I fear that it will become less readable. > > 3. On the branch I see a 'txm_snapshot_cleanser' structure > in this file. But not in the email. Can't review it. Why is > it called 'cleanser' instead of 'cleaner'? What is it doing? Shame on me, maybe I forgot to add in. In a new version it's there, with comments. btw renamed is as 'cleaner' > \ >> + do { \ >> + int rc = first ? name##_base(iterator, ret) \ >> + : hash_iterator_ge_base(iterator, ret); \ > 4. Seems like unnecessary branching. If you know you will specially > handle only the first iteration, then why no to make it before the > cycle? And eliminate 'first' + '?' branch. Also use prefix 'is_' for > flag names. Or 'has_'/'does_'/etc. The same for all the other new > flags, including 'preserve_old_tuple'. names - ok, but again this work for a compiler https://godbolt.org/z/vbEeEP I could change it if you insist but compiled code will be merely the same. > >> + if (rc != 0 || *ret == NULL) \ >> + return rc; \ >> + first = false; \ >> + *ret = txm_tuple_clarify(txn, *ret, iid, 0, is_rw); \ >> + } while (*ret == NULL); \ >> + return 0; \ >> +} \ > 5. Please, use tabs for alignment. In other places too. done > >> +struct forgot_to_add_semicolon > 6. What is this? That's a standard guard that prohibits usage of macro w/o semicolon in the end of line If somebody forgets to add ; he will get an error message with 'forgot_to_add_semicolon'. > >> + >> 7. Why did you remove the hash_iterator_ge() call? You still can use >> it here, with the new name hash_iterator_ge_base(). fixed > >> + bool is_rw = txn != NULL; >> + *ret = txm_tuple_clarify(txn, *res, ptr->index->def->iid, 0, is_rw); > 8. Why isn't it a cycle? because there can be only one tuple with the desired key in the hash table. > > 9. Why 'txn != NULL' can't be done inside txm_tuple_clarify()? It > takes txn pointer anyway, and you calculate 'is_rw' everywhere > before the call. Historical, will fix it. > >> + return 0; >> } >> >> + > 10. Unnecessary new line. ok > >> + struct memtx_tree_iterator *ti = &it->tree_iterator; \ >> + uint32_t iid = iterator->index->def->iid; \ >> + bool is_multikey = iterator->index->def->key_def->is_multikey; \ > 11. All these dereferences are going to cost a lot, even when > there are no concurrent txns. Can they be done in a lazy mode? > Only if the found tuple is dirty. The same applies to all the > other places. A compiler should surely handle it, since ..._clarify() is a static inline member. Even a processor would handle it, it also reorders instructions, but usually it has nothing to do while the tuple is fetching from memory, and I guess it will try to do something even outside a branch.