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 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;
 };
 
 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)
 {
@@ -1505,9 +1513,8 @@ 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));
+	struct netbox_request *request = lua_touserdata(L, 1);
+	double timeout = request->iterator_timeout;
 	if (luaL_isnull(L, 2)) {
 		/* The previous call returned an error. */
 		goto stop;
@@ -1575,16 +1582,17 @@ error:
 static int
 luaT_netbox_request_pairs(struct lua_State *L)
 {
+	struct netbox_request *request = luaT_check_netbox_request(L, 1);
+	double timeout = TIMEOUT_INFINITY;
 	if (!lua_isnoneornil(L, 2)) {
-		if (lua_type(L, 2) != LUA_TNUMBER || lua_tonumber(L, 2) < 0)
+		if (lua_type(L, 2) != LUA_TNUMBER ||
+		    (timeout = lua_tonumber(L, 2)) < 0)
 			luaL_error(L, "Usage: future:pairs(timeout)");
-	} else {
-		if (lua_isnil(L, 2))
-			lua_pop(L, 1);
-		lua_pushnumber(L, TIMEOUT_INFINITY);
 	}
-	lua_pushcclosure(L, luaT_netbox_request_iterator_next, 2);
-	lua_pushnil(L);
+	request->iterator_timeout = timeout;
+	lua_rawgeti(L, LUA_REGISTRYINDEX,
+		    luaT_netbox_request_iterator_next_ref);
+	lua_insert(L, 1);
 	lua_pushinteger(L, 0);
 	return 3;
 }
@@ -1750,6 +1758,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 },