[Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation
Serge Petrenko
sergepetrenko at tarantool.org
Tue Aug 17 15:15:05 MSK 2021
16.08.2021 02:59, Timur Safin via Tarantool-patches пишет:
> * Version #5 changes:
>
> - After performance evaluations it was decided to switch data type used
> by `datetime.secs` from `int64_t` (which is boxed in LuaJIT) to
> `double` (which is unboxed number).
> - please see perf/datetime*.cc perf test as a benchmark we used for
> comparison of 2 veriants of C implementation. Results are here -
> https://gist.github.com/tsafin/618fbf847d258f6e7f5a75fdf9ea945b
Hi! Thanks for the patchset!
Generally looks ok. Most of my comments are style related,
except the ones covering patches 4, 5.
I like the amount of tests you introduce.
> * Version #4 changes:
>
> A lot.
>
> - Got rid of closures usage here and there. Benchmarks revealed there is
> 20x speed difference of hash with closure handlers to the case when we
> check keys using sequence of simple ifs.
> And despite the fact that it looks ugler, we have updated all places which
> used to use closures - so much performance impact does not worth it;
>
> - c-dt library now is accompanied with corresponding unit-tests;
>
> - due to a new (old) way of exporting public symbols, which is now used by
> Tarantool build, we have modified c-dt sources to use approach similar
> to that used by xxhash (or ICU) - "namespaces" for public symbols.
> If there is DT_NAMESPACE macro (e.g. DT_NAMESPACE=tnt_) defined at the
> moment of compilation we rename all relevant public symbols to
> use that "namespace" as prefix
>
> #define dt_from_rdn DT_NAME(DT_NAMESPACE, dt_from_rdn)
>
> C sources continue to use original name (now as a macro), but in Lua ffi
> we have to use renamed symbol - `tnt_dt_from_rdn`.
>
> - We have separated code which implement MessagePack related funcionality
> from generic (i.e. stringization) support for datetime types.
> Stringization is in `src/core/datetime.c`, but MessagePack support is in
> `src/core/mp_datetime.c`.
>
> - `ctime` and `asctime` now use reentrant versions (i.e. `ctime_r` instead of
> `ctime`, and `asctime_r` instead of `asctime`). This allows us to avoid
> nasty problem with GC where it was possible to corrupt returned
> static buffer regardless the fact that it was immediately passed
> to `ffi.string`.
> `test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua` updated to check our
> new api `datetime.ctime`, and `datetime.asctime`.
>
> - Introduced MessagePack unit-test which check C implementation (mp
> encode/decode, mp_snprint, mp_fprint)
>
> - Some special efforts have been taken to make sure we deal correctly
> with datetime stamps which were before Unix Epoch (1970-01-01), i.e.
> for negative stamps or timezones against epoch.
>
> - And last, but not the least: in addition to usual getters like
> `timestamp`, `nanoseconds`, `epoch`, we have introduced function
> modifiers `to_utc()` or `to_tz(offset)` which allow to manipulate with
> datetime timezone `.offset` field. Actually it does not do much now
> (only reattributing object with different offset, but not changed
> actual timestamp, which is UTC normalized). But different timestamp
> modifies textual representation, which simplify debugging.
>
> * Version #3 changes:
>
> - renamed `struct datetime_t` to `struct datetime`, and `struct
> datetime_interval_t` to `struct datetime_interval`;
> - significantly reworked arguments checks in module api entries;
> - fixed datetime comparisons;
> - changed hints calculation to take into account fractional part;
> - provided more comments here and there;
>
> NB! There are MacOSX problems due to GLIBC specific code used (as Vlad
> has already pointed out) - so additional patch, making it more
> cross-compatible coming here soon...
>
> * Version #2 changes:
>
> - fixed problem with overloaded '-' and '+' operations for datetime
> arguments;
> - fixed messagepack serialization problems;
> - heavily documented MessagePack serialization schema in the code;
> - introduced working implementation of datetime hints for storage engines;
> - made interval related names be more consistent, renamed durations and period
> to intervals, i.e. t_datetime_duration to datetime_interval_t,
> duration_* to interval_*, period to interval;
> - properly implemented all reasonable cases of datetime+interval
> arithmetic;
> - moved all initialization code to utils.c;
> - renamed core/mp_datetime.c to core/datetime.c because it makes more
> sense now;
>
> * Version #1 - initial RFC
>
> In brief
> --------
>
> This patchset implements datetime lua support in box, with serialization
> to messagepack, yaml, json and lua mode. Also it contains storage
> engines' indices implementation for datetime type introduced.
>
> * Current implementation is heavily influenced by Sci-Lua lua-time module
> https://github.com/stepelu/lua-time
> e.g. you could find very similar approach for handling of operations
> with year or month long intervals (which should be handled differently than
> usual intervals of seconds, or days).
>
> * But internally we actually use Christian Hanson' c-dt module
> https://github.com/chansen/c-dt
> (though it has been modified slightly for cleaner integration
> into our cmake-based build process).
>
>
> Datetime Module API
> -------------------
>
> Small draft of documentation for our `datetime` module API has been
> extracted to the discussion topic there -
> https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043988
> We try to update documentation there once any relevant change introdcued
> to the implementaton.
>
> Messagepack serialization schema
> --------------------------------
>
> In short it looks like:
> - now we introduce new MP_EXT extension type #4;
> - we may save 1 required and 2 optional fields for datetime field;
>
> In all gory details it's explained in MessagePack serialization schema depicted here:
> https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043990
>
>
> https://github.com/tarantool/tarantool/issues/5941
> https://github.com/tarantool/tarantool/issues/5946
>
> https://github.com/tarantool/tarantool/tree/tsafin/gh-5941-datetime-v4
>
> Timur Safin (8):
> build: add Christian Hansen c-dt to the build
> lua: built-in module datetime
> lua, datetime: display datetime
> box, datetime: messagepack support for datetime
> box, datetime: datetime comparison for indices
> lua, datetime: time intervals support
> datetime: perf test for datetime parser
> datetime: changelog for datetime module
>
> .gitmodules | 3 +
> CMakeLists.txt | 8 +
> .../gh-5941-datetime-type-support.md | 4 +
> cmake/BuildCDT.cmake | 10 +
> extra/exports | 33 +
> perf/CMakeLists.txt | 3 +
> perf/datetime-common.h | 105 +++
> perf/datetime-compare.cc | 213 +++++
> perf/datetime-parser.cc | 105 +++
> src/CMakeLists.txt | 5 +-
> src/box/field_def.c | 53 +-
> src/box/field_def.h | 4 +
> src/box/lua/serialize_lua.c | 7 +-
> src/box/memtx_space.c | 3 +-
> src/box/msgpack.c | 7 +-
> src/box/tuple_compare.cc | 77 ++
> src/box/vinyl.c | 3 +-
> src/lib/core/CMakeLists.txt | 5 +-
> src/lib/core/datetime.c | 176 ++++
> src/lib/core/datetime.h | 115 +++
> src/lib/core/mp_datetime.c | 189 ++++
> src/lib/core/mp_datetime.h | 89 ++
> src/lib/core/mp_extension_types.h | 1 +
> src/lib/mpstream/mpstream.c | 11 +
> src/lib/mpstream/mpstream.h | 4 +
> src/lua/datetime.lua | 880 ++++++++++++++++++
> src/lua/init.c | 4 +-
> src/lua/msgpack.c | 12 +
> src/lua/msgpackffi.lua | 18 +
> src/lua/serializer.c | 4 +
> src/lua/serializer.h | 2 +
> src/lua/utils.c | 28 +-
> src/lua/utils.h | 12 +
> test/app-tap/datetime.test.lua | 370 ++++++++
> .../gh-5632-6050-6259-gc-buf-reuse.test.lua | 74 +-
> test/engine/datetime.result | 77 ++
> test/engine/datetime.test.lua | 35 +
> test/unit/CMakeLists.txt | 3 +-
> test/unit/datetime.c | 381 ++++++++
> test/unit/datetime.result | 471 ++++++++++
> third_party/c-dt | 1 +
> third_party/lua-cjson/lua_cjson.c | 8 +
> third_party/lua-yaml/lyaml.cc | 6 +-
> 43 files changed, 3591 insertions(+), 28 deletions(-)
> create mode 100644 changelogs/unreleased/gh-5941-datetime-type-support.md
> create mode 100644 cmake/BuildCDT.cmake
> create mode 100644 perf/datetime-common.h
> create mode 100644 perf/datetime-compare.cc
> create mode 100644 perf/datetime-parser.cc
> create mode 100644 src/lib/core/datetime.c
> create mode 100644 src/lib/core/datetime.h
> create mode 100644 src/lib/core/mp_datetime.c
> create mode 100644 src/lib/core/mp_datetime.h
> create mode 100644 src/lua/datetime.lua
> create mode 100755 test/app-tap/datetime.test.lua
> create mode 100644 test/engine/datetime.result
> create mode 100644 test/engine/datetime.test.lua
> create mode 100644 test/unit/datetime.c
> create mode 100644 test/unit/datetime.result
> create mode 160000 third_party/c-dt
>
--
Serge Petrenko
More information about the Tarantool-patches
mailing list