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

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

Thanks for the patch!

See 11 comments below.

> diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c
> index 67eaf6f..f3ab74f 100644
> --- a/src/box/memtx_bitset.c
> +++ b/src/box/memtx_bitset.c
> @@ -198,19 +199,26 @@ bitset_index_iterator_next(struct iterator *iterator, struct tuple **ret)
>  	assert(iterator->free == bitset_index_iterator_free);
>  	struct bitset_index_iterator *it = bitset_index_iterator(iterator);
>  
> -	size_t value = tt_bitset_iterator_next(&it->bitset_it);
> -	if (value == SIZE_MAX) {
> -		*ret = NULL;
> -		return 0;
> -	}
> -
> +	do {
> +		size_t value = tt_bitset_iterator_next(&it->bitset_it);
> +		if (value == SIZE_MAX) {
> +			*ret = NULL;
> +			return 0;
> +		}
>  #ifndef OLD_GOOD_BITSET
> -	struct memtx_bitset_index *index =
> -		(struct memtx_bitset_index *)iterator->index;
> -	*ret = memtx_bitset_index_value_to_tuple(index, value);
> +		struct memtx_bitset_index *index =
> +			(struct memtx_bitset_index *)iterator->index;
> +		struct tuple *tuple =
> +			memtx_bitset_index_value_to_tuple(index, value);
>  #else /* #ifndef OLD_GOOD_BITSET */
> -	*ret = value_to_tuple(value);
> +		struct tuple *tuple =value_to_tuple(value);

1. Missing whitespace afrer =.

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

> +	} while (*ret == NULL);
> +
>  	return 0;
>  }
>  
> diff --git a/src/box/memtx_hash.c b/src/box/memtx_hash.c

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?

> index cdd531c..b3ae60c 100644
> --- a/src/box/memtx_hash.c
> +++ b/src/box/memtx_hash.c
> @@ -128,6 +129,31 @@ hash_iterator_gt(struct iterator *ptr, struct tuple **ret)
>  	return 0;
>  }
>  
> +#define WRAP_ITERATOR_METHOD(name)                                             \
> +static int                                                                     \
> +name(struct iterator *iterator, struct tuple **ret)                            \
> +{                                                                              \
> +	struct txn *txn = in_txn();                                            \
> +	bool is_rw = txn != NULL;                                              \
> +	uint32_t iid = iterator->index->def->iid;                              \
> +	bool first = true;                                                     \
> +	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'.

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

> +struct forgot_to_add_semicolon

6. What is this?

> +
> +WRAP_ITERATOR_METHOD(hash_iterator_ge);
> +WRAP_ITERATOR_METHOD(hash_iterator_gt);
> +
> +#undef WRAP_ITERATOR_METHOD
> +
> @@ -136,12 +162,25 @@ hash_iterator_eq_next(MAYBE_UNUSED struct iterator *it, struct tuple **ret)
>  }
>  
>  static int
> -hash_iterator_eq(struct iterator *it, struct tuple **ret)
> +hash_iterator_eq(struct iterator *ptr, struct tuple **ret)
>  {
> -	it->next = hash_iterator_eq_next;
> -	return hash_iterator_ge(it, ret);
> +	ptr->next = hash_iterator_eq_next;
> +	assert(ptr->free == hash_iterator_free);
> +	struct hash_iterator *it = (struct hash_iterator *) ptr;
> +	struct memtx_hash_index *index = (struct memtx_hash_index *)ptr->index;
> +	struct tuple **res = light_index_iterator_get_and_next(&index->hash_table,
> +							       &it->iterator);

7. Why did you remove the hash_iterator_ge() call? You still can use
it here, with the new name hash_iterator_ge_base().

> +	if (res == NULL) {
> +		*ret = NULL;
> +		return 0;
> +	}
> +	struct txn *txn = in_txn();
> +	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?

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.

> +	return 0;
>  }
>  
> +

10. Unnecessary new line.

>  /* }}} */
> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> index 612fcb2..992a422 100644
> --- a/src/box/memtx_rtree.c
> +++ b/src/box/memtx_rtree.c
> @@ -304,6 +305,45 @@ tree_iterator_prev_equal(struct iterator *iterator, struct tuple **ret)
>  	return 0;
>  }
>  
> +#define WRAP_ITERATOR_METHOD(name)                                             \
> +static int                                                                     \
> +name(struct iterator *iterator, struct tuple **ret)                            \
> +{                                                                              \
> +	struct memtx_tree *tree =                                              \
> +		&((struct memtx_tree_index *)iterator->index)->tree;           \
> +	struct tree_iterator *it = tree_iterator(iterator);                    \
> +	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.

> +	struct txn *txn = in_txn();                                            \
> +	bool is_rw = txn != NULL;                                              \
> +	do {                                                                   \
> +		int rc = name##_base(iterator, ret);                           \
> +		if (rc != 0 || *ret == NULL)                                   \
> +			return rc;                                             \
> +		uint32_t mk_index = 0;                                         \
> +		if (is_multikey) {                                             \
> +			struct memtx_tree_data *check =                        \
> +				memtx_tree_iterator_get_elem(tree, ti);        \
> +			assert(check != NULL);                                 \
> +			mk_index = check->hint;                                \
> +		}                                                              \
> +		*ret = txm_tuple_clarify(txn, *ret, iid, mk_index, is_rw);     \
> +	} while (*ret == NULL);                                                \
> +	tuple_unref(it->current.tuple);                                        \
> +	it->current.tuple = *ret;                                              \
> +	tuple_ref(it->current.tuple);                                          \
> +	return 0;                                                              \
> +}                                                                              \
> +struct forgot_to_add_semicolon
> +
> +WRAP_ITERATOR_METHOD(tree_iterator_next);
> +WRAP_ITERATOR_METHOD(tree_iterator_prev);
> +WRAP_ITERATOR_METHOD(tree_iterator_next_equal);
> +WRAP_ITERATOR_METHOD(tree_iterator_prev_equal);
> +
> +#undef WRAP_ITERATOR_METHOD
> +