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 0A1D57030C; Fri, 5 Mar 2021 00:22:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0A1D57030C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614892962; bh=oU9iaoTFXJTA4y9mo4nARIAmJuLj3Jt5IYARFgCi11g=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=ixusJExmC/O0lm+2mREtIE0PsqvBNCjebAekBZitU7AojF1q7ee/4H960CtMQPZdY YTQFITT409ripAyWgkwHMjqTqal6QK6m+TTA/AJ7E4AF9OH98xRSr4Tma8YXISUHc0 PYowS32biRjuZzLc8cxpjwsy1BUsXB9X5tom0VKg= Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 C3A5B7030C for ; Fri, 5 Mar 2021 00:22:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C3A5B7030C Received: by smtp52.i.mail.ru with esmtpa (envelope-from ) id 1lHvQd-0006RA-U9; Fri, 05 Mar 2021 00:22:40 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org References: Message-ID: <0522c6f4-7c8a-b424-eeb1-0bb0b10084d7@tarantool.org> Date: Fri, 5 Mar 2021 00:22:39 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92A98208ECBDD29F5A6709E93A77B8502A831FF4624BDF5D2182A05F538085040669E8A5EF2D498F0A7FBE75C80C645EF05CF20515000932034B5C2E0DEB0E29D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75644E22E05AA81AEB287FD4696A6DC2FA8DF7F3B2552694A4E2F5AFA99E116B42401471946AA11AF23F8577A6DFFEA7CEEF2A6A9DCF3640F8F08D7030A58E5ADC58D69EE07B14084C6CDE5D1141D2B1C8F5498A2E2252C7C0736EE98B4DF6F7796FB1EA54740DAD39FA2833FD35BB23D9E625A9149C048EE33AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BF1175FABE1C0F9B6A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC82A69198438CA1493AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F7900637A5ED7754355CFA81D81D268191BDAD3D698AB9A7B718F8C442539A7722CA490C13377AFFFEAFD26923F8577A6DFFEA7C1156E5889A6D309193EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6104AF9A031C589D7089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E7028C9DFF55498CEFB0BD9CCCA9EDD067B1EDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24A6D60772A99906F8E1CD14B953EB46D44CFE0C75D4C95FD355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A50C77A97C600BEDA71E70DAE5AC5F8EBEC8833752200B8E1CD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AB80C45F81B80D634712C72AA314680179147AD2AB5D9F4C9E3D7D8ACD9B3494457CD810D1A5AE6A1D7E09C32AA3244CFE1853181C3181654454D2DA369EEEF25595C85A795C7BAEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojSsmoOoMLSh3rbICkIG5j8A== X-Mailru-Sender: 583F1D7ACE8F49BD3369739A12991F4D81F811BF6A39B47775348518ECD43B4327EE51B2F7AE0B0F23E75C7104EB1B885DEE61814008E47C7013064206BFB89F93956FB04BA385BE9437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH vshard 09/11] ref: introduce vshard.storage.ref module 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" Hi! I've looked again. See 3 comments/questions below. On 23.02.2021 03:15, Vladislav Shpilevoy wrote: > +local function ref_session_new(sid) > + -- Session object does store its internal hot attributes in a table. Because > + -- it would mean access to any session attribute would cost at least one > + -- table indexing operation. Instead, all internal fields are stored as > + -- upvalues referenced by the methods defined as closures. > + -- > + -- This means session creation may not very suitable for jitting, but it is > + -- very rare and attempts to optimize the most common case. > + -- > + -- Still the public functions take 'self' object to make it look normally. > + -- They even use it a bit. > + > + -- Ref map to get ref object by its ID. > + local ref_map = {} > + -- Ref heap sorted by their deadlines. > + local ref_heap = lheap.new(heap_min_deadline_cmp) > + -- Total number of refs of the session. Is used to drop the session without > + -- fullscan of the ref map. Heap size can't be used because not all refs are > + -- stored here. See more on that below. > + local count = 0 Maybe it's better to rename it to "global_count". Sometimes it's quite confusing to see `M.count +=` near `count += `. Also you have "global_map" and "global_heap" so no reasons to call it just "count". > + -- Cache global session storages as upvalues to save on M indexing. > + local global_heap = M.session_heap > + local global_map = M.session_map > + > + local function ref_session_discount(self, del_count) > + local new_count = M.count - del_count > + assert(new_count >= 0) > + M.count = new_count > + > + new_count = count - del_count > + assert(new_count >= 0) > + count = new_count > + end > + > + local function ref_session_update_deadline(self) > + local ref = ref_heap:top() > + if not ref then > + self.deadline = DEADLINE_INFINITY > + global_heap:update(self) > + else > + local deadline = ref.deadline > + if deadline ~= self.deadline then > + self.deadline = deadline > + global_heap:update(self) > + end > + end > + end > + > + -- > + -- Garbage collect at most 2 expired refs. The idea is that there is no a > + -- dedicated fiber for expired refs collection. It would be too expensive to > + -- wakeup a fiber on each added or removed or updated ref. > + -- > + -- Instead, ref GC is mostly incremental and works by the principle "remove > + -- more than add". On each new ref added, two old refs try to expire. This > + -- way refs don't stack infinitely, and the expired refs are eventually > + -- removed. Because removal is faster than addition: -2 for each +1. > + -- > + local function ref_session_gc_step(self, now) > + -- This is inlined 2 iterations of the more general GC procedure. The > + -- latter is not called in order to save on not having a loop, > + -- additional branches and variables. > + if self.deadline > now then > + return > + end > + local top = ref_heap:top() > + ref_heap:remove_top() > + ref_map[top.id] = nil > + top = ref_heap:top() > + if not top then > + self.deadline = DEADLINE_INFINITY > + global_heap:update(self) > + ref_session_discount(self, 1) > + return > + end > + local deadline = top.deadline > + if deadline >= now then > + self.deadline = deadline > + global_heap:update(self) > + ref_session_discount(self, 1) > + return > + end > + ref_heap:remove_top() > + ref_map[top.id] = nil > + top = ref_heap:top() > + if not top then > + self.deadline = DEADLINE_INFINITY > + else > + self.deadline = top.deadline > + end > + global_heap:update(self) > + ref_session_discount(self, 2) > + end > + > + -- > + -- GC expired refs until they end or the limit on the number of iterations > + -- is exhausted. The limit is supposed to prevent too long GC which would > + -- occupy TX thread unfairly. > + -- > + -- Returns false if nothing to GC, or number of iterations left from the > + -- limit. The caller is supposed to yield when 0 is returned, and retry GC > + -- until it returns false. > + -- The function itself does not yield, because it is used from a more > + -- generic function GCing all sessions. It would not ever yield if all > + -- sessions would have less than limit refs, even if total ref count would > + -- be much bigger. > + -- > + -- Besides, the session might be killed during general GC. There must not be > + -- any yields in session methods so as not to introduce a support of dead > + -- sessions. > + -- > + local function ref_session_gc(self, limit, now) > + if self.deadline >= now then > + return false > + end Here you mix "booleans" and "numbers" as return values. Maybe it's better to return "nil" here? > + local top = ref_heap:top() > + local del = 1 > + local rest = 0 > + local deadline > + repeat > + ref_heap:remove_top() > + ref_map[top.id] = nil > + top = ref_heap:top() > + if not top then > + self.deadline = DEADLINE_INFINITY > + rest = limit - del > + break > + end > + deadline = top.deadline > + if deadline >= now then > + self.deadline = deadline > + rest = limit - del > + break > + end > + del = del + 1 > + until del >= limit > + ref_session_discount(self, del) > + global_heap:update(self) > + return rest > + end > + > + local function ref_session_add(self, rid, deadline, now) > + if ref_map[rid] then > + return nil, lerror.vshard(lerror.code.STORAGE_REF_ADD, > + 'duplicate ref') > + end > + local ref = { > + deadline = deadline, > + id = rid, > + -- Used by the heap. > + index = -1, > + } > + ref_session_gc_step(self, now) > + ref_map[rid] = ref > + ref_heap:push(ref) > + if deadline < self.deadline then > + self.deadline = deadline > + global_heap:update(self) > + end > + count = count + 1 > + M.count = M.count + 1 > + return true > + end > + > + -- > + -- Ref use means it can't be expired until deleted explicitly. Should be > + -- done when the request affecting the whole storage starts. After use it is > + -- important to call del afterwards - GC won't delete it automatically now. > + -- Unless the entire session is killed. > + -- > + local function ref_session_use(self, rid) > + local ref = ref_map[rid] > + if not ref then > + return nil, lerror.vshard(lerror.code.STORAGE_REF_USE, 'no ref') > + end > + ref_heap:remove(ref) > + ref_session_update_deadline(self) > + return true > + end > + > + local function ref_session_del(self, rid) > + local ref = ref_map[rid] > + if not ref then > + return nil, lerror.vshard(lerror.code.STORAGE_REF_DEL, 'no ref') > + end > + ref_heap:remove_try(ref) > + ref_map[rid] = nil > + ref_session_update_deadline(self) > + ref_session_discount(self, 1) > + return true > + end > + > + local function ref_session_kill(self) > + global_map[sid] = nil > + global_heap:remove(self) > + ref_session_discount(self, count) > + end > + > + -- Don't use __index. It is useless since all sessions use closures as > + -- methods. Also it is probably slower because on each method call would > + -- need to get the metatable, get __index, find the method here. While now > + -- it is only an index operation on the session object. Side note: for heap you still use "__index" even heap uses closures as methods.