[tarantool-patches] [PATCH 1/1] core: make tt_static buffers a singleton array

[tarantool-patches] [PATCH 1/1] core: make tt_static buffers a singleton array

[Vladislav Shpilevoy]

tt_static_buf() is a function rotating 4 thread local buffers
declared inside this function as 'static' variables. They are
used very often for a plenty of purposes from string formatting
to small MessagePack encoding/decoding.

This array of 4 buffers was supposed to be a singleton object,
but function tt_static_buf() did not belong to a one certain
linking item - it was 'static inline' function in a header
'trivia/util.h'. It means that *each* linking item (each .c file
using these buffers) included its own version of this function
with its own *copy* of these 4 buffers.

Totally there were nearly 50 copies of 4Kb buffer in various
files, according to the difference seen in 'vmmap' program on
Mac (like /proc/pid/maps on Linux). This patch frees 200Kb of
BSS section.
---
Branch: https://github.com/tarantool/tarantool/tree/gerold103/make-static-bufs-real-singleton

 src/lib/core/util.c |  3 +++
 src/trivia/util.h   | 16 ++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/lib/core/util.c b/src/lib/core/util.c
index 9458695b9..8c7e74fb9 100644
--- a/src/lib/core/util.c
+++ b/src/lib/core/util.c
@@ -45,6 +45,9 @@
 
 #include "say.h"
 
+__thread char tt_static_buf_storage[TT_STATIC_BUF_COUNT][TT_STATIC_BUF_LEN];
+__thread int tt_static_buf_i = TT_STATIC_BUF_COUNT - 1;
+
 /** Find a string in an array of strings.
  *
  * @param haystack  Array of strings. Either NULL
diff --git a/src/trivia/util.h b/src/trivia/util.h
index a3e84e6d5..d17badff3 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -479,8 +479,16 @@ int clock_gettime(uint32_t clock_id, struct timespec *tp);
 #define CLOCK_THREAD_CPUTIME_ID		3
 #endif
 
+enum {
+	TT_STATIC_BUF_COUNT = 4,
+};
+/** Can't be enum - used by preprocessor for static assertions. */
 #define TT_STATIC_BUF_LEN 1028
 
+extern __thread char tt_static_buf_storage[TT_STATIC_BUF_COUNT]
+					  [TT_STATIC_BUF_LEN];
+extern __thread int tt_static_buf_i;
+
 /**
  * Return a thread-local statically allocated temporary buffer of size
  * \a TT_STATIC_BUF_LEN
@@ -488,12 +496,8 @@ int clock_gettime(uint32_t clock_id, struct timespec *tp);
 static inline char *
 tt_static_buf(void)
 {
-	enum { TT_STATIC_BUFS = 4 };
-	static __thread char bufs[TT_STATIC_BUFS][TT_STATIC_BUF_LEN];
-	static __thread int bufno = TT_STATIC_BUFS - 1;
-
-	bufno = (bufno + 1) % TT_STATIC_BUFS;
-	return bufs[bufno];
+	tt_static_buf_i = (tt_static_buf_i + 1) % TT_STATIC_BUF_COUNT;
+	return tt_static_buf_storage[tt_static_buf_i];
 }
 
 /**
-- 
2.20.1 (Apple Git-117)

[tarantool-patches] Re: [PATCH 1/1] core: make tt_static buffers a singleton array

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/27 15:39]:
> tt_static_buf() is a function rotating 4 thread local buffers
> declared inside this function as 'static' variables. They are
> used very often for a plenty of purposes from string formatting
> to small MessagePack encoding/decoding.
> 
> This array of 4 buffers was supposed to be a singleton object,
> but function tt_static_buf() did not belong to a one certain
> linking item - it was 'static inline' function in a header
> 'trivia/util.h'. It means that *each* linking item (each .c file
> using these buffers) included its own version of this function
> with its own *copy* of these 4 buffers.
> 
> Totally there were nearly 50 copies of 4Kb buffer in various
> files, according to the difference seen in 'vmmap' program on
> Mac (like /proc/pid/maps on Linux). This patch frees 200Kb of
> BSS section.

OK to push. Please push to 2.1 and 1.10 as well.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32

[tarantool-patches] Re: [PATCH 1/1] core: make tt_static buffers a singleton array

[Vladislav Shpilevoy]

On 28/04/2019 00:11, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/27 15:39]:
>> tt_static_buf() is a function rotating 4 thread local buffers
>> declared inside this function as 'static' variables. They are
>> used very often for a plenty of purposes from string formatting
>> to small MessagePack encoding/decoding.
>>
>> This array of 4 buffers was supposed to be a singleton object,
>> but function tt_static_buf() did not belong to a one certain
>> linking item - it was 'static inline' function in a header
>> 'trivia/util.h'. It means that *each* linking item (each .c file
>> using these buffers) included its own version of this function
>> with its own *copy* of these 4 buffers.
>>
>> Totally there were nearly 50 copies of 4Kb buffer in various
>> files, according to the difference seen in 'vmmap' program on
>> Mac (like /proc/pid/maps on Linux). This patch frees 200Kb of
>> BSS section.
> 
> OK to push. Please push to 2.1 and 1.10 as well.

I will not push this commit into the master, because it is
fixed by the static memory allocator. See the new mail thread.

Talking of 1.10 and 2.1 - I could push if you approve that the
bug will be fixed differently in 1.10/2.1 vs the master. Not
sure if we should push a new allocator into these branches.

> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> 

[tarantool-patches] Re: [PATCH 1/1] core: make tt_static buffers a singleton array

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/28 20:01]:
> I will not push this commit into the master, because it is
> fixed by the static memory allocator. See the new mail thread.
> 
> Talking of 1.10 and 2.1 - I could push if you approve that the
> bug will be fixed differently in 1.10/2.1 vs the master. Not
> sure if we should push a new allocator into these branches.

It's a bit annoying you don't push an approved patch :), OK, I
agree, please fix the bug in 1.10 and 2.1 and push the static
allocator to master. The static alloc patch is basically good to
go, I would get rid of tt_static.h altogether and move all these
utility functions to small, as a minor comment.

I will reply to these patches separately.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32

[tarantool-patches] Re: [PATCH 1/1] core: make tt_static buffers a singleton array

[Vladislav Shpilevoy]

Pushed to 2.1 and 1.10.

On 28/04/2019 21:31, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/28 20:01]:
>> I will not push this commit into the master, because it is
>> fixed by the static memory allocator. See the new mail thread.
>>
>> Talking of 1.10 and 2.1 - I could push if you approve that the
>> bug will be fixed differently in 1.10/2.1 vs the master. Not
>> sure if we should push a new allocator into these branches.
> 
> It's a bit annoying you don't push an approved patch :), OK, I
> agree, please fix the bug in 1.10 and 2.1 and push the static
> allocator to master. The static alloc patch is basically good to
> go, I would get rid of tt_static.h altogether and move all these
> utility functions to small, as a minor comment.
> 
> I will reply to these patches separately.
> 
> -- 
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
>