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 206026EC59; Tue, 9 Mar 2021 11:03:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 206026EC59 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615277018; bh=d80cPD77DEQ0w9L6uSk3D/PCDEqJVshShDGyqRN2vG8=; 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=Pwdu80w6XaN2kw0bz39EqKlkPVswP2XcWGtWVZQ03/tj5IkzrO1oX+gJfMoBevAIA KVI+aX5DdgfUckGbrOvhAsp/ZdsJYes+Af9R9GtCjo7ijUl0zN51xA4DJgQB/NGe6L 3OZ6mUZGURETZXg/Li8uhu0pSSZkAidvIYRfmDC8= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 60D946EC59 for ; Tue, 9 Mar 2021 11:03:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 60D946EC59 Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1lJXL5-0001Ck-Bq; Tue, 09 Mar 2021 11:03:35 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org References: <0522c6f4-7c8a-b424-eeb1-0bb0b10084d7@tarantool.org> <4afd6986-fa28-c0d0-cba4-f9d56fd19146@tarantool.org> Message-ID: <8561a6ce-dcc4-983c-1b4b-01e0968859b9@tarantool.org> Date: Tue, 9 Mar 2021 11:03:34 +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: <4afd6986-fa28-c0d0-cba4-f9d56fd19146@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D3134714A9BDB69B9676070C5F64E418DB309FBA590CF44700894C459B0CD1B98CD7D1185731C1501CB98E8CA099B2FF27AC468E0AD0A919599FBF3E6EA3F319 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72AC9FB60380F23AEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377A7A7D315BEE81B48638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C1F4A82545730C76185E0599223F129C2E35DBFE8C12BB5C9A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3E478A468B35FE767117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF056D5A8E4C6B598EBA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B66E22809C8288D2D976E601842F6C81A1F004C90652538430CDED94BCBF13EF3B93EC92FD9297F6718AA50765F7900637F42730888E86278DA7F4EDE966BC389F395957E7521B51C24C7702A67D5C33162DBA43225CD8A89FBFBFE0634520CEB9A91E23F1B6B78B78B5C8C57E37DE458B4C7702A67D5C3316FA3894348FB808DB48C21F01D89DB561574AF45C6390F7469DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5223EDC32685893AE5407656C1D0AD63CBC73040580EB3954D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34753B45383CEFF204EE356B91B256798A21A33F6EA4E4F5EBCFE5111A3FCBD8A8D1868767DDF72AAE1D7E09C32AA3244C489BB53456DC3ABDFB719C1C6DC0309F435BF7150578642FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUqxEkzHTt/hWWJt3ERcXvg== X-Mailru-Sender: 583F1D7ACE8F49BD3369739A12991F4D156E2ABA1BC0C9F8FA205B34D90C30ECBCC37416C19CA54D23E75C7104EB1B885DEE61814008E47C7013064206BFB89F93956FB04BA385BE9437F6177E88F7363CDA0F3B3F5B9367 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! Thanks for fixes. LGTM. On 06.03.2021 01:06, Vladislav Shpilevoy wrote: > Hi! Thanks for the review! > >>> +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". > I have global_map and global_heap variables because I also have normal map and > heap, local to the session. To distinguish between them I added 'global_' > prefix to the global ones. > > The count here is not global. It is local to the session. But I see the point. > I renamed it to `ref_count` to be consistent with `ref_map` and `ref_heap`. > > ==================== > diff --git a/vshard/storage/ref.lua b/vshard/storage/ref.lua > index 7589cb9..27f7804 100644 > --- a/vshard/storage/ref.lua > +++ b/vshard/storage/ref.lua > @@ -84,7 +84,7 @@ local function ref_session_new(sid) > -- 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 > + local ref_count = 0 > -- Cache global session storages as upvalues to save on M indexing. > local global_heap = M.session_heap > local global_map = M.session_map > @@ -94,9 +94,9 @@ local function ref_session_new(sid) > assert(new_count >= 0) > M.count = new_count > > - new_count = count - del_count > + new_count = ref_count - del_count > assert(new_count >= 0) > - count = new_count > + ref_count = new_count > end > > local function ref_session_update_deadline(self) > @@ -224,7 +224,7 @@ local function ref_session_new(sid) > self.deadline = deadline > global_heap:update(self) > end > - count = count + 1 > + ref_count = ref_count + 1 > M.count = M.count + 1 > return true > end > @@ -260,7 +260,7 @@ local function ref_session_new(sid) > local function ref_session_kill(self) > global_map[sid] = nil > global_heap:remove(self) > - ref_session_discount(self, count) > + ref_session_discount(self, ref_count) > end > > -- Don't use __index. It is useless since all sessions use closures as > ==================== > >>> + >>> +    -- >>> +    -- 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? > No problem: > > ==================== > diff --git a/vshard/storage/ref.lua b/vshard/storage/ref.lua > index 27f7804..d31e3ed 100644 > --- a/vshard/storage/ref.lua > +++ b/vshard/storage/ref.lua > @@ -164,9 +164,9 @@ local function ref_session_new(sid) > -- 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 > + -- Returns nil 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. > + -- until it returns nil. > -- 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 > @@ -178,7 +178,7 @@ local function ref_session_new(sid) > -- > local function ref_session_gc(self, limit, now) > if self.deadline >= now then > - return false > + return nil > end > local top = ref_heap:top() > local del = 1 > ==================== > >>> + >>> +    -- 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. > Indeed, I should have thought of this. I updated the part1 branch, and rebased the > part2 branch. See the part1 email thread for the diff.