[Tarantool-patches] [PATCH v3 2/9] lua: built-in module datetime

[Vladislav Shpilevoy]

Thanks for the fixes!

>>> +local datetime_t = ffi.typeof('struct datetime_t')
>>> +local interval_t = ffi.typeof('struct datetime_interval_t')
>>> +
>>> +local function is_interval(o)
>>> +    return type(o) == 'cdata' and ffi.istype(interval_t, o)
>>> +end
>>> +
>>> +local function is_datetime(o)
>>> +    return type(o) == 'cdata' and ffi.istype(datetime_t, o)
>>> +end
>>> +
>>> +local function is_date_interval(o)
>>> +    return is_datetime(o) or is_interval(o)
>>
>> 4. You make type(o) == 'cdata' check 2 times here.
> 
> Yes. It's not a problem, IMHO. LuaJIT after inlining of those 2 functions should take care of that. (I believe)

Please, remove the double-check. If you believe in something, it does not
necessarily is true, and sometimes things can't be jitted at all or not
as you expect. I would understand reliance on gcc, but LuaJIT is much
less reliable and sucks much more when something can't be jitted.

>> Do you have any tests? In this commit I see zero tests for
>> your code. Only very few tests for dt library. Please, add
>> tests for everything you have here.
> 
> A lot. It started to be added with the next commit, and added with any other commit adding functionality to datetime.lua. It's pity you didn't have a chance to see next commits.

No, I am really sorry, but this is not how it works. You need to send
atomic commits. Self-sufficient, covered with tests if possible. I can't
look at all your commits at once trying to understand what errors you
fixed in later commits and how.

>>> +        error(('value %d of %s is out of allowed range [%d, %d]'):
>>> +              format(v, txt, range[1], range[2]))
>>> +    end
>>> +end
>>> +
>>> +local datetime_index_handlers = {
>>> +    unixtime = function(self)
>>> +        return self.secs
>>> +    end,
>>> +
>>> +    timestamp = function(self)
>>> +        return tonumber(self.secs) + self.nsec / 1e9
>>
>> 11. If you are saying the Lua number value range is enough,
>> why do you store them as 64bit integers in 'self' and
>> convert to a number only for the external API?
> 
> May be I misunderstood your sentence, but let me elaborate here.
> I need seconds and nanoseconds kept separately for their efficient and precise handling, and for passing to c-dt.
> 
> If we would keep 32-bit integers in seconds then we would be able to handle only dates upto 2038 year, thus we need 64-bit seconds for extended range.
> 
> OTOH, not whole 64-bit range is practical, and required for expected in real-life datetime range values. It's not a problem that Lua number and int64_t have very different range for precise integer values. Lua number could handle integer values upto 9e15, which is corresponding to ...

I know all that. The question is why do you keep storing cdata 64 bits numbers
inside of the datetime objects, if you convert them all to Lua numbers before
return? You could store self.secs as just number. And the other fields too. Lua
number is double, it does not loose precision for integers < 2^53, which should
be enough for the ranges you want to support. Correct?

>>> +local function datetime_new(o)
>>> +    if o == nil then
>>> +        return datetime_new_raw(0, 0, 0)
>>> +    end
>>> +    local secs = 0
>>> +    local nsec = 0
>>> +    local offset = 0
>>> +    local easy_way = false
>>> +    local y = 0
>>> +    local M = 0
>>> +    local d = 0
>>> +    local ymd = false
>>> +
>>> +    local h = 0
>>> +    local m = 0
>>> +    local s = 0
>>> +    local frac = 0
>>> +    local hms = false
>>> +
>>> +    local dt = 0
>>> +
>>> +    local handlers = {
>>
>> 17. Please, stop using closures in the constructor of a
>> potentionally very frequently created/deleted object >
> 
> I still disagreed that datetime_new constructor will be on critical path, the most frequent case expected to be parse*() -> datetime_new_dt() -> datetime_new_raw(). i.e. when we parse millions of timestamps in logs and then proceed them.
> 
> datetime_new(builder_object) expected to be called in rare cases when object got created from Lua code, not on performance critical paths, thus convenience here is more important than performance.
> 
> And BTW I've created simple benchmark for comparing datetime_new_dt vs datetime_new(from builder object) vs datetime_new_2 which would be not using hash object with handlers, but rather sequence of ifs.
> 
> https://gist.github.com/tsafin/31cc9b0872b6015904fcc90d97740770
> ```
>  ~/datetime/tarantoolt [tsafin/gh-5941-datetime-v3]
> 00:39 $ ./build/src/tarantool ../bench-hash-dt.lua
> datetime_new_dt 0.002718448638916
> datetime_new    5.1318607330322
> datetime_new(precreated)        4.4448664188385
> datetime_new_2(ifs)     4.4977836608887
> ```
> 
> Note, that creation from builder object is much, much slower than raw cdata object creation (that's expected). If we move object out of loop, and precreate it then we save some time (as expected) 5.1 vs 4.4.
> But if we change implementation from hash with closure handlers to sequence of ifs - we actually degrade performance (a bit) from 4.44 to 4.49.
> 
> Biggest performance impact here - traversing of object attributes, but whether it's using closures or not using them, it makes no much difference.
> 
> But using closures makes this code look much more elegant. So keeping it here for today.

I want to see the version you created with ifs, please. With the closures
you do the same calculations + create the functions, it can't be slower
unless something is wrong with the version you did with ifs.

>> 19. It can ignore some fields (here it ignores 'year'):
>>
>> tarantool> d1 = datetime({secs = 100, year = 1})
>> ---
>> ...
>>
>> tarantool> d2 = datetime({secs = 100, year = 2})
>> ---
>> ...
>>
>> tarantool> d1 == d2
>> ---
>> - true
>> ...
>>
> 
> Yes, and that's by design. .secs defines 'easy_way' of initializing object via secs/nsec/offset, while .year selects 'ymd' mode. And `easy_way` has precedence. It looks reasonable for me, please, suggest your idea how to handle it even better without much redesign?

You could for instance, by looking at a first key you see in the dictionary,
decide how will you parse the entire dict. If you see first 'secs', you parse
only secs, nsecs and offset. Everything else raises an error. If you see
first 'year', you raise an error for the other non-related keys.

Ignorance of some keys does not look acceptable.

And by the way, does it mean I can't specify a date using year, month, day,
hours, and secs? Why isn't it valid?

>> 20. Serialization does not seem to be working in the console
>> despite you tried to implement it above:
>>
>> tarantool> d = datetime({secs = 100})
>> ---
>> ...
>>
>> tarantool> d
>> ---
>> - 'cdata<struct datetime_t>: 0x0103ad9c50'
>> ...
> 
> Yup, redefining of __serialize is not enough for cdata object by whatever reason (I didn't look deep). But after the next patch(es) it's start to stringize properly.

But don't you have __serialize implementation in this commit already?
(I don't remember and didn't see the latest code yet.)

>>> +            o = right_t == datetime_t and interval_new() or datetime_new()
>>> +            o.secs, o.nsec = _normalize_nsec(lhs.secs - rhs.secs,
>>> +                                            lhs.nsec - rhs.nsec)
>>> +            return o
>>> +        else
>>> +            error_incompatible("operator -")
>>> +        end
>>> +    -- both left and right are generic intervals
>>> +    elseif left_t == interval_t and right_t == interval_t then
>>> +        o = interval_new()
>>> +        o.secs, o.nsec = _normalize_nsec(lhs.secs - rhs.secs,
>>> +                                        lhs.nsec - rhs.nsec)
>>> +        return o
>>> +    else
>>> +        error_incompatible("operator -")
>>> +    end
>>> +end
>>
>> 22. Did you really test this function?
>>
>> tarantool> dt = require('datetime')
>> ---
>> ...
>>
>> tarantool> d1 = dt()
>> ---
>> ...
>>
>> tarantool> d2 - 1
>> ---
>> - error: 'builtin/datetime.lua:487: bad argument #1 to ''typeof'' (C type expected,
>>      got number)'
>> ...
> 
> All kinds of arguments checking implemented in the separate patch about interval arithmetic operations, please find a moment to look into them.

Please, do not introduce broken functions. Make the correct ones right away and
add tests. The new code does not need to introduce full functionality if it
can be done in an atomic way, but it must not be visibly broken at least. Here
it is.

>>> +
>>> +local function datetime_from(o)
>>> +    if o == nil or type(o) == 'table' then
>>> +        return datetime_new(o)
>>> +    elseif type(o) == 'string' then
>>> +        return parse(o)
>>> +    end
>>> +end
>>> +
>>> +local function local_now()
>>> +    local p_tv = ffi.new('struct timeval [1]')
>>> +    local rc = builtin.gettimeofday(p_tv, nil)
>>
>> 24. Why can't you use fiber time? It should be enough in
>> nearly all usecases and is much faster. For real-real time
>> talking directly to the kernel we have 'clock' Lua module
>> already. You could check if results of clock functions can
>> be passed into your module to create 'real' timestamps not
>> depending on the eventloop.
> 
> Fiber time, which actually return cached ev_rt_now, looks imprecise and depends on event loop circumstances, though eventually it calls the same clock_gettime(). So fiber-time is looking like a cached timestamp value from [hopefully, non-distant] past.
> 
> I'd rather prefer to have more realtime values, and clock_realtime*/clock_monotonic* look relevant, they call clock_gettime() for their purposes, which in turn calls the same gettimeofday(). So in this case we call same api, but with 2 extra call layers above.

By using gettimeofday you drag a lot of cdef garbage including the function
itself and what it returns. Please, don't. Anyway, the standard says struct
timeval can have more members than the usually used seconds and microseconds:

	https://pubs.opengroup.org/onlinepubs/7908799/xsh/systime.h.html

	The <sys/time.h> header defines the timeval structure that includes
	**at least** the following members

> IMHO, not worth extra complexity if we could just simply call gettimeofday().

Now you added a lot of extra complexity exactly because of this function.

>> 25. Functions like ctime can't be used in Lua, because according
>> to their Linux man:
>>
>>     The return value points to a statically allocated string
>>     which might be overwritten by subsequent calls to any of the
>>     date and time functions
>>
>> Which means they use a global buffer. If you will
>> use datetime module in any of __gc handlers, it will
>> return garbage sometimes or just override results
>> somethere. That is exactly the same problem as existed
>> with tarantool's static buffer.
> 
> I do not believe it's the same problem - in this case content of static buffer is immediately interned in Lua via ffi.string()

Well, it is exactly the same problem. How do you think the static
buffer used to be copied to Lua until we purged it from everywhere? Using
the same ffi.string(). Before ffi.string() will copy anything, it
will start GC on demand, and inside of GC the string might be overwritten.
So when GC ends, ffi.string() will copy garbage or some new foreign string.

> https://luajit.org/ext_ffi_api.html
> "The Lua string is an (interned) copy of the data and bears no relation to the original data area anymore."

It does not say anything about when GC is started, and that was the
problem with already 2 global buffers - our static one and one from
luacjson library. I don't see how a third global buffer is different.
It is broken in the same way precisely.

Look at this commit: acf8745ed8fef47e6d1f1c31708c7c9d6324d2f3
("uuid: drop tt_uuid_str() from Lua"). It is your case exactly.
A global buffer is returned to Lua via ffi and then ffi.string()
was called. There is a test which breaks when you do so. Strings
become wrong and/or garbage.

>>> diff --git a/test/unit/datetime.c b/test/unit/datetime.c
>>> new file mode 100644
>>> index 000000000..b6f568c03
>>> --- /dev/null
>>> +++ b/test/unit/datetime.c
>>
>> 26. This file tests dt library. It does not test your code. Please, add
>> tests for everything you implemented in this commit. For c-dt library
>> the tests must be added in the previous commit, and the test file
>> must be called test/unit/cdt.c then.
> 
> Unit-test has been moved to the commit introducing c-dt. And in turn, initial datetime test (checking parser and ctime/asctime/now) squashed to the same commit where we introduce module with parser.

Thanks, that should be better.