From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 0AA946EC55; Sat, 31 Jul 2021 09:29:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0AA946EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627712974; bh=pioI4DgCcuLpLpU3PBMX0HwpzrgY14y6ev+h5IaFpkU=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=bPgO7WrCaqs89X05JhVdvSE8AeXK+qJOOP7q0/Fv90JYAnrP6RXMNeBd6bdNgOjgX PdM/uWVpSf3P6U9K70wJGBlPnWLEUshu2eK1pljWvLOsj+rqM+hpSP+WGzcHLJJ3yi IXfaURfMNHCVQ8zsGwCI8WL8cFdBN6/aGy/oc7NQ= Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7A9FC6EC55 for ; Sat, 31 Jul 2021 09:29:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7A9FC6EC55 Received: by smtp47.i.mail.ru with esmtpa (envelope-from ) id 1m9iV1-0000t1-FL; Sat, 31 Jul 2021 09:29:31 +0300 To: Timur Safin , v.shpilevoy@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <50175cbd00c57e3bc8eed222951a551f4bb7effb.1627468002.git.tsafin@tarantool.org> <2f63968b-490f-9abf-ab36-325553bd7609@tarantool.org> <040c01d78575$2e9dcb80$8bd96280$@tarantool.org> Message-ID: <6278fe67-a7a2-d98d-399f-0293b413c187@tarantool.org> Date: Sat, 31 Jul 2021 09:29:30 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <040c01d78575$2e9dcb80$8bd96280$@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B2770D7874BA03B4AE182A05F5380850406BCDC417F71D6893A8086465B83964BE941DE740187BD905882C1CB59CADF0B4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78AC0750F3304E924EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D70459434292EC88638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80613EBB2054720D0E97CEC046DCB0EE6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE4B6963042765DA4BA7B8E9D6D956BB52D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3A7DFDF579AB090EFC0837EA9F3D19764C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637A63D8D502DFAF8B5731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5693F36BA55A70DBECED2515BEF47B117AF7C5C791EE64C90D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3438212F20E1E78D8E91E1ED9179311F923AC11917A1DA9907876469E77D209EF252B882D9289C1DBE1D7E09C32AA3244C906EFCAEE23D96D435A14B3A3FC9AC7824AF4FAF06DA24FDFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojNLQ8Cqcv1ivW3J5QN9tc2g== X-Mailru-Sender: 583F1D7ACE8F49BD1042885CEC987B6BC993A1ADB7520040A8086465B83964BE825871508D9F39707019711D9D5B048E1458020726E2BC9FD5ECBA0B92C0A936CDC7563AA7CEBD287402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH resend v2 02/11] lua: built-in module datetime X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Oleg Babin via Tarantool-patches Reply-To: Oleg Babin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for your answers. On 30.07.2021 22:00, Timur Safin wrote: > Ok, here is a selection list which I've seen: > - decimal is in LuaC; > - uuid is in ffi; > > It looked equally acceptable and I've selected the one which is > more convenient - via ffi. Now, you've pointed me to the probable > performance issues we have with ffi, which are not there with LuaC. > I believe they have been fixed with that Mike fix for NYI in > structure allocations (after Sergos dirty fix) and once we will land > this patch to our local fork those performance issues for structures > allocations will gone. > > In any case, 1st version of module is not yet a proper moment to start > to worry about performance, more rather about clarity and quality of API. > We need to return to benchmarking of this code soon after code freeze > lifted. I don't know why uuid is implemented via FFI. Uuid as module was introduced at least 8 yeard ago. Probably there were supporters of the theory that FFI/LuaJIT implementation could be fast. But currently we can see something like https://github.com/luafun/luafun/issues/31#issuecomment-277508076 and rewrite netbox in C. And it's not about patches in LuaJIT about structure initialization. You can easily cherry-pick it into our fork and show that FFI is really better than Lua C API. It's about an approach. We saw benchmarks when decimal was introduced but currently we didn't see anything. > : > + local y, M, d, ymd > : > + y, M, d, ymd = 0, 0, 0, false > : > + > : > + local h, m, s, frac, hms > : > + h, m, s, frac, hms = 0, 0, 0, 0, false > : > + > : > + local dt = 0 > : > + > : > + for key, value in pairs(o) do > : > + local handlers = { > : > : Here you recreate handlers for each iteration of the loop. I think it > : should be reworked. > > Yes, let me check how better will be if this handlers array will be invariant > in the loop, and not created each iteration. > > I still prefer this tag to function approach because of a better clarity, > but I need to verify in benchmark whether it provides acceptable performance. > > I do not expect thought that this attribute object creation approach would become > a bootleneck in any case and might be on a critical path anywhere (I'd rather expect > it for datetime objects created after parsing of massive bunch of log text), > but need to look into timings we would have with invariant table. > > : > : Currenlty it's quite slow I think even if-elseif-else branches will work > : faster without > : > : creating redundant GC objects. > > Will see ... I'll simplify your task a bit: local clock = require('clock') local mt_fun = function(_, key)     local mt = {         ['key'] = function()             return 'value'         end     }     return mt[key] ~= nil and mt[key]() end local mt_table = {     ['key'] = function()         return 'value'     end } local t1 = setmetatable({}, {__index = mt_fun}) local t2 = setmetatable({}, {__index = mt_table}) local _ local start = clock.time() for _ = 1, 1e6 do     _ = t1['key'] end print('function', clock.time() - start) collectgarbage() collectgarbage() local start = clock.time() for _ = 1, 1e6 do     _ = t2['key'] end print('table', clock.time() - start) tarantool test.lua function    0.14332509040833 table    0.0002901554107666 The difference about x500 times on my Mac. > : > +end > : > + > : > +local function strftime(fmt, o) > : > + assert(ffi.typeof(o) == datetime_t) > : > + local sz = 50 > : Why 50? > > Good question, for ISO-8601 formats, for normal date range > (from 0000 till 9999 years) it's more like closer to 40, but > well, there will be various timezones and all crazy kinds of > input format combinations, so we need to either be precautious > with larger sizes allocations. On the one side you are right. But if user want to format string with length more than 50 it will be silently truncated. ``` tarantool> datetime.strftime('%d' .. string.rep('content', 50), datetime.new()) --- - 01contentcontentcontentcontentcontentcontentconten ... ``` I'm not sure that anybody will use strftime in such way but it's strange. Maybe you could use preallocated buffer for short strings and allocate huge if it's not enough. Some kind of optimistic approach. > Or probably rely on glibc convention (which is apparently not in > POSIX/SUSV) with 2 passes approach: > 1. call with NULL so it will return size of required buffer; > 2. then, after allocation, with the adjusted allocated buffer; > > https://www.gnu.org/software/libc/manual/html_node/Formatting-Calendar-Time.html > https://pubs.opengroup.org/onlinepubs/000095399/functions/strftime.html > > Something like that (beware of inline patch): > > ---------------------------------------------- > local function strftime(fmt, o) > check_date(o, "datetime.strftime(fmt, date)") > - local sz = 50 > - local buff = ffi.new('char[?]', sz) > local p_tm = datetime_to_tm_ptr(o) > - native.strftime(buff, sz, fmt, p_tm) > + local sz = builtin.strftime(nil, 1024, fmt, p_tm) > + local buff = ffi.new('char[?]', sz + 1) > + builtin.strftime(buff, sz, fmt, p_tm) > return ffi.string(buff) > end > ---------------------------------------------- > > : > + local buff = ffi.new('char[?]', sz) > : > + local p_tm = datetime_to_tm_ptr(o) > : > + native.strftime(buff, sz, fmt, p_tm) > : > + return ffi.string(buff) > : > +end > : > + > : > +-- strftime may be redirected to datetime:fmt("format") > : > +local function datetime_fmt() > : > +end > : > + > : > + > : > +ffi.metatype(interval_t, interval_mt) > : > +ffi.metatype(datetime_t, datetime_mt) > : > + > : > +return setmetatable( > : > + { > : > + datetime = datetime_new, > : > + interval = interval_new, > : > + > : > + parse = parse_str, > : > + parse_date = parse_date, > : > + parse_time = parse_time, > : > + parse_zone = parse_zone, > : > + fmt = datetime_fmt, > : > + > : > + now = local_now, > : > + -- strptime = strptime; > : It should be dropped if you don't need it. > : > + strftime = strftime, > : > + asctime = asctime, > : > + ctime = ctime, > : > + }, { > : > + __call = function(self, ...) return datetime_from(...) end > : > + } > : > +) > > Thanks, > Timur >