Re: [PATCH v2 6/9] box: load persistent Lua functions on creation

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Jun 06, 2019 at 03:04:02PM +0300, Kirill Shcherbatov wrote:
> This patch proceed persistent Lua function load on function
> object creation.
> 
> Part of #4182
> Needed for #1260
> ---
>  src/box/func.c                    |  77 +++++++++++++++--
>  src/box/func.h                    |  30 +++++--
>  src/box/func_def.h                |   6 ++
>  test/box/persistent_func.result   | 136 ++++++++++++++++++++++++++++++
>  test/box/persistent_func.test.lua |  61 ++++++++++++++
>  5 files changed, 292 insertions(+), 18 deletions(-)
>  create mode 100644 test/box/persistent_func.result
>  create mode 100644 test/box/persistent_func.test.lua
> 
> diff --git a/src/box/func.c b/src/box/func.c
> index 71c6bb6eb..b21221d9e 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -438,6 +438,7 @@ func_new(struct func_def *def)
>  		 */
>  	} else {
>  		assert(def->language == FUNC_LANGUAGE_LUA);
> +		func->lua_ref = LUA_REFNIL;
>  		func->vtab = &func_lua_vtab;
>  	}
>  	return func;

I think we should check function code in the constructor - it's more
user-friendly than checking it on the first execution.

> @@ -627,7 +677,12 @@ func_lua_call(struct func *func, struct port *port, const char *args,
>  	assert(func != NULL && func->def->language == FUNC_LANGUAGE_LUA);
>  	assert(func->vtab == &func_lua_vtab);
>  
> +	if (func_def_is_persistent(func->def) && func->lua_ref == LUA_REFNIL &&
> +	    func_lua_load(func) != 0)
> +		return -1;
> +

Branching in a virtual method doesn't look good IMO. Why not add
a separate class for persistent Lua functions?

> diff --git a/src/box/func_def.h b/src/box/func_def.h
> index 7a920f65e..8953e4776 100644
> --- a/src/box/func_def.h
> +++ b/src/box/func_def.h
> @@ -89,6 +89,12 @@ func_def_sizeof(uint32_t name_len, uint32_t body_len)
>  	return sz;
>  }
>  
> +static inline bool
> +func_def_is_persistent(struct func_def *def)
> +{
> +	return def->body != NULL;
> +}
> +

Pointless helper IMO - it's okay to check 'body' directly in Lua
constructor.