From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 D8D2E469710 for ; Tue, 17 Nov 2020 23:35:03 +0300 (MSK) Date: Tue, 17 Nov 2020 20:35:02 +0000 From: Nikita Pettik Message-ID: <20201117203502.GC19460@tarantool.org> References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-5-roman.habibov@tarantool.org> <20201105222621.GD8188@tarantool.org> <8C7AB70A-E70A-483B-BFA7-6F0C9E82DDDD@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8C7AB70A-E70A-483B-BFA7-6F0C9E82DDDD@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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org On 10 Nov 15:18, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Nov 6, 2020, at 01:26, Nikita Pettik wrote: > > > > 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. > > > > Would be nice pointing out where leak occured. Also realloc usage looks > > better to me (27 lines vs 8 with straight execution flaw). > 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. Region vs malloc allocation performance benefit is not a point at all as we speak about parsing process (at least for DDL statements). Ok, as you wish, let's leave patch as is. > >> Needed for #2349 > >> --- > >> src/box/sql/build.c | 48 +++++++++++++++++++++++++++++++++------------ > >> 1 file changed, 35 insertions(+), 13 deletions(-) > >> > >> 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, > >> } > >> > >> /** > >> - * 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 = space->index_count; > >> - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); > >> - struct index **idx = (struct index **) realloc(space->index, > >> - indexes_sz); > >> - if (idx == NULL) { > >> - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); > >> - return; > >> - } > >> - space->index = idx; > >> + size_t size = 0; > >> + struct index **idx = NULL; > >> + struct region *region = &parse->region; > >> + if (idx_count == 0) { > >> + idx = region_alloc_array(region, typeof(struct index *), 1, > >> + &size); > >> + if (idx == NULL) > >> + goto alloc_error; > >> + /* > >> + * Reallocate each time the idx_count becomes equal to the > >> + * power of two. > >> + */ > >> + } else if ((idx_count & (idx_count - 1)) == 0) { > >> + idx = region_alloc_array(region, typeof(struct index *), > >> + idx_count * 2, &size); > >> + if (idx == NULL) > >> + goto alloc_error; > >> + memcpy(idx, space->index, idx_count * sizeof(struct index *)); > >> + } > >> + if (idx != NULL) > >> + space->index = idx; > >> /* Make sure that PK always comes as first member. */ > >> if (index->def->iid == 0 && idx_count != 0) > >> SWAP(space->index[0], index); > >> space->index[space->index_count++] = index; > >> space->index_id_max = MAX(space->index_id_max, index->def->iid);; > >> + return 0; > >> + > >> +alloc_error: > >> + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); > >> + parse->is_aborted = true; > >> + return -1; > >> } > >> > >> int > >> @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { > >> sqlVdbeAddOp0(vdbe, OP_Expire); > >> } > >> > >> - if (tbl_name != NULL) > >> + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) > >> goto exit_create_index; > >> - table_add_index(space, index); > >> index = NULL; > >> > >> /* Clean up before exiting. */ > >> -- > >> 2.24.3 (Apple Git-128) > >> > > commit 459410f559a1c2f36a2da8925565a32a4caf4ae3 (HEAD) > Author: Roman Khabibov > Date: Thu Sep 3 14:26:47 2020 +0300 > > sql: use parser's region of "index" array > > 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. > > 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, > } > > /** > - * 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 = space->index_count; > - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); > - struct index **idx = (struct index **) realloc(space->index, > - indexes_sz); > - if (idx == NULL) { > - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); > - return; > - } > - space->index = idx; > + size_t size = 0; > + struct index **idx = NULL; > + struct region *region = &parse->region; > + if (idx_count == 0) { > + idx = region_alloc_array(region, typeof(struct index *), 1, > + &size); > + if (idx == NULL) > + goto alloc_error; > + /* > + * Reallocate each time the idx_count becomes equal to the > + * power of two. > + */ > + } else if ((idx_count & (idx_count - 1)) == 0) { > + idx = region_alloc_array(region, typeof(struct index *), > + idx_count * 2, &size); > + if (idx == NULL) > + goto alloc_error; > + memcpy(idx, space->index, idx_count * sizeof(struct index *)); > + } > + if (idx != NULL) > + space->index = idx; > /* Make sure that PK always comes as first member. */ > if (index->def->iid == 0 && idx_count != 0) > SWAP(space->index[0], index); > space->index[space->index_count++] = index; > space->index_id_max = MAX(space->index_id_max, index->def->iid);; > + return 0; > + > +alloc_error: > + diag_set(OutOfMemory, size, "region_alloc_array", "idx"); > + parse->is_aborted = true; > + return -1; > } > > int > @@ -2717,9 +2740,8 @@ sql_create_index(struct Parse *parse) { > sqlVdbeAddOp0(vdbe, OP_Expire); > } > > - if (tbl_name != NULL) > + if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) > goto exit_create_index; > - table_add_index(space, index); > index = NULL; > > /* Clean up before exiting. */ > >