Re: [Tarantool-patches] [PATCH v2] refactoring: drop excess 16Kb bss buffer

[Leonid Vasiliev <lvasiliev@tarantool.org>]

On 2/1/20 1:30 AM, Sergey Kaplun wrote:
> Hi!
> 
> Thanks for your feedback!
> 
> On 30.01.20, Leonid Vasiliev wrote:
>>
>> Hi.
>>
>> On 1/29/20 4:36 PM, Sergey Ostanevich wrote:
>>> Hi!
>>>
>>> Thanks for the patch!
>>>
>>> LGTM after updates below.
>>>
>>> Regards,
>>> Sergos
>>>
>>>
>>> On 23 Jan 23:04, Vladislav Shpilevoy wrote:
>>>> Hi! Thanks for the patch!
>>>>
>>>> I agree with everything what Leonid said.
>>>
>>> I will object both assertion and NULL handling. Just have a look:
>>>
>>>>> +   char *backtrace_buf = (char *)static_alloc(SMALL_STATIC_SIZE);
>>>
>>> And in static_reserve(size_t size) called from static_alloc():
>>>
>>>                   if (size > SMALL_STATIC_SIZE)
>>>                                           return NULL;
>>>
>>> is the only place NULL can be returned. No need to test if
>>> SMALL_STATIC_SIZE > SMALL_STATIC_SIZE, just ask me :)
>>
>> Hmm, it's sounds like:"Client must known about implementation of library
>> function". But implementation can be changed with the contract save.
>>
> 
> To be honest, I do not see any dependencies on internal API in this,
> since the module determines SMALL_STATIC_SIZE and provides
> 
>      extern __thread char static_storage_buffer[SMALL_STATIC_SIZE];
> 
> - all of this is part of API, isn't it?
> 
> Proposal: we can use sizeof(static_storage_buffer) to avoid usage
> of "magic" SMALL_STATIC_SIZE.
> 

Ok)
Just for clarity.
My point of view on an interface of small static_alloc function: "It's 
return a pointer which can be a NULL and I must to check it". In my mind 
the realization of the static_alloc can be changed to "return NULL;" and 
my code must continue to work fine.
Your point of  view: "static_alloc can alloc/reserve of size <= 
SMALL_STATIC_SIZE in any case and it's interface guarantees this".
In this case I'm agree with Sergey Ostanevich.

>>>
>>>
>>>
>>>>
>>>> On 23/01/2020 09:44, Sergey Kaplun wrote:
>>>>> We already have 12Kb thread-safe static buffer
>>>>> in `lib/small/small/static.h`, that can be used instead of 16Kb
>>>>> bss buffer in `src/lib/core/backtrace.cc` for backtrace payload.
>>>>> Closes #4650
>>>>> ---
>>>>>
>>>>> branch: https://github.com/tarantool/tarantool/tree/skaplun/drop-bss-buff
>>>>> issue: https://github.com/tarantool/tarantool/issues/4650
>>>>>
>>>>>    src/lib/core/backtrace.cc | 8 +++++---
>>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc
>>>>> index 77f77b05c..c70576b53 100644
>>>>> --- a/src/lib/core/backtrace.cc
>>>>> +++ b/src/lib/core/backtrace.cc
>>>>> @@ -33,6 +33,7 @@
>>>>>    
>>>>>    #include <stdlib.h>
>>>>>    #include <stdio.h>
>>>>> +#include <assert.h>
>>>>>    
>>>>>    #include <cxxabi.h>
>>>>>    
>>>>> @@ -47,6 +48,7 @@
>>>>>    #include <libunwind.h>
>>>>>    
>>>>>    #include "small/region.h"
>>>>> +#include "small/static.h"
>>>>>    /*
>>>>>     * We use a global static buffer because it is too late to do any
>>>>>     * allocation when we are printing backtrace and fiber stack is
>>>
>>> Just change "global static buffer" for the "static buffer interface".
>>>
>>>>> @@ -55,8 +57,6 @@
>>>>>    
>>>>>    #define BACKTRACE_NAME_MAX 200
>>>>>    
>>>>> -static char backtrace_buf[4096 * 4];
>>>>> -
>>>>>    static __thread struct region cache_region;
>>>>>    static __thread struct mh_i64ptr_t *proc_cache = NULL;
>>>>>    
>>>>> @@ -140,8 +140,10 @@ backtrace()
>>>>>    	unw_getcontext(&unw_context);
>>>>>    	unw_cursor_t unw_cur;
>>>>>    	unw_init_local(&unw_cur, &unw_context);
>>>>> +	char *backtrace_buf = (char *)static_alloc(SMALL_STATIC_SIZE);
>>>>> +	assert(backtrace_buf);
>>>>>    	char *p = backtrace_buf;
>>>>> -	char *end = p + sizeof(backtrace_buf) - 1;
>>>>> +	char *end = p + SMALL_STATIC_SIZE - 1;
>>>>>    	int unw_status;
>>>>>    	*p = '\0';
>>>>>    	while ((unw_status = unw_step(&unw_cur)) > 0) {
>>>>>