From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [PATCH 09/10] Access buffer_defaults in BVAR if there's no local binding Date: Thu, 19 Nov 2020 13:08:16 -0500 Message-ID: References: <87mtzil519.fsf@catern.com> <20201119153814.17541-10-sbaugh@catern.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="7971"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: Arnold Noronha , Dmitry Gutov , emacs-devel@gnu.org To: Spencer Baugh Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Thu Nov 19 19:09:33 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kfoNA-0001wT-ME for ged-emacs-devel@m.gmane-mx.org; Thu, 19 Nov 2020 19:09:32 +0100 Original-Received: from localhost ([::1]:48846 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kfoN9-0007mx-OH for ged-emacs-devel@m.gmane-mx.org; Thu, 19 Nov 2020 13:09:31 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:52850) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kfoM5-0007Ib-0h for emacs-devel@gnu.org; Thu, 19 Nov 2020 13:08:25 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:36205) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kfoM1-0005Wy-Cp for emacs-devel@gnu.org; Thu, 19 Nov 2020 13:08:24 -0500 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id B3F0D44080E; Thu, 19 Nov 2020 13:08:19 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id A7B9E4403CC; Thu, 19 Nov 2020 13:08:17 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1605809297; bh=YXY27XkT/tfpjLxchg49bg4yzZIY0vaoqplP3U/uH+Y=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=GOlvfZ/5MijSreO+TyX0k8cMpOREQgfyM+QPiaQ6Ozydgwo6jqK0FQ4wEoM3fNshi 3UmqWLGZrzwRjGa78OR9qt00JVLLKR2WtX7NSkYK578LtVM27VDvRmmi1huHy9kPC2 WNMmnGglFlGLbYLN2/u7LegiQC1SzzjiBFTZ5axIqDrGhXfzrbkvr4X30DhV2LX2+K 3aRRyr7BMfKD7tuRJZhgPb652ptnzV6EhBw1B1/5OTEaaIBv7oN85CTBKpDEKOLZBV Jsi9dQBsB++yGn9bgpKpr91N/7BkDDFY/TNIyY1MF+G1jv9KzrhJW31XWuDr8S6LAT SScX5c7UxcZ2w== Original-Received: from alfajor (unknown [157.52.9.240]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 73320120320; Thu, 19 Nov 2020 13:08:17 -0500 (EST) In-Reply-To: <20201119153814.17541-10-sbaugh@catern.com> (Spencer Baugh's message of "Thu, 19 Nov 2020 10:38:13 -0500") Received-SPF: pass client-ip=132.204.25.50; envelope-from=monnier@iro.umontreal.ca; helo=mailscanner.iro.umontreal.ca X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:259445 Archived-At: > If the buffer_local_flags Lisp_Object is 0, though, it means this > field in buffer_local_flags wasn't initialized, Actually, no it means it was explicitly set (otherwise it would be Qnil). 0 means that this slot isn't exposed as a lisp variable (and those fields don't have a "default value"). > which means this field is always buffer-local (and therefore doesn't > use buffer_defaults). That's right. > @@ -825,9 +825,31 @@ set_per_buffer_value (struct buffer *b, int offset, Lisp_Object value) > *(Lisp_Object *)(offset + (char *) b) = value; > } > > +INLINE bool > +SHOULD_USE_BUFFER_DEFAULTS(struct buffer *b, ptrdiff_t offset) ^^ SPC Remember to use a space before opening parens (there are other occurrences in your patches). > +{ > + Lisp_Object obj = *((Lisp_Object *) (offset + (char *) &buffer_local_flags)); I think you want to use `PER_BUFFER_IDX` here. > + if (obj.i == 0) > + return false; Please don't use `obj.i`, which relies on the internal representation of `Lisp_Object`. Also, this test is not needed: a zero value here (i.e. obj == Qnil) would be a bug. We already test this elsewhere, but even if we don't test it here, `XFIXNUM` could signal an error if `obj` is not a fixnum, so this test shouldn't be needed even for debugging purposes. > + int idx = XFIXNUM(obj); > + if (idx < 1) > + return false; > + return b->local_flags[idx] == 0; I'd write it as return idx > 0 && !PER_BUFFER_VALUE_P (b, idx); Largely because I dislike writing the constant `true` or `false`, especially within `if` branches. > +INLINE Lisp_Object > +bvar_get(struct buffer *b, ptrdiff_t offset) > +{ > + if (SHOULD_USE_BUFFER_DEFAULTS(b, offset)) { > + return per_buffer_value(&buffer_defaults, offset); > + } else { > + return per_buffer_value(b, offset); > + } Remember also that our coding style puts braces on their own lines like: if (SHOULD_USE_BUFFER_DEFAULTS (b, offset)) { return per_buffer_value (&buffer_defaults, offset); } else { return per_buffer_value (b, offset); } But here you can just drop the braces (and add the spaces and use `per_buffer_default`): if (SHOULD_USE_BUFFER_DEFAULTS (b, offset)) return per_buffer_default (offset); else return per_buffer_value (b, offset); aka return SHOULD_USE_BUFFER_DEFAULTS (b, offset)) ? per_buffer_value (&buffer_defaults, offset) : per_buffer_value (b, offset); > -#define BVAR(buf, field) ((void)0, (buf)->field ## _) > +#define BVAR(buf, field) bvar_get(buf, PER_BUFFER_VAR_OFFSET (field)) This will unnecessarily slow down access to those fields which have an `idx < 1` since I can't imagine the compiler will be able to figure out that `SHOULD_USE_BUFFER_DEFAULTS` will always return false. So maybe those fields should be accessed via BVAR_DIRECT (which could include an `eassert` to make sure that `SHOULD_USE_BUFFER_DEFAULTS` indeed is false). Your patch series will also presumably slow down BVAR access to the other fields since they need to test `SHOULD_USE_BUFFER_DEFAULTS` now, but I think this is the price to pay. It should be negligible for accesses to those vars from Elisp code, but of the 600 or so uses of BVAR in the current code, I wonder if some of them could show a measurable speed penalty. Stefan