Re: [Tarantool-patches] [PATCH v8 3/4] box/cbox: implement cbox Lua module

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

The module leaks somewhere. It never unloads the shared file. I
tried this:

	cbox = require('cbox')
	f = cbox.func.load('function1.test_push')
	f()
	f = nil
	cbox.func.unload('function1.test_push')
	collectgarbage('collect')

Then I use 'lsof -p' to see what files are loaded, and I see
function1.dylib here. When I do all the same using _func space
and box.func.call, the file is unloaded correctly.

(function1.dylib I took from test/box/.)

See 14 comments below. I didn't review the last commit yet due
to too many comments here. Will do after we fix everything here.

On 14.10.2020 15:35, Cyrill Gorcunov wrote:
> Currently to run "C" function from some external module
> one have to register it first in "_func" system space. This
> is a problem if node is in read-only mode (replica).
> 
> Still people would like to have a way to run such functions
> even in ro mode. For this sake we implement "cbox" lua module.
> 
> Fixes #4692
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> @TarantoolBot document
> Title: cbox module
> 
> Overview
> ========
> 
> `cbox` module provides a way to create, delete and execute
> `C` procedures. Unlinke `box.schema.func` functionality this
> the functions created with `cbox` help are not persistent and
> live purely in memory. Once a node get turned off they are
> vanished. An initial purpose for them is to execute them on
> nodes which are running in read-only mode.
> 
> Module functions
> ================
> 
> `cbox.func.load([dso.]name) -> obj | nil, err`
> ----------------------------------------------
> 
> Loads a new function with name `name` from module `dso.`.
> The module name is optional and if not provided implies
> to be the same as `name`.
> 
> The `load` call must be paired with `unload` and these
> calls are accounted. Until coupled `unload` is called
> the instance is present in memory. Any `load` calls
> followed by another `load` with same name simply
> increase a reference to the existing function.
> 
> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: function name is too long.
>  - IllegalParams: function references limit exceeded.
>  - OutOfMemory: unable to allocate a function.
> 
> On success a new callable object is returned,
> otherwise `nil, error` pair.
> 
> Example:
> 
> ``` Lua
> f, err = require('cbox').func.load('func')
> if not f then
>     print(err)
> end
> ```
> 
> Once function is loaded it is possible to execute it
> in a traditional Lua way, ie to call it as a function.
> 
> ``` Lua
> -- call with agruments arg1 and arg2

1. agruments -> arguments.

> f(arg1, arg2)
> ```
> 
> `cbox.func.unload([dso.]name) -> true | nil, err`
> -------------------------------------------------
> 
> Unload a function with name `[dso.]name`. Since function
> instances are accounted the function is not unloaded until
> number of `unload` calls matched to the number of `load`
> calls.
> 
> Possible errors:
>  - IllegalParams: function name is either not supplied
>    or not a string.
>  - IllegalParams: the function does not exist.
> 
> On success `true` is returned, otherwise `nil, error` pair.
> 
> Example:
> 
> ``` Lua
> ok, err = require('cbox').func.unload('func')
> if not ok then
>     print(err)
> end
> ```
> 
> `cbox.module.reload(name) -> true | nil, err`
> ---------------------------------------------
> 
> Reloads a module with name `name` and all functions which
> were associated for the module. Each module keeps a list of
> functions belonging to the module and reload procedure cause
> the bound function to update their addresses such that
> function execution will be routed via a new library.
> 
> Modules are loaded with that named local binding which means
> that reload of module symbols won't affect the functions which
> are started execution already, only new calls will be rerouted.
> 
> Possible errors:
>  - IllegalParams: module name is either not supplied
>    or not a string.
>  - ClientError: a module with the name provided does
>    not exist.
> 
> On success `true` is returned, otherwise `nil,error` pair.
> 
> Example:
> 
> ``` Lua
> ok, err = require('cbox').module.reload('func')
> if not ok then
>     print(err)
> end
> ```

2. In the previous review I said:

	3. It seems the function invocation itself is not documented?

You answered:

	Good point, seems this would be useful. I though 'cause this
	is mostly an alias for "_func" space it would be obvious. But
	indeed without real example the doc looks incomplete. Thanks!

Tbh, I don't see any difference regarding this. It is
still not documented how to call a function after you
loaded it, and what does it return.

> ---
>  src/box/CMakeLists.txt |   1 +
>  src/box/box.cc         |   5 +
>  src/box/lua/cbox.c     | 486 +++++++++++++++++++++++++++++++++++++++++
>  src/box/lua/cbox.h     |  39 ++++
>  src/box/lua/init.c     |   2 +
>  5 files changed, 533 insertions(+)
>  create mode 100644 src/box/lua/cbox.c
>  create mode 100644 src/box/lua/cbox.h
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 2485b79f3..f20761e8f 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -75,6 +75,7 @@
>  #include "systemd.h"
>  #include "call.h"
>  #include "module_cache.h"
> +#include "lua/cbox.h"
>  #include "sequence.h"
>  #include "sql_stmt_cache.h"
>  #include "msgpack.h"
> @@ -2246,6 +2247,7 @@ box_free(void)
>  		tuple_free();
>  		port_free();
>  #endif
> +		cbox_free();

3. box_init and box_free are not for Lua modules. Please,
use box_lua_init(). Also I don't unserstand, why do you have two
cbox init functions. If it is a Lua module, it should have lua init
function and be in lua/ folder. If it is a C module, it should have
a usual init function and not be in a lua/ folder. Not both. See
examples such as tuple and error modules. They have separate Lua
and C part. In your case I don't understand why would you need a
separate C part. Cbox is entirely for Lua. We don't need cbox in C.
So for your case box_lua_init should be enough.

>  		iproto_free();
>  		replication_free();
>  		sequence_free();
> @@ -2647,6 +2649,9 @@ box_init(void)
>  	if (module_init() != 0)
>  		diag_raise();
>  
> +	if (cbox_init() != 0)
> +		diag_raise();
> +
>  	if (tuple_init(lua_hash) != 0)
>  		diag_raise();
>  
> diff --git a/src/box/lua/cbox.c b/src/box/lua/cbox.c
> new file mode 100644
> index 000000000..0d8208024
> --- /dev/null
> +++ b/src/box/lua/cbox.c
> @@ -0,0 +1,486 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + */
> +
> +#include <string.h>
> +#include <lua.h>
> +
> +#define RB_COMPACT 1
> +#include <small/rb.h>
> +
> +#include "diag.h"
> +
> +#include "box/module_cache.h"
> +#include "box/error.h"
> +#include "box/port.h"
> +
> +#include "trivia/util.h"
> +#include "lua/utils.h"
> +
> +/**
> + * Function descriptor.
> + */
> +struct cbox_func {
> +	/**
> +	 * Gather functions into rbtree.
> +	 */
> +	rb_node(struct cbox_func) nd;

4. Why rbtree? Search in the tree is going to be slower than in
the hash. Hash would be perfect here, because all the searches
are always by ==, not < or >, no iteration. Only lookup.

In the cover letter you said

	"i don't like the idea of unexpected rehashing of
	values in case of massive number of functions
	allocated"

but AFAIK, according to our benches, when a hash is used as an index,
it is always faster than any tree on lookups. Although I don't
remember benches hash vs tree out of an index. Since your are doing
such dubious "optimizations", is there a bench you can show, that
a tree is faster than a hash?

> +
> +	/**
> +	 * Symbol descriptor for the function in
> +	 * an associated module.
> +	 */
> +	struct module_sym mod_sym;
> +
> +	/**
> +	 * Number of references to the function
> +	 * instance.
> +	 */
> +	ssize_t ref;

5. Why is it ssize_t? Is there any single place, where we use ssize_t
for reference counting? Why ssize_t, if it is has nothing to do with
size of anything?

Also it probably would be better to name it 'load_count'. Because
to the end of the patch I was thinking you decided to count Lua refs
too somewhy, and realized it is load count only afterwards, when
started writing the comments.

7. Why are there so many extra empty lines after each member? I am
going to ask again you not to invent a new code style on each next
patch you send.

> +	/** Function name. */
> +	const char *name;
> +
> +	/** Function name length. */
> +	size_t name_len;
> +
> +	/** Function name keeper. */
> +	char inplace[0];> +};
> +
> +/**
> + * A tree to lookup functions by name.
> + */
> +typedef rb_tree(struct cbox_func) func_rb_t;
> +static func_rb_t func_rb_root;
> +
> +static int
> +cbox_func_cmp(const struct cbox_func *a, const struct cbox_func *b)> +{
> +	ssize_t len = (ssize_t)a->name_len - (ssize_t)b->name_len;
> +	if (len == 0)
> +		return strcmp(a->name, b->name);
> +	return len < 0 ? -1 : 1;
> +}
> +
> +rb_gen(MAYBE_UNUSED static, func_rb_, func_rb_t,
> +       struct cbox_func, nd, cbox_func_cmp);
> +
> +/**
> + * Find function in a tree.
> + */
> +struct cbox_func *
> +cbox_func_find(const char *name, size_t name_len)

8. It should be static.

> +{
> +	struct cbox_func v = {
> +		.name		= name,
> +		.name_len	= name_len,
> +	};
> +	return func_rb_search(&func_rb_root, &v);
> +}
> +
> +/**
> + * Unreference a function instance.
> + */
> +static void
> +cbox_func_unref(struct cbox_func *cf)
> +{
> +	assert(cf->ref > 0);
> +	if (cf->ref-- == 1)
> +		func_rb_remove(&func_rb_root, cf);
> +}
> +
> +/**
> + * Reference a function instance.
> + */
> +static bool
> +cbox_func_ref(struct cbox_func *cf)
> +{
> +	assert(cf->ref >= 0);
> +
> +	/*
> +	 * Hardly to ever happen but just
> +	 * to make sure.

9. We already discussed it a ton of times, literally exactly the
same 'problem' with struct error objects. A 64 bit integer
will not overflow for your lifetime even if it would increment
every nanosecond. How is it possible? Why the code should get
complicated by making a simple 'ref' function be able to fail?

> +	 */
> +	if (cf->ref == SSIZE_MAX) {
> +		const char *fmt =
> +			"Too many function references (max %zd)";
> +		diag_set(IllegalParams, fmt, SSIZE_MAX);
> +		return false;
> +	}
> +
> +	if (cf->ref++ == 0)
> +		func_rb_insert(&func_rb_root, cf);
> +
> +	return true;
> +}
> +
> +/**
> + * Allocate a new function instance.
> + */
> +static struct cbox_func *
> +cbox_func_new(const char *name, size_t name_len)
> +{
> +	const ssize_t cf_size = sizeof(struct cbox_func);
> +	ssize_t size = cf_size + name_len + 1;
> +	if (size < 0) {

10. If this is intended to check for an overflow, it does not work.
If I will pass name_len = UINT64_MAX (on my machine size_t == 8 bytes),
then UINT64_MAX + 1 = 0, you will allocate only sizeof(), and then
will do memcpy for UINT64_MAX below.

I would suggest not to get crazy with such checks. You can't cover
everything, and it is not needed.

> +		const size_t max_len = SSIZE_MAX - cf_size - 1;
> +		const char *fmt = "Function name is too long (max %zd)";
> +		diag_set(IllegalParams, fmt, max_len);
> +		return NULL;
> +	}
> +
> +	struct cbox_func *cf = malloc(size);
> +	if (cf == NULL) {
> +		diag_set(OutOfMemory, size, "malloc", "cf");
> +		return NULL;
> +	}
> +
> +	cf->mod_sym.addr	= NULL;
> +	cf->mod_sym.module	= NULL;
> +	cf->ref			= 0;
> +	cf->mod_sym.name	= cf->inplace;
> +	cf->name		= cf->inplace;
> +	cf->name_len		= name_len;
> +
> +	memcpy(cf->inplace, name, name_len);
> +	cf->inplace[name_len] = '\0';
> +
> +	memset(&cf->nd, 0, sizeof(cf->nd));
> +	return cf;
> +}
> +
> +/**
> + * Load a new function.
> + *
> + * This function takes a function name from the caller
> + * stack @a L and creates a new function object. If
> + * the function is already loaded we simply return
> + * a reference to existing one.
> + *
> + * Possible errors:
> + *
> + * - IllegalParams: function name is either not supplied
> + *   or not a string.
> + * - IllegalParams: function references limit exceeded.
> + * - OutOfMemory: unable to allocate a function.
> + *
> + * @returns function object on success or {nil,error} on error,
> + * the error is set to the diagnostics area.
> + */
> +static int
> +lcbox_func_load(struct lua_State *L)
> +{
> +	const char *method = "cbox.func.load";
> +	struct cbox_func *cf = NULL;
> +
> +	if (lua_gettop(L) != 1 || !lua_isstring(L, 1)) {
> +		const char *fmt =
> +			"Expects %s(\'name\') but no name passed";
> +		diag_set(IllegalParams, fmt, method);
> +		return luaT_push_nil_and_error(L);
> +	}
> +
> +	size_t name_len;
> +	const char *name = lua_tolstring(L, 1, &name_len);
> +
> +	cf = cbox_func_find(name, name_len);
> +	if (cf == NULL) {
> +		cf = cbox_func_new(name, name_len);
> +		if (cf == NULL)
> +			return luaT_push_nil_and_error(L);
> +	}
> +	if (!cbox_func_ref(cf))
> +		return luaT_push_nil_and_error(L);

11. 'cf' leaks if it was just created a few lines above.

> +
> +	lua_newtable(L);
> +
> +	lua_pushstring(L, "name");
> +	lua_pushstring(L, cf->name);
> +	lua_settable(L, -3);
> +
> +	lua_newtable(L);
> +
> +	/*
> +	 * A new variable should be callable for
> +	 * convenient use in Lua.
> +	 */
> +	lua_pushstring(L, "__call");
> +	lua_pushcfunction(L, lcbox_func_call);
> +	lua_settable(L, -3);> +
> +	/*
> +	 * We will release the memory associated
> +	 * with the objet if only no active refs
> +	 * are left.
> +	 */
> +	lua_pushstring(L, "__gc");
> +	lua_pushcfunction(L, lcbox_func_gc);
> +	lua_settable(L, -3);
> +
> +	/*
> +	 * Carry the pointer to the function so
> +	 * we won't need to run a lookup when
> +	 * calling.
> +	 */
> +	lua_pushstring(L, "__cbox_func");
> +	*(struct cbox_func **)lua_newuserdata(L, sizeof(cf)) = cf;
> +	lua_settable(L, -3);

12. I can assure you, that creation of all these Lua GC objects:
C functions, metatables, is going to cost much much more than the
optimizations you tried to do with the tree vs hash. Order of
magnitude slower. I suggest you to look at lua/tuple.c to check
how to avoid creation of a new metatable on each function object.

A single metatable object should be shared by all cbox function
objects to reduce the GC pressure.

Also you don't need to push a table with __cbox_func field in it.
You can push userdata right away and make it look like a proper Lua
object. See luaT_pusherror(), luaT_pushtuple().

> +
> +	lua_setmetatable(L, -2);
> +	return 1;
> +}
> +
> +/**
> + * Initialize cbox Lua module.
> + *
> + * @param L Lua state where to register the cbox module.
> + */
> +void
> +box_lua_cbox_init(struct lua_State *L)
> +{
> +	static const struct luaL_Reg cbox_methods[] = {
> +		{ NULL, NULL }
> +	};
> +	luaL_register_module(L, "cbox", cbox_methods);
> +
> +	/* func table */
> +	static const struct luaL_Reg func_table[] = {
> +		{ "load",	lcbox_func_load },
> +		{ "unload",	lcbox_func_unload },
> +	};
> +
> +	lua_newtable(L);
> +	for (size_t i = 0; i < lengthof(func_table); i++) {
> +		lua_pushstring(L, func_table[i].name);
> +		lua_pushcfunction(L, func_table[i].func);
> +		lua_settable(L, -3);
> +	}
> +	lua_setfield(L, -2, "func");
> +
> +	/* module table */
> +	static const struct luaL_Reg module_table[] = {
> +		{ "reload",	lcbox_module_reload },
> +	};
> +
> +	lua_newtable(L);
> +	for (size_t i = 0; i < lengthof(module_table); i++) {
> +		lua_pushstring(L, module_table[i].name);
> +		lua_pushcfunction(L, module_table[i].func);
> +		lua_settable(L, -3);
> +	}
> +	lua_setfield(L, -2, "module");

13. Why couldn't you simply use luaL_register() instead of these
cycles?

> +
> +	lua_pop(L, 1);
> +}
> +
> +/**
> + * Initialize cbox module.
> + *
> + * @return 0 on success, -1 on error (diag is set).	

14. Trailing tab. In the header file too.

> + */
> +int
> +cbox_init(void)
> +{
> +	func_rb_new(&func_rb_root);
> +	return 0;
> +}