[Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Vladislav Shpilevoy]

Users keep complaining about too short fiber name. New limit is
255, should be enough for any sane name.

Closes #4394

@TarantoolBot document
Title: fiber.name length limit.

It was 32, now it is 255. Besides, it seems like `fiber.name`
`{truncate = true}` option is not documented.

By default, if a new name is too long, `fiber.name(new_name)`
fails with an exception. To make it always succeed there is an
option truncate: `fiber.name(new_name, {truncate = true})`. It
truncates the name to the max length if it is too long.
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4394-fiber-name
Issue: https://github.com/tarantool/tarantool/issues/4394

@ChangeLog
- Fiber.name length new limit is 255. Previously was 32 (gh-4394).

I made it panic() if set_name fails with OOM, because otherwise it
leads to ugly changes in places, where it was assumed
fiber_set_name() never fails: cord_create() and swim_cfg().

cord_create() can't fail, since is called from a newly created
thread. Allowance to fail would lead to necessity to be able to
return an error from a new thread after it is already started.

swim_cfg() shall not fail at the moment of calling
fiber_set_name(), because otherwise it will make swim_cfg() not
atomic. I then will need something like fiber_reserve_name() to
call before it is too late to fail.

However, I can do these changed if a reviewer wants.

 src/lib/core/fiber.c    | 13 +++++++++++--
 src/lib/core/fiber.h    |  4 ++--
 test/app/fiber.result   |  8 ++++----
 test/app/fiber.test.lua |  2 +-
 test/unit/fiber.cc      |  6 +++---
 test/unit/fiber.result  |  2 +-
 6 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index ada7972cb..abd281dab 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -912,8 +912,13 @@ fiber_loop(MAYBE_UNUSED void *data)
 void
 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);
+	if (new_name == NULL)
+		panic("fiber_set_name() can't fail");
+	fiber->name = new_name;
+	memcpy(new_name, name, len);
+	new_name[len] = 0;
 }
 
 static inline void *
@@ -1242,6 +1247,8 @@ fiber_destroy(struct cord *cord, struct fiber *f)
 	region_destroy(&f->gc);
 	fiber_stack_destroy(f, &cord->slabc);
 	diag_destroy(&f->diag);
+	free(f->name);
+	f->name = NULL;
 }
 
 void
@@ -1357,6 +1364,7 @@ cord_create(struct cord *cord, const char *name)
 	fiber_reset(&cord->sched);
 	diag_create(&cord->sched.diag);
 	region_create(&cord->sched.gc, &cord->slabc);
+	cord->sched.name = NULL;
 	fiber_set_name(&cord->sched, "sched");
 	cord->fiber = &cord->sched;
 
@@ -1405,6 +1413,7 @@ cord_destroy(struct cord *cord)
 	}
 	region_destroy(&cord->sched.gc);
 	diag_destroy(&cord->sched.diag);
+	free(cord->sched.name);
 	slab_cache_destroy(&cord->slabc);
 }
 
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 8a3e5aef5..43c5e422e 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -111,7 +111,7 @@ struct cpu_stat {
 
 #endif /* ENABLE_FIBER_TOP */
 
-enum { FIBER_NAME_MAX = 32 };
+enum { FIBER_NAME_MAX = 255 };
 
 /**
  * Fiber ids [0; 100] are reserved.
@@ -510,7 +510,7 @@ struct fiber {
 	struct ipc_wait_pad *wait_pad;
 	/** Exception which caused this fiber's death. */
 	struct diag diag;
-	char name[FIBER_NAME_MAX + 1];
+	char *name;
 };
 
 /** Invoke on_stop triggers and delete them. */
diff --git a/test/app/fiber.result b/test/app/fiber.result
index 6d9604ad8..7331f61b3 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1186,7 +1186,7 @@ fiber = nil
 ---
 ...
 --
--- gh-2622, gh-4011: fiber.name() truncates new name.
+-- gh-2622, gh-4011, gh-4394: fiber.name() truncates new name.
 --
 fiber = require('fiber')
 ---
@@ -1214,14 +1214,14 @@ fiber.name(long_name, {truncate = true})
 ...
 fiber.name()
 ---
-- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 ...
 f = fiber.self()
 ---
 ...
 fiber.name(f)
 ---
-- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 ...
 fiber.name(f, 'new_name')
 ---
@@ -1239,7 +1239,7 @@ fiber.name(f, long_name, {truncate = true})
 ...
 fiber.name(f)
 ---
-- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 ...
 --
 -- gh-3493 fiber.create() does not roll back memtx transaction
diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua
index 6df210d9c..b8e9abc6e 100644
--- a/test/app/fiber.test.lua
+++ b/test/app/fiber.test.lua
@@ -504,7 +504,7 @@ test_run:cmd("setopt delimiter ''");
 fiber = nil
 
 --
--- gh-2622, gh-4011: fiber.name() truncates new name.
+-- gh-2622, gh-4011, gh-4394: fiber.name() truncates new name.
 --
 fiber = require('fiber')
 long_name = string.rep('a', 300)
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index 91f7d43f9..b0dfc9b14 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -171,9 +171,9 @@ fiber_name_test()
 
 	note("set new fiber name: %s.\n", fiber_name(fiber()));
 
-	const char *long_name = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
-		"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"\
-		"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+	char long_name[FIBER_NAME_MAX + 30];
+	memset(long_name, 'a', sizeof(long_name));
+	long_name[sizeof(long_name) - 1] = 0;
 
 	fiber_set_name(fiber(), long_name);
 
diff --git a/test/unit/fiber.result b/test/unit/fiber.result
index 7c9f85dcd..a61e0a2b8 100644
--- a/test/unit/fiber.result
+++ b/test/unit/fiber.result
@@ -5,7 +5,7 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc
 
 # set new fiber name: Horace.
 
-# fiber name is truncated: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.
+# fiber name is truncated: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.
 
 	*** fiber_name_test: done ***
 	*** fiber_join_test ***
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Cyrill Gorcunov]

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 :(

If 32 bytes is really not enough lest make it 64 (or 128)
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;

Re: [Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Vladislav Shpilevoy]

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;

Re: [Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Vladislav Shpilevoy]

Besides, most of the users don't use long names. So for them
it would be overkill to allocate such a big name memory for each
fiber.

Re: [Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Cyrill Gorcunov]

On Sat, Mar 14, 2020 at 06:32:01PM +0100, Vladislav Shpilevoy wrote:
> Besides, most of the users don't use long names. So for them
> it would be overkill to allocate such a big name memory for each
> fiber.

Look, currently all fiber data (including name) placed in one
continious memory area. After the patch the name gonna be "somewhere",
moreover this "somewhere" is completely unpredictable and access
to it will cause (i'm pretty sure) useless cache drains. If we
really need to support long names then we better make some union
which would carry both, short 32 byte names or long dynamically
allocated names.

And no, the statement "heap allocation doesn't affect anyone"
is simply wrong, this is not how memory works on a low level
(otherwise we wouldn't use "small" helpers at all, plain libc
malloc would fit us without a problem :).

Re: [Tarantool-patches] [PATCH 1/1] fiber: extend max fiber name length to 255

[Vladislav Shpilevoy]

On 14/03/2020 20:48, Cyrill Gorcunov wrote:
> On Sat, Mar 14, 2020 at 06:32:01PM +0100, Vladislav Shpilevoy wrote:
>> Besides, most of the users don't use long names. So for them
>> it would be overkill to allocate such a big name memory for each
>> fiber.
> 
> Look, currently all fiber data (including name) placed in one
> continious memory area. After the patch the name gonna be "somewhere",

Yeah, I know what is memory fragmentation. My point is that we
don't use heap for any performance critical data. Everything
in memtx is stored in slab arena. It is not affected by heap
fragmentation.

> moreover this "somewhere" is completely unpredictable and access
> to it will cause (i'm pretty sure) useless cache drains. If we

fiber name is used only in logs and in Lua. Here +1 indirect access
to its name won't add even -0.00001% to total application perf.

> really need to support long names then we better make some union
> which would carry both, short 32 byte names or long dynamically
> allocated names.

Ok, we discussed that in private and agreed to use this way:
'inline' or 'hybrid' string. 32 bytes will be kept as is for
short names, and longs will be allocated on the heap on demand.

> And no, the statement "heap allocation doesn't affect anyone"
> is simply wrong, this is not how memory works on a low level
> (otherwise we wouldn't use "small" helpers at all, plain libc
> malloc would fit us without a problem :).

Exactly, we use 'small' for everything except long living meta
such as spaces, indexes, key defs, functions, including all their
names, which can be up to 65KB btw. And this meta is accessed
*much* more often - on every request. And still, it is not even
close to the slowest paths we have.

Besides, there actually was an experiment to drop 'small' completely.
And use jemalloc for all allocations. It was long time ago, when
our perf was even better than now. And from what I remember we
got just about -10% or so (I may be wrong, the results were not
persisted anywhere). Even lower than after comparators were complicated
with functional and json indexes.