[tarantool-patches] Re: [PATCH v2 1/2] box: create bigrefs for tuples

[Vladislav Shpilevoy]

Hello. Thanks for the patch! I have pushed some minor fixes. Look at
them, squash. See the rest 3 comments below.

> diff --git a/test/unit/tuple_bigref.c b/test/unit/tuple_bigref.c
> new file mode 100644
> index 0000000..fd166fd
> --- /dev/null
> +++ b/test/unit/tuple_bigref.c
> @@ -0,0 +1,156 @@
> +#include "vy_iterators_helper.h"
> +#include "memory.h"
> +#include "fiber.h"
> +#include "unit.h"
> +#include <msgpuck.h>
> +#include "trivia/util.h"
> +
> +enum
> +{
> +	BIGREF_DIFF = 10,
> +	BIGREF_COUNT = 100003,

1. Please, reduce the count to 70k for example. There is no
difference 100k or 70k for bigref list, but the test will be
faster.

> +	BIGREF_CAPACITY = 107,
> +};
> +
> +/**
> + * This test checks that indexes are given as intended.

2. Actually this test works exactly like the first one. See the
comments below.

> + */
> +static int
> +test_bigrefs_3()
> +{
> +	struct tuple **tuples = (struct tuple **) malloc(BIGREF_CAPACITY *
> +							 sizeof(*tuples));
> +	for(int i = 0; i < BIGREF_CAPACITY; ++i) {
> +		tuples[i] = create_tuple();
> +		tuple_ref(tuples[i]);
> +	}
> +	for(int i = 0; i < BIGREF_CAPACITY; ++i) {
> +		assert(tuples[i]->refs == 1);
> +		for(int j = 1; j < BIGREF_COUNT; ++j)
> +			box_tuple_ref(tuples[i]);
> +		assert(tuples[i]->is_bigref);
> +		if(i % BIGREF_DIFF == 0) {
> +			for(int j = 1; j < BIGREF_COUNT; ++j)
> +				box_tuple_unref(tuples[i]);
> +		}

3. Here when i % BIGREF_DIFF, you had added the tuple to bigref list,
and then have deleted it immediately. So the list has no gaps and works
exactly like in the first test.

Please, rework this test to test a list with gaps. You should fill it,
then remove some of tuples, for example, each BIGREF_DIFF, and check,
that other tuples are not destroyed. And that new tuples occupy the
gaps.

And please, use gcov to check coverage of tuple bigrefs. I have run
gcov, and saw this:

         -:  362:static inline void
         -:  363:bigref_list_delete_index(uint16_t index)
         -:  364:{
       648:  365:	bigref_list.refs[index] = 0;
       648:  366:	if (--bigref_list.size == 0) {
        15:  367:		bigref_list_reset();
        15:  368:		return;
         -:  369:	}
         -:  370:	/* Drop the 'take' hint. */
       633:  371:	bigref_list.hint_index_to_take = 0;
      1263:  372:	if (bigref_list.capacity == BIGREF_MIN_CAPACITY ||
       630:  373:	    bigref_list.size > bigref_list.capacity / BIGREF_FACTOR)
       249:  374:		return;
         -:  375:
       384:  376:	uint16_t top_index = bigref_list.hint_index_to_free;
      1086:  377:	while (bigref_list.refs[top_index] == 0)
       159:  378:		top_index--;
       384:  379:	bigref_list.hint_index_to_free = top_index;
         -:  380:
       384:  381:	uint16_t needed_capacity = top_index + 1;
       384:  382:	if (needed_capacity < BIGREF_MIN_CAPACITY)
     #####:  383:		needed_capacity = BIGREF_MIN_CAPACITY;
       384:  384:	if (needed_capacity > bigref_list.capacity / BIGREF_FACTOR)
       384:  385:		return;
         -:  386:	/* Round up capacity to the next highest power of 2. */
         -:  387:	assert(sizeof(needed_capacity) == sizeof(uint16_t));
     #####:  388:	needed_capacity--;
     #####:  389:	needed_capacity |= needed_capacity >> 1;
     #####:  390:	needed_capacity |= needed_capacity >> 2;
     #####:  391:	needed_capacity |= needed_capacity >> 4;
     #####:  392:	needed_capacity |= needed_capacity >> 8;
     #####:  393:	assert(needed_capacity < UINT16_MAX);
     #####:  394:	needed_capacity++;
     #####:  395:	uint32_t *refs =
     #####:  396:		(uint32_t *) realloc(bigref_list.refs, needed_capacity *
         -:  397:				     sizeof(*bigref_list.refs));
     #####:  398:	if (refs == NULL) {
     #####:  399:		panic("failed to reallocate %zu bytes: Cannot allocate "\
         -:  400:		      "memory.", needed_capacity * sizeof(*bigref_list.refs));
         -:  401:	}
     #####:  402:	bigref_list.refs = refs;
     #####:  403:	bigref_list.capacity = needed_capacity;
       648:  404:}


##### means this line had never been executed. You should write such a test, that
will cover these lines.

To use gcov you can build Tarantool with ENABLE_GCOV:

     cmake . -DENABLE_GCOV=1
     make -j

Then you run the test:

     ./test/unit/tuple_bigref.test

Then you convert .gcda/.gcno files to .gcov files:

     gcov src/box/tuple.c -o src/box/CMakeFiles/tuple.dir/tuple.c.gcno

Now tuple.c.gcov stores the coverage info.
These commands work on Mac. Maybe on Linux it differs. If cmake ENABLE_GCOV
does not work, then try this:

     @@ -4,10 +4,7 @@ set(ENABLE_GCOV_DEFAULT OFF)
      option(ENABLE_GCOV "Enable integration with gcov, a code coverage program" ${ENABLE_GCOV_DEFAULT})
  
      if (ENABLE_GCOV)
     -    if (NOT HAVE_GCOV)
     -    message (FATAL_ERROR
     -         "ENABLE_GCOV option requested but gcov library is not found")
     -    endif()

and run cmake again.