Re: [Tarantool-patches] [PATCH 00/20] Rewrite performance critical parts of net.box in C

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

On Thu, Jul 29, 2021 at 02:33:13PM +0300, Vladimir Davydov wrote:
> On Thu, Jul 29, 2021 at 12:51:18AM +0200, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patchset!
> > 
> > Here is a first part of my review. I will return later to continue.
> > 
> > Commits 02-05, 07-08, 10 are LGTM.
> 
> Pushed to master.
> 
> > 
> > > Asynchronous calls don't show as much of an improvement as synchronous,
> > > because per each asynchronous call we still have to create a 'future'
> > > object in Lua. Still, the improvement is quite noticeable - 30% for
> > > REPLACE, 10% for UPDATE, 20% for SELECT, 25% for CALL.
> > 
> > I didn't reach the end of the patchset yet, but did you try to create
> > the futures as cdata objects? They could be allocated on mempool, their
> > GC pressure might be optimized by doing similar to luaT_tuple_encode_table_ref
> > optimization (there was found a way to make __gc and other C functions
> > cheaper when it comes to amount of GC objects in Lua).
> > 
> > The API would stay the same, they just would become C structs with
> > methods instead of Lua tables.
> 
> Good call. Going to to try that. Thanks.

Quickly whipped up a patch that converts userdata and cdata. Applied on
top of the series. Surprisingly, it only made things worse:

With the patch (future is cdata):

==== FUTURE ====
  REPLACE: WALL 221.182 PROC 343.368 KRPS
   UPDATE: WALL 178.918 PROC 291.504 KRPS
   SELECT: WALL 220.815 PROC 248.843 KRPS
     CALL: WALL 218.313 PROC 315.670 KRPS


Without the patch (future is userdata):

==== FUTURE ====
  REPLACE: WALL 262.454 PROC 450.425 KRPS
   UPDATE: WALL 191.538 PROC 322.888 KRPS
   SELECT: WALL 288.498 PROC 333.393 KRPS
     CALL: WALL 247.463 PROC 375.180 KRPS

The patch is below. Note, it isn't entirely correct - future:pairs
doesn't work, because luaL_checkcdata doesn't seem to handle upvalues,
but it shouldn't affect the test.
--
From 8dc651e67f124a394cb6f4973b71080626d84d50 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Thu, 29 Jul 2021 18:06:32 +0300
Subject: [PATCH] net.box: convert netbox_request to Lua cdata


diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 957d4ff18a18..1fb3c566d59f 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -32,6 +32,7 @@
 #include <sys/socket.h>
 
 #include <small/ibuf.h>
+#include <small/mempool.h>
 #include <msgpuck.h> /* mp_store_u32() */
 #include "scramble.h"
 
@@ -135,8 +136,9 @@ struct netbox_request {
 
 };
 
+struct mempool netbox_request_pool;
+static uint32_t CTID_STRUCT_NETBOX_REQUEST_PTR;
 static const char netbox_registry_typename[] = "net.box.registry";
-static const char netbox_request_typename[] = "net.box.request";
 
 /* Passed to mpstream_init() to set a boolean flag on error. */
 static void
@@ -1394,7 +1396,11 @@ luaT_netbox_registry_reset(struct lua_State *L)
 static inline struct netbox_request *
 luaT_check_netbox_request(struct lua_State *L, int idx)
 {
-	return luaL_checkudata(L, idx, netbox_request_typename);
+	uint32_t cdata_type;
+	struct netbox_request **ptr = luaL_checkcdata(L, idx, &cdata_type);
+	if (ptr == NULL || cdata_type != CTID_STRUCT_NETBOX_REQUEST_PTR)
+		luaL_error(L, "expected net.box future as %d argument", idx);
+	return *ptr;
 }
 
 static int
@@ -1403,6 +1409,7 @@ luaT_netbox_request_gc(struct lua_State *L)
 	struct netbox_request *request = luaT_check_netbox_request(L, 1);
 	netbox_request_unregister(request);
 	netbox_request_destroy(request);
+	mempool_free(&netbox_request_pool, request);
 	return 0;
 }
 
@@ -1642,10 +1649,14 @@ netbox_perform_async_request_impl(struct lua_State *L, int idx,
 static int
 netbox_perform_async_request(struct lua_State *L)
 {
-	struct netbox_request *request = lua_newuserdata(L, sizeof(*request));
+	struct netbox_request *request = mempool_alloc(&netbox_request_pool);
+	if (request == NULL)
+		return luaL_error(L, "out of memory");
 	netbox_request_create(request);
-	luaL_getmetatable(L, netbox_request_typename);
-	lua_setmetatable(L, -2);
+	*(struct netbox_request **)
+		luaL_pushcdata(L, CTID_STRUCT_NETBOX_REQUEST_PTR) = request;
+	lua_pushcfunction(L, luaT_netbox_request_gc);
+	luaL_setcdatagc(L, -2);
 	netbox_perform_async_request_impl(L, 1, request);
 	return 1;
 }
@@ -2020,6 +2031,13 @@ netbox_console_loop(struct lua_State *L)
 int
 luaopen_net_box(struct lua_State *L)
 {
+	mempool_create(&netbox_request_pool, cord_slab_cache(),
+		       sizeof(struct netbox_request));
+
+	luaL_cdef(L, "struct netbox_request;");
+	CTID_STRUCT_NETBOX_REQUEST_PTR = luaL_ctypeid(
+		L, "struct netbox_request *");
+
 	static const struct luaL_Reg netbox_registry_meta[] = {
 		{ "__gc",           luaT_netbox_registry_gc },
 		{ "reset",          luaT_netbox_registry_reset },
@@ -2027,17 +2045,6 @@ luaopen_net_box(struct lua_State *L)
 	};
 	luaL_register_type(L, netbox_registry_typename, netbox_registry_meta);
 
-	static const struct luaL_Reg netbox_request_meta[] = {
-		{ "__gc",           luaT_netbox_request_gc },
-		{ "is_ready",       luaT_netbox_request_is_ready },
-		{ "result",         luaT_netbox_request_result },
-		{ "wait_result",    luaT_netbox_request_wait_result },
-		{ "discard",        luaT_netbox_request_discard },
-		{ "pairs",          luaT_netbox_request_pairs },
-		{ NULL, NULL }
-	};
-	luaL_register_type(L, netbox_request_typename, netbox_request_meta);
-
 	static const luaL_Reg net_box_lib[] = {
 		{ "decode_greeting",netbox_decode_greeting },
 		{ "new_registry",   netbox_new_registry },
@@ -2048,6 +2055,14 @@ luaopen_net_box(struct lua_State *L)
 		{ "iproto_loop",    netbox_iproto_loop },
 		{ "console_setup",  netbox_console_setup },
 		{ "console_loop",   netbox_console_loop },
+
+		/* Request methods. */
+		{ "request_is_ready",     luaT_netbox_request_is_ready },
+		{ "request_result",       luaT_netbox_request_result },
+		{ "request_wait_result",  luaT_netbox_request_wait_result },
+		{ "request_discard",      luaT_netbox_request_discard },
+		{ "request_pairs",        luaT_netbox_request_pairs },
+
 		{ NULL, NULL}
 	};
 	/* luaL_register_module polutes _G */
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 9da900933fbc..2aa76427a940 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -53,6 +53,24 @@ local M_COUNT       = 16
 -- Injects raw data into connection. Used by console and tests.
 local M_INJECT      = 17
 
+local request_t = ffi.typeof('struct netbox_request')
+
+local request_methods = {
+    ['is_ready']    = internal.request_is_ready,
+    ['result']      = internal.request_result,
+    ['wait_result'] = internal.request_wait_result,
+    ['discard']     = internal.request_discard,
+    ['pairs']       = internal.request_pairs,
+}
+
+ffi.metatype(request_t, {
+    __index = function(self, key)
+        return request_methods[key]
+    end,
+    -- Lua 5.2 compatibility
+    __pairs = internal.request_pairs,
+})
+
 -- utility tables
 local is_final_state         = {closed = 1, error = 1}