Hi! Thanks for the review!

On 6/7/21 11:28 AM, Cyrill Gorcunov wrote:

> On Fri, Jun 04, 2021 at 02:13:10PM +0300, Egor Elchinov via Tarantool-patches wrote:
>> From: Egor2001 <elchinov.es@gmail.com>
>>
>> For now fiber creation backtrace is stored
>> in the separate subtable of fiber.info
>> called backtrace_parent for convenience.
>>
>> Lua stacks of fiber creation
>> aren't preserved in backtrace yet because
>> of need to somehow handle parent Lua state
>> inside the child fiber for this sake.
>>
>> Backtrace caching and demangling
>> aren't present yet too as this is a
>> proof-of-concept implementation.
>>
>> Needed for: #4002
>> ---
>>   
>> +/**
>> + * Collect up to `limit' IP register values
>> + * for frames of the current stack into `ip_buf'.
>> + * Must be by far faster than usual backtrace according to the
>> + * libunwind doc for unw_backtrace().
>> + */
>> +void
>> +fast_trace_collect(void **ip_buf, int limit)
>> +{
>> +	memset(ip_buf, 0, limit * sizeof(*ip_buf));
>> +	unw_backtrace(ip_buf, limit);
>> +}
> This is not guaranteed to be faster, so I would name it
> backtrace_collect_ip. Also you have to mark it as NOINLINE,
> otherwise IPs gonna be screwed. Moreover you put a special
> offset into IPs resolving inside fast_trace_foreach to hide
> the fast_trace_collect call itself, I think we should be
> very loud about, otherwise other people might get confusing
> what we're doing with frame numbers and why we skip the first
> frame.

Thanks, fixed.

> Also I personally not sure if we must collect fiber's creation
> backtrace for every fiber in a system even if we never need it,
> I'm pretty sure that backtrace is very far from cheap. But I
> left it up to you to decide. I guess we might need some kind
> of dynamic settings similar to fiber_top?

Good point, I've consulted with Alexander Lyapunov and
it was decided to add a dynamic setting enabling this feature.

>> +void
>> +fast_trace_foreach(backtrace_cb cb, void **ip_buf, int limit, void *cb_ctx)
>> +{
>> +	static __thread char proc_name[BACKTRACE_NAME_MAX];
> Why do you need it be per thread? There must be very strong reason why some
> data is put into TLS.

Thanks, fixed.

>> +	int frame_no = 0;
> This is shor routine and I think plain `n` would be more than enough.

For now that's true, but `frame_no` name was chosen mostly
for consistency with the present `backtrace` routines.
Also, routine will be more complicated after addition of
DARWIN `#ifdef` and demangling support.

>> +	unw_word_t ip = 0, offset = 0;
>> +	unw_proc_info_t pi;
>> +	int ret = 0;
>> +	char* proc = NULL;
>> +
>> +	unw_accessors_t* acc = unw_get_accessors(unw_local_addr_space);
>> +	assert(acc);
>> +
>> +	for (frame_no = 0; frame_no < limit && ip_buf[frame_no] != NULL;
>> +	     ++frame_no) {
>> +		ip = (unw_word_t)ip_buf[frame_no];
>> +		if (acc->get_proc_name == NULL) {
>> +			ret = unw_get_proc_info_by_ip(unw_local_addr_space,
>> +						      ip, &pi, NULL);
>> +			offset = ip - pi.start_ip;
> Why proc is left untouched here? it may carry old value from
> previous acc->get_proc_name call, is it ok?

It's ok, because if `proc` gets non-NULL value then
`acc->get_proc_name` routine is present in the system and
this part of code will never be called.
Otherwise `proc` can't be retrieved by the
libunwind API and has to be NULL.

>> +		} else {
>> +			ret = acc->get_proc_name(unw_local_addr_space, ip,
>> +			    			 proc_name, sizeof(proc_name),
>> +			    			 &offset, NULL);
>> +			proc = proc_name;
>> +		}
>> +
>> +		if (ret != 0 || (frame_no > 0 &&
>> +		    cb(frame_no - 1, (void *)ip, proc,
>> +	 	       (size_t)offset, cb_ctx) != 0))
>> +			break;
>> +	}
> Egor, I think this is very good PoC code! Here is the diff I made
> on top of your patch to share ideas. Not insisting anyhow on code
> refactoring, vars renaming and etc.
>
> Also I CC Vlad, since he might have own vision on overall code design.
> And since Vlad is the final line before code goes upstream better wait
> for his opinion.

Thanks a lot for your diff!
Also I haven't decided yet how to grab Lua stacks of
fiber creation in a cheap way.
I would be glad for any suggestions on this issue too.