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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

>>>   #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.

I don't see how can it become less readable. You just move a few lines out
of the loop.

Talking of the compiler - I am sure it can handle simple cases like you've
shown in the online compiler. But still I wouldn't rely on it so much. So
I still would better advise to move the reads of the cycle.

>>> +    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.

I can't insist on anything. I am just a middle dev. But I would better
reduce the amount of trust we put on the compiler.

>>> +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'.

We never use it anywhere. Please, drop. No need to complicate things. I don't
see any problems about not using ';' in the end of this macro.

>>> +    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.

Yeah, but one tuple with a key can have a history of the tuples with the
same key.

>>> +    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.

It will speculate, yes. But in 99% of cases it will speculate into the branch
where the tuple is not dirty, because that happens almost always (supposed to
happen). So the reads won't be done unless necessary, in rare cases. Currently
they are done always.