[Tarantool-patches] [PATCH 15/20] net.box: rewrite request implementation in C
Vladimir Davydov
vdavydov at tarantool.org
Wed Aug 4 15:30:01 MSK 2021
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
More information about the Tarantool-patches
mailing list