Re: [Tarantool-patches] [PATCH v6 4/5] datetime: perf test for datetime parser

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Thu, Aug 19, 2021 at 01:29:06PM +0300, Safin Timur wrote:
> On 19.08.2021 13:19, Serge Petrenko wrote:
> > 
> > 
> > 19.08.2021 05:56, Timur Safin пишет:
> > > It was told that if field `datetime.secs` would be `double` we should get
> > > better performance in LuaJIT instead of `uint64_t` type, which is
> > > used at the
> > > moment.
> > > 
> > > So we have created benchmark, which was comparing implementations of
> > > functions
> > > from `datetime.c` if we would use `double` or `int64_t` for
> > > `datetime.secs` field.
> > > 
> > > Despite expectations, based on prior experience with floaing-point on x86
> > > processors, comparison shows that `double` provides similar or
> > > sometimes better timings. And picture stays consistent be it SSE2,
> > > AVX1 or
> > > AVX2 code.
> > > 
> > > Part of #5941
> > > ---
> > 
> > I agree with Vladimir here.
> > Looks like this perf test doesn't belong to Tarantool repository.
> > Would you mind dropping it?
> 
> Here is the case (we both aware of) I want to avoid here - today we do not
> have saved _that_ decimal perf test, basing on which we have preferred LuaC
> and dropped FFI implementation. We could not rerun it today, within a newer
> LuaJIT implementation, to verify that situation didn't change. This is
> similar case - we have made a decision basing on some evaluations using this
> code, in a future we may decide to further optimize data structure (like
> Vova suggested elsewhere to split int64 into 2 fields), and it would be
> better if at that moment we would still have performance test around for
> adaptations and rerun.
> 
> Yes, it's another test of performance test we used to see in perf directory
> (hehe, there is only single test at the moment), kind of one time shot in a
> history, important for design decision, but from longer prospective I assume
> it should be still around.
> 
> Does my reasoning make some sense?

A test that forks parts of Tarantool internal code and runs some
benchmarks on them (like this one) will inevitably diverge from the code
it was forked from. So pretty soon its results won't be trusted. Having
it in the main repository is a maintenance burden.

A test that checks FFI vs ccall performance or double vs int64_t should
be independent on Tarantool internals. Such a test is probably okay to
have in the main repository, although it could just as well live in a
separate repository with performance tests, but this is a matter of
policy.