[Tarantool-patches] [PATCH v2 0/3] box_return_mp

[Tarantool-patches] [PATCH v2 0/3] box_return_mp

[Vladislav Shpilevoy]

The patchset extends the public C API with a function
box_return_mp(). It allows to return arbitrary MessagePack from
user's code.

First patch introduces a new struct port - port_c. It replaces
port_tuple for stored C functions, and is able to store both
tuples and raw MessagePack.

Second patch introduces the new public function.

Third patch removes no longer needed struct port_tuple, since it
appeared to be not faster than struct port_c.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4641-c-function-ret-mp
Issue: https://github.com/tarantool/tarantool/issues/4641

Changes in v2:
- Review fixes for comments from Nikita and Igor;
- port_tuple is removed.

Vladislav Shpilevoy (3):
  box: introduce port_c
  box: introduce box_return_mp() public C function
  box: replace port_tuple with port_c everywhere

 extra/exports               |   1 +
 src/box/box.cc              |  12 ++-
 src/box/box.h               |  19 ++++
 src/box/execute.c           |  10 +-
 src/box/execute.h           |  13 ++-
 src/box/func.c              |   2 +-
 src/box/key_list.c          |   4 +-
 src/box/lua/execute.c       |   6 +-
 src/box/lua/misc.cc         |  18 ++--
 src/box/lua/schema.lua      |  29 +++---
 src/box/port.c              | 201 +++++++++++++++++++++++++-----------
 src/box/port.h              |  91 ++++++++--------
 src/box/sql/func.c          |  22 ++--
 src/lib/core/port.h         |   4 +-
 test/box/function1.c        |  37 +++++++
 test/box/function1.result   |  33 ++++++
 test/box/function1.test.lua |  16 +++
 17 files changed, 367 insertions(+), 151 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 1/3] box: introduce port_c

[Vladislav Shpilevoy]

Port_c is a new descendant of struct port. It is used now for
public C functions to store their result. Currently they can
return only a tuple, but it will change soon, they will be able to
return arbitrary MessagePack.

Port_tuple is not removed, because still is used for box_select(),
for functional indexes, and in SQL as a base for port_sql.
Although that may be changed later. Functional indexes really need
only a single MessagePack object from their function. While
box_select() working via port_tuple or port_c didn't show any
significant difference during micro benchmarks.

Part of #4641
---
 src/box/box.cc      |   2 +-
 src/box/func.c      |   2 +-
 src/box/lua/misc.cc |  20 +++++
 src/box/port.c      | 206 +++++++++++++++++++++++++++++++++++++++++---
 src/box/port.h      |  50 +++++++++++
 src/box/sql/func.c  |  22 +++--
 6 files changed, 281 insertions(+), 21 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index f4541b577..12f3a50a7 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1072,7 +1072,7 @@ boxk(int type, uint32_t space_id, const char *format, ...)
 int
 box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple)
 {
-	return port_tuple_add(ctx->port, tuple);
+	return port_c_add_tuple(ctx->port, tuple);
 }
 
 /* schema_find_id()-like method using only public API */
diff --git a/src/box/func.c b/src/box/func.c
index 431577127..04b04b6ad 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -523,7 +523,7 @@ func_c_call(struct func *base, struct port *args, struct port *ret)
 	if (data == NULL)
 		return -1;
 
-	port_tuple_create(ret);
+	port_c_create(ret);
 	box_function_ctx_t ctx = { ret };
 
 	/* Module can be changed after function reload. */
diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index 4348dcbb2..55677ed61 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -78,6 +78,26 @@ port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
 	}
 }
 
+extern "C" void
+port_c_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
+{
+	struct port_c *port = (struct port_c *)base;
+	if (!is_flat)
+		lua_createtable(L, port->size, 0);
+	struct port_c_entry *pe = port->first;
+	const char *mp;
+	for (int i = 0; pe != NULL; pe = pe->next) {
+		if (pe->mp_size == 0) {
+			luaT_pushtuple(L, pe->tuple);
+		} else {
+			mp = pe->mp;
+			luamp_decode(L, luaL_msgpack_default, &mp);
+		}
+		if (!is_flat)
+			lua_rawseti(L, -2, ++i);
+	}
+}
+
 extern "C" void
 port_msgpack_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
 {
diff --git a/src/box/port.c b/src/box/port.c
index 6e2fe3a6e..2c1fadb5c 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -37,7 +37,18 @@
 #include <fiber.h>
 #include "errinj.h"
 
-static struct mempool port_tuple_entry_pool;
+/**
+ * The pools is used both by port_c and port_tuple, since their
+ * entires are almost of the same size. Also port_c can use
+ * objects from the pool to store result data in their memory,
+ * when it fits.
+ */
+static struct mempool port_entry_pool;
+
+enum {
+	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
+			      sizeof(struct port_tuple_entry)),
+};
 
 int
 port_tuple_add(struct port *base, struct tuple *tuple)
@@ -49,7 +60,7 @@ port_tuple_add(struct port *base, struct tuple *tuple)
 		e = &port->first_entry;
 		port->first = port->last = e;
 	} else {
-		e = mempool_alloc(&port_tuple_entry_pool);
+		e = mempool_alloc(&port_entry_pool);
 		if (e == NULL) {
 			diag_set(OutOfMemory, sizeof(*e), "mempool_alloc", "e");
 			return -1;
@@ -87,7 +98,7 @@ port_tuple_destroy(struct port *base)
 		struct port_tuple_entry *cur = e;
 		e = e->next;
 		tuple_unref(cur->tuple);
-		mempool_free(&port_tuple_entry_pool, cur);
+		mempool_free(&port_entry_pool, cur);
 	}
 }
 
@@ -127,28 +138,203 @@ port_tuple_dump_msgpack(struct port *base, struct obuf *out)
 extern void
 port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat);
 
+static inline void
+port_c_destroy_entry(struct port_c_entry *pe)
+{
+	/*
+	 * See port_c_add_*() for algorithm of how and where to
+	 * store data, to understand why it is freed differently.
+	 */
+	if (pe->mp_size == 0)
+		tuple_unref(pe->tuple);
+	else if (pe->mp_size <= PORT_ENTRY_SIZE)
+		mempool_free(&port_entry_pool, pe->mp);
+	else
+		free(pe->mp);
+}
+
+static void
+port_c_destroy(struct port *base)
+{
+	struct port_c *port = (struct port_c *)base;
+	struct port_c_entry *pe = port->first;
+	if (pe == NULL)
+		return;
+	port_c_destroy_entry(pe);
+	/*
+	 * Port->first is skipped, it is pointing at
+	 * port_c.first_entry, and is freed together with the
+	 * port.
+	 */
+	pe = pe->next;
+	while (pe != NULL) {
+		struct port_c_entry *cur = pe;
+		pe = pe->next;
+		port_c_destroy_entry(cur);
+		mempool_free(&port_entry_pool, cur);
+	}
+}
+
+static inline struct port_c_entry *
+port_c_new_entry(struct port_c *port)
+{
+	struct port_c_entry *e;
+	if (port->size == 0) {
+		e = &port->first_entry;
+		port->first = e;
+		port->last = e;
+	} else {
+		e = mempool_alloc(&port_entry_pool);
+		if (e == NULL) {
+			diag_set(OutOfMemory, sizeof(*e), "mempool_alloc", "e");
+			return NULL;
+		}
+		port->last->next = e;
+		port->last = e;
+	}
+	e->next = NULL;
+	++port->size;
+	return e;
+}
+
+int
+port_c_add_tuple(struct port *base, struct tuple *tuple)
+{
+	struct port_c *port = (struct port_c *)base;
+	struct port_c_entry *pe = port_c_new_entry(port);
+	if (pe == NULL)
+		return -1;
+	/* 0 mp_size means the entry stores a tuple. */
+	pe->mp_size = 0;
+	pe->tuple = tuple;
+	tuple_ref(tuple);
+	return 0;
+}
+
+int
+port_c_add_mp(struct port *base, const char *mp, const char *mp_end)
+{
+	struct port_c *port = (struct port_c *)base;
+	struct port_c_entry *pe;
+	assert(mp_end > mp);
+	uint32_t size = mp_end - mp;
+	char *dst;
+	if (size <= PORT_ENTRY_SIZE) {
+		/*
+		 * Alloc on a mempool is several times faster than
+		 * on the heap. And it perfectly fits any
+		 * MessagePack number, a short string, a boolean.
+		 */
+		dst = mempool_alloc(&port_entry_pool);
+		if (dst == NULL) {
+			diag_set(OutOfMemory, size, "mempool_alloc", "dst");
+			return -1;
+		}
+	} else {
+		dst = malloc(size);
+		if (dst == NULL) {
+			diag_set(OutOfMemory, size, "malloc", "dst");
+			return -1;
+		}
+	}
+	pe = port_c_new_entry(port);
+	if (pe != NULL) {
+		memcpy(dst, mp, size);
+		pe->mp = dst;
+		pe->mp_size = size;
+		return 0;
+	}
+	if (size <= PORT_ENTRY_SIZE)
+		mempool_free(&port_entry_pool, dst);
+	else
+		free(dst);
+	return -1;
+}
+
+static int
+port_c_dump_msgpack_16(struct port *base, struct obuf *out)
+{
+	struct port_c *port = (struct port_c *)base;
+	struct port_c_entry *pe;
+	for (pe = port->first; pe != NULL; pe = pe->next) {
+		uint32_t size = pe->mp_size;
+		if (size == 0) {
+			if (tuple_to_obuf(pe->tuple, out) != 0)
+				return -1;
+		} else if (obuf_dup(out, pe->mp, size) != size) {
+			diag_set(OutOfMemory, size, "obuf_dup", "data");
+			return -1;
+		}
+		ERROR_INJECT(ERRINJ_PORT_DUMP, {
+			diag_set(OutOfMemory,
+				 size == 0 ? tuple_size(pe->tuple) : size,
+				 "obuf_dup", "data");
+			return -1;
+		});
+	}
+	return port->size;
+}
+
+static int
+port_c_dump_msgpack(struct port *base, struct obuf *out)
+{
+	struct port_c *port = (struct port_c *)base;
+	char *size_buf = obuf_alloc(out, mp_sizeof_array(port->size));
+	if (size_buf == NULL) {
+		diag_set(OutOfMemory, mp_sizeof_array(port->size), "obuf_alloc",
+			 "size_buf");
+		return -1;
+	}
+	mp_encode_array(size_buf, port->size);
+	if (port_c_dump_msgpack_16(base, out) < 0)
+		return -1;
+	return 1;
+}
+
+extern void
+port_c_dump_lua(struct port *port, struct lua_State *L, bool is_flat);
+
+extern struct sql_value *
+port_c_get_vdbemem(struct port *base, uint32_t *size);
+
+static const struct port_vtab port_c_vtab = {
+	.dump_msgpack = port_c_dump_msgpack,
+	.dump_msgpack_16 = port_c_dump_msgpack_16,
+	.dump_lua = port_c_dump_lua,
+	.dump_plain = NULL,
+	.get_msgpack = NULL,
+	.get_vdbemem = port_c_get_vdbemem,
+	.destroy = port_c_destroy,
+};
+
+void
+port_c_create(struct port *base)
+{
+	struct port_c *port = (struct port_c *)base;
+	port->vtab = &port_c_vtab;
+	port->first = NULL;
+	port->last = NULL;
+	port->size = 0;
+}
+
 void
 port_init(void)
 {
-	mempool_create(&port_tuple_entry_pool, &cord()->slabc,
-		       sizeof(struct port_tuple_entry));
+	mempool_create(&port_entry_pool, &cord()->slabc, PORT_ENTRY_SIZE);
 }
 
 void
 port_free(void)
 {
-	mempool_destroy(&port_tuple_entry_pool);
+	mempool_destroy(&port_entry_pool);
 }
 
-extern struct sql_value *
-port_tuple_get_vdbemem(struct port *base, uint32_t *size);
-
 const struct port_vtab port_tuple_vtab = {
 	.dump_msgpack = port_tuple_dump_msgpack,
 	.dump_msgpack_16 = port_tuple_dump_msgpack_16,
 	.dump_lua = port_tuple_dump_lua,
 	.dump_plain = NULL,
 	.get_msgpack = NULL,
-	.get_vdbemem = port_tuple_get_vdbemem,
+	.get_vdbemem = NULL,
 	.destroy = port_tuple_destroy,
 };
diff --git a/src/box/port.h b/src/box/port.h
index fa6c1374d..4d6a5b399 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -145,6 +145,56 @@ void
 port_vdbemem_create(struct port *base, struct sql_value *mem,
 		    uint32_t mem_count);
 
+/**
+ * The struct is PACKED to avoid unnecessary 4 byte padding
+ * after mp_size. These objects are never stored on stack, neither
+ * allocated as an array, so the padding is not needed.
+ */
+struct PACKED port_c_entry {
+	struct port_c_entry *next;
+	union {
+		/** Valid if mp_size is 0. */
+		struct tuple *tuple;
+		/**
+		 * Valid if mp_size is > 0. MessagePack is
+		 * allocated either on heap or on the port entry
+		 * mempool, if it fits into a pool object.
+		 */
+		char *mp;
+	};
+	uint32_t mp_size;
+};
+
+static_assert(sizeof(struct port_c_entry) == 20,
+	      "port_c_entry is expected to be perfectly aligned and packed");
+
+/**
+ * C port is used by C functions from the public API. They can
+ * return tuples and arbitrary MessagePack.
+ */
+struct port_c {
+	const struct port_vtab *vtab;
+	struct port_c_entry *first;
+	struct port_c_entry *last;
+	struct port_c_entry first_entry;
+	int size;
+};
+
+static_assert(sizeof(struct port_c) <= sizeof(struct port),
+	      "sizeof(struct port_c) must be <= sizeof(struct port)");
+
+/** Create a C port object. */
+void
+port_c_create(struct port *base);
+
+/** Append a tuple to the port. Tuple is referenced. */
+int
+port_c_add_tuple(struct port *port, struct tuple *tuple);
+
+/** Append raw MessagePack to the port. It is copied. */
+int
+port_c_add_mp(struct port *port, const char *mp, const char *mp_end);
+
 void
 port_init(void);
 
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 376837217..2c510940b 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -301,9 +301,9 @@ error:
 }
 
 struct sql_value *
-port_tuple_get_vdbemem(struct port *base, uint32_t *size)
+port_c_get_vdbemem(struct port *base, uint32_t *size)
 {
-	struct port_tuple *port = (struct port_tuple *)base;
+	struct port_c *port = (struct port_c *)base;
 	*size = port->size;
 	if (*size == 0 || *size > 1) {
 		diag_set(ClientError, ER_SQL_FUNC_WRONG_RET_COUNT, "C", *size);
@@ -317,14 +317,18 @@ port_tuple_get_vdbemem(struct port *base, uint32_t *size)
 	if (val == NULL)
 		return NULL;
 	int i = 0;
-	struct port_tuple_entry *pe;
+	const char *data;
+	struct port_c_entry *pe;
 	for (pe = port->first; pe != NULL; pe = pe->next) {
-		const char *data = tuple_data(pe->tuple);
-		if (mp_typeof(*data) != MP_ARRAY ||
-			mp_decode_array(&data) != 1) {
-			diag_set(ClientError, ER_SQL_EXECUTE,
-				 "Unsupported type passed from C");
-			goto error;
+		if (pe->mp_size == 0) {
+			data = tuple_data(pe->tuple);
+			if (mp_decode_array(&data) != 1) {
+				diag_set(ClientError, ER_SQL_EXECUTE,
+					 "Unsupported type passed from C");
+				goto error;
+			}
+		} else {
+			data = pe->mp;
 		}
 		uint32_t len;
 		const char *str;
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 2/3] box: introduce box_return_mp() public C function

[Vladislav Shpilevoy]

Closes #4641

@TarantoolBot document
Title: box_return_mp() public C function

Stored C functions could return a result only via
`box_return_tuple()` function. That made users create a tuple
every time they wanted to return something from a C function.

Now public C API offers another way to return - `box_return_mp()`.
It allows to return arbitrary MessagePack, not wrapped into a
tuple object. This is simpler to use for small results like a
number, boolean, or a short string. Besides, `box_return_mp()` is
much faster than `box_return_tuple()`, especially for small
MessagePack.

Note, that it is faster only if an alternative is to create a
tuple by yourself. If an already existing tuple was obtained from
an iterator, and you want to return it, then of course it is
faster to return via `box_return_tuple()`, than via extraction of
tuple data, and calling `box_return_mp()`.

Here is the function declaration from module.h:
```C
/**
 * Return MessagePack from a stored C procedure. The MessagePack
 * is copied, so it is safe to free/reuse the passed arguments
 * after the call.
 * MessagePack is not validated, for the sake of speed. It is
 * expected to be a single encoded object. An attempt to encode
 * and return multiple objects without wrapping them into an
 * MP_ARRAY or MP_MAP is undefined behaviour.
 *
 * \param ctx An opaque structure passed to the stored C procedure
 *        by Tarantool.
 * \param mp Begin of MessagePack.
 * \param mp_end End of MessagePack.
 * \retval -1 Error.
 * \retval 0 Success.
 */
API_EXPORT int
box_return_mp(box_function_ctx_t *ctx, const char *mp, const char *mp_end);
```
---
 extra/exports               |  1 +
 src/box/box.cc              |  6 ++++++
 src/box/box.h               | 19 +++++++++++++++++++
 test/box/function1.c        | 37 +++++++++++++++++++++++++++++++++++++
 test/box/function1.result   | 33 +++++++++++++++++++++++++++++++++
 test/box/function1.test.lua | 16 ++++++++++++++++
 6 files changed, 112 insertions(+)

diff --git a/extra/exports b/extra/exports
index d6b02e714..ec80a78a1 100644
--- a/extra/exports
+++ b/extra/exports
@@ -210,6 +210,7 @@ box_tuple_extract_key
 box_tuple_compare
 box_tuple_compare_with_key
 box_return_tuple
+box_return_mp
 box_space_id_by_name
 box_index_id_by_name
 box_select
diff --git a/src/box/box.cc b/src/box/box.cc
index 12f3a50a7..9fd10d64c 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1075,6 +1075,12 @@ box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple)
 	return port_c_add_tuple(ctx->port, tuple);
 }
 
+int
+box_return_mp(box_function_ctx_t *ctx, const char *mp, const char *mp_end)
+{
+	return port_c_add_mp(ctx->port, mp, mp_end);
+}
+
 /* schema_find_id()-like method using only public API */
 uint32_t
 box_space_id_by_name(const char *name, uint32_t len)
diff --git a/src/box/box.h b/src/box/box.h
index c94e500ab..557542a83 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -284,6 +284,25 @@ typedef struct box_function_ctx box_function_ctx_t;
 API_EXPORT int
 box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple);
 
+/**
+ * Return MessagePack from a stored C procedure. The MessagePack
+ * is copied, so it is safe to free/reuse the passed arguments
+ * after the call.
+ * MessagePack is not validated, for the sake of speed. It is
+ * expected to be a single encoded object. An attempt to encode
+ * and return multiple objects without wrapping them into an
+ * MP_ARRAY or MP_MAP is undefined behaviour.
+ *
+ * \param ctx An opaque structure passed to the stored C procedure
+ *        by Tarantool.
+ * \param mp Begin of MessagePack.
+ * \param mp_end End of MessagePack.
+ * \retval -1 Error.
+ * \retval 0 Success.
+ */
+API_EXPORT int
+box_return_mp(box_function_ctx_t *ctx, const char *mp, const char *mp_end);
+
 /**
  * Find space id by name.
  *
diff --git a/test/box/function1.c b/test/box/function1.c
index a28431e86..84bbdd710 100644
--- a/test/box/function1.c
+++ b/test/box/function1.c
@@ -252,3 +252,40 @@ test_push(box_function_ctx_t *ctx, const char *args, const char *args_end)
 	(void)ctx;
 	return box_session_push(args, args_end);
 }
+
+int
+test_return_mp(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void) args;
+	(void) args_end;
+	char buf[512];
+	char *pos = mp_encode_uint(buf, 1);
+	int rc = box_return_mp(ctx, buf, pos);
+	if (rc != 0)
+		return rc;
+
+	pos = mp_encode_int(buf, -1);
+	rc = box_return_mp(ctx, buf, pos);
+	if (rc != 0)
+		return rc;
+
+	pos = mp_encode_uint(buf, UINT64_MAX);
+	rc = box_return_mp(ctx, buf, pos);
+	if (rc != 0)
+		return rc;
+
+	const char *str = "123456789101112131415";
+	pos = mp_encode_str(buf, str, strlen(str));
+	rc = box_return_mp(ctx, buf, pos);
+	if (rc != 0)
+		return rc;
+
+	pos = mp_encode_array(buf, 1);
+	pos = mp_encode_uint(pos, 2);
+	box_tuple_t *tuple = box_tuple_new(box_tuple_format_default(),
+					   buf, pos);
+	if (tuple == NULL)
+		return -1;
+	rc = box_return_tuple(ctx, tuple);
+	return rc;
+}
diff --git a/test/box/function1.result b/test/box/function1.result
index b91d63c51..905a4cdab 100644
--- a/test/box/function1.result
+++ b/test/box/function1.result
@@ -791,6 +791,39 @@ box.schema.func.drop("function1.multireturn")
 ---
 ...
 --
+-- gh-4641: box_return_mp() C API to return arbitrary MessagePack
+-- from C functions.
+--
+name = 'function1.test_return_mp'
+---
+...
+box.schema.func.create(name, {language = "C", exports = {'LUA'}})
+---
+...
+box.func[name]:call()
+---
+- 1
+- -1
+- 18446744073709551615
+- '123456789101112131415'
+- [2]
+...
+box.schema.user.grant('guest', 'super')
+---
+...
+-- Netbox:call() returns not the same as local call for C
+-- functions, see #4799.
+net:connect(box.cfg.listen):call(name)
+---
+- [1, -1, 18446744073709551615, '123456789101112131415', [2]]
+...
+box.schema.user.revoke('guest', 'super')
+---
+...
+box.schema.func.drop(name)
+---
+...
+--
 -- gh-4182: Introduce persistent Lua functions.
 --
 test_run:cmd("setopt delimiter ';'")
diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
index b1841f3ad..ab7b586a0 100644
--- a/test/box/function1.test.lua
+++ b/test/box/function1.test.lua
@@ -268,6 +268,22 @@ box.schema.func.create('function1.multireturn', {language = "C", exports = {'LUA
 box.execute("SELECT \"function1.multireturn\"()")
 box.schema.func.drop("function1.multireturn")
 
+--
+-- gh-4641: box_return_mp() C API to return arbitrary MessagePack
+-- from C functions.
+--
+name = 'function1.test_return_mp'
+box.schema.func.create(name, {language = "C", exports = {'LUA'}})
+box.func[name]:call()
+
+box.schema.user.grant('guest', 'super')
+-- Netbox:call() returns not the same as local call for C
+-- functions, see #4799.
+net:connect(box.cfg.listen):call(name)
+box.schema.user.revoke('guest', 'super')
+
+box.schema.func.drop(name)
+
 --
 -- gh-4182: Introduce persistent Lua functions.
 --
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Vladislav Shpilevoy]

Port_tuple is exactly the same as port_c, but is not able to store
raw MessagePack. I theory it sounds like port_tuple should be a
bit simpler and therefore faster, but in fact it is not.
Microbenchmarks didn't reveal any difference. So port_tuple is no
longer needed, all its functionality is covered by port_c.

Follow up #4641
---
 src/box/box.cc         |   4 +-
 src/box/execute.c      |  10 ++--
 src/box/execute.h      |  13 +++--
 src/box/key_list.c     |   4 +-
 src/box/lua/execute.c  |   6 +--
 src/box/lua/misc.cc    |  18 -------
 src/box/lua/schema.lua |  29 ++++++-----
 src/box/port.c         | 109 ++---------------------------------------
 src/box/port.h         |  41 ----------------
 src/lib/core/port.h    |   4 +-
 10 files changed, 41 insertions(+), 197 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 9fd10d64c..c01b08613 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1199,7 +1199,7 @@ box_select(uint32_t space_id, uint32_t index_id,
 	int rc = 0;
 	uint32_t found = 0;
 	struct tuple *tuple;
-	port_tuple_create(port);
+	port_c_create(port);
 	while (found < limit) {
 		rc = iterator_next(it, &tuple);
 		if (rc != 0 || tuple == NULL)
@@ -1208,7 +1208,7 @@ box_select(uint32_t space_id, uint32_t index_id,
 			offset--;
 			continue;
 		}
-		rc = port_tuple_add(port, tuple);
+		rc = port_c_add_tuple(port, tuple);
 		if (rc != 0)
 			break;
 		found++;
diff --git a/src/box/execute.c b/src/box/execute.c
index 66eac9814..6feb88036 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -103,7 +103,7 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out);
 static void
 port_sql_destroy(struct port *base)
 {
-	port_tuple_vtab.destroy(base);
+	port_c_vtab.destroy(base);
 	struct port_sql *port_sql = (struct port_sql *) base;
 	if (port_sql->do_finalize)
 		sql_stmt_finalize(((struct port_sql *)base)->stmt);
@@ -123,7 +123,7 @@ static void
 port_sql_create(struct port *port, struct sql_stmt *stmt,
 		enum sql_serialization_format format, bool do_finalize)
 {
-	port_tuple_create(port);
+	port_c_create(port);
 	port->vtab = &port_sql_vtab;
 	struct port_sql *port_sql = (struct port_sql *) port;
 	port_sql->stmt = stmt;
@@ -271,7 +271,7 @@ sql_row_to_port(struct sql_stmt *stmt, int column_count,
 	if (tuple == NULL)
 		goto error;
 	region_truncate(region, svp);
-	return port_tuple_add(port, tuple);
+	return port_c_add_tuple(port, tuple);
 
 error:
 	region_truncate(region, svp);
@@ -492,13 +492,13 @@ port_sql_dump_msgpack(struct port *port, struct obuf *out)
 			return -1;
 		}
 		pos = mp_encode_uint(pos, IPROTO_DATA);
-		if (port_tuple_vtab.dump_msgpack(port, out) < 0)
+		if (port_c_vtab.dump_msgpack(port, out) < 0)
 			return -1;
 		break;
 	}
 	case DML_EXECUTE: {
 		int keys = 1;
-		assert(((struct port_tuple *)port)->size == 0);
+		assert(((struct port_c *)port)->size == 0);
 		struct stailq *autoinc_id_list =
 			vdbe_autoinc_id_list((struct Vdbe *)stmt);
 		uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
diff --git a/src/box/execute.h b/src/box/execute.h
index 5e2327f4a..c08cfbf25 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -91,17 +91,16 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
 /**
  * Port implementation that is used to store SQL responses and
  * output them to obuf or Lua. This port implementation is
- * inherited from the port_tuple structure. This allows us to use
- * this structure in the port_tuple methods instead of port_tuple
- * itself.
+ * inherited from the port_c structure. This allows us to use
+ * this structure in the port_c methods instead of port_c itself.
  *
- * The methods of port_tuple are called via explicit access to
- * port_tuple_vtab just like C++ does with BaseClass::method, when
+ * The methods of port_c are called via explicit access to
+ * port_c_vtab just like C++ does with BaseClass::method, when
  * it is called in a child method.
  */
 struct port_sql {
-	/* port_tuple to inherit from. */
-	struct port_tuple port_tuple;
+	/** Base port struct. To be able to use port_c methods. */
+	struct port_c base;
 	/* Prepared SQL statement. */
 	struct sql_stmt *stmt;
 	/**
diff --git a/src/box/key_list.c b/src/box/key_list.c
index 7c1fa70e3..6143b84a1 100644
--- a/src/box/key_list.c
+++ b/src/box/key_list.c
@@ -56,8 +56,8 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	struct func *func = index_def->key_def->func_index_func;
 
 	struct port out_port, in_port;
-	port_tuple_create(&in_port);
-	port_tuple_add(&in_port, tuple);
+	port_c_create(&in_port);
+	port_c_add_tuple(&in_port, tuple);
 	int rc = func_call(func, &in_port, &out_port);
 	port_destroy(&in_port);
 	if (rc != 0) {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index b5db3c9a2..b4c464af7 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -175,12 +175,12 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 		lua_createtable(L, 0, 2);
 		lua_sql_get_metadata(stmt, L, sql_column_count(stmt));
 		lua_setfield(L, -2, "metadata");
-		port_tuple_vtab.dump_lua(port, L, false);
+		port_c_vtab.dump_lua(port, L, false);
 		lua_setfield(L, -2, "rows");
 		break;
 	}
 	case DML_EXECUTE: {
-		assert(((struct port_tuple *) port)->size == 0);
+		assert(((struct port_c *) port)->size == 0);
 		struct stailq *autoinc_id_list =
 			vdbe_autoinc_id_list((struct Vdbe *) stmt);
 		lua_createtable(L, 0, stailq_empty(autoinc_id_list) ? 1 : 2);
@@ -237,7 +237,7 @@ port_sql_dump_lua(struct port *port, struct lua_State *L, bool is_flat)
 		break;
 	}
 	case DML_PREPARE : {
-		assert(((struct port_tuple *) port)->size == 0);
+		assert(((struct port_c *) port)->size == 0);
 		/* Format is following:
 		 * stmt_id,
 		 * param_count,
diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index 55677ed61..ae8fbb682 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -60,24 +60,6 @@ lbox_encode_tuple_on_gc(lua_State *L, int idx, size_t *p_len)
 	return (char *) region_join_xc(gc, *p_len);
 }
 
-/**
- * Dump port_tuple content to Lua as a table. Used in box/port.c,
- * but implemented here to eliminate port.c dependency on Lua.
- */
-extern "C" void
-port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
-{
-	struct port_tuple *port = port_tuple(base);
-	if (!is_flat)
-		lua_createtable(L, port->size, 0);
-	struct port_tuple_entry *pe = port->first;
-	for (int i = 0; pe != NULL; pe = pe->next) {
-		luaT_pushtuple(L, pe->tuple);
-		if (!is_flat)
-			lua_rawseti(L, -2, ++i);
-	}
-}
-
 extern "C" void
 port_c_dump_lua(struct port *base, struct lua_State *L, bool is_flat)
 {
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 10584494b..fd1628930 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -79,17 +79,21 @@ ffi.cdef[[
     box_txn_savepoint_t *
     box_txn_savepoint();
 
-    struct port_tuple_entry {
-        struct port_tuple_entry *next;
-        struct tuple *tuple;
+    struct __attribute__((packed)) port_c_entry {
+        struct port_c_entry *next;
+        union {
+            struct tuple *tuple;
+            char *mp;
+        };
+        uint32_t mp_size;
     };
 
-    struct port_tuple {
+    struct port_c {
         const struct port_vtab *vtab;
-        int size;
-        struct port_tuple_entry *first;
-        struct port_tuple_entry *last;
-        struct port_tuple_entry first_entry;
+        struct port_c_entry *first;
+        struct port_c_entry *last;
+        struct port_c_entry first_entry;
+        int count;
     };
 
     void
@@ -1278,8 +1282,7 @@ local iterator_gen_luac = function(param, state)
 end
 
 -- global struct port instance to use by select()/get()
-local port_tuple = ffi.new('struct port_tuple')
-local port_tuple_entry_t = ffi.typeof('struct port_tuple_entry')
+local port_c = ffi.new('struct port_c')
 
 -- Helper function to check space:method() usage
 local function check_space_arg(space, method)
@@ -1533,7 +1536,7 @@ base_index_mt.select_ffi = function(index, key, opts)
     local key, key_end = tuple_encode(key)
     local iterator, offset, limit = check_select_opts(opts, key + 1 >= key_end)
 
-    local port = ffi.cast('struct port *', port_tuple)
+    local port = ffi.cast('struct port *', port_c)
 
     if builtin.box_select(index.space_id, index.id,
         iterator, offset, limit, key, key_end, port) ~= 0 then
@@ -1541,8 +1544,8 @@ base_index_mt.select_ffi = function(index, key, opts)
     end
 
     local ret = {}
-    local entry = port_tuple.first
-    for i=1,tonumber(port_tuple.size),1 do
+    local entry = port_c.first
+    for i=1,tonumber(port_c.count),1 do
         ret[i] = tuple_bless(entry.tuple)
         entry = entry.next
     end
diff --git a/src/box/port.c b/src/box/port.c
index 2c1fadb5c..9d9fc1dbc 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -38,106 +38,15 @@
 #include "errinj.h"
 
 /**
- * The pools is used both by port_c and port_tuple, since their
- * entires are almost of the same size. Also port_c can use
- * objects from the pool to store result data in their memory,
- * when it fits.
+ * The pools is used by port_c to allocate entries and to store
+ * result data when it fits into an object from the pool.
  */
 static struct mempool port_entry_pool;
 
 enum {
-	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
-			      sizeof(struct port_tuple_entry)),
+	PORT_ENTRY_SIZE = sizeof(struct port_c_entry),
 };
 
-int
-port_tuple_add(struct port *base, struct tuple *tuple)
-{
-	struct port_tuple *port = port_tuple(base);
-	struct port_tuple_entry *e;
-	if (port->size == 0) {
-		tuple_ref(tuple);
-		e = &port->first_entry;
-		port->first = port->last = e;
-	} else {
-		e = mempool_alloc(&port_entry_pool);
-		if (e == NULL) {
-			diag_set(OutOfMemory, sizeof(*e), "mempool_alloc", "e");
-			return -1;
-		}
-		tuple_ref(tuple);
-		port->last->next = e;
-		port->last = e;
-	}
-	e->tuple = tuple;
-	e->next = NULL;
-	++port->size;
-	return 0;
-}
-
-void
-port_tuple_create(struct port *base)
-{
-	struct port_tuple *port = (struct port_tuple *)base;
-	port->vtab = &port_tuple_vtab;
-	port->size = 0;
-	port->first = NULL;
-	port->last = NULL;
-}
-
-static void
-port_tuple_destroy(struct port *base)
-{
-	struct port_tuple *port = port_tuple(base);
-	struct port_tuple_entry *e = port->first;
-	if (e == NULL)
-		return;
-	tuple_unref(e->tuple);
-	e = e->next;
-	while (e != NULL) {
-		struct port_tuple_entry *cur = e;
-		e = e->next;
-		tuple_unref(cur->tuple);
-		mempool_free(&port_entry_pool, cur);
-	}
-}
-
-static int
-port_tuple_dump_msgpack_16(struct port *base, struct obuf *out)
-{
-	struct port_tuple *port = port_tuple(base);
-	struct port_tuple_entry *pe;
-	for (pe = port->first; pe != NULL; pe = pe->next) {
-		if (tuple_to_obuf(pe->tuple, out) != 0)
-			return -1;
-		ERROR_INJECT(ERRINJ_PORT_DUMP, {
-			diag_set(OutOfMemory, tuple_size(pe->tuple), "obuf_dup",
-				 "data");
-			return -1;
-		});
-	}
-	return port->size;
-}
-
-static int
-port_tuple_dump_msgpack(struct port *base, struct obuf *out)
-{
-	struct port_tuple *port = port_tuple(base);
-	char *size_buf = obuf_alloc(out, mp_sizeof_array(port->size));
-	if (size_buf == NULL) {
-		diag_set(OutOfMemory, mp_sizeof_array(port->size), "obuf_alloc",
-			 "size_buf");
-		return -1;
-	}
-	mp_encode_array(size_buf, port->size);
-	if (port_tuple_dump_msgpack_16(base, out) < 0)
-		return -1;
-	return 1;
-}
-
-extern void
-port_tuple_dump_lua(struct port *base, struct lua_State *L, bool is_flat);
-
 static inline void
 port_c_destroy_entry(struct port_c_entry *pe)
 {
@@ -297,7 +206,7 @@ port_c_dump_lua(struct port *port, struct lua_State *L, bool is_flat);
 extern struct sql_value *
 port_c_get_vdbemem(struct port *base, uint32_t *size);
 
-static const struct port_vtab port_c_vtab = {
+const struct port_vtab port_c_vtab = {
 	.dump_msgpack = port_c_dump_msgpack,
 	.dump_msgpack_16 = port_c_dump_msgpack_16,
 	.dump_lua = port_c_dump_lua,
@@ -328,13 +237,3 @@ port_free(void)
 {
 	mempool_destroy(&port_entry_pool);
 }
-
-const struct port_vtab port_tuple_vtab = {
-	.dump_msgpack = port_tuple_dump_msgpack,
-	.dump_msgpack_16 = port_tuple_dump_msgpack_16,
-	.dump_lua = port_tuple_dump_lua,
-	.dump_plain = NULL,
-	.get_msgpack = NULL,
-	.get_vdbemem = NULL,
-	.destroy = port_tuple_destroy,
-};
diff --git a/src/box/port.h b/src/box/port.h
index 4d6a5b399..0100bac9a 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -40,47 +40,6 @@ extern "C" {
 
 struct tuple;
 
-struct port_tuple_entry {
-	struct port_tuple_entry *next;
-	struct tuple *tuple;
-};
-
-/**
- * Port implementation used for storing tuples.
- */
-struct port_tuple {
-	const struct port_vtab *vtab;
-	int size;
-	struct port_tuple_entry *first;
-	struct port_tuple_entry *last;
-	struct port_tuple_entry first_entry;
-};
-static_assert(sizeof(struct port_tuple) <= sizeof(struct port),
-	      "sizeof(struct port_tuple) must be <= sizeof(struct port)");
-
-extern const struct port_vtab port_tuple_vtab;
-
-/**
- * Convert an abstract port instance to a tuple port.
- */
-static inline struct port_tuple *
-port_tuple(struct port *port)
-{
-	return (struct port_tuple *)port;
-}
-
-/**
- * Create a port for storing tuples.
- */
-void
-port_tuple_create(struct port *port);
-
-/**
- * Append a tuple to a port.
- */
-int
-port_tuple_add(struct port *port, struct tuple *tuple);
-
 /** Port implementation used for storing raw data. */
 struct port_msgpack {
 	const struct port_vtab *vtab;
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index bfdfa4656..ade12eadf 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -41,6 +41,8 @@ struct obuf;
 struct lua_State;
 struct port;
 
+extern const struct port_vtab port_c_vtab;
+
 /**
  * A single port represents a destination of any output. One such
  * destination can be a Lua stack, or the binary protocol. An
@@ -113,7 +115,7 @@ struct port_vtab {
 
 /**
  * Abstract port instance. It is supposed to be converted to
- * a concrete port realization, e.g. port_tuple.
+ * a concrete port realization, e.g. port_c.
  */
 struct port {
 	/** Virtual method table. */
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 1/3] box: introduce port_c

[Igor Munkin]

Vlad,

Thanks for the patch! I left a single nit below, otherwise LGTM.

On 23.04.20, Vladislav Shpilevoy wrote:
> Port_c is a new descendant of struct port. It is used now for
> public C functions to store their result. Currently they can
> return only a tuple, but it will change soon, they will be able to
> return arbitrary MessagePack.
> 
> Port_tuple is not removed, because still is used for box_select(),
> for functional indexes, and in SQL as a base for port_sql.
> Although that may be changed later. Functional indexes really need
> only a single MessagePack object from their function. While
> box_select() working via port_tuple or port_c didn't show any
> significant difference during micro benchmarks.
> 
> Part of #4641
> ---
>  src/box/box.cc      |   2 +-
>  src/box/func.c      |   2 +-
>  src/box/lua/misc.cc |  20 +++++
>  src/box/port.c      | 206 +++++++++++++++++++++++++++++++++++++++++---
>  src/box/port.h      |  50 +++++++++++
>  src/box/sql/func.c  |  22 +++--
>  6 files changed, 281 insertions(+), 21 deletions(-)
> 

<snipped>

> diff --git a/src/box/port.c b/src/box/port.c
> index 6e2fe3a6e..2c1fadb5c 100644
> --- a/src/box/port.c
> +++ b/src/box/port.c
> @@ -37,7 +37,18 @@
>  #include <fiber.h>
>  #include "errinj.h"
>  
> -static struct mempool port_tuple_entry_pool;
> +/**
> + * The pools is used both by port_c and port_tuple, since their

Typo: s/is/are/.

> + * entires are almost of the same size. Also port_c can use
> + * objects from the pool to store result data in their memory,
> + * when it fits.
> + */
> +static struct mempool port_entry_pool;
> +
> +enum {
> +	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
> +			      sizeof(struct port_tuple_entry)),
> +};
>  
>  int
>  port_tuple_add(struct port *base, struct tuple *tuple)

<snipped>

> -- 
> 2.21.1 (Apple Git-122.3)
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/3] box: introduce box_return_mp() public C function

[Igor Munkin]

Vlad,

Thanks for the comment, LGTM.

On 23.04.20, Vladislav Shpilevoy wrote:
> Closes #4641
> 

<snipped>

> -- 
> 2.21.1 (Apple Git-122.3)
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/3] box: introduce port_c

[Vladislav Shpilevoy]

Hi! Thanks for the review!

>> diff --git a/src/box/port.c b/src/box/port.c
>> index 6e2fe3a6e..2c1fadb5c 100644
>> --- a/src/box/port.c
>> +++ b/src/box/port.c
>> @@ -37,7 +37,18 @@
>>  #include <fiber.h>
>>  #include "errinj.h"
>>  
>> -static struct mempool port_tuple_entry_pool;
>> +/**
>> + * The pools is used both by port_c and port_tuple, since their
> 
> Typo: s/is/are/.

Fixed:

====================
diff --git a/src/box/port.c b/src/box/port.c
index 2c1fadb5c..eabc19ded 100644
--- a/src/box/port.c
+++ b/src/box/port.c
@@ -38,7 +38,7 @@
 #include "errinj.h"
 
 /**
- * The pools is used both by port_c and port_tuple, since their
+ * The pool is used both by port_c and port_tuple, since their
  * entires are almost of the same size. Also port_c can use
  * objects from the pool to store result data in their memory,
  * when it fits.

Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Igor Munkin]

Vlad,

Thanks for the patch! Please consider the comments I left below.

On 23.04.20, Vladislav Shpilevoy wrote:
> Port_tuple is exactly the same as port_c, but is not able to store
> raw MessagePack. I theory it sounds like port_tuple should be a

Typo: s/I theory/In theory/.

> bit simpler and therefore faster, but in fact it is not.
> Microbenchmarks didn't reveal any difference. So port_tuple is no
> longer needed, all its functionality is covered by port_c.
> 
> Follow up #4641
> ---
>  src/box/box.cc         |   4 +-
>  src/box/execute.c      |  10 ++--
>  src/box/execute.h      |  13 +++--
>  src/box/key_list.c     |   4 +-
>  src/box/lua/execute.c  |   6 +--
>  src/box/lua/misc.cc    |  18 -------
>  src/box/lua/schema.lua |  29 ++++++-----
>  src/box/port.c         | 109 ++---------------------------------------
>  src/box/port.h         |  41 ----------------
>  src/lib/core/port.h    |   4 +-
>  10 files changed, 41 insertions(+), 197 deletions(-)
> 

<snipped>

> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 10584494b..fd1628930 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -79,17 +79,21 @@ ffi.cdef[[
>      box_txn_savepoint_t *
>      box_txn_savepoint();
>  
> -    struct port_tuple_entry {
> -        struct port_tuple_entry *next;
> -        struct tuple *tuple;
> +    struct __attribute__((packed)) port_c_entry {
> +        struct port_c_entry *next;
> +        union {
> +            struct tuple *tuple;
> +            char *mp;
> +        };
> +        uint32_t mp_size;
>      };
>  
> -    struct port_tuple {
> +    struct port_c {
>          const struct port_vtab *vtab;
> -        int size;
> -        struct port_tuple_entry *first;
> -        struct port_tuple_entry *last;
> -        struct port_tuple_entry first_entry;
> +        struct port_c_entry *first;
> +        struct port_c_entry *last;
> +        struct port_c_entry first_entry;
> +        int count;

It's still <size> in src/box/port.h.

>      };
>  
>      void

<snipped>

> @@ -1541,8 +1544,8 @@ base_index_mt.select_ffi = function(index, key, opts)
>      end
>  
>      local ret = {}
> -    local entry = port_tuple.first
> -    for i=1,tonumber(port_tuple.size),1 do
> +    local entry = port_c.first
> +    for i=1,tonumber(port_c.count),1 do

It's still <size> in src/box/port.h.

>          ret[i] = tuple_bless(entry.tuple)
>          entry = entry.next
>      end
> diff --git a/src/box/port.c b/src/box/port.c
> index 2c1fadb5c..9d9fc1dbc 100644
> --- a/src/box/port.c
> +++ b/src/box/port.c
> @@ -38,106 +38,15 @@
>  #include "errinj.h"
>  
>  /**
> - * The pools is used both by port_c and port_tuple, since their
> - * entires are almost of the same size. Also port_c can use
> - * objects from the pool to store result data in their memory,
> - * when it fits.
> + * The pools is used by port_c to allocate entries and to store

Typo: Just a reminder. I see you've already fixed it in the previous
patch.

> + * result data when it fits into an object from the pool.
>   */
>  static struct mempool port_entry_pool;
>  
>  enum {
> -	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
> -			      sizeof(struct port_tuple_entry)),
> +	PORT_ENTRY_SIZE = sizeof(struct port_c_entry),

Minor: PORT_ENTRY_SIZE is introduced in the first patch of the series.
After applying these changes it looks excess. Feel free to ignore if you
see any rationale to leave this constant.

>  };
>  

<snipped>

> diff --git a/src/lib/core/port.h b/src/lib/core/port.h
> index bfdfa4656..ade12eadf 100644
> --- a/src/lib/core/port.h
> +++ b/src/lib/core/port.h
> @@ -41,6 +41,8 @@ struct obuf;
>  struct lua_State;
>  struct port;
>  
> +extern const struct port_vtab port_c_vtab;

As we discussed this line should be moved to src/box/port.h.

> +
>  /**
>   * A single port represents a destination of any output. One such
>   * destination can be a Lua stack, or the binary protocol. An

<snipped>

> -- 
> 2.21.1 (Apple Git-122.3)
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 25/04/2020 02:21, Igor Munkin wrote:
> Vlad,
> 
> Thanks for the patch! Please consider the comments I left below.
> 
> On 23.04.20, Vladislav Shpilevoy wrote:
>> Port_tuple is exactly the same as port_c, but is not able to store
>> raw MessagePack. I theory it sounds like port_tuple should be a
> 
> Typo: s/I theory/In theory/.

Fixed.

>> bit simpler and therefore faster, but in fact it is not.
>> Microbenchmarks didn't reveal any difference. So port_tuple is no
>> longer needed, all its functionality is covered by port_c.
>>
>> Follow up #4641
>> ---
>>  src/box/box.cc         |   4 +-
>>  src/box/execute.c      |  10 ++--
>>  src/box/execute.h      |  13 +++--
>>  src/box/key_list.c     |   4 +-
>>  src/box/lua/execute.c  |   6 +--
>>  src/box/lua/misc.cc    |  18 -------
>>  src/box/lua/schema.lua |  29 ++++++-----
>>  src/box/port.c         | 109 ++---------------------------------------
>>  src/box/port.h         |  41 ----------------
>>  src/lib/core/port.h    |   4 +-
>>  10 files changed, 41 insertions(+), 197 deletions(-)
>>
>> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
>> index 10584494b..fd1628930 100644
>> --- a/src/box/lua/schema.lua
>> +++ b/src/box/lua/schema.lua
>> @@ -79,17 +79,21 @@ ffi.cdef[[
>>      box_txn_savepoint_t *
>>      box_txn_savepoint();
>>  
>> -    struct port_tuple_entry {
>> -        struct port_tuple_entry *next;
>> -        struct tuple *tuple;
>> +    struct __attribute__((packed)) port_c_entry {
>> +        struct port_c_entry *next;
>> +        union {
>> +            struct tuple *tuple;
>> +            char *mp;
>> +        };
>> +        uint32_t mp_size;
>>      };
>>  
>> -    struct port_tuple {
>> +    struct port_c {
>>          const struct port_vtab *vtab;
>> -        int size;
>> -        struct port_tuple_entry *first;
>> -        struct port_tuple_entry *last;
>> -        struct port_tuple_entry first_entry;
>> +        struct port_c_entry *first;
>> +        struct port_c_entry *last;
>> +        struct port_c_entry first_entry;
>> +        int count;
> 
> It's still <size> in src/box/port.h.

Forgot to revert this place after tried rename to
'count' everywhere.

====================
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index fd1628930..7c80f9c8a 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -93,7 +93,7 @@ ffi.cdef[[
         struct port_c_entry *first;
         struct port_c_entry *last;
         struct port_c_entry first_entry;
-        int count;
+        int size;
     };
 
     void
@@ -1545,7 +1545,7 @@ base_index_mt.select_ffi = function(index, key, opts)
 
     local ret = {}
     local entry = port_c.first
-    for i=1,tonumber(port_c.count),1 do
+    for i=1,tonumber(port_c.size),1 do
         ret[i] = tuple_bless(entry.tuple)
         entry = entry.next
     end

====================

>> diff --git a/src/box/port.c b/src/box/port.c
>> index 2c1fadb5c..9d9fc1dbc 100644
>> --- a/src/box/port.c
>> +++ b/src/box/port.c
>> @@ -38,106 +38,15 @@
>> + * result data when it fits into an object from the pool.
>>   */
>>  static struct mempool port_entry_pool;
>>  
>>  enum {
>> -	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
>> -			      sizeof(struct port_tuple_entry)),
>> +	PORT_ENTRY_SIZE = sizeof(struct port_c_entry),
> 
> Minor: PORT_ENTRY_SIZE is introduced in the first patch of the series.
> After applying these changes it looks excess. Feel free to ignore if you
> see any rationale to leave this constant.

If I change it, I need to change its usage place too, and increase
the diff. If we want to make the diff even smaller and remove this
constant, it would be better to merge this commit and the first one.
I can do that, if you want.

>>  };
>> diff --git a/src/lib/core/port.h b/src/lib/core/port.h
>> index bfdfa4656..ade12eadf 100644
>> --- a/src/lib/core/port.h
>> +++ b/src/lib/core/port.h
>> @@ -41,6 +41,8 @@ struct obuf;
>>  struct lua_State;
>>  struct port;
>>  
>> +extern const struct port_vtab port_c_vtab;
> 
> As we discussed this line should be moved to src/box/port.h.

Yes. I accidentally added it to core/port.h instead of box/port.h

====================
diff --git a/src/box/port.h b/src/box/port.h
index 0100bac9a..76bb5ed1b 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -40,6 +40,8 @@ extern "C" {
 
 struct tuple;
 
+extern const struct port_vtab port_c_vtab;
+
 /** Port implementation used for storing raw data. */
 struct port_msgpack {
 	const struct port_vtab *vtab;
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index ade12eadf..bb7787679 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -41,8 +41,6 @@ struct obuf;
 struct lua_State;
 struct port;
 
-extern const struct port_vtab port_c_vtab;
-
 /**
  * A single port represents a destination of any output. One such
  * destination can be a Lua stack, or the binary protocol. An

====================

Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Igor Munkin]

Vlad,

Thanks for the fixes.

On 26.04.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> On 25/04/2020 02:21, Igor Munkin wrote:
> > Vlad,
> > 
> > Thanks for the patch! Please consider the comments I left below.
> > 
> > On 23.04.20, Vladislav Shpilevoy wrote:

<snipped>

> >> diff --git a/src/box/port.c b/src/box/port.c
> >> index 2c1fadb5c..9d9fc1dbc 100644
> >> --- a/src/box/port.c
> >> +++ b/src/box/port.c
> >> @@ -38,106 +38,15 @@
> >> + * result data when it fits into an object from the pool.
> >>   */
> >>  static struct mempool port_entry_pool;
> >>  
> >>  enum {
> >> -	PORT_ENTRY_SIZE = MAX(sizeof(struct port_c_entry),
> >> -			      sizeof(struct port_tuple_entry)),
> >> +	PORT_ENTRY_SIZE = sizeof(struct port_c_entry),
> > 
> > Minor: PORT_ENTRY_SIZE is introduced in the first patch of the series.
> > After applying these changes it looks excess. Feel free to ignore if you
> > see any rationale to leave this constant.
> 
> If I change it, I need to change its usage place too, and increase
> the diff. If we want to make the diff even smaller and remove this
> constant, it would be better to merge this commit and the first one.
> I can do that, if you want.

I see just little occurences and all:
| $ grep -rnF 'PORT_ENTRY_SIZE' src | cut -f 1 -d ':' | sort | uniq -c
|       5 src/box/port.c
But, this comment definitely doesn't deserve merging indepentent
commits. Let's leave this as is or until Nikita asks to fix it in
another way.

> 

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Igor Munkin]

Sorry, mistyped in previous reply.

On 27.04.20, Igor Munkin wrote:
> Vlad,
> 
> Thanks for the fixes.
> 
> On 26.04.20, Vladislav Shpilevoy wrote:

<snipped>

> 
> I see just little occurences and all:

I see just a little occurences and all are localized in box/port.c.

<snipped>

> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 3/3] box: replace port_tuple with port_c everywhere

[Nikita Pettik]

On 23 Apr 02:12, Vladislav Shpilevoy wrote:
> Port_tuple is exactly the same as port_c, but is not able to store
> raw MessagePack. I theory it sounds like port_tuple should be a
> bit simpler and therefore faster, but in fact it is not.
> Microbenchmarks didn't reveal any difference. So port_tuple is no
> longer needed, all its functionality is covered by port_c.
> 
> Follow up #4641
> ---

LGTM.

Re: [Tarantool-patches] [PATCH v2 2/3] box: introduce box_return_mp() public C function

[Nikita Pettik]

On 23 Apr 02:12, Vladislav Shpilevoy wrote:
> +
> +int
> +test_return_mp(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	(void) args;
> +	(void) args_end;
> +	char buf[512];
> +	char *pos = mp_encode_uint(buf, 1);
> +	int rc = box_return_mp(ctx, buf, pos);
> +	if (rc != 0)
> +		return rc;
> +
> +	pos = mp_encode_int(buf, -1);
> +	rc = box_return_mp(ctx, buf, pos);
> +	if (rc != 0)
> +		return rc;
> +
> +	pos = mp_encode_uint(buf, UINT64_MAX);
> +	rc = box_return_mp(ctx, buf, pos);
> +	if (rc != 0)
> +		return rc;
> +
> +	const char *str = "123456789101112131415";
> +	pos = mp_encode_str(buf, str, strlen(str));
> +	rc = box_return_mp(ctx, buf, pos);
> +	if (rc != 0)
> +		return rc;
> +
> +	pos = mp_encode_array(buf, 1);
> +	pos = mp_encode_uint(pos, 2);
> +	box_tuple_t *tuple = box_tuple_new(box_tuple_format_default(),
> +					   buf, pos);
> +	if (tuple == NULL)
> +		return -1;
> +	rc = box_return_tuple(ctx, tuple);

Probably I'm missing something (since I've never used C API) but
when I do:

res = net:connect(box.cfg.listen):call(name)
print(type(res[5]))

I get table. But shouldn't it be tuple (cdata), since it (last member)
is explicitly wrapped into tuple)? I guess it is due to certain
convention but failed to find it in docs. Could you please clarify?

Re: [Tarantool-patches] [PATCH v2 2/3] box: introduce box_return_mp() public C function

[Vladislav Shpilevoy]

Thanks for the question!

On 27/04/2020 17:14, Nikita Pettik wrote:
> On 23 Apr 02:12, Vladislav Shpilevoy wrote:
>> +
>> +int
>> +test_return_mp(box_function_ctx_t *ctx, const char *args, const char *args_end)
>> +{
>> +	(void) args;
>> +	(void) args_end;
>> +	char buf[512];
>> +	char *pos = mp_encode_uint(buf, 1);
>> +	int rc = box_return_mp(ctx, buf, pos);
>> +	if (rc != 0)
>> +		return rc;
>> +
>> +	pos = mp_encode_int(buf, -1);
>> +	rc = box_return_mp(ctx, buf, pos);
>> +	if (rc != 0)
>> +		return rc;
>> +
>> +	pos = mp_encode_uint(buf, UINT64_MAX);
>> +	rc = box_return_mp(ctx, buf, pos);
>> +	if (rc != 0)
>> +		return rc;
>> +
>> +	const char *str = "123456789101112131415";
>> +	pos = mp_encode_str(buf, str, strlen(str));
>> +	rc = box_return_mp(ctx, buf, pos);
>> +	if (rc != 0)
>> +		return rc;
>> +
>> +	pos = mp_encode_array(buf, 1);
>> +	pos = mp_encode_uint(pos, 2);
>> +	box_tuple_t *tuple = box_tuple_new(box_tuple_format_default(),
>> +					   buf, pos);
>> +	if (tuple == NULL)
>> +		return -1;
>> +	rc = box_return_tuple(ctx, tuple);
> 
> Probably I'm missing something (since I've never used C API) but
> when I do:
> 
> res = net:connect(box.cfg.listen):call(name)
> print(type(res[5]))
> 
> I get table. But shouldn't it be tuple (cdata), since it (last member)
> is explicitly wrapped into tuple)? I guess it is due to certain
> convention but failed to find it in docs. Could you please clarify?

That is not about C functions. It is about tuple data type lost during
its marshaling over network. You will get the same result, if you
write that in a Lua stored function. We solve that problem by introducing
MP_EXT to be able to restore types on the client side for uuids, errors,
decimals. But not for tuples. So they turn into tables when
serialized-deserialized from MessagePack during sending over the network.

Exception is IPROTO_SELECT and other DML/DQL mapped onto exact IPROTO_*
commands, because client always knows they return tuples, and can create
them on the client side instead of tables.

Re: [Tarantool-patches] [PATCH v2 2/3] box: introduce box_return_mp() public C function

[Nikita Pettik]

On 27 Apr 23:29, Vladislav Shpilevoy wrote:
> Thanks for the question!
> 
> On 27/04/2020 17:14, Nikita Pettik wrote:
> > On 23 Apr 02:12, Vladislav Shpilevoy wrote:
> >> +
> >> +int
> >> +test_return_mp(box_function_ctx_t *ctx, const char *args, const char *args_end)
> >> +{
> >> +	(void) args;
> >> +	(void) args_end;
> >> +	char buf[512];
> >> +	char *pos = mp_encode_uint(buf, 1);
> >> +	int rc = box_return_mp(ctx, buf, pos);
> >> +	if (rc != 0)
> >> +		return rc;
> >> +
> >> +	pos = mp_encode_int(buf, -1);
> >> +	rc = box_return_mp(ctx, buf, pos);
> >> +	if (rc != 0)
> >> +		return rc;
> >> +
> >> +	pos = mp_encode_uint(buf, UINT64_MAX);
> >> +	rc = box_return_mp(ctx, buf, pos);
> >> +	if (rc != 0)
> >> +		return rc;
> >> +
> >> +	const char *str = "123456789101112131415";
> >> +	pos = mp_encode_str(buf, str, strlen(str));
> >> +	rc = box_return_mp(ctx, buf, pos);
> >> +	if (rc != 0)
> >> +		return rc;
> >> +
> >> +	pos = mp_encode_array(buf, 1);
> >> +	pos = mp_encode_uint(pos, 2);
> >> +	box_tuple_t *tuple = box_tuple_new(box_tuple_format_default(),
> >> +					   buf, pos);
> >> +	if (tuple == NULL)
> >> +		return -1;
> >> +	rc = box_return_tuple(ctx, tuple);
> > 
> > Probably I'm missing something (since I've never used C API) but
> > when I do:
> > 
> > res = net:connect(box.cfg.listen):call(name)
> > print(type(res[5]))
> > 
> > I get table. But shouldn't it be tuple (cdata), since it (last member)
> > is explicitly wrapped into tuple)? I guess it is due to certain
> > convention but failed to find it in docs. Could you please clarify?
> 
> That is not about C functions. It is about tuple data type lost during
> its marshaling over network. You will get the same result, if you
> write that in a Lua stored function. We solve that problem by introducing
> MP_EXT to be able to restore types on the client side for uuids, errors,
> decimals. But not for tuples. So they turn into tables when
> serialized-deserialized from MessagePack during sending over the network.

Ok, thanks. Just in case - patch LGTM.
 
> Exception is IPROTO_SELECT and other DML/DQL mapped onto exact IPROTO_*
> commands, because client always knows they return tuples, and can create
> them on the client side instead of tables.

Re: [Tarantool-patches] [PATCH v2 0/3] box_return_mp

[Kirill Yukhin]

Hello,

On 23 апр 02:12, Vladislav Shpilevoy wrote:
> The patchset extends the public C API with a function
> box_return_mp(). It allows to return arbitrary MessagePack from
> user's code.
> 
> First patch introduces a new struct port - port_c. It replaces
> port_tuple for stored C functions, and is able to store both
> tuples and raw MessagePack.
> 
> Second patch introduces the new public function.
> 
> Third patch removes no longer needed struct port_tuple, since it
> appeared to be not faster than struct port_c.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4641-c-function-ret-mp
> Issue: https://github.com/tarantool/tarantool/issues/4641

I've checked your patchset into master.

--
Regards, Kirill Yukhin