From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com
 [209.85.167.66])
 (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 1134B469719
 for <tarantool-patches@dev.tarantool.org>;
 Sat, 14 Nov 2020 21:00:37 +0300 (MSK)
Received: by mail-lf1-f66.google.com with SMTP id r9so18842785lfn.11
 for <tarantool-patches@dev.tarantool.org>;
 Sat, 14 Nov 2020 10:00:37 -0800 (PST)
Date: Sat, 14 Nov 2020 21:00:34 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Message-ID: <20201114180034.GM2021@grain>
References: <20201114172823.8217-1-sergepetrenko@tarantool.org>
 <20201114172823.8217-2-sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201114172823.8217-2-sergepetrenko@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/2] box: speed up
 tuple_field_map_create
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: v.shpilevoy@tarantool.org, tarantool-patches@dev.tarantool.org

On Sat, Nov 14, 2020 at 08:28:22PM +0300, Serge Petrenko wrote:
...
> +static int
> +tuple_field_map_create_plain(struct tuple_format *format, const char *tuple,
> +			     bool validate, struct field_map_builder *builder)
> +{
> +#define check_field_type(field, pos) {					       \
> +	if (validate &&							       \
> +	    !field_mp_type_is_compatible(field->type, pos,		       \
> +					 tuple_field_is_nullable(field))) {    \
> +		diag_set(ClientError, ER_FIELD_TYPE, tuple_field_path(field),  \
> +			 field_type_strs[field->type]);			       \
> +		return -1;						       \
> +	}								       \
> +}

Serge, I'm completely not familiar with the code thus may be simply wrong but
@check_field_type test for @validate first, right?
...
> +
> +	field = json_tree_lookup_entry(&format->fields, &format->fields.root,
> +				       &token, struct tuple_field, token);
> +	/* Check 1st field's type, but don't store offset to it. */
> +	check_field_type(field, pos);

	check_field_type -> if (validate ...)

> +	if (validate)
> +		bit_clear(required_fields, field->id);

and here we test for if (validate) again. Should not we simply
drop if (validate) from check_field_type and call this macro under
the caller's if? IOW

	if (validate) {
		check_field_type();
		bit_clear();
	}

While check_field_type will be something like

#define check_field_type(field, pos)					\
({									\
	bool nullable = tuple_field_is_nullable(field);			\
	if (!field_mp_type_is_compatible(field->type, pos, nullable)) {	\
		diag_set(ClientError, ER_FIELD_TYPE,			\
			tuple_field_path(field),			\
			field_type_strs[field->type]);			\
		return -1;						\
	}								\
})

 - if I'm right we may fix it on top (actually since these two ifs are close
   to each other they won't hurt hw branch predictor even in current form or
   may be compiler merge these two basic blocks under one "if" flow)

> +		check_field_type(field, pos);
> +		if (validate)
> +			bit_clear(required_fields, field->id);

and here too.

> +		if (field->offset_slot != TUPLE_OFFSET_SLOT_NIL &&
> +		    field_map_builder_set_slot(builder, field->offset_slot,
> +					       pos - tuple, MULTIKEY_NONE,
> +					       0, NULL) != 0) {
> +			return -1;
> +		}
> +	}