From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 6224E25E91 for ; Thu, 7 Jun 2018 17:24:23 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hrzfMLGDQ6Si for ; Thu, 7 Jun 2018 17:24:23 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B297225E72 for ; Thu, 7 Jun 2018 17:24:22 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/2] box: create bigrefs for tuples References: <6bfce1d3b6f797a6ebf3e56f81fad4181e855453.1528388742.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Fri, 8 Jun 2018 00:24:19 +0300 MIME-Version: 1.0 In-Reply-To: <6bfce1d3b6f797a6ebf3e56f81fad4181e855453.1528388742.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@tarantool.org 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 > +#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.