From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 15410469719 for ; Sat, 14 Mar 2020 20:25:51 +0300 (MSK) References: <3d2907a0285c640ea09eec86c8922e43c5844953.1584198310.git.v.shpilevoy@tarantool.org> <20200314163432.GW27301@uranus> From: Vladislav Shpilevoy Message-ID: <5b4578df-aab3-968b-f9bd-a00b60bd6591@tarantool.org> Date: Sat, 14 Mar 2020 18:25:48 +0100 MIME-Version: 1.0 In-Reply-To: <20200314163432.GW27301@uranus> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org On 14/03/2020 17:34, Cyrill Gorcunov wrote: > On Sat, Mar 14, 2020 at 04:16:35PM +0100, Vladislav Shpilevoy wrote: >> Users keep complaining about too short fiber name. New limit is >> 255, should be enough for any sane name. >> >> Closes #4394 >> >> fiber_set_name(struct fiber *fiber, const char *name) >> { >> - assert(name != NULL); >> - snprintf(fiber->name, sizeof(fiber->name), "%s", name); >> + size_t len = MIN(strlen(name), FIBER_NAME_MAX); >> + char *new_name = realloc(fiber->name, len + 1); > > I don't like it completely. The fiber cache has been made > not just to eliminate new memory allocation but also to > reduce memory fragmentation and now we give a user a hand > to shuffle memory in easy path :( Heap fragmentation does not affect anything. We allocate data not on the heap, but on slab arena. Heap is used for metadata such as space_defs, index_defs, key_defs. Moreover, heap is used for everything used by Lua. I don't see any problem with allocating non-data objects on the heap. Talking of fiber cache - I think *this* may be a bad thing. Because fibers are never deleted. So from a certain point of time the mempool does not do speed up any alloc/free calls. And just occupies slabs that could be used for data, and increases the arena fragmentation. > If 32 bytes is really not enough lest make it 64 (or 128) If we make it 64 are more, we just aggravate the problem I described above. > and allocate together with fiber cache. > >> + if (new_name == NULL) >> + panic("fiber_set_name() can't fail"); >> + fiber->name = new_name; >> + memcpy(new_name, name, len); >> + new_name[len] = 0;