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

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Thu, Oct 29, 2020 at 11:15:58PM +0100, Vladislav Shpilevoy wrote:
> 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/.)

I'll try to repeat. That;s interesting, actually I've been trying
to force gc to trigger but without much luck. Letme investigate
this.

> > 
> > ``` Lua
> > -- call with agruments arg1 and arg2
> 
> 1. agruments -> arguments.

Thanks! I managed to miss this typo while been running aspell.

> > 
> > ``` 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.

Here is a paragraph above
---
    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
    f(arg1, arg2)
    ```
---

I'm not sure what else we could put here?

> > 
> > 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.

cbox engine uses rbtree to keep functions in memory and we have
to initialize it first and clean up it on exit. The lua init provides
Lua's interface over the engine. I can move initialization to box_lua_init
but where to clean things up?! Please look into popen module -- it does
exactly the same: there is C part and Lua parts.

I'll read tuple and error modules maybe I miss something obvious. Thanks!

> > +/**
> > + * 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.

And lookup in our hash table causes iterations as well in case
of collisions.

> 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?

The main reason for rbtree is the memory. Initially I used our mhash
engine but I don't expect there be that many functions which allows
me to not allocate such big structure as a hash table, instead I only
need two pointers for lookup (ie 16 bytes per entry).

Do you really think it worth it to allocate hash table instead? I'm ready
to make lookup a bit slower just to eliminate redundant memory overhead.

I expect at worst case to have up to 1000 functions which may require up
to 10 attempts.  I hate (with a passon) idea that in a middle of inserting
new key our hash engine start a rehashing procedure.

And note that lookup happens only _once_ when you loading a new function,
we keep the pointer later and calling function goes directly without any
lookups. So I think keeping low memory usage a way more preferred. Also
we might consider moving memory allocation for functions in our own "small"
instace (where mh hashes simply use system's malloc/calloc calls).

And no, I didn't measure performance here because it was not a priority
when _inserting_ a new function. And I'm very sceptical about "we ran
a benchmark and it worked a way more better". We've been using both
trees and hashes for decades and open-hashing (like ours) works fine
until some moment where collision starts to happen and performace flushed
to the toiled because you start walking over the table causing pages
to swap in, massive tlb flushed and all this friends. On big data and
higly loaded systems it is very fragile area and there is still no
answer which structures are better :( Sometimes if your data is small
enough at sits near in memory old plain linear lookup a way more
faster than enything else, because theory is theory but in real life
too many components with own limists involved.

Summarizing: I can easily switch back to mhash if you prefer.

> > +
> > +	/**
> > +	 * 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.

Strictly speaking plain int should fit here better but signed size
just guaranteed to be large enough to cover address space. That said
I agree that using ssize_t here is a git misleading. Will change and
rename. Thanks!

> 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.

As far as I see there is no any requirements about spacing outs so
for me it looks a way more readable. I'll shrink it if you prefer.

> > +/**
> > + * Find function in a tree.
> > + */
> > +struct cbox_func *
> > +cbox_func_find(const char *name, size_t name_len)
> 
> 8. It should be static.

+1

> > +
> > +/**
> > + * 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?

This has nothing to do for fair use. It prevents from errors where
you occasionally screwed the counter. I don't know why but for some
reason you think that counters are never screwed. What we really
need is a general kind of kref_t and it *must* include saturation
check somewhere inside and panic on overflow.

> 
> > +	 */
> > +	if (cf->ref == SSIZE_MAX) {
> > +		const char *fmt =
> > +			"Too many function references (max %zd)";
> > +		diag_set(IllegalParams, fmt, SSIZE_MAX);
> > +		return false;
> > +	}

...

> > +/**
> > + * 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.

It is a typo, it should be `if (size <= 0)`. I think we must check
what users pass us and don't allow to screw a program. So this test
is not crazy at all. I can drop it of course but at least we can
set up some assert() here? Seriously we must not trust user data.

> > +static int
> > +lcbox_func_load(struct lua_State *L)
> > +{
> > +
> 
> 11. 'cf' leaks if it was just created a few lines above.

Good cactch! Thanks!

> > +	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().

Will do, thanks for suggestion.

> > +
> > +	/* 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?

For some reason when I've been using luaL_register it didn't appear
on newly create objects. Letme try to use luaL_register again maybe
I simply miss something obvious.

> > +
> > +/**
> > + * Initialize cbox module.
> > + *
> > + * @return 0 on success, -1 on error (diag is set).	
> 
> 14. Trailing tab. In the header file too.

Ok, thanks!