Re: [Tarantool-patches] [PATCH 14/16] tx: indexes

[Aleksandr Lyapunov <alyapunov@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.