From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] [PATCH v2 1/1] box: restore region after box.snapshot() References: <90fd04c36016067157b95cf689eb8780daf79e96.1542192128.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <36a23d77-0f7d-797d-66de-6e6903c28208@tarantool.org> Date: Wed, 14 Nov 2018 14:08:26 +0300 MIME-Version: 1.0 In-Reply-To: <90fd04c36016067157b95cf689eb8780daf79e96.1542192128.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: imeevma@tarantool.org, tarantool-patches@freelists.org, Vladimir Davydov List-ID: Vova, please, review. On 14/11/2018 13:44, imeevma@tarantool.org wrote: > Before this patch region wasn't restored after box.snapshot() so > some tests fail due to "memory leak". This patch adds additional > memory management and fixes mentioned failures. > > Closes #3732 > --- > Issue: https://github.com/tarantool/tarantool/issues/assigned/ImeevMA > Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3732-additional-memory-managament-in-box_snapshot > > src/box/memtx_engine.c | 41 ++++++++++++++++++++--------------------- > src/box/vy_log.c | 11 ++++++++--- > test/engine/snapshot.result | 35 +++++++++++++++++++++++++++++++++++ > test/engine/snapshot.test.lua | 17 +++++++++++++++++ > 4 files changed, 80 insertions(+), 24 deletions(-) > > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 5a5e87e..12be0f2 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -589,28 +589,34 @@ struct checkpoint { > bool touch; > }; > > -static int > -checkpoint_init(struct checkpoint *ckpt, const char *snap_dirname, > - uint64_t snap_io_rate_limit) > +static struct checkpoint * > +checkpoint_new(const char *snap_dirname, uint64_t snap_io_rate_limit) > { > + struct checkpoint *ckpt = malloc(sizeof(*ckpt)); > + if (ckpt == NULL) { > + diag_set(OutOfMemory, sizeof(*ckpt), "malloc", > + "struct checkpoint"); > + return NULL; > + } > rlist_create(&ckpt->entries); > ckpt->waiting_for_snap_thread = false; > xdir_create(&ckpt->dir, snap_dirname, SNAP, &INSTANCE_UUID); > ckpt->snap_io_rate_limit = snap_io_rate_limit; > vclock_create(&ckpt->vclock); > ckpt->touch = false; > - return 0; > + return ckpt; > } > > static void > -checkpoint_destroy(struct checkpoint *ckpt) > +checkpoint_delete(struct checkpoint *ckpt) > { > struct checkpoint_entry *entry; > rlist_foreach_entry(entry, &ckpt->entries, link) { > entry->iterator->free(entry->iterator); > + free(entry); > } > - rlist_create(&ckpt->entries); > xdir_destroy(&ckpt->dir); > + free(ckpt); > } > > > @@ -625,11 +631,10 @@ checkpoint_add_space(struct space *sp, void *data) > if (!pk) > return 0; > struct checkpoint *ckpt = (struct checkpoint *)data; > - struct checkpoint_entry *entry; > - entry = region_alloc_object(&fiber()->gc, struct checkpoint_entry); > + struct checkpoint_entry *entry = malloc(sizeof(*entry)); > if (entry == NULL) { > diag_set(OutOfMemory, sizeof(*entry), > - "region", "struct checkpoint_entry"); > + "malloc", "struct checkpoint_entry"); > return -1; > } > rlist_add_tail_entry(&ckpt->entries, entry, link); > @@ -693,19 +698,13 @@ memtx_engine_begin_checkpoint(struct engine *engine) > struct memtx_engine *memtx = (struct memtx_engine *)engine; > > assert(memtx->checkpoint == NULL); > - memtx->checkpoint = region_alloc_object(&fiber()->gc, struct checkpoint); > - if (memtx->checkpoint == NULL) { > - diag_set(OutOfMemory, sizeof(*memtx->checkpoint), > - "region", "struct checkpoint"); > - return -1; > - } > - > - if (checkpoint_init(memtx->checkpoint, memtx->snap_dir.dirname, > - memtx->snap_io_rate_limit) != 0) > + memtx->checkpoint = checkpoint_new(memtx->snap_dir.dirname, > + memtx->snap_io_rate_limit); > + if (memtx->checkpoint == NULL) > return -1; > > if (space_foreach(checkpoint_add_space, memtx->checkpoint) != 0) { > - checkpoint_destroy(memtx->checkpoint); > + checkpoint_delete(memtx->checkpoint); > memtx->checkpoint = NULL; > return -1; > } > @@ -790,7 +789,7 @@ memtx_engine_commit_checkpoint(struct engine *engine, > xdir_add_vclock(&memtx->snap_dir, &memtx->checkpoint->vclock); > } > > - checkpoint_destroy(memtx->checkpoint); > + checkpoint_delete(memtx->checkpoint); > memtx->checkpoint = NULL; > } > > @@ -818,7 +817,7 @@ memtx_engine_abort_checkpoint(struct engine *engine) > INPROGRESS); > (void) coio_unlink(filename); > > - checkpoint_destroy(memtx->checkpoint); > + checkpoint_delete(memtx->checkpoint); > memtx->checkpoint = NULL; > } > > diff --git a/src/box/vy_log.c b/src/box/vy_log.c > index fc8ede5..c7ed7cb 100644 > --- a/src/box/vy_log.c > +++ b/src/box/vy_log.c > @@ -761,6 +761,7 @@ vy_log_flush(void) > stailq_foreach_entry(record, &vy_log.tx, in_tx) > tx_size++; > > + size_t used = region_used(&fiber()->gc); > struct journal_entry *entry = journal_entry_new(tx_size); > if (entry == NULL) > return -1; > @@ -770,7 +771,7 @@ vy_log_flush(void) > tx_size * sizeof(struct xrow_header), > alignof(struct xrow_header)); > if (rows == NULL) > - return -1; > + goto err; > > /* > * Encode buffered records. > @@ -780,7 +781,7 @@ vy_log_flush(void) > assert(i < tx_size); > struct xrow_header *row = &rows[i]; > if (vy_log_record_encode(record, row) < 0) > - return -1; > + goto err; > entry->rows[i] = row; > i++; > } > @@ -791,12 +792,16 @@ vy_log_flush(void) > * so as not to block the tx thread. > */ > if (wal_write_vy_log(entry) != 0) > - return -1; > + goto err; > > + region_truncate(&fiber()->gc, used); > /* Success. Free flushed records. */ > region_reset(&vy_log.pool); > stailq_create(&vy_log.tx); > return 0; > +err: > + region_truncate(&fiber()->gc, used); > + return -1; > } > > void > diff --git a/test/engine/snapshot.result b/test/engine/snapshot.result > index a5aa08d..42009c2 100644 > --- a/test/engine/snapshot.result > +++ b/test/engine/snapshot.result > @@ -238,3 +238,38 @@ box.space.test.index.secondary:select{} > box.space.test:drop() > --- > ... > +-- > +-- sql: fix sql/gh-3199-no-mem-leaks.test.lua > +-- One of the reason why this test failed - box.snapshot didn't > +-- free region. > +-- > +fiber = require('fiber') > +--- > +... > +-- Should be 0. > +fiber.info()[fiber.self().id()].memory.used > +--- > +- 0 > +... > +box.snapshot() > +--- > +- ok > +... > +-- Should be 0. > +fiber.info()[fiber.self().id()].memory.used > +--- > +- 0 > +... > +box.snapshot() > +--- > +- ok > +... > +box.snapshot() > +--- > +- ok > +... > +-- Should be 0. > +fiber.info()[fiber.self().id()].memory.used > +--- > +- 0 > +... > diff --git a/test/engine/snapshot.test.lua b/test/engine/snapshot.test.lua > index 4bbbe40..2a32280 100644 > --- a/test/engine/snapshot.test.lua > +++ b/test/engine/snapshot.test.lua > @@ -41,3 +41,20 @@ box.space.test:select{} > box.space.test.index.primary:select{} > box.space.test.index.secondary:select{} > box.space.test:drop() > + > +-- > +-- sql: fix sql/gh-3199-no-mem-leaks.test.lua > +-- One of the reason why this test failed - box.snapshot didn't > +-- free region. > +-- > + > +fiber = require('fiber') > +-- Should be 0. > +fiber.info()[fiber.self().id()].memory.used > +box.snapshot() > +-- Should be 0. > +fiber.info()[fiber.self().id()].memory.used > +box.snapshot() > +box.snapshot() > +-- Should be 0. > +fiber.info()[fiber.self().id()].memory.used >