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 D8C0E273D6 for ; Mon, 2 Apr 2018 06:27:18 -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 4EXIyQdLYxxd for ; Mon, 2 Apr 2018 06:27:18 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 8F399273A5 for ; Mon, 2 Apr 2018 06:27:18 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH 1/2] vinyl: allocate upsert counter on lsregion From: "v.shpilevoy@tarantool.org" In-Reply-To: Date: Mon, 2 Apr 2018 13:27:14 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: 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 Cc: kostja@tarantool.org Hello. Please, see 2 comments below. > 30 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 18:47, Vladimir = Davydov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > Currently, we store upsert counter in tuple metadata (that's what > upsert_format is for), but since it's only relevant for tuples of > the memory level, we can store it on lsregion, right before tuple > data. Let's do it now so that we can get rid of upsert_format. > --- > src/box/vy_stmt.c | 24 ++++++++++++++++-------- > src/box/vy_stmt.h | 23 ++++++++++++++--------- > 2 files changed, 30 insertions(+), 17 deletions(-) >=20 > diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c > index 84182e76..ac60c89e 100644 > --- a/src/box/vy_stmt.c > +++ b/src/box/vy_stmt.c > @@ -134,13 +134,26 @@ struct tuple * > vy_stmt_dup_lsregion(const struct tuple *stmt, struct lsregion = *lsregion, > int64_t alloc_id) > { > + enum iproto_type type =3D vy_stmt_type(stmt); > size_t size =3D tuple_size(stmt); > + size_t alloc_size =3D size; > struct tuple *mem_stmt; > - mem_stmt =3D lsregion_alloc(lsregion, size, alloc_id); > + > + /* Reserve one byte for UPSERT counter. */ > + if (type =3D=3D IPROTO_UPSERT) > + alloc_size++; > + > + mem_stmt =3D lsregion_alloc(lsregion, alloc_size, alloc_id); > if (mem_stmt =3D=3D NULL) { > diag_set(OutOfMemory, size, "lsregion_alloc", = "mem_stmt"); > return NULL; > } > + > + if (type =3D=3D IPROTO_UPSERT) { > + *(uint8_t *)mem_stmt =3D 0; 1. How about to at first, set mem_stmt and at second, call = vy_stmt_set_n_upserts? I think, that this pointers magic must be hidden where possible. > + mem_stmt =3D (struct tuple *)((uint8_t *)mem_stmt + 1); > + } > + > memcpy(mem_stmt, stmt, size); > /* > * Region allocated statements can't be referenced or = unreferenced >=20 > struct tuple * > diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h > index a33739d6..8958adb5 100644 > --- a/src/box/vy_stmt.h > +++ b/src/box/vy_stmt.h > @@ -146,23 +146,28 @@ vy_stmt_set_type(struct tuple *stmt, enum = iproto_type type) > ((struct vy_stmt *) stmt)->type =3D type; > } >=20 > -/** Get upserts count of the vinyl statement. */ > +/** > + * Get upserts count of the vinyl statement. > + * Only for UPSERT statements allocated on lsregion. 2. Can you, please, add an assertion for this? For example: assert(! vy_stmt_is_refable(stmt)). > + */ > static inline uint8_t > vy_stmt_n_upserts(const struct tuple *stmt) > { > - assert(tuple_format(stmt)->extra_size =3D=3D sizeof(uint8_t)); > - return *((const uint8_t *) tuple_extra(stmt)); > + assert(stmt->refs =3D=3D 0); > + assert(vy_stmt_type(stmt) =3D=3D IPROTO_UPSERT); > + return *((uint8_t *)stmt - 1); > } >=20 > -/** Set upserts count of the vinyl statement. */ > +/** > + * Set upserts count of the vinyl statement. > + * Only for UPSERT statements allocated on lsregion. > + */ > static inline void > vy_stmt_set_n_upserts(struct tuple *stmt, uint8_t n) > { > - struct tuple_format *format =3D tuple_format(stmt); > - assert(format->extra_size =3D=3D sizeof(uint8_t)); > - char *extra =3D (char *) stmt + stmt->data_offset - > - tuple_format_meta_size(format); > - *((uint8_t *) extra) =3D n; > + assert(stmt->refs =3D=3D 0); > + assert(vy_stmt_type(stmt) =3D=3D IPROTO_UPSERT); > + *((uint8_t *)stmt - 1) =3D n; > } >=20 > /** Get the column mask of the specified tuple. */ > --=20 > 2.11.0 >=20 >=20