Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region
Date: Fri, 25 Sep 2020 00:31:58 +0200	[thread overview]
Message-ID: <0f0862f5-1a3c-1d98-d2fb-8f39e76fb37a@tarantool.org> (raw)
In-Reply-To: <da6ca8df638d041ed558480a601f2cee42e41318.1600817803.git.alexander.turenko@tarantool.org>

Hi! Thanks for the patch!

See 2 comments below.

> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> index 16ee9f414..618ba8045 100644
> --- a/src/lib/core/fiber.h
> +++ b/src/lib/core/fiber.h
> @@ -386,6 +386,107 @@ struct slab_cache;
>  API_EXPORT struct slab_cache *
>  cord_slab_cache(void);
>  
> +/**
> + * box region allocator
> + *
> + * It is the region allocator from the small library. It is useful
> + * for allocating tons of small objects and free them at once.
> + *
> + * Typical usage is illustrated in the sketch below.
> + *
> + *  | size_t region_svp = box_region_used();
> + *  | while (<...>) {
> + *  |     char *buf = box_region_alloc(<...>);
> + *  |     <...>
> + *  | }
> + *  | box_region_truncate(region_svp);
> + *
> + * There are module API functions that return a result on
> + * this region. In this case a module is responsible to free the
> + * result:
> + *
> + *  | size_t region_svp = box_region_used();
> + *  | char *buf = box_<...>(<...>);
> + *  | <...>
> + *  | box_region_truncate(region_svp);
> + *
> + * This API provides better compatibility guarantees over using
> + * the small library directly in a module. A binary layout of
> + * internal structures may be changed in a future, but
> + * <box_region_*>() functions will remain API and ABI compatible.
> + *
> + * Data allocated on the region are guaranteed to be valid until
> + * a fiber yield or a call of a function from the certain set:
> + *
> + * - Related to transactions.
> + * - Ones that may cause box initialization (box.cfg()).
> + * - Ones that may involve SQL execution.
> + *
> + * FIXME: Provide more strict list of functions, which may
> + * invalidate the data: ones that may lead to calling of
> + * fiber_gc().
> + *
> + * It is safe to call simple box APIs around tuples, key_defs and
> + * so on -- they don't invalidate the allocated data.
> + *
> + * Don't assume this region as fiber local. This is an
> + * implementation detail and may be changed in a future.
> + *
> + * Another useful property is that data allocated on this region
> + * are freed eventually, so a cold path may omit
> + * <box_region_truncate>() call: it will not lead to a memory
> + * leak. It is not recommended to exploit this property on a hot
> + * path, because of the risk to exhaust available memory for this
> + * region before it'll be freed.

1. That looks like a dangerous advice. We do truncate the region in some
places, but that may change any moment. For example, fiber_gc() in
the transactions may be replaced with
region_truncate(<size_at_transaction_start>). I would better say, that
if you allocated something, or get something allocated implicitly from
another module.h function, you shall free it. You can't rely on it be
freed automatically. If you use it in a hot path to allocate a small
number of objects, and box_region_truncate() really becomes a bottleneck,
you may need to reconsider design of your allocation policy, and use
something else but the region.

> + *
> + * @sa <fiber_gc>() (internal function)
> + */
> +
> +/** How much memory is used by the box region. */
> +API_EXPORT size_t
> +box_region_used(void);
> +
> +/** Allocate size bytes from the box region. */
> +API_EXPORT void *
> +box_region_alloc(size_t size);
> +
> +/**
> + * Allocate size bytes from the box region with given alingment.
> + */
> +API_EXPORT void *
> +box_region_aligned_alloc(size_t size, size_t alignment);
> +
> +/*
> + * Allocate storage for an object of given type with respect to
> + * its alignment.
> + *
> + * @param T    type of an object
> + * @param size where to store size of an object (size_t *)
> + */
> +#define box_region_alloc_object(T, size) ({					\
> +	*(size) = sizeof(T);							\
> +	(T *)box_region_aligned_alloc(sizeof(T), alignof(T));			\
> +})
> +
> +/*
> + * Allocate storage for array of objects of given type.
> + *
> + * @param T     type of an object
> + * @param count amount of objects to allocate (size_t)
> + * @param size  where to store size of an object (size_t *)
> + */
> +#define box_region_alloc_array(T, count, size) ({				\
> +	int _tmp_ = sizeof(T) * (count);					\
> +	*(size) = _tmp_;							\
> +	(T *)box_region_aligned_alloc(_tmp_, alignof(T));			\
> +})

2. Nah, lets better drop this sugar. If somebody wants them, they can
implement them in their own code. It is not something what needs to
be in module.h, and IMO we need to keep here only really necessary
things.

> +
> +/**
> + * Truncate the box region to the given size.
> + */
> +void
> +box_region_truncate(size_t size);
> +
>  /** \endcond public */
>  
>  /**
> 

  reply	other threads:[~2020-09-24 22:32 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23  1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region Alexander Turenko
2020-09-24 22:31   ` Vladislav Shpilevoy [this message]
2020-10-08 19:21     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko
2020-09-24 22:32   ` Vladislav Shpilevoy
2020-10-08 21:46     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 04/14] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko
2020-09-24 22:32   ` Vladislav Shpilevoy
2020-10-12 19:06     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 07/14] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-07 11:30     ` Alexander Turenko
2020-10-07 22:12       ` Vladislav Shpilevoy
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09 21:54     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09  9:33     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09  1:46     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09  1:14     ` Alexander Turenko
2020-10-10  1:21       ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() Alexander Turenko
2020-09-25 22:59   ` Vladislav Shpilevoy
2020-10-09  1:22     ` Alexander Turenko
2020-09-23  1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() Alexander Turenko
2020-09-28 21:26     ` Vladislav Shpilevoy
2020-10-05 11:58       ` Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 03/16] module api: get rid of typedef redefinitions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 04/16] WIP: module api: expose box region Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 05/16] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 06/16] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 07/16] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 09/16] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key() Alexander Turenko
2020-10-05  7:26 ` [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0f0862f5-1a3c-1d98-d2fb-8f39e76fb37a@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox