From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Jens Schmidt via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#67005: 30.0.50; improve nadivce/comp/trampoline handling Date: Tue, 14 Nov 2023 09:02:02 +0100 Message-ID: <875y24zrt1.fsf@sappc2.fritz.box> References: <874jhv9921.fsf@sappc2.fritz.box> Reply-To: Jens Schmidt Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39652"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 67005@debbugs.gnu.org To: andrea corallo , Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Nov 14 09:03:55 2023 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 1r2oOo-000A3L-Mm for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 14 Nov 2023 09:03:54 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r2oOQ-0003GY-E8; Tue, 14 Nov 2023 03:03:30 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r2oOH-0003Er-CM for bug-gnu-emacs@gnu.org; Tue, 14 Nov 2023 03:03:21 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1r2oOH-00076b-1X for bug-gnu-emacs@gnu.org; Tue, 14 Nov 2023 03:03:21 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r2oOw-00072D-Ey for bug-gnu-emacs@gnu.org; Tue, 14 Nov 2023 03:04:02 -0500 X-Loop: help-debbugs@gnu.org In-Reply-To: <874jhv9921.fsf@sappc2.fritz.box> Resent-From: Jens Schmidt Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 14 Nov 2023 08:04:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 67005 X-GNU-PR-Package: emacs Original-Received: via spool by 67005-submit@debbugs.gnu.org id=B67005.169994898826973 (code B ref 67005); Tue, 14 Nov 2023 08:04:02 +0000 Original-Received: (at 67005) by debbugs.gnu.org; 14 Nov 2023 08:03:08 +0000 Original-Received: from localhost ([127.0.0.1]:59995 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r2oO3-00070y-Uw for submit@debbugs.gnu.org; Tue, 14 Nov 2023 03:03:08 -0500 Original-Received: from mr3.vodafonemail.de ([145.253.228.163]:55386) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r2oO2-00070S-4u for 67005@debbugs.gnu.org; Tue, 14 Nov 2023 03:03:07 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vodafonemail.de; s=vfde-mb-mr2-23sep; t=1699948938; bh=+tTls7Ta1Lw+g3XI5NEH+LOhw/ESc0MXnhpO6oj5Krw=; h=From:To:Subject:References:Date:Message-ID:Content-Type:From; b=xCnGZldJl1c0tNBdvmEYinBa6E9LcyRGAnED085VXmuWWACi/pmXTpNQqdrdgCheV wmcvBAsHfvvF8ZHsyMaOPr+/krFWc/tJsXHs4EzHBl99npM2aDopdRFP0JHi36ciEo +KK/njbzognpPqF85jAf0UvEXjFG6qDGSJ6aTGM8= Original-Received: from smtp.vodafone.de (unknown [10.0.0.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by mr3.vodafonemail.de (Postfix) with ESMTPS id 4STzKj60T5z204w; Tue, 14 Nov 2023 08:02:17 +0000 (UTC) Original-Received: from sappc2 (port-92-196-38-191.dynamic.as20676.net [92.196.38.191]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp.vodafone.de (Postfix) with ESMTPSA id 4STzKV4c4gz9sPj; Tue, 14 Nov 2023 08:02:03 +0000 (UTC) X-purgate-type: clean X-purgate: clean X-purgate-size: 12422 X-purgate-ID: 155817::1699948933-BB7FD7FF-47A1A40B/0/0 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-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:274301 Archived-At: Thanks to both of you for taking the time to read through this! I tried my best to combine both replies in one mail. >>>>> "SM" == Stefan Monnier writes: >>>>> "AC" == Andrea Corallo writes: >> 1. Since Stefan has removed the advices on uniquify functions in >> 1a724cc2d2e7, that special handling of `rename-buffer' in >> `advice--add-function' and `native-comp-never-optimize-functions' >> seems to be obsolete. Away with it: SM> +1 AC> +1 >> 2. The comment in `advice--add-function' says that `macroexpand' >> causes a circular dependency, and there *are* still advices done >> on `macroexpand' (at least in `cl-symbol-macrolet'). But >> probably these are not executed during the bootstrap? Let's try. SM> +1 AC> +1 >> 3. But then on the other hand, function Ffset also calls >> `comp-subr-trampoline-install', and if we have condition >> >> `(subr-primitive-p (gv-deref ref))' >> >> fulfilled in `advice--add-function', then the following `(setf >> (gv-deref ref) ...)' should ultimately call `fset'. So probably >> we can completely remove that `comp-subr-trampoline-install' call >> from `advice--add-function', like this: SM> It looks good to me, tho I must admit I do not understand why we SM> have this `comp-subr-trampoline-install` in addition to the one in SM> `Ffset`, so maybe I'm missing something. AC> Unfortanatelly I can't remember why we have this specific handling, AC> I guess there was a reason but anyway... If there is still a good AC> reason we'll discover it. I might have found a reason ... see section "The Bigger Picture" below. >> 5. And actually I think that b) above probably is a bug: It means >> that for advised subrs, the optimzation to indirect calls in >> function `comp-call-optim-form-call' never takes place. >> >> So I tried to recognize subrs in function >> `comp-call-optim-form-call' even when they are advised: SM> I think this is not a correct optimization, as you mention: >> But: Natively compiled code now does NOT execute the advice on >> `rename-buffer' added in step 4, since its call gets optimized >> but no trampoline has been created for it. SM> So it's a -1 from me on this patch. AC> It's good we optimize the call but we can't apply this patch if the AC> this issue is not solved. Please let me try to address your concerns. TL;DR: - If `rename-buffer' is advised *during* bootstrap (as with patch 4), no native trampoline gets created for it because of `native-comp-enable-subr-trampolines' being nil during loadup.el. - Without native trampoline, the fix that I propose in patch 5 lead to the advice not being called. However: - If `rename-buffer' (or any other primitive) is advised *after* bootstrap, a trampoline gets properly created and the advice is being called even with my fix in patch 5. Full version: See section "More Background Information ..." >> 6. So there is the question whether we should actively disallow >> advices during bootstrap, now that we are free of them. Like >> this: AC> Others can have different opinions but if it's not a big limitation AC> for the future it sounds like a good idea to me. SM> Rather than `dump-mode`, I'd test `purify-flag`. SM> This is because `purify-flag` is not set while building SM> `src/bootstrap-emacs`. SM> Part of the issue here is that during the build of SM> `src/bootstrap-emacs` we load a lot more ELisp code than in the SM> "real" build so it's good to restrict this constraint to the "real SM> build". Will do. More Background Information on that Suspicous Fix ... ----------------------------------------------------- Now for more background on why I think the current way of detecting primitives in function `comp-call-optim-form-call' is buggy. I provide that background not to lecture anybody, but to make my reasoning hopefully more transparent. There are two different items in the native compilation area called "trampoline": - There are "funcall trampolines", which are instructions to call a function from natively compiled code through a `funcall' indirection. A call of a primitive through such a funcall trampoline is pretty close to a function call of that primitive from interpreted Elisp and always executes advices. - And there are "native trampolines", which are natively compiled wrappers for primitives. If and only if a primitive has such a native trampoline, advices on the primitive are executed even if the primitive is called from natively compiled code *without* a funcall trampoline. To see the difference, start with a vanilla master emacs, without any of my patches, and rewrite the function `comp-call-optim' with extended logging to a file test.el as follows: ------------------------- test.el ------------------------- (with-eval-after-load 'comp (defun comp-call-optim (_) "Try to optimize out funcall trampoline usage when possible." (maphash (lambda (_ f) (when (and (>= (comp-func-speed f) 2) (comp-func-l-p f)) (let ((comp-func f)) (when (eq (comp-func-name f) 'foo) (comp-log "\nPre comp-call-optim" 0) (comp-log-func f 0)) (comp-call-optim-func) (when (eq (comp-func-name f) 'foo) (comp-log "\nPost comp-call-optim" 0) (comp-log-func f 0))))) (comp-ctxt-funcs-h comp-ctxt)))) ------------------------- test.el ------------------------- And the following to a file foo.el: ------------------------- foo.el ------------------------- ;;; -*- lexical-binding: t -*- (defun foo (x) (sqrt x)) ------------------------- foo.el ------------------------- Then start "./src/emacs -Q -l test.el" and evaluate the following in *scratch*: ------------------------- snip(A) ------------------------- (with-current-buffer (get-buffer-create "*Native-compile-Log*") (setq buffer-read-only nil) (erase-buffer)) (load-file (native-compile "foo.el")) (pop-to-buffer "*Native-compile-Log*") ------------------------- snip(A) ------------------------- The interesting part in the resulting *Native-compile-Log* is how the call to `sqrt' in function `foo' gets optimized. (I chose that as example primitive because it is not as often called as `rename-buffer' and causes less noise during tests.) ------------------------- LIMPEL ------------------------- Pre comp-call-optim (set #(mvar 23578551540518 1 float) (callref funcall #(mvar 23578553446684 1 (member sqrt)) #(mvar 23578554498624 2 t))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Post comp-call-optim (set #(mvar 23578551540518 1 float) (call sqrt #(mvar 23578554498624 2 t))) ^^^^ ------------------------- LIMPEL ------------------------- So the indirect `callref funcall' got optimized to a direct `call', and: An advice on `sqrt' is executed even if the natively compiled `foo' is called: ------------------------- snip(B) ------------------------- (defun sqrt-advice (&rest _) (message "Advice called")) (advice-add #'sqrt :before #'sqrt-advice) (foo 0) => "Advice called" echoed in message area ------------------------- snip(B) ------------------------- Now re-evaluate the snip(A) again and see how `foo' is *not* optimized, this time because of the advice on `sqrt' and because of what I consider the bug in `comp-call-optim-form-call': ------------------------- LIMPEL ------------------------- Pre comp-call-optim (set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Post comp-call-optim (set #(mvar 23487733705560 1 float) (callref funcall #(mvar 23487733185614 1 (member sqrt)) #(mvar 23487733838350 2 t))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ------------------------- LIMPEL ------------------------- To summarize the current, unpatched master behavior: - The result of native compilation of a call to a primitive depends on whether the primitive is advised or not. - If it is not advised, the result is an optimized call that executes advices if a native trampoline exists. - If it is advised, the result is a non-optimized call (that always executes advices). In my opinion, we should instead: - always use optimized calls, unconditionally whether an advice exists at the time of the compilation or not; - ensure that native trampolines get installed when a primitive is advised; - ensure that advices are ruled out when native trampolines cannot be installed during bootstrap. The Bigger Picture ------------------ I might be wrong with my following litany, after all things have been working fine for quite some time. In that case I apologize in advance to whom I'll suspect in wrong-doing. But to me it seems that things are working only "by chance" and without a, well, bigger picture. So for me the bigger picture or problem is that it is not always enough to rely on a simple `(symbol-function 'foo)' when you want to process function `foo'. It is enough if you just want to call it, but not if you want to do fancy things with it, like compiling it. In these fancy cases you might really want to dig through all possibly existing advices on `foo' and process the _original_ function instead. One instance is the detection of primitives in `comp-call-optim-form-call', which I have tried to address above. Another might be the following (on emacs master, without my fixes): ------------------------- snip ------------------------- (defun bar () nil) (advice-add 'bar :before 'ignore) (native-compile 'bar) => comp--native-compile: Native compiler error: bar, "can't native compile an already byte-compiled function" ------------------------- snip ------------------------- Here `native-compile' falsely (?) detects and reports `bar' as already byte-compiled, again because it just inspects `(symbol-function function-name)' in `comp-spill-lap-function ((function-name symbol))'. However, here the solution is not as simple as just slapping `advice--cd*r' onto the problem, since function `byte-compile' *also* recognizes the advised `bar' as "already compiled": ------------------------- snip ------------------------- (defun bar () nil) (advice-add 'bar :before 'ignore) (byte-compile 'bar) => Function bar is already compiled ------------------------- snip ------------------------- When you slap `advice--cd*r' onto *that* problem as well, function `bar' both natively compiles and byte-compiles even if advised. Another suspectible spot is the following from function `Ffset': if (!NILP (Vnative_comp_enable_subr_trampolines) && SUBRP (function) && !SUBR_NATIVE_COMPILEDP (function)) CALLN (Ffuncall, Qcomp_subr_trampoline_install, symbol); Here FUNCTION would not be recognized as primitive once advised. This is not a problem if FUNCTION is properly recognized as a primitive *before* it is advised for the first time. But if that moment gets missed, you first have to remove all advices before a trampoline is generated on the next advice addition. I wanted to demonstrate that on an unpatched master Emacs by tweaking variable `native-comp-enable-subr-trampoline' in the right fashion, but failed to do so: In that scenario `advice--add-function' also calls `comp-subr-trampoline-install'! And it does so even if `native-comp-enable-subr-trampoline' equals nil! So probably Andrea added that second unconditional call to `comp-subr-trampoline-install' as a second line of defense for "funny cases". Which would be OK on the one side, but also problematical on the other, since with `native-comp-enable-subr-trampoline' equaling nil no trampolines should be generated according to the docstring of that variable. Oh my. What do you think?