Re: [Tarantool-patches] [PATCH 13/20] net.box: rewrite send_and_recv_{iproto, console} in C

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

On Mon, Aug 02, 2021 at 11:49:51PM +0200, Vladislav Shpilevoy wrote:
> > +netbox_communicate(int fd, struct ibuf *send_buf, struct ibuf *recv_buf,
> > +		   size_t limit, const void *boundary, size_t boundary_len,
> > +		   double timeout, size_t *limit_or_boundary_pos)
> 
> 1. IMO, the name looks really bulky. I would just call it `size` or `end_pos`.
> Up to you.

Right. Renamed to size.

> > +				diag_set(OutOfMemory, NETBOX_READAHEAD,
> > +					 "ibuf", "recv_buf");
> 
> 2. For OutOfMemory errors as a rule we use the format
> 
> 	diag_set(OutOfMemory, size, "name of the allocation function",
> 	         "name of the variable")
> 
> So here it would be
> 
> 	diag_set(OutOfMemory, NETBOX_READAHEAD, "ibuf_reserve", "p");
> 
> I know it is violated in a lot of old code and I gave up trying to
> enforce it in new patches to exact that form. Up to you.
> 
> The same in the other new OutOfMemory in the other patches.

Fixed.

> > +        local hdr, body_rpos, body_end = internal.send_and_recv_iproto(
> 
> 3. Indexing 'internal' via '.' is not free. It is a lookup
> in a hash. You might want to save internal.send_and_recv_iproto into
> a global variable when the module loads first time and use the
> cached value. Non-cached version is a bit faster only for FFI, but
> here you are using Lua C - cache should be good.
> 
> > +            connection:fd(), send_buf, recv_buf, timeout)
> 
> Another idea is to cache 'connection:fd()' result into a variable in
> the root of create_transport() function. And update it when the
> connetion is re-established. Although you probably move this all to
> C later as well, I didn't reach the last commits yet.

The calling function is moved to C later in the patch set so these
comments will become irrelevant.

Regarding caching function name (instead of accessing via dot operator),
eventually there will be only two hot C functions that could benefit
from this:

  internal.perform_request
  internal.perform_async_request

I tried caching their names, but saw no performance gain at all in my
test. I also tried removing fiber_self and fiber_clock aliases from
net_box.lua and accessing these functions as fiber.<name> - again no
difference.

I will do some research about this (try to bench this separately) and
add a separate commit to cache the internal function names on top of the
series if it helps. Will follow-up in reply to this email.

Incremental diff is below.
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index cb80d2efa364..6d6d09acafdb 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -437,7 +437,7 @@ netbox_decode_greeting(lua_State *L)
 
 /**
  * Reads data from the given socket until the limit or boundary is reached.
- * Returns 0 and sets *limit_or_boundary_pos to limit/boundary_pos on success.
+ * Returns 0 and sets *size to limit or boundary position on success.
  * On error returns -1 and sets diag.
  *
  * The need for this function arises from not wanting to
@@ -452,7 +452,7 @@ netbox_decode_greeting(lua_State *L)
 static int
 netbox_communicate(int fd, struct ibuf *send_buf, struct ibuf *recv_buf,
 		   size_t limit, const void *boundary, size_t boundary_len,
-		   double timeout, size_t *limit_or_boundary_pos)
+		   double timeout, size_t *size)
 {
 	const int NETBOX_READAHEAD = 16320;
 	if (timeout < 0) {
@@ -464,7 +464,7 @@ netbox_communicate(int fd, struct ibuf *send_buf, struct ibuf *recv_buf,
 		/* reader serviced first */
 check_limit:
 		if (ibuf_used(recv_buf) >= limit) {
-			*limit_or_boundary_pos = limit;
+			*size = limit;
 			return 0;
 		}
 		const char *p;
@@ -472,7 +472,7 @@ check_limit:
 					recv_buf->rpos,
 					ibuf_used(recv_buf),
 					boundary, boundary_len)) != NULL) {
-			*limit_or_boundary_pos = p - recv_buf->rpos;
+			*size = p - recv_buf->rpos;
 			return 0;
 		}
 
@@ -480,7 +480,7 @@ check_limit:
 			void *p = ibuf_reserve(recv_buf, NETBOX_READAHEAD);
 			if (p == NULL) {
 				diag_set(OutOfMemory, NETBOX_READAHEAD,
-					 "ibuf", "recv_buf");
+					 "ibuf_reserve", "p");
 				return -1;
 			}
 			ssize_t rc = recv(