From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 1A228469719 for ; Tue, 17 Mar 2020 20:24:51 +0300 (MSK) Date: Tue, 17 Mar 2020 17:24:50 +0000 From: Nikita Pettik Message-ID: <20200317172450.GA3717@tarantool.org> References: <642b3f52ff8670b139e62012b46fb45750e777cf.1581940900.git.k.sosnin@tarantool.org> <20200316141630.GF14628@tarantool.org> <35c443bb-9416-d7a2-0eb2-99c299dd5abb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <35c443bb-9416-d7a2-0eb2-99c299dd5abb@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 16 Mar 23:53, Vladislav Shpilevoy wrote: > Hi! Thanks for your comments! > > On 16/03/2020 15:16, Nikita Pettik wrote: > > On 17 Feb 15:12, Chris Sosnin wrote: > >> As it is mentioned in implementation, it is important > >> that _session_settings options are sorted by name, so > >> there is no need in linear lookup and we can replace it > >> with binary search. > > > > Is there any sufficient performance benefit except for code complication?:) > > I really doubt that this part should be optimized for such rare in > > terms of usage place. > > We didn't measure perf increase, but for such a simple change it > did not seem necessary. I mean there can be no even performance benefit at all since binary search on small data sets can be slower than sequential access (there's linear search mode even in bps tree, but it is compile time option). > Talking of rareness of settings change - this is arguable. Firstly, > every new session will change settings, if a user's project uses some > of them permanently. Secondly, some settings, such as default engine, > or constraints related, or similar can be changed on per-request basis, > in case user wants to make some requests not checking FK, and others > should check them, inside one session, for instance. Similar necessity > can arise for other settings. I think we can actually measure how faster > lookup became for that case. > > > What is more, there's only dozen entries, so > > binary search doesn't really make any sence. Idk why Kirill assigned 2.4.1 > > It is only dozen for now. This will change. We already have read-only > session coming. > > > milestone, IMHO there are way far more important things to elaborate on. > > Honestly, I was thinking there is not much to elaborate. This is > trivial bsearch. I mean work on more improtant features or/and severe bugs. Nevermind, it is already done anyway. Personally I would reject this patch if there's no any performance increase (otherwise, it simply complicates code).