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 748E670201; Sat, 6 Mar 2021 01:06:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 748E670201 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614982005; bh=z8vRHrAjL71G5M1/66G4pp6dpljFDTr57W7q5u+0AVg=; 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=nEpfZBD4m8Z4Ux2RCMbBWx6SXLcUPBwXIVgCjtzHkBS8sr5wNgksPgunKx2Al6S1F ZccbJPbBu7oltnzOeS0/vE9DDOUqYqq7KsDBSboMR6k46ek+u2xSo1wetAXsXdQ6sU Jix6hFa7M2lGJ/5S+FFFwysgOZHIm6CT2m8vqEpA= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4783D70201 for ; Sat, 6 Mar 2021 01:06:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4783D70201 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lIIac-0003o5-Bg; Sat, 06 Mar 2021 01:06:30 +0300 To: Oleg Babin , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org References: <0522c6f4-7c8a-b424-eeb1-0bb0b10084d7@tarantool.org> Message-ID: <4afd6986-fa28-c0d0-cba4-f9d56fd19146@tarantool.org> Date: Fri, 5 Mar 2021 23:06:29 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <0522c6f4-7c8a-b424-eeb1-0bb0b10084d7@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D3134714A9BDB69B6D9CBB8C4C1E2C2453F3F16C3D6F3AC500894C459B0CD1B9B38D6B55F1A41DB8A5E2988E887B3FF08484C038DC1C36873E4D764DDA6573C6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721D130CF676D2164EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D071468F2F69688EA1F7E6F0F101C67CDEEF6D7F21E0D1D174C73DBBBFC7664F89A9E1B1F8764173B1F5FCCE01744C4F23B6E50A1295FF2389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B64854413538E1713FCC7F00164DA146DA6F5DAA56C3B73B23C77107234E2CFBA567F23339F89546C55F5C1EE8F4F765FCB07C9E286C61B7F975ECD9A6C639B01BBD4B6F7A4D31EC0BC0CAF46E325F83A522CA9DD8327EE4930A3850AC1BE2E735CC4B623DB76FBBCBC4224003CC836476C0CAF46E325F83A50BF2EBBBDD9D6B0FECB2555BB02FD5A93B503F486389A921A5CC5B56E945C8DA X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2BBE337FB72E923155C0AF1600DCBC20B2BFB33BE6C544648283DA90AE663375C X-C1DE0DAB: 0D63561A33F958A563E765FC953344E87F2186DF64B7670565019223CC37BBE5D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346E67BC984B8ABCB531AAB9CB83D12A5060CA8AFFB8F36E2AB4A0F944F2C75426D39ACF84AAAC28411D7E09C32AA3244C2E9D69BDDDDD8C213AEEA83DAD95F59B7C0C08F7987826B9FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXmjzTEesUQF0K/5dg9MQGw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A06382296361558BDEA4FE313E9D47EC6659E973841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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.