Re: [tarantool-patches] Re: [PATCH v4 2/5] iproto: replace obuf by mpstream in execute.c

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 04/12/2018 11:26, Vladimir Davydov wrote:
> On Mon, Dec 03, 2018 at 11:48:26PM +0300, Vladislav Shpilevoy wrote:
>>
>>
>> On 03/12/2018 18:21, Vladimir Davydov wrote:
>>> On Sun, Dec 02, 2018 at 02:03:21PM +0300, imeevma@tarantool.org wrote:
>>>> This patch is the most dubious patch due to the implicit use of
>>>> mpstream as a stream for obuf. Discussion and patch below.
>>>>
>>>> It is worth noting that in this version of the patch nothing
>>>> changes. At this point there is no approved solution for this
>>>> patch.
>>>>
>>>>
>>>> On 11/30/18 1:55 PM, Vladimir Davydov wrote:
>>>>> On Fri, Nov 30, 2018 at 01:45:48PM +0300, Vladislav Shpilevoy wrote:
>>>>>>
>>>>>>
>>>>>> On 30/11/2018 13:19, Vladimir Davydov wrote:
>>>>>>> On Thu, Nov 29, 2018 at 05:04:06PM +0300, Vladislav Shpilevoy wrote:
>>>>>>>> On 29/11/2018 13:53, Vladimir Davydov wrote:
>>>>>>>>> On Tue, Nov 27, 2018 at 10:25:43PM +0300, imeevma@tarantool.org wrote:
>>>>>>>>>> @@ -625,81 +608,53 @@ sql_prepare_and_execute(const struct sql_request *request,
>>>>>>>>>>    }
>>>>>>>>>>    int
>>>>>>>>>> -sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
>>>>>>>>>> +sql_response_dump(struct sql_response *response, int *keys,
>>>>>>>>>> +		  struct mpstream *stream)
>>>>>>>>>>    {
>>>>>>>>>>    	sqlite3 *db = sql_get();
>>>>>>>>>>    	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
>>>>>>>>>> -	struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
>>>>>>>>>>    	int rc = 0, column_count = sqlite3_column_count(stmt);
>>>>>>>>>>    	if (column_count > 0) {
>>>>>>>>>> -		if (sql_get_description(stmt, out, column_count) != 0) {
>>>>>>>>>> +		if (sql_get_description(stmt, stream, column_count) != 0) {
>>>>>>>>>>    err:
>>>>>>>>>>    			rc = -1;
>>>>>>>>>>    			goto finish;
>>>>>>>>>>    		}
>>>>>>>>>>    		*keys = 2;
>>>>>>>>>> -		int size = mp_sizeof_uint(IPROTO_DATA) +
>>>>>>>>>> -			   mp_sizeof_array(port_tuple->size);
>>>>>>>>>> -		char *pos = (char *) obuf_alloc(out, size);
>>>>>>>>>> -		if (pos == NULL) {
>>>>>>>>>> -			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>>>>>>>>>> -			goto err;
>>>>>>>>>> -		}
>>>>>>>>>> -		pos = mp_encode_uint(pos, IPROTO_DATA);
>>>>>>>>>> -		pos = mp_encode_array(pos, port_tuple->size);
>>>>>>>>>> -		/*
>>>>>>>>>> -		 * Just like SELECT, SQL uses output format compatible
>>>>>>>>>> -		 * with Tarantool 1.6
>>>>>>>>>> -		 */
>>>>>>>>>> -		if (port_dump_msgpack_16(&response->port, out) < 0) {
>>>>>>>>>> +		mpstream_encode_uint(stream, IPROTO_DATA);
>>>>>>>>>> +		mpstream_flush(stream);
>>>>>>>>>> +		if (port_dump_msgpack(&response->port, stream->ctx) < 0) {
>>>>>>>>>
>>>>>>>>> stream->ctx isn't guaranteed to be an obuf
>>>>>>>>>
>>>>>>>>> And when you introduce vstream later, you simply move this code to
>>>>>>>>> another file. This is confusing. May be we should pass alloc/reserve
>>>>>>>>> used in mpstream to port_dump instead of obuf?
>>>>>>>>
>>>>>>>> Good idea, though not sure, if it is worth slowing down port_dump_msgpack
>>>>>>>> adding a new level of indirection. Since port_dump_msgpack is a hot path
>>>>>>>> and is used for box.select.
>>>>>>>>
>>>>>>>> Maybe it is better to just rename port_dump_msgpack to port_dump_obuf
>>>>>>>> and rename vstream_port_dump to vstream_port_dump_obuf? If we ever will
>>>>>>>> dump port to not obuf, then we will just add a new method to port_vtab.
>>>>>>>>
>>>>>>>> Also, it would make port_dump_obuf name consistent with port_dump_lua -
>>>>>>>> in both cases we not just dump in a specific format, but to a concrete
>>>>>>>> destination: obuf and lua stack. Now port_dump_msgpack anyway is restricted
>>>>>>>> by obuf destination.
>>>>>>>
>>>>>>> There's port_dump_plain, which dumps port contents in a specific format.
>>>>>>> So port_dump_obuf would look ambiguous.
>>>>>>>
>>>>>>>>
>>>>>>>> If you worry about how to call sql_response_dump() to not obuf, then there
>>>>>>>> is another option. Anyway rename port_dump_msgpack to port_dump_obuf and
>>>>>>>> introduce a new method: port_dump_mpstream. It will take mpstream and use
>>>>>>>> its reserve/alloc/error functions. It allows us to do not slow down box.select,
>>>>>>>> but use the full power of virtual functions in execute.c, which definitely is
>>>>>>>> not hot.
>>>>>>>
>>>>>>> That would interconnect port and mpstream, make them dependent on each
>>>>>>> other. I don't think that would be good.
>>>>>>>
>>>>>>>>
>>>>>>>> mpstream implementation of vstream will call port_dump_mpstream, and
>>>>>>>> luastream implementation of vstream will call port_dump_lua as it does now.
>>>>>>>> box.select and iproto_call will use port_dump_obuf.
>>>>>>>>
>>>>>>>> I prefer the second option: introduce port_dump_mpstream. It is ok for you?
>>>>>>>
>>>>>>> I may be wrong, but IMO there isn't much point in optimizing box.select,
>>>>>>> because it's very limited in its applicability. People already prefer to
>>>>>>> use box.call over box.insert/select/etc over iproto, and with the
>>>>>>> appearance of box.execute they are likely to stop using plain box.select
>>>>>>> at all.
>>>>>>>
>>>>>>> That said, personally I would try to pass reserve/alloc methods to port,
>>>>>>> see how it goes.
>>>>>>>
>>>>>>
>>>>>> I do not see a reason to slow down box.select if we can don't do it.
>>>>>> Yeas, people use IPROTO_CALL, but in stored functions they use box
>>>>>> functions including select.
>>>>>
>>>>> box.select called from Lua code doesn't use port_dump_msgpack.
>>>>>
>>>>>>
>>>>>> Ok, instead of port_dump_mpstream we can rename port_dump_msgpack to
>>>>>> port_dump_obuf and add port_dump_msgpack which does not depend on
>>>>>> mpstream and takes alloc/reserve/ctx directly.
>>>>>
>>>>> Better call the optimized version (the one without callbacks)
>>>>> port_dump_msgpack_obuf to avoid confusion IMO.
>>>>>
>>>>> Anyway, I'd try to run cbench to see if it really perfomrs better
>>>>> than the one using callbacks.
>>>>
>>>> @@ -625,81 +608,53 @@ sql_prepare_and_execute(const struct sql_request *request,
>>>>    }
>>>>    int
>>>> -sql_response_dump(struct sql_response *response, int *keys, struct obuf *out)
>>>> +sql_response_dump(struct sql_response *response, int *keys,
>>>> +		  struct mpstream *stream)
>>>>    {
>>>>    	sqlite3 *db = sql_get();
>>>>    	struct sqlite3_stmt *stmt = (struct sqlite3_stmt *) response->prep_stmt;
>>>> -	struct port_tuple *port_tuple = (struct port_tuple *) &response->port;
>>>>    	int rc = 0, column_count = sqlite3_column_count(stmt);
>>>>    	if (column_count > 0) {
>>>> -		if (sql_get_description(stmt, out, column_count) != 0) {
>>>> +		if (sql_get_description(stmt, stream, column_count) != 0) {
>>>>    err:
>>>>    			rc = -1;
>>>>    			goto finish;
>>>>    		}
>>>>    		*keys = 2;
>>>> -		int size = mp_sizeof_uint(IPROTO_DATA) +
>>>> -			   mp_sizeof_array(port_tuple->size);
>>>> -		char *pos = (char *) obuf_alloc(out, size);
>>>> -		if (pos == NULL) {
>>>> -			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>>>> -			goto err;
>>>> -		}
>>>> -		pos = mp_encode_uint(pos, IPROTO_DATA);
>>>> -		pos = mp_encode_array(pos, port_tuple->size);
>>>> -		/*
>>>> -		 * Just like SELECT, SQL uses output format compatible
>>>> -		 * with Tarantool 1.6
>>>> -		 */
>>>> -		if (port_dump_msgpack_16(&response->port, out) < 0) {
>>>> +		mpstream_encode_uint(stream, IPROTO_DATA);
>>>> +		mpstream_flush(stream);
>>>> +		if (port_dump_msgpack(&response->port, stream->ctx) < 0) {
>>>
>>> Still, I'm quite convinced that we need to pass alloc/reserve methods
>>> along with ctx to port_dump_msgpack(), because implicitly assumping that
>>> mpstream->ctx is, in fact, an obuf looks very fragile. However, Vlad is
>>> right that it may indeed affect performance in a negative way. So let's
>>> perhaps do the following:
>>>
>>>    1. Run cbench to see how badly indirect obuf_alloc/reserve slows
>>>       things down.
>>>
>>>    2. Consider the possibility of using templates or macro definitions
>>>       instead of function pointers.
>>>
>>> What do you think?
>>>
>>
>> Good plan except one thing in its second point: port still must feature
>> double-virtualized method taking alloc/reserve to be "dumpable" via
>> mpstream. Yes, we can leave obuf method, even add region dump method in
>> future, but for mpstream it requires virtual alloc/reserve anyway
>> (until mpstream is templated). My point is in saving every single
>> percent of perf for calls and selects. For SQL alloc/reserve is enough.
> 
> Anyway, it'd be nice to see how much it's going to save, exactly.

Ok. Kostja yesterday verbally said that it is not worth benching such minor
thing and we should not introduce second virtualization level, but lets wait
for a written approval.