From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AD9AE469719 for ; Tue, 10 Nov 2020 15:18:06 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <20201105222621.GD8188@tarantool.org> Date: Tue, 10 Nov 2020 15:18:05 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8C7AB70A-E70A-483B-BFA7-6F0C9E82DDDD@tarantool.org> References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-5-roman.habibov@tarantool.org> <20201105222621.GD8188@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Nov 6, 2020, at 01:26, Nikita Pettik = wrote: >=20 > On 09 Oct 16:45, Roman Khabibov wrote: >> Allocate memory for the "index" array of ephemeral space on the >> parser's region instead of a heap as it was before. Fixed a memory >> leak that realloc() generated. >=20 > Would be nice pointing out where leak occured. Also realloc usage = looks > better to me (27 lines vs 8 with straight execution flaw).=20 Vlad said that main point of the region - speed, and it is also harder to leave a memory leak, because the region is destroyed in the end of parsing entirely. >> Needed for #2349 >> --- >> src/box/sql/build.c | 48 = +++++++++++++++++++++++++++++++++------------ >> 1 file changed, 35 insertions(+), 13 deletions(-) >>=20 >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 3b3c8099a..fd8e7ed1e 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse = *parse, uint32_t space_id, >> } >>=20 >> /** >> - * Add new index to table's indexes list. >> + * Add new index to ephemeral @a space's "index" array. Reallocate >> + * memory on @a parse's region if needed. >> + * >> * We follow convention that PK comes first in list. >> * >> + * @param parse Parsing structure. >> * @param space Space to which belongs given index. >> * @param index Index to be added to list. >> */ >> -static void >> -table_add_index(struct space *space, struct index *index) >> +static int >> +sql_space_add_index(struct Parse *parse, struct space *space, >> + struct index *index) >> { >> uint32_t idx_count =3D space->index_count; >> - size_t indexes_sz =3D sizeof(struct index *) * (idx_count + 1); >> - struct index **idx =3D (struct index **) realloc(space->index, >> - indexes_sz); >> - if (idx =3D=3D NULL) { >> - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); >> - return; >> - } >> - space->index =3D idx; >> + size_t size =3D 0; >> + struct index **idx =3D NULL; >> + struct region *region =3D &parse->region; >> + if (idx_count =3D=3D 0) { >> + idx =3D region_alloc_array(region, typeof(struct index = *), 1, >> + &size); >> + if (idx =3D=3D NULL) >> + goto alloc_error; >> + /* >> + * Reallocate each time the idx_count becomes equal to the >> + * power of two. >> + */ >> + } else if ((idx_count & (idx_count - 1)) =3D=3D 0) { >> + idx =3D region_alloc_array(region, typeof(struct index = *), >> + idx_count * 2, &size); >> + if (idx =3D=3D NULL) >> + goto alloc_error; >> + memcpy(idx, space->index, idx_count * sizeof(struct = index *)); >> + } >> + if (idx !=3D NULL) >> + space->index =3D idx; >> /* Make sure that PK always comes as first member. */ >> if (index->def->iid =3D=3D 0 && idx_count !=3D 0) >> SWAP(space->index[0], index); >> space->index[space->index_count++] =3D index; >> space->index_id_max =3D MAX(space->index_id_max, = index->def->iid);; >> + return 0; >> + >> +alloc_error: >> + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); >> + parse->is_aborted =3D true; >> + return -1; >> } >>=20 >> int >> @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { >> sqlVdbeAddOp0(vdbe, OP_Expire); >> } >>=20 >> - if (tbl_name !=3D NULL) >> + if (tbl_name !=3D NULL || sql_space_add_index(parse, space, = index) !=3D 0) >> goto exit_create_index; >> - table_add_index(space, index); >> index =3D NULL; >>=20 >> /* Clean up before exiting. */ >> --=20 >> 2.24.3 (Apple Git-128) >>=20 commit 459410f559a1c2f36a2da8925565a32a4caf4ae3 (HEAD) Author: Roman Khabibov Date: Thu Sep 3 14:26:47 2020 +0300 sql: use parser's region of "index" array =20 Allocate memory for the "index" array of ephemeral space on the parser's region instead of a heap as it was before. Fixed a memory leak that realloc() generated for the index array of ephemeral space. The memory was never released. =20 Needed for #2349 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 3b3c8099a..fd8e7ed1e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2228,29 +2228,52 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, = uint32_t space_id, } =20 /** - * Add new index to table's indexes list. + * Add new index to ephemeral @a space's "index" array. Reallocate + * memory on @a parse's region if needed. + * * We follow convention that PK comes first in list. * + * @param parse Parsing structure. * @param space Space to which belongs given index. * @param index Index to be added to list. */ -static void -table_add_index(struct space *space, struct index *index) +static int +sql_space_add_index(struct Parse *parse, struct space *space, + struct index *index) { uint32_t idx_count =3D space->index_count; - size_t indexes_sz =3D sizeof(struct index *) * (idx_count + 1); - struct index **idx =3D (struct index **) realloc(space->index, - indexes_sz); - if (idx =3D=3D NULL) { - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); - return; - } - space->index =3D idx; + size_t size =3D 0; + struct index **idx =3D NULL; + struct region *region =3D &parse->region; + if (idx_count =3D=3D 0) { + idx =3D region_alloc_array(region, typeof(struct index = *), 1, + &size); + if (idx =3D=3D NULL) + goto alloc_error; + /* + * Reallocate each time the idx_count becomes equal to the + * power of two. + */ + } else if ((idx_count & (idx_count - 1)) =3D=3D 0) { + idx =3D region_alloc_array(region, typeof(struct index = *), + idx_count * 2, &size); + if (idx =3D=3D NULL) + goto alloc_error; + memcpy(idx, space->index, idx_count * sizeof(struct = index *)); + } + if (idx !=3D NULL) + space->index =3D idx; /* Make sure that PK always comes as first member. */ if (index->def->iid =3D=3D 0 && idx_count !=3D 0) SWAP(space->index[0], index); space->index[space->index_count++] =3D index; space->index_id_max =3D MAX(space->index_id_max, = index->def->iid);; + return 0; + +alloc_error: + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); + parse->is_aborted =3D true; + return -1; } =20 int @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } =20 - if (tbl_name !=3D NULL) + if (tbl_name !=3D NULL || sql_space_add_index(parse, space, = index) !=3D 0) goto exit_create_index; - table_add_index(space, index); index =3D NULL; =20 /* Clean up before exiting. */