Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new()
Date: Mon, 12 Oct 2020 13:37:07 +0300	[thread overview]
Message-ID: <20201012103707.kzx54ljqq67eazrc@tkn_work_nb> (raw)
In-Reply-To: <33039bab-b31e-0da7-359d-dbbc8464aa55@tarantool.org>

> > Disallow creating a tuple from objects on the Lua stack (idx == 0) in
> > luaT_tuple_new() for simplicity. There are no such usages in tarantool.
> > The function is not exposed yet to the module API. This is only
> > necessary in box.tuple.new(), which anyway raises Lua errors by its
> > contract.
> 
> 1. But why? The case of creating a tuple from values may be much faster,
> when there is a lot of values not wrapped into a table. Table wrap is
> costly.
> 
> Could you just merge luaT_tuple_encode_values and luaT_tuple_encode_table
> into one function, withuout splitting them?

I started with this variant, but then found that it'll require copying
of all arguments before pcall() (at least if we must leave them on the
stack after exiting from the function). Even if we'll decide to include
a remark like 'the function pops all values in case of idx == 0', we'll
need to put a function before the arguments and so we'll move all stack
values. Anyway it looks lopsided: in one case arguments are popped, but
in another they are kept on the stack.

I guess it would have a chance to be useful if it would allow to pass a
range of lua stack indices. But not sure.

If we'll find it actually useful, nothing prevent us from exposing
luaT_tuple_field_encode() (to control what to encode from a Lua stack in
a user side code). Or consider some other way (even support idx == 0 in
luaT_tuple_encode()): a real use case will help us with designing good
API.

> > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> > index ed97c85e4..18cfef979 100644
> > --- a/src/box/lua/tuple.c
> > +++ b/src/box/lua/tuple.c
> > @@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
> >  
> >  uint32_t CTID_STRUCT_TUPLE_REF;
> >  
> > +int luaT_tuple_encode_table_ref = LUA_NOREF;
> 
> 2. Better make it static. It is not used outside of this file.

Thanks!

> >  static char *
> >  luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
> >  			      size_t *tuple_len_ptr)
> >  {
> > -	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
> > +	assert(idx != 0);
> > +	if (!lua_istable(L, idx) && !luaT_istuple(L, idx)) {
> >  		diag_set(IllegalParams, "A tuple or a table expected, got %s",
> >  			 lua_typename(L, lua_type(L, idx)));
> >  		return NULL;
> >  	}
> >  
> > -	struct ibuf *buf = tarantool_lua_ibuf;
> > -	ibuf_reset(buf);
> > -	struct mpstream stream;
> > -	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
> > -		      luamp_error, L);
> > -	if (idx == 0) {
> > -		/*
> > -		 * Create the tuple from lua stack
> > -		 * objects.
> > -		 */
> > -		int argc = lua_gettop(L);
> > -		mpstream_encode_array(&stream, argc);
> > -		for (int k = 1; k <= argc; ++k) {
> > -			luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
> > -		}
> > -	} else {
> > -		/* Create the tuple from a Lua table. */
> > -		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
> > -	}
> > -	mpstream_flush(&stream);
> > +	int top = lua_gettop(L);
> > +
> > +	/* Calculate absolute value in the stack. */
> > +	if (idx < 0)
> > +		idx = top + idx + 1;> +
> > +	assert(luaT_tuple_encode_table_ref != LUA_NOREF);
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
> > +	assert(lua_isfunction(L, -1));
> > +
> > +	lua_pushvalue(L, idx);
> > +
> > +	int rc = luaT_call(L, 1, 0);
> > +	lua_settop(L, top);
> > +	if (rc != 0)
> > +		return NULL;
> > +
> >  	if (tuple_len_ptr != NULL)
> > -		*tuple_len_ptr = ibuf_used(buf);
> > -	return buf->buf;
> > +		*tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
> 
> 3. Since you assume the function at luaT_tuple_encode_table_ref alwaus
> uses ibuf, maybe it would be better to add 'on_lua_ibuf' to its name.

The next commit passes mpstream initialization function to it and I want
to avoid double renaming to make diffs more readable.

> Or move the length calculation out of the current function, because
> this one already has 'on_lua_ibuf', so it wouldn't look so strange to
> get size of tarantool_lua_ibuf after it.

Don't get a purpose of the proposed changes.

Anyway, for defense of given variant, the next commit adds
luaT_tuple_encode() with quite similar behaviour and signature and it
looks nice, when those functions are virtually same, but based on top of
different allocators.

> > @@ -156,15 +198,30 @@ lbox_tuple_new(lua_State *L)
> >  		lua_newtable(L); /* create an empty tuple */
> >  		++argc;
> >  	}
> > +
> > +	box_tuple_format_t *fmt = box_tuple_format_default();
> > +
> 
> 4. You could keep it after the comment below to reduce the diff.

Heh. Good spot. I'll do.

> >  	/*
> >  	 * Use backward-compatible parameters format:
> > -	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
> > -	 * box.tuple.new({1, 2, 3}) (idx == 1).
> > +	 * box.tuple.new(1, 2, 3).
> >  	 */
> > -	int idx = argc == 1 && (lua_istable(L, 1) ||
> > -		luaT_istuple(L, 1));
> > -	box_tuple_format_t *fmt = box_tuple_format_default();
> > -	struct tuple *tuple = luaT_tuple_new(L, idx, fmt);
> > +	if (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
> > +		struct ibuf *buf = tarantool_lua_ibuf;
> > +		luaT_tuple_encode_values(L); /* may raise */
> 
> 5. We usually put comments on separate line.

 | /* May raise. */
 | luaT_tuple_encode_values(L);

...would make unclear whether it applies to just next call or several
ones. We can wrap the call and the comment with empty lines above and
below, but it is too much for such small thing. Or we can explicitly
point to the call in the comment:

 | /* The next call may raise. */
 | luaT_tuple_encode_values(L);

However such comment sometimes displaced by inaccurate commits and
befomes useless. So we should mention the function name explicitly, but
it, in turn, looks too tautological.

I'm aware about the convention, but I still think that one-two word
remarks are complelety okay to be placed inline. I don't exploit this
much.

I'll change it somehow, if it disturbs (and you'll highlight it), but
personally I think it is okay.

  reply	other threads:[~2020-10-12 10:36 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-11 12:57 [Tarantool-patches] [PATCH v2 00/15] RFC: module api: extend for external key_def Lua module Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 01/15] module api: get rid of typedef redefinitions Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 02/15] module api: expose box region Alexander Turenko
2020-10-11 15:26   ` Vladislav Shpilevoy
2020-10-12  6:07     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 03/15] module api/lua: add luaL_iscdata() function Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 04/15] lua: factor out tuple encoding from luaT_tuple_new Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 05/15] lua: don't raise a Lua error from luaT_tuple_new() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12 10:37     ` Alexander Turenko [this message]
2020-10-12 13:34       ` Timur Safin
2020-10-14 23:41       ` Vladislav Shpilevoy
2020-10-15 19:43         ` Alexander Turenko
2020-10-15 22:10           ` Vladislav Shpilevoy
2020-10-11 17:47   ` Igor Munkin
2020-10-11 18:08     ` Igor Munkin
2020-10-12 10:37     ` Alexander Turenko
2020-10-12 10:51       ` Igor Munkin
2020-10-12 18:41         ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 06/15] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12 10:35     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 07/15] module api/lua: expose luaT_tuple_new() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12  6:11     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 08/15] module api/lua: add API_EXPORT to tuple functions Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 09/15] module api: add API_EXPORT to key_def functions Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 10/15] module api: add box_key_def_new_v2() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12  7:21     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 11/15] module api: add box_key_def_dump_parts() Alexander Turenko
2020-10-11 15:25   ` Vladislav Shpilevoy
2020-10-12  6:50     ` Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 12/15] module api: expose box_key_def_validate_tuple() Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 13/15] WIP: module api: expose box_key_def_merge() Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 14/15] WIP: module api: expose box_key_def_extract_key() Alexander Turenko
2020-10-11 12:57 ` [Tarantool-patches] [PATCH v2 15/15] WIP: module api: add box_key_def_validate_key() 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=20201012103707.kzx54ljqq67eazrc@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 05/15] lua: don'\''t raise a Lua error from luaT_tuple_new()' \
    /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