Re: [Tarantool-patches] [PATCH v3 02/10] xrow: introduce a PROMOTE entry

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

16.04.2021 02:19, Vladislav Shpilevoy пишет:
> I appreciate the work you did here!
>
> See 2 comments below.
>
>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>> index cc8e43ed4..5d515ce92 100644
>> --- a/src/box/xrow.c
>> +++ b/src/box/xrow.c
>> @@ -884,28 +884,63 @@ xrow_encode_dml(const struct request *request, struct region *region,
>>   	return iovcnt;
>>   }
>>   
>> -void
>> -xrow_encode_synchro(struct xrow_header *row,
>> -		    struct synchro_body_bin *body,
>> -		    const struct synchro_request *req)
>> +static void
>> +xrow_encode_synchro_body(struct synchro_body_bin *body,
>> +		         const struct synchro_request *req)
>>   {
>>   	/*
>> -	 * A map with two elements. We don't compress
>> +	 * A map with two or three elements. We don't compress
>>   	 * numbers to have this structure constant in size,
>>   	 * which allows us to preallocate it on stack.
>>   	 */
>> -	body->m_body = 0x80 | 2;
>> +	body->m_body = 0x80 | (req->type == IPROTO_PROMOTE ? 3 : 2);
>>   	body->k_replica_id = IPROTO_REPLICA_ID;
>>   	body->m_replica_id = 0xce;
>>   	body->v_replica_id = mp_bswap_u32(req->replica_id);
>>   	body->k_lsn = IPROTO_LSN;
>>   	body->m_lsn = 0xcf;
>>   	body->v_lsn = mp_bswap_u64(req->lsn);
>> +}
>> +
>> +void
>> +xrow_encode_synchro(struct xrow_header *row,
>> +		    struct synchro_body_bin *body,
>> +		    const struct synchro_request *req)
>> +{
>> +	assert(req->type == IPROTO_CONFIRM || req->type == IPROTO_ROLLBACK);
>> +
>> +	xrow_encode_synchro_body(body, req);
>>   
>>   	memset(row, 0, sizeof(*row));
>> +	row->type = req->type;
>> +	row->body[0].iov_base = body;
>> +	row->body[0].iov_len = sizeof(*body);
>> +	row->bodycnt = 1;
>> +}
>> +
>> +static inline void
>> +xrow_encode_promote_body(struct promote_body_bin *body,
>> +			 const struct synchro_request *req)
> 1. I would propose to inline it. It is used in a single place,
> and now it looks like if we would have more than 1 place where
> we would need the promote body.
>
> But more interestingly, it looks you could keep it a single
> function xrow_encode_synchro. Although we wouldn't be able to
> have a PACKED struct with predefined fields. Not a big deal
> anyway.
>
> The reasoning is similar to xrow_encode_dml(). It also uses
> a single struct request for all kinds of DML, and conditionally
> encodes the non-zero fields. I think your case is the same. It
> would simplify some code, and remove branching from other
> places. For example, from txn_limbo_write_synchro(), where you
> branch between PROMOTE and non-PROMOTE when decide what to encode.
> We even had the same issue when tried to encode CONFIRM and
> ROLLBACK via separate functions.

Ok, let's encode the entries together.

Incremental diff is below.

>
>> +{
>> +	xrow_encode_synchro_body(&body->base, req);
>> +
>> +	body->k_term = IPROTO_TERM;
>> +	body->m_term = 0xcf;
>> +	body->v_term = mp_bswap_u64(req->term);
>> +}
>> +
>>   
>> +void
>> +xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body,
>> +		    const struct synchro_request *req)
>> +{
>> +	assert(req->type == IPROTO_PROMOTE);
>> +
>> +	xrow_encode_promote_body(body, req);
>> +
>> +	memset(row, 0, sizeof(*row));
>>   	row->type = req->type;
>> -	row->body[0].iov_base = (void *)body;
>> +	row->body[0].iov_base = body;
> 2. Unnecessary change. But I don't mind, up to you.

This was accidental, but let's keep the change.
I'm either removing the unnecessary cast from xrow_encode_synchro()
or adding it to xrow_encode_promote().

I'd better remove it rather than add another place where it's used.

>
>>   	row->body[0].iov_len = sizeof(*body);
>>   	row->bodycnt = 1;
>>   }


=======================================================

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index addcb0f97..c96e497c6 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -331,7 +331,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, 
uint16_t type, int64_t lsn)
       * This is a synchronous commit so we can
       * allocate everything on a stack.
       */
-    struct synchro_body_bin body;
+    char body[XROW_SYNCHRO_BODY_LEN_MAX];
      struct xrow_header row;
      char buf[sizeof(struct journal_entry) +
           sizeof(struct xrow_header *)];
@@ -339,7 +339,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, 
uint16_t type, int64_t lsn)
      struct journal_entry *entry = (struct journal_entry *)buf;
      entry->rows[0] = &row;

-    xrow_encode_synchro(&row, &body, &req);
+    xrow_encode_synchro(&row, body, &req);

      journal_entry_create(entry, 1, xrow_approx_len(&row),
                   txn_limbo_write_cb, fiber());
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 5b689a86a..2e364cea5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -884,56 +884,34 @@ xrow_encode_dml(const struct request *request, 
struct region *region,
      return iovcnt;
  }

-static void
-xrow_encode_synchro_body(struct synchro_body_bin *body,
-                 const struct synchro_request *req)
-{
-    /*
-     * A map with two or three elements. We don't compress
-     * numbers to have this structure constant in size,
-     * which allows us to preallocate it on stack.
-     */
-    body->m_body = 0x80 | (req->type == IPROTO_PROMOTE ? 3 : 2);
-    body->k_replica_id = IPROTO_REPLICA_ID;
-    body->m_replica_id = 0xce;
-    body->v_replica_id = mp_bswap_u32(req->replica_id);
-    body->k_lsn = IPROTO_LSN;
-    body->m_lsn = 0xcf;
-    body->v_lsn = mp_bswap_u64(req->lsn);
-}
-
  void
-xrow_encode_synchro(struct xrow_header *row,
-            struct synchro_body_bin *body,
+xrow_encode_synchro(struct xrow_header *row, char *body,
              const struct synchro_request *req)
  {
-    assert(req->type == IPROTO_CONFIRM || req->type == IPROTO_ROLLBACK);
+    assert(iproto_type_is_synchro_request(req->type));

-    xrow_encode_synchro_body(body, req);
+    char *pos = body;

-    memset(row, 0, sizeof(*row));
-    row->type = req->type;
-    row->body[0].iov_base = body;
-    row->body[0].iov_len = sizeof(*body);
-    row->bodycnt = 1;
-}
+    pos = mp_encode_map(pos,
+                iproto_type_is_promote_request(req->type) ? 3 : 2);

-void
-xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body,
-            const struct synchro_request *req)
-{
-    assert(req->type == IPROTO_PROMOTE);
+    pos = mp_encode_uint(pos, IPROTO_REPLICA_ID);
+    pos = mp_encode_uint(pos, req->replica_id);

-    xrow_encode_synchro_body(&body->base, req);
+    pos = mp_encode_uint(pos, IPROTO_LSN);
+    pos = mp_encode_uint(pos, req->lsn);
+
+    if (iproto_type_is_promote_request(req->type)) {
+        pos = mp_encode_uint(pos, IPROTO_TERM);
+        pos = mp_encode_uint(pos, req->term);
+    }

-    body->k_term = IPROTO_TERM;
-    body->m_term = 0xcf;
-    body->v_term = mp_bswap_u64(req->term);
+    assert(pos - body < XROW_SYNCHRO_BODY_LEN_MAX);

      memset(row, 0, sizeof(*row));
      row->type = req->type;
      row->body[0].iov_base = body;
-    row->body[0].iov_len = sizeof(*body);
+    row->body[0].iov_len = pos - body;
      row->bodycnt = 1;
  }

diff --git a/src/box/xrow.h b/src/box/xrow.h
index 80a5cd909..b3c664be2 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -49,6 +49,7 @@ enum {
      XROW_IOVMAX = XROW_HEADER_IOVMAX + XROW_BODY_IOVMAX,
      XROW_HEADER_LEN_MAX = 52,
      XROW_BODY_LEN_MAX = 256,
+    XROW_SYNCHRO_BODY_LEN_MAX = 32,
      IPROTO_HEADER_LEN = 28,
      /** 7 = sizeof(iproto_body_bin). */
      IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7,
@@ -260,25 +261,6 @@ struct synchro_request {
      uint64_t term;
  };

-/** Synchro request xrow's body in MsgPack format. */
-struct PACKED synchro_body_bin {
-    uint8_t m_body;
-    uint8_t k_replica_id;
-    uint8_t m_replica_id;
-    uint32_t v_replica_id;
-    uint8_t k_lsn;
-    uint8_t m_lsn;
-    uint64_t v_lsn;
-};
-
-/** PROMOTE request's xrow body in MsgPack format. */
-struct PACKED promote_body_bin {
-    struct synchro_body_bin base;
-    uint8_t k_term;
-    uint8_t m_term;
-    uint64_t v_term;
-};
-
  /**
   * Encode synchronous replication request.
   * @param row xrow header.
@@ -286,18 +268,7 @@ struct PACKED promote_body_bin {
   * @param req Request parameters.
   */
  void
-xrow_encode_synchro(struct xrow_header *row,
-            struct synchro_body_bin *body,
-            const struct synchro_request *req);
-
-/**
- * Encode a promote request.
- * @param row xrow header.
- * @param body A place to encode promote body.
- * @param req Request parameters.
- */
-void
-xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body,
+xrow_encode_synchro(struct xrow_header *row, char *body,
              const struct synchro_request *req);

  /**

-- 
Serge Petrenko