Fix bug in small allocator in case of small slab_alloc_factor #27

EvgenyMekhanik · 2020-11-05T11:31:55Z

In case of small slab_alloc_factor, factor_pool_cache
array have no enought place. This caused us to use a pool
with incorrect object sizes. This patch changed allocator
behavior, now we allocate pools on the stage of allocator
creation. Also we search for the desired pool using binary
search in an ordered array, which is generally faster
than searching in a red-black tree.

Closes #5216

Gerold103 · 2020-11-09T10:29:36Z

I didn't look at the patch carefully, but generally we split bugfix and optimizations into separate commits. From this commit message I see that you fixed the bug and did an optimization. Can you split it?

EvgenyMekhanik · 2020-11-09T16:53:06Z

I didn't look at the patch carefully, but generally we split bugfix and optimizations into separate commits. From this commit message I see that you fixed the bug and did an optimization. Can you split it?

Ok! Now I am testing the performance of my changes, if it turns out that it is lower than it was before, then I will only fix the bug. If the performance is better, I will split the patch into several parts.

alyapunov · 2020-12-04T12:49:30Z

perf/small_alloc_perf.c

+static inline
+long long int timediff(struct timespec *tm1, struct timespec *tm2)
+{
+	return NANOSEC_PER_SEC * (tm2->tv_sec - tm1->tv_sec) + (tm2->tv_nsec - tm1->tv_nsec);


If you declare to return value as long long you should cast to long long before multiplication.
Imagine a system that has time_t = int = long = int32_t, long long = int64_t.
NANOSEC_PER_SEC is int32, tm2->tv_sec is int32, their product could overflow.
I would define NANOSEC_PER_SEC as const unsigned long long, that would be enough.

alyapunov · 2020-12-04T12:58:13Z

perf/small_alloc_perf.c

+	OBJECTS_MAX = 1000
+};
+
+#define SLAB_SIZE_MIN 4 * 1024 * 1024


Please don't use macros as constants.
Usually we use enums, I prerer static const (I like static const has a explicit type).

alyapunov · 2020-12-04T15:01:35Z

perf/small_alloc_perf.c

+	} else {
+		size = floor(256 * pow(pow_factor, pos));
+	}
+	fail_unless(size > 0 && size <= size_max);


We should remove all consistency check in perf test.

alyapunov · 2020-12-04T15:03:31Z

perf/small_alloc_perf.c

+{
+	struct timespec tm1, tm2;
+	if(human) {
+		fprintf(stderr, "|              SMALL RANDOM ALLOCATION RESULT TABLE                  |\n");


We usually limit code width with 80 symbols. Even in tests.

alyapunov · 2020-12-04T15:03:55Z

perf/small_alloc_perf.c

+	slab_arena_create(&arena, &quota, 0, slab_size, MAP_PRIVATE);
+	slab_cache_create(&cache, &arena);
+	if (human) {
+		fprintf(stderr, "|__________________________________|_________________________________|\n");


I like the output!

alyapunov · 2020-12-04T15:06:37Z

perf/small_alloc_perf.c

+		fprintf(stderr, "%s\n", json_output);
+	}
+	return EXIT_SUCCESS;
+}


It says that there is no newline at the end of file. It's better to avoid it.

alyapunov · 2020-12-04T15:08:00Z

perf/small_alloc_perf.c

+	if (human) {
+		fprintf(stderr, "|              LARGE RANDOM ALLOCATION RESULT TABLE                  |\n");
+		fprintf(stderr, "|____________________________________________________________________|\n");
+		fprintf(stderr, "|           alloc_factor           |            time, s              |\n");


As I see in seconds all periods are very small, like 0.007. I think we should measure time in milliseconds.

alyapunov · 2020-12-04T15:26:09Z

small/small.c

-	factor_tree_new(&alloc->factor_pools);
-	(void) factor_pool_create(alloc, NULL, alloc->objsize_max);
+	small_class_create(&alloc->small_class, sizeof(intptr_t), alloc->factor, objsize_min);
+	(void) factor_pool_create(alloc);


Why do you need (void) cast?

alyapunov · 2020-12-04T15:37:53Z

small/small.c

-		 */
-		return upper_bound;
+	size_t objsize = 0;
+	for (alloc->factor_pool_cache_size = 0; objsize < alloc->objsize_max &&


Please make a new line after ;. I've just started to write a review comment because I didn't noticed objsize < alloc->objsize_max condition.

alyapunov

LGTM except commit about CentOS6 built. It's not relevant to this task and must be linked to another issue.

Since fcac155 mempool.c depends on trivia/util.h, that is not a part of small library but a part of tarantool. Actually there's only one thing that is needed from the header - alignof macro. Extract it and make small library independent from tarantool. This issue has been fixed but this patch is more correct

small_alloc uses a collection of mempools of different sizes. If small_alloc stores all mempools in an array then it have to determine an offset in that array where the most suitable mempool is. Let's name the offset as 'size class' and the size that the corresponding mempool allocates as 'class size'. Historically the class sizes grow incrementally up to some point and then (at some size class) grow exponentially with user-provided factor. Apart from incremental part the exponential part is not very obvious. Binary search and floating-point logarithm could be used for size class determination but both approaches seem to be too slow. This patch implements faster size class evaluation based on integral operations. Part of #5216

In previous version allocator created new pool if necessary and inserted it in the pools tree. Now we allocate pools on the stage of allocator creation according alloc_factor. We use small_alloc class for this purpose also we use it to find necessary pool when we alloc memory. This is faster then previous allocator behavior and also fixes #5216. Closes #5216

EvgenyMekhanik requested a review from alyapunov November 5, 2020 11:31

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch 2 times, most recently from caed2ff to a2bd37d Compare November 9, 2020 09:25

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch 9 times, most recently from 6ec1f65 to 09d459a Compare November 13, 2020 17:25

EvgenyMekhanik assigned alyapunov Nov 30, 2020

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch from 09d459a to 30995ca Compare December 6, 2020 09:59

alyapunov suggested changes Dec 17, 2020

View reviewed changes

alyapunov assigned EvgenyMekhanik and unassigned alyapunov Dec 17, 2020

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch from 725b4e8 to 47aea0a Compare December 17, 2020 14:45

alyapunov approved these changes Dec 17, 2020

View reviewed changes

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch 3 times, most recently from 7b1a812 to 6bb3dfc Compare December 22, 2020 06:52

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch 5 times, most recently from b8224e8 to e6b5807 Compare December 24, 2020 17:47

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch 2 times, most recently from 89ef1fe to bdbe73c Compare December 25, 2020 11:18

alyapunov and others added 3 commits December 28, 2020 15:27

test: add small allocator performance test

509e9b5

EvgenyMekhanik force-pushed the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch from bdbe73c to e78e5a3 Compare December 28, 2020 12:29

Gerold103 self-requested a review December 28, 2020 12:40

Gerold103 approved these changes Dec 28, 2020

View reviewed changes

alyapunov closed this Feb 10, 2021

alyapunov deleted the mechanik20051988/gh-5216-fix-strange-allocator-behavior branch December 6, 2021 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix bug in small allocator in case of small slab_alloc_factor #27

Fix bug in small allocator in case of small slab_alloc_factor #27

Uh oh!

EvgenyMekhanik commented Nov 5, 2020

Uh oh!

Gerold103 commented Nov 9, 2020

Uh oh!

EvgenyMekhanik commented Nov 9, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov Dec 4, 2020

Uh oh!

alyapunov left a comment

Uh oh!

Uh oh!

Fix bug in small allocator in case of small slab_alloc_factor #27

Fix bug in small allocator in case of small slab_alloc_factor #27

Uh oh!

Conversation

EvgenyMekhanik commented Nov 5, 2020

Uh oh!

Gerold103 commented Nov 9, 2020

Uh oh!

EvgenyMekhanik commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyapunov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!