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 C70757F605; Sun, 7 Feb 2021 21:13:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C70757F605 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1612721626; bh=SbnxJw6ySjU0TFv+jEubGyIrAvxM6HyVKtUwiVvnxA8=; 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=Oj58/HJtKj89ZlS+Cry3xp0r2Yn5tyQZDGdOTvquN/f3LsPcqma7fpvmXdcVhWJIZ R94Qu9lPFPvRgwXsSolGbLlP8oUM7SFtKviju8Ctv4evTklM1d2hJ10jxF7y61LfQ2 XIB5eQazMt2v262sWV/7G6xFT+WHeIkKgtzH1QfI= Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 4B38264660 for ; Sun, 7 Feb 2021 20:20:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4B38264660 Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1l8njn-000867-VM; Sun, 07 Feb 2021 20:20:44 +0300 To: Cyrill Gorcunov , tml Cc: Mons Anderson References: <20210205185436.638894-1-gorcunov@gmail.com> <20210205185436.638894-10-gorcunov@gmail.com> Message-ID: Date: Sun, 7 Feb 2021 18:20:42 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210205185436.638894-10-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD953AC099BC0052A9C4647521586BE7E6324520D2A088600D8182A05F53808504079D25AD1D097979FAFD019E12DD9A003B8C18F413AAF8F66E9544BC53203A238 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76DA79C5AFF329FDBEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637770CCD3A0ADFB7EEEA1F7E6F0F101C674E70A05D1297E1BBC6CDE5D1141D2B1CB8E02677C752DF747B057D543284227C7D5CAB606BECADB89FA2833FD35BB23D9E625A9149C048EE33AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B5FC25ED3FCEC3375A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FCC109CDBF71E2B98BD81D268191BDAD3DBD4B6F7A4D31EC0BEA7A3FFF5B025636D81D268191BDAD3D78DA827A17800CE7832357308142D352EC76A7562686271EEC990983EF5C032935872C767BF85DA29E625A9149C048EEB89ED3C7A6281781D2D576BCF940C7364AD6D5ED66289B524E70A05D1297E1BB35872C767BF85DA227C277FBC8AE2E8B9BFB91CAEB05C77775ECD9A6C639B01B4E70A05D1297E1BBC6867C52282FAC85D9B7C4F32B44FF57E8FBB06288C1946000306258E7E6ABB4E4A6367B16DE6309 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24A6D60772A99906F8E1CD14B953EB46DE15501A9F2622ADD355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A5A8B1262E482B0D051A20DF262F1B28F7F2C24EB4F8040192D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D348BF433665406F390B0AB40B8C48B7DD4D29EF116BDF6CDA459E00F6C529EFDBE1062677C52B024881D7E09C32AA3244C0A1F37C25F87814DD04983D82EBABA32853296C06374E602FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMxr0zTsJFiSyp9tMptoSpw== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA3110E20A360582E3622AD4792E4B24BA6139F2D8EB02FD139D1C07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v15 09/11] module_cache: use own hash for box.schema.func requests 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" Thanks for the patch! On 05.02.2021 19:54, Cyrill Gorcunov via Tarantool-patches wrote: > This is needed to distinguish requests from future `cmod` module > (which gonna be detecting modules renew automatically) and old > `box.schema.func` interface. To make it clear use `box_schema_hash` > for such modules. After this patch you made module_cache depend on box.schema.func. Which should not happen. Module_cache is at the lower level. It should not depend on anything in box except basic things like ClientError, port. Besides, you made cmod depend on box.schema.func implicitly. Depend on the storage. Which was an issue of the first versions of the patchset. We should not go there again. Both this patch and the previous one should not really exist. If you have a hash specific for box.schema.func, it should be there. In box/func.c for example. AFAIS, this issue should disappear when you will have a second hash on top of the first one. Then module_sym on first load will get a module from module_cache. You will put it into the box.schema.func hash, increase reference counter. And all next loads will see module_sym.module != NULL even if it was deleted from the lower hash. So it won't lead to load of a new module. But even if we wouldn't make the second hash on top, but on the side, the hack in this commit also could be fixed. You would need to extract module_sym first load into a new function. The regular sym load would accept a module object. And also you would need to expose methods to load/unload an entire module, not just module_sym. Then you could have a separate hash in another file. However as we discussed verbally, better go for the two-level hash approach. I suggest to try this really-really hard. It won't help if you will send a new patchset almost every day. It will only slow things down. P.S. I see you even did separate the module load/unload into new functions, like I advised above. This should be a sign that the module_cache literally asks us not to make 2 independent hash tables in it. > diff --git a/src/box/module_cache.c b/src/box/module_cache.c > index 786e49cba..22b906fd7 100644 > --- a/src/box/module_cache.c > +++ b/src/box/module_cache.c > @@ -23,7 +23,7 @@ > #include "module_cache.h" > > /** Modules name to descriptor hash. */ > -static struct mh_strnptr_t *mod_hash = NULL; > +static struct mh_strnptr_t *box_schema_hash = NULL; > > /** > * Parsed symbol and package names. > @@ -45,6 +45,16 @@ struct func_name { > const char *package_end; > }; > > +/** > + * Return module hash depending on where request comes > + * from: if it is legacy `box.schema.func` interface or not. > + */ > +static inline struct mh_strnptr_t * > +hash_tbl(bool is_box_schema) > +{ > + return is_box_schema ? box_schema_hash : NULL; > +} > + > /*** > * Split function name to symbol and package names. > * > @@ -391,7 +401,7 @@ module_sym(struct module *module, const char *name) > } > > int > -module_sym_load(struct module_sym *mod_sym) > +module_sym_load(struct module_sym *mod_sym, bool is_box_schema) The amount of hacks like 'is_box_schema' flag, 'hash_tbl' function, 'module hash pointer' in the module object, and even usage of word 'schema' in a subsystem which has nothing to do with the storage, should be a red flag that something is wrong.