Re: [Tarantool-patches] [PATCH 15/20] net.box: rewrite request implementation in C

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Wed, Aug 04, 2021 at 06:35:26PM +0300, Vladimir Davydov wrote:
> On Wed, Aug 04, 2021 at 03:30:01PM +0300, Vladimir Davydov wrote:
> > On Mon, Aug 02, 2021 at 11:54:43PM +0200, Vladislav Shpilevoy wrote:
> > > > +	lua_pushcclosure(L, luaT_netbox_request_iterator_next, 2);
> > > 
> > > 8. Push of cfunctions, especially closures, on a regular basis might
> > > be expensive. Could you please try to make it a non-closure function
> > > and cache its reference like I showed in the proposal about request __gc?
> > 
> > My test doesn't check performance of the request iterator so I need to
> > come up with a new test first to check this hypothesis. I'll reply to
> > this email with the results once they're ready.
> 
> Using getref instead of pushcclosure does improve performance of
> the final version by about 10%:
> 
>                 KRPS (WALL TIME)   KRPS (PROC TIME)
> pushcclosure :     233 +-  3     :     344 +-  7
> getref       :     259 +-  4     :     404 +- 10
> 
> The test is here:
> 
> https://gist.github.com/locker/9df4eaaf1bc889b16706640711c946f7
> 
> Amended the patch, thanks. The incremental diff is below:
> --
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 7ef40792320e..fadd71a6813b 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -131,11 +131,20 @@ struct netbox_request {
>  	 * the response hasn't been received yet.
>  	 */
>  	struct error *error;
> +	/** Timeout for luaT_netbox_request_iterator_next(). */
> +	double iterator_timeout;
>  };

It's totally wrong to store iterator_timeout in netbox_request, because
there may be more than one iterator open for the same request. Instead
we should push a table with {request, timeout}, just like the Lua
implementation did. This is a bit slower, but still faster than using
pushcclosure:

                   KRPS (WALL TIME)   KRPS (PROC TIME)
 pushcclosure   :     233 +-  3     :     344 +-  7
 getref         :     259 +-  4     :     404 +- 10
 getref + table :     257 +-  7     :     397 +- 15

The new incremental diff is below:
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index de9c11736603..45b0c7f3bac3 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -136,6 +136,13 @@ struct netbox_request {
 static const char netbox_registry_typename[] = "net.box.registry";
 static const char netbox_request_typename[] = "net.box.request";
 
+/**
+ * Instead of pushing luaT_netbox_request_iterator_next with lua_pushcclosure
+ * in luaT_netbox_request_pairs, we get it by reference, because it works
+ * faster.
+ */
+static int luaT_netbox_request_iterator_next_ref;
+
 static void
 netbox_request_destroy(struct netbox_request *request)
 {
@@ -1488,8 +1495,8 @@ luaT_netbox_request_discard(struct lua_State *L)
 
 /**
  * Gets the next message or the final result. Takes the index of the last
- * returned message as a second argument (the first argument is ignored).
- * The request and timeout are passed as up-values (see request.pairs()).
+ * returned message as a second argument. The request and timeout are passed in
+ * the first argument as a table (see request.pairs()).
  *
  * On success returns the index of the current message (used by the iterator
  * implementation to continue iteration) and an object, which is either the
@@ -1504,9 +1511,12 @@ luaT_netbox_request_discard(struct lua_State *L)
 static int
 luaT_netbox_request_iterator_next(struct lua_State *L)
 {
-	struct netbox_request *request = luaT_check_netbox_request(
-		L, lua_upvalueindex(1));
-	double timeout = lua_tonumber(L, lua_upvalueindex(2));
+	/* The first argument is a table: {request, timeout}. */
+	lua_rawgeti(L, 1, 1);
+	struct netbox_request *request = luaT_check_netbox_request(L, -1);
+	lua_rawgeti(L, 1, 2);
+	double timeout = lua_tonumber(L, -1);
+	/* The second argument is the index of the last returned message. */
 	if (luaL_isnull(L, 2)) {
 		/* The previous call returned an error. */
 		goto stop;
@@ -1582,8 +1592,17 @@ luaT_netbox_request_pairs(struct lua_State *L)
 			lua_pop(L, 1);
 		lua_pushnumber(L, TIMEOUT_INFINITY);
 	}
-	lua_pushcclosure(L, luaT_netbox_request_iterator_next, 2);
-	lua_pushnil(L);
+	lua_settop(L, 2);
+	/* Create a table passed to next(): {request, timeout}. */
+	lua_createtable(L, 2, 0);
+	lua_insert(L, 1);
+	lua_rawseti(L, 1, 2); /* timeout */
+	lua_rawseti(L, 1, 1); /* request */
+	/* Push the next() function. It must go first. */
+	lua_rawgeti(L, LUA_REGISTRYINDEX,
+		    luaT_netbox_request_iterator_next_ref);
+	lua_insert(L, 1);
+	/* Push the iterator index. */
 	lua_pushinteger(L, 0);
 	return 3;
 }
@@ -1749,6 +1768,9 @@ netbox_dispatch_response_console(struct lua_State *L)
 int
 luaopen_net_box(struct lua_State *L)
 {
+	lua_pushcfunction(L, luaT_netbox_request_iterator_next);
+	luaT_netbox_request_iterator_next_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+
 	static const struct luaL_Reg netbox_registry_meta[] = {
 		{ "__gc",           luaT_netbox_registry_gc },
 		{ "reset",          luaT_netbox_registry_reset },