Re: [Tarantool-patches] [PATCH luajit 2/2] test: rewrite sysprof test using managed execution

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!
Thanks for the fixes!
Please, consider my comments below.

On 05.12.23, Igor Munkin wrote:
> Sergey,
> 
> Thanks for your review!
> 
> On 05.12.23, Sergey Kaplun wrote:
> > Hi, Igor!
> > Thanks for the patch!
> > Really awesome improvement for our sysprof testing!
> > Please consider my comments below.
> > 
> > On 03.12.23, Maxim Kokryashkin wrote:

<snipped>

> > > > +/* Sysprof "/dev/null" stream helpers. {{{ */
> > > > +
> > > > +/*
> > > > + * Yep, 8Mb. Tuned in order not to bother the platform with too
> > > > + * often flushes.
> > > > + */
> > 
> > This comment is misleading since there is no any flushes in the writer
> > function.
> 
> Well, it's not about I/O flushes, but rather about sysprof write-buffer
> pressure, isn't it? Hence, 8Mb is needed to make the test omit almost
> all buffer-flush related callbacks.

Ok, got your point. So mention this in the comment instead. And remove
"/dev/null" mentioning.

> 
> > Also, do we need such big buffer? Maybe `PATH_MAX` * 2 (or something
> > like that) should be enough?
> 
> Could you elaborate the reason for such a strange constant, please?

Considering your comment above this isn't relevant no more. Ignore
please.

> 
> > 
> > > > +#define STREAM_BUFFER_SIZE (8 * 1024 * 1024)
> > > > +#define DEVNULL -1
> > 
> > `DEVNULL` name is misleading (like we've actually opened `/dev/null`).
> > Maybe `DUMMY_FD` is better? Also, I suppose we can drop the `fd` field
> > at all. We can just use static structure in this translation unit and
> > compare pointer against it, so there is no need for an additional field
> > fd in the structure itself.
> 
> I see little reasoning in this suggestion: the approach with the
> structure is quite straight-forward and copied from another sysprof test
> implemented in C. Re naming: I prefer to leave it as is.

I don't get why it is straight-forward. This approach with unused fields
is just misleading. In
<test/tarantool-c-tests/misclib-sysprof-capi.test.c> the `fd` field is
used since the "/dev/null" is opened and data written to it.

Also, since we can use custom writer, that test may be updated wo using
`fd` too. But this should be done as a separate refactoring patch.

> 
> In general, your nit is nice, but I believe it should be applied
> for the patch moving <ptrace> tricks to utils library.

I don't see any relations with ptrace here. It's just the usage of
sysprof's C API.

> 
> > 
> > > > +

<snipped>

> > 
> > Typo: s/<lj_fff_res1>/the <lj_fff_res1>/
> > 
> > > > +	 * FIXME: Make this part test agnostic.
> > 
> > Typo: s/test agnostic/test-agnostic/
> > 
> > Nit: TODO looks more valid for me (since it's working).
> > Feel free to ignore.
> 
> Fixed.
> 
> ================================================================================
> 
> diff --git a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
> index 44421812..036f8141 100644
> --- a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
> +++ b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
> @@ -153,8 +153,8 @@ static int tracee(const char *luacode)
>  
>  	/*
>  	 * XXX: The only event to be streamed must be FFUNC at
> -	 * <lj_fff_res1> instruction.
> -	 * FIXME: Make this part test agnostic.
> +	 * the <lj_fff_res1> instruction.
> +	 * TODO: Make this part test agnostic.

Typo: s/test agnostic/test-agnostic/


> 

<snipped>

> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun