From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Daniel Mendler Newsgroups: gmane.emacs.bugs Subject: bug#46326: 27.1.50; Excessive memory allocations with minibuffer-with-setup-hook Date: Fri, 23 Apr 2021 21:28:24 +0200 Message-ID: <781821e3-2b06-e946-6616-806f5a83540d@daniel-mendler.de> References: <62c490ed0d8d24d8b259ac1ba55ea79e@mendler.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="27893"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 46326@debbugs.gnu.org To: Stefan Monnier , jakanakaevangeli@chiru.no Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Apr 23 21:31:02 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1la1W1-00077w-KR for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 23 Apr 2021 21:31:01 +0200 Original-Received: from localhost ([::1]:52110 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1la1W0-0001ER-MG for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 23 Apr 2021 15:31:00 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:40444) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1la1U7-0000bQ-85 for bug-gnu-emacs@gnu.org; Fri, 23 Apr 2021 15:29:04 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:55581) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1la1U6-0006Kl-IE for bug-gnu-emacs@gnu.org; Fri, 23 Apr 2021 15:29:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1la1U6-00064d-FK for bug-gnu-emacs@gnu.org; Fri, 23 Apr 2021 15:29:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Daniel Mendler Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 23 Apr 2021 19:29:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 46326 X-GNU-PR-Package: emacs Original-Received: via spool by 46326-submit@debbugs.gnu.org id=B46326.161920611623310 (code B ref 46326); Fri, 23 Apr 2021 19:29:02 +0000 Original-Received: (at 46326) by debbugs.gnu.org; 23 Apr 2021 19:28:36 +0000 Original-Received: from localhost ([127.0.0.1]:38894 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1la1Tf-00063u-M7 for submit@debbugs.gnu.org; Fri, 23 Apr 2021 15:28:36 -0400 Original-Received: from server.qxqx.de ([178.63.65.180]:34773 helo=mail.qxqx.de) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1la1Td-00063g-J6 for 46326@debbugs.gnu.org; Fri, 23 Apr 2021 15:28:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=qxqx.de; s=mail1392553390; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=Ha/gz8YEJWpgOK4oeAgtR8lZOaGJ2HWKQvM2d9rbJas=; b=Fk8G03tckpyuu/irU1fX4Poo4l XGAz0LYJSc4aWhLwql5IWHitG7k1vYN2w7aSOu+4fVcBIF3F265yXEyPbWAA/NRGH4kvO6160Vfzm Q8avfWh4f5vOWh2rLzoX621hgtD7GR/AyjqvcT0Fh9KU90KulR2S/PYDC6T7GRThjmXQ=; In-Reply-To: Content-Language: en-US X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:204756 Archived-At: For me it seems to fix the issue. @jakanakaevangeli, can you confirm? I would still prefer to see a "proper fix". But given the backward compatibility requirements such a fix may not exist. Perhaps one could introduce some deprecation behavior. If a hook is removed and the object is not found via eq but found via equal, then print a warning? And then change the add-hook/remove-hook functions to eq only in some later version. Furthermore as a stop-gap measure one may still apply my patched symbol-indirection `minibuffer-with-setup-hook`, and revert it once the proper fix has been applied. (Using the symbol indirection seems to have other debuggability advantages. Closures are not particularly nice to debug in elisp, I hope we will also see some improvements regarding that. It is at least on my Elisp wishlist to have better introspection for closures, location info etc.) Note that `set-transient-map` already uses the symbol indirection. It may make sense to link to this bug from there such that one can adjust this function also at some later point depending on the resolution of this issue. The comment in `set-transient-map` reads like a bug to me "Don't use letrec, because equal (in add/remove-hook) would get trapped in a cycle." :) Daniel On 4/23/21 8:26 PM, Stefan Monnier wrote: >> I have an issue on 27.1.50 with excessive memory allocations when using >> minibuffer-with-setup-hook with large closures and :append. > > Indeed, we have a problem there. I think it's fairly hard to fix it > for good without introducing incompatibilities, because `add-hook` has > been defined to compare its functions with `equal` "for ever" and > changing it to use `eq` or `function-equal` will inevitably break > code out there in subtle ways. > > IOW I think the better fix is to change `minibuffer-with-setup-hook` to > use an indirection via a symbol. > > As for reducing the impact of the underlying issue, I see we could > reduce the amount of `equal` tests being performed, by using `eq` for > the lookups in `hook--depth-alist`. > So before we install the "real" solution, could you try the patch below > and report how much it helps (if at all)? > > > Stefan > > > diff --git a/lisp/subr.el b/lisp/subr.el > index c2be26a15f5..7b718a48a8d 100644 > --- a/lisp/subr.el > +++ b/lisp/subr.el > @@ -1830,12 +1834,13 @@ add-hook > (unless (member function hook-value) > (when (stringp function) ;FIXME: Why? > (setq function (purecopy function))) > + ;; All those `equal' tests performed between functions can end up being > + ;; costly since those functions may be large recursive and even cyclic > + ;; structures, so we index `hook--depth-alist' with `eq'. (bug#46326) > (when (or (get hook 'hook--depth-alist) (not (zerop depth))) > ;; Note: The main purpose of the above `when' test is to avoid running > ;; this `setf' before `gv' is loaded during bootstrap. > - (setf (alist-get function (get hook 'hook--depth-alist) > - 0 'remove #'equal) > - depth)) > + (push (cons function depth) (get hook 'hook--depth-alist))) > (setq hook-value > (if (< 0 depth) > (append hook-value (list function)) > @@ -1845,8 +1850,8 @@ add-hook > (setq hook-value > (sort (if (< 0 depth) hook-value (copy-sequence hook-value)) > (lambda (f1 f2) > - (< (alist-get f1 depth-alist 0 nil #'equal) > - (alist-get f2 depth-alist 0 nil #'equal)))))))) > + (< (alist-get f1 depth-alist 0 nil #'eq) > + (alist-get f2 depth-alist 0 nil #'eq)))))))) > ;; Set the actual variable > (if local > (progn > @@ -1907,11 +1912,20 @@ remove-hook > (not (and (consp (symbol-value hook)) > (memq t (symbol-value hook))))) > (setq local t)) > - (let ((hook-value (if local (symbol-value hook) (default-value hook)))) > + (let ((hook-value (if local (symbol-value hook) (default-value hook))) > + (old-fun nil)) > ;; Remove the function, for both the list and the non-list cases. > (if (or (not (listp hook-value)) (eq (car hook-value) 'lambda)) > - (if (equal hook-value function) (setq hook-value nil)) > - (setq hook-value (delete function (copy-sequence hook-value)))) > + (when (equal hook-value function) > + (setq old-fun hook-value) > + (setq hook-value nil)) > + (when (setq old-fun (car (member function hook-value))) > + (setq hook-value (remq old-fun hook-value)))) > + (when old-fun > + ;; Remove auxiliary depth info to avoid leaks. > + (put hook 'hook--depth-alist > + (delq (assq old-fun (get hook 'hook--depth-alist)) > + (get hook 'hook--depth-alist)))) > ;; If the function is on the global hook, we need to shadow it locally > ;;(when (and local (member function (default-value hook)) > ;; (not (member (cons 'not function) hook-value))) >