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

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

On Mon, Aug 02, 2021 at 11:54:43PM +0200, Vladislav Shpilevoy wrote:
> In an early version I saw on the branch you tried to keep the
> discarded requests in the hash. It is not so anymore, correct?
> Now discarded requests are GCed even their response didn't arrive
> yet? (It should be so, but I decided to clarify just in case.)

Both original version and the current version pushed to the branch
remove discarded requests from the hash (both luaT_netbox_request_gc and
luaT_netbox_request_discard call netbox_request_unregister, which
removes the request from the hash table).

> > +struct netbox_registry {
> > +	/* sync -> netbox_request */
> 
> 1. For out of function comments we usually use /**. This includes the
> comments for structs and their members.

Fixed here, there, and everywhere.

> > +struct netbox_request {
> > +	/*
> > +	 * Reference to the request result or LUA_NOREF if the response hasn't
> > +	 * been received yet. If the response was decoded to a user-provided
> > +	 * buffer, the result stores the length of the decoded data.
> 
> 2. Do you mean result_ref stores the length? I can't find it in the code.
> It always stores a real Lua ref from what I see.

Yes, it always stores a Lua ref. In case the result is written to a
user-provided buffer, it stores a reference to the result lenght.
Updated the comment to clarify that.

> > +	 */
> > +	int result_ref;
> > +	/*
> > +	 * Error if the request failed (ref incremented). NULL on success or if
> > +	 * the response hasn't been received yet.
> > +	 */
> > +	struct error *error;
> > +
> 
> 3. Extra empty line.

Removed.

> > +static bool
> > +netbox_request_is_ready(struct netbox_request *request)
> 
> 4. The request can be made 'const'.

Added.

> > +static bool
> > +netbox_request_wait(struct netbox_request *request, double *timeout)
> > +{
> > +	double ts = ev_monotonic_now(loop());
> 
> 5. fiber_clock() might be a little shorter (does the same). The same below.
> Although it is not inline. Up to you. Also see a related comment below.

We use ev_monotonic_now in more places in the code so I'm going to stick to it:

~/src/tarantool$ git grep fiber_clock src | grep -c -v -e '\.lua:'
19
~/src/tarantool$ git grep ev_monotonic_now src | grep -c -v -e '\.lua:'
60

> 
> 6. Have you tried making these 1-5 line functions explicitly inline?
> I remember with Mergen we saw some actual perf difference in SQL code
> when did so. Although that time the functions were in the header vs in
> a .c file.
> 
> I see you used inline for some functions in this commit. Why not for
> the ones like these?

Marked all short, frequently called functions 'inline'. It didn't affect
the test results though.

> > +static int
> > +luaT_netbox_request_wait_result(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 ||
> > +		    (timeout = lua_tonumber(L, 2)) < 0)
> > +			luaL_error(L, "Usage: future:wait_result(timeout)");
> > +	}
> > +	while (!netbox_request_is_ready(request)) {
> > +		if (!netbox_request_wait(request, &timeout)) {
> > +			luaL_testcancel(L);
> > +			diag_set(ClientError, ER_TIMEOUT);
> 
> 7. In some places you use box_error_raise(), in others the
> explicit diag_set(ClientError). Why? For instance:
> 
> 	box_error_raise(ER_NO_CONNECTION, "%s", strerror(errno));
> 	box_error_raise(ER_NO_CONNECTION, "Peer closed");
> 
> In others the raise is justified because you do not know the
> error code and its message at compile time there. In these 2
> I probably do not know something?

box_error_raise() allows you to set an arbitrary error message while diag_set()
uses error messages corresponding to error codes (src/box/errcode.h). E.g. for
ER_NO_CONNECTION, it's always "Connection is not established".

I use box_error_raise() here in order to avoid changing error messages which
were reported by Lua code originally.

> > +static int
> > +luaT_netbox_request_pairs(struct lua_State *L)
> > +{
> > +	if (!lua_isnoneornil(L, 2)) {
> > +		if (lua_type(L, 2) != LUA_TNUMBER || 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);
> 
> 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.

> > +static int
> > +netbox_new_request(struct lua_State *L)
> > +{
> > +	struct netbox_request *request = lua_newuserdata(L, sizeof(*request));
> 
> 9. Does it help perf if the requests are allocated on mempool?

I tried to allocate requests on mempool. To achieve that, we have to
turn them into C data. It resulted in a performance degradation, see the
parallel thread:

https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/025039.html

The code is here (last two commits):

https://github.com/tarantool/tarantool/tree/vdavydov/net-box-optimization-cdata-request

Lua does a lot of malloc calls under the hood. Eliminating just this one
newuserdata doesn't seem to make any difference.

> > +	netbox_request_create(request);
> 
> 10. Below you override a lot of fields initialized in
> netbox_request_create(). Do you really need this create(), can you
> inline it without the unnecessary assignments?

Right. Removed netbox_request_create.

> > +	luaL_getmetatable(L, netbox_request_typename);
> > +	lua_setmetatable(L, -2);
> > +	struct netbox_registry *registry = luaT_check_netbox_registry(L, 1);
> > +	request->sync = luaL_touint64(L, 2);
> > +	request->buffer = (struct ibuf *) lua_topointer(L, 3);
> > +	lua_pushvalue(L, 3);
> > +	request->buffer_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	request->skip_header = lua_toboolean(L, 4);
> > +	request->method = lua_tointeger(L, 5);
> > +	assert(request->method < netbox_method_MAX);
> > +	lua_pushvalue(L, 6);
> > +	request->on_push_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	lua_pushvalue(L, 7);
> > +	request->on_push_ctx_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	if (!lua_isnil(L, 8))
> > +		request->format = lbox_check_tuple_format(L, 8);
> > +	else
> > +		request->format = tuple_format_runtime;
> > +	tuple_format_ref(request->format);
> > +	if (netbox_request_register(request, registry) != 0)
> > +		luaT_error(L);
> > +	return 1;
> > +}
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 0ad6cac022f2..4bc66940ea2a 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -468,20 +304,8 @@ local function create_transport(host, port, user, password, callback,
> >          local id = next_request_id
> >          encode_method(method, send_buf, id, ...)
> >          next_request_id = next_id(id)
> > -        -- Request in most cases has maximum 10 members:
> > -        -- method, buffer, skip_header, id, cond, errno, response,
> > -        -- on_push, on_push_ctx and format.
> > -        local request = setmetatable(table_new(0, 10), request_mt)
> > -        request.method = method
> > -        request.buffer = buffer
> > -        request.skip_header = skip_header
> > -        request.id = id
> > -        request.cond = fiber.cond()
> > -        requests[id] = request
> > -        request.on_push = on_push
> > -        request.on_push_ctx = on_push_ctx
> > -        request.format = format
> > -        return request
> > +        return internal.new_request(requests, id, buffer, skip_header, method,
> > +                                    on_push, on_push_ctx, format)
> 
> 11. You might want to try to cache internal.new_request globally,
> use it without '.' operator, and re-check the benches. Might be nothing
> or not, but '.' is definitely not free. The same for
> dispatch_response_iproto, dispatch_response_console.

Replied in the parallel thread:

https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025137.html

> > -            msg, real_end, request.errno = decode_push(body_rpos, body_end)
> 
> 12. decode_push is not unused.

Deleted it.

The incremental diff is below.
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 6859e955ac6e..e7cb5b3d810f 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -79,79 +79,63 @@ enum netbox_method {
 };
 
 struct netbox_registry {
-	/* sync -> netbox_request */
+	/** sync -> netbox_request */
 	struct mh_i64ptr_t *requests;
 };
 
 struct netbox_request {
 	enum netbox_method method;
-	/*
+	/**
 	 * Unique identifier needed for matching the request with its response.
 	 * Used as a key in the registry.
 	 */
 	uint64_t sync;
-	/*
+	/**
 	 * The registry this request belongs to or NULL if the request has been
 	 * completed.
 	 */
 	struct netbox_registry *registry;
-	/* Format used for decoding the response (ref incremented). */
+	/** Format used for decoding the response (ref incremented). */
 	struct tuple_format *format;
-	/* Signaled when the response is received. */
+	/** Signaled when the response is received. */
 	struct fiber_cond cond;
-	/*
+	/**
 	 * A user-provided buffer to which the response body should be copied.
 	 * If NULL, the response will be decoded to Lua stack.
 	 */
 	struct ibuf *buffer;
-	/*
+	/**
 	 * Lua reference to the buffer. Used to prevent garbage collection in
 	 * case the user discards the request.
 	 */
 	int buffer_ref;
-	/*
+	/**
 	 * Whether to skip MessagePack map header and IPROTO_DATA key when
 	 * copying the response body to a user-provided buffer. Ignored if
 	 * buffer is not set.
 	 */
 	bool skip_header;
-	/* Lua references to on_push trigger and its context. */
+	/** Lua references to on_push trigger and its context. */
 	int on_push_ref;
 	int on_push_ctx_ref;
-	/*
-	 * Reference to the request result or LUA_NOREF if the response hasn't
-	 * been received yet. If the response was decoded to a user-provided
-	 * buffer, the result stores the length of the decoded data.
+	/**
+	 * Lua reference to the request result or LUA_NOREF if the response
+	 * hasn't been received yet. If the response was decoded to a
+	 * user-provided buffer (see buffer_ref), result_ref stores a Lua
+	 * reference to an integer value that contains the length of the
+	 * decoded data.
 	 */
 	int result_ref;
-	/*
+	/**
 	 * Error if the request failed (ref incremented). NULL on success or if
 	 * the response hasn't been received yet.
 	 */
 	struct error *error;
-
 };
 
 static const char netbox_registry_typename[] = "net.box.registry";
 static const char netbox_request_typename[] = "net.box.request";
 
-static void
-netbox_request_create(struct netbox_request *request)
-{
-	request->method = netbox_method_MAX;
-	request->sync = -1;
-	request->registry = NULL;
-	request->format = NULL;
-	fiber_cond_create(&request->cond);
-	request->buffer = NULL;
-	request->buffer_ref = LUA_REFNIL;
-	request->skip_header = false;
-	request->on_push_ref = LUA_REFNIL;
-	request->on_push_ctx_ref = LUA_REFNIL;
-	request->result_ref = LUA_NOREF;
-	request->error = NULL;
-}
-
 static void
 netbox_request_destroy(struct netbox_request *request)
 {
@@ -206,19 +190,19 @@ netbox_request_unregister(struct netbox_request *request)
 	mh_i64ptr_del(h, k, NULL);
 }
 
-static bool
-netbox_request_is_ready(struct netbox_request *request)
+static inline bool
+netbox_request_is_ready(const struct netbox_request *request)
 {
 	return request->registry == NULL;
 }
 
-static void
+static inline void
 netbox_request_signal(struct netbox_request *request)
 {
 	fiber_cond_broadcast(&request->cond);
 }
 
-static void
+static inline void
 netbox_request_complete(struct netbox_request *request)
 {
 	netbox_request_unregister(request);
@@ -229,7 +213,7 @@ netbox_request_complete(struct netbox_request *request)
  * Waits on netbox_request::cond. Subtracts the wait time from the timeout.
  * Returns false on timeout or if the fiber was cancelled.
  */
-static bool
+static inline bool
 netbox_request_wait(struct netbox_request *request, double *timeout)
 {
 	double ts = ev_monotonic_now(loop());
@@ -238,14 +222,14 @@ netbox_request_wait(struct netbox_request *request, double *timeout)
 	return rc == 0;
 }
 
-static void
+static inline void
 netbox_request_set_result(struct netbox_request *request, int result_ref)
 {
 	assert(request->result_ref == LUA_NOREF);
 	request->result_ref = result_ref;
 }
 
-static void
+static inline void
 netbox_request_set_error(struct netbox_request *request, struct error *error)
 {
 	assert(request->error == NULL);
@@ -297,7 +281,7 @@ netbox_registry_destroy(struct netbox_registry *registry)
 /**
  * Looks up a request by id (sync). Returns NULL if not found.
  */
-static struct netbox_request *
+static inline struct netbox_request *
 netbox_registry_lookup(struct netbox_registry *registry, uint64_t sync)
 {
 	struct mh_i64ptr_t *h = registry->requests;
@@ -1637,9 +1621,6 @@ static int
 netbox_new_request(struct lua_State *L)
 {
 	struct netbox_request *request = lua_newuserdata(L, sizeof(*request));
-	netbox_request_create(request);
-	luaL_getmetatable(L, netbox_request_typename);
-	lua_setmetatable(L, -2);
 	struct netbox_registry *registry = luaT_check_netbox_registry(L, 1);
 	request->sync = luaL_touint64(L, 2);
 	request->buffer = (struct ibuf *)lua_topointer(L, 3);
@@ -1657,8 +1638,15 @@ netbox_new_request(struct lua_State *L)
 	else
 		request->format = tuple_format_runtime;
 	tuple_format_ref(request->format);
-	if (netbox_request_register(request, registry) != 0)
+	fiber_cond_create(&request->cond);
+	request->result_ref = LUA_NOREF;
+	request->error = NULL;
+	if (netbox_request_register(request, registry) != 0) {
+		netbox_request_destroy(request);
 		luaT_error(L);
+	}
+	luaL_getmetatable(L, netbox_request_typename);
+	lua_setmetatable(L, -2);
 	return 1;
 }
 
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 1379d04268d9..20dec5648926 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -68,11 +68,6 @@ local M_INJECT      = 17
 -- utility tables
 local is_final_state         = {closed = 1, error = 1}
 
-local function decode_push(raw_data)
-    local response, raw_end = decode(raw_data)
-    return response[IPROTO_DATA_KEY][1], raw_end
-end
-
 local function version_id(major, minor, patch)
     return bit.bor(bit.lshift(major, 16), bit.lshift(minor, 8), patch)
 end