From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Andrea Corallo Newsgroups: gmane.emacs.bugs Subject: bug#67005: 30.0.50; improve nadivce/comp/trampoline handling Date: Tue, 14 Nov 2023 03:31:09 -0500 Message-ID: References: <874jhv9921.fsf@sappc2.fritz.box> <875y24zrt1.fsf@sappc2.fritz.box> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21247"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Stefan Monnier , 67005@debbugs.gnu.org To: Jens Schmidt Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Nov 14 09:31:46 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 1r2opl-0005Kz-Ou for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 14 Nov 2023 09:31:45 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1r2opR-0001JL-2n; Tue, 14 Nov 2023 03:31:25 -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 1r2opN-0001J7-AV for bug-gnu-emacs@gnu.org; Tue, 14 Nov 2023 03:31: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 1r2opM-00047G-SO for bug-gnu-emacs@gnu.org; Tue, 14 Nov 2023 03:31:21 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1r2oq2-0007j2-Do for bug-gnu-emacs@gnu.org; Tue, 14 Nov 2023 03:32:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Andrea Corallo Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 14 Nov 2023 08:32: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.169995072129689 (code B ref 67005); Tue, 14 Nov 2023 08:32:02 +0000 Original-Received: (at 67005) by debbugs.gnu.org; 14 Nov 2023 08:32:01 +0000 Original-Received: from localhost ([127.0.0.1]:60003 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r2oq0-0007im-5b for submit@debbugs.gnu.org; Tue, 14 Nov 2023 03:32:00 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:45620) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1r2opy-0007iY-AV for 67005@debbugs.gnu.org; Tue, 14 Nov 2023 03:31:59 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1r2opB-00046T-I1; Tue, 14 Nov 2023 03:31:09 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-Version:Date:References:In-Reply-To:Subject:To: From; bh=zR2B7b2jr1nLLqda/RI1XSYtFaJBhYexQ5LxopZcf1E=; b=P3wWzMojL2l9e94i76gk Z3ux0dRPIKPMX2SmVRMmUcbecemPW5gAmmD/HbjwkGjDQex/xe6dXH3d64zYY1YySImythqz7SG7p kEEZT4k5hFCf+rQCd6+PKI6KADKDlIq8Rip5mz0KmFFoy0a0MWdrpx/4nXcSoE042mapEl2xRYSQ3 jaOZSMx0uG282/wdlJBEj8LenPw0PVqQ4Nmjy1Fpz6ZINYBaRc94lmkCuRlAdXEpGiBhK4Mtm8ew4 0B4/X1ZRXINPYMf99j9mjAt/T9/LBMC+PFz0DIY93s1PkU0uEWqFnSsRfVFSZjcmQNzFOEhtustsV 0CkPmutq4RRyOQ==; Original-Received: from acorallo by fencepost.gnu.org with local (Exim 4.90_1) (envelope-from ) id 1r2opB-0001Dh-BA; Tue, 14 Nov 2023 03:31:09 -0500 In-Reply-To: <875y24zrt1.fsf@sappc2.fritz.box> (Jens Schmidt's message of "Tue, 14 Nov 2023 09:02:02 +0100") 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:274302 Archived-At: Jens Schmidt writes: > 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. That's correct. > - If it is not advised, the result is an optimized call that executes > advices if a native trampoline exists. If it's installed I'd say. > - If it is advised, the result is a non-optimized call (that always > executes advices). Correct. IIRC I have not considered this a big deal for two reasons. 1- If one advised a primitives is probably not very much interested in having it called directly from native code for performance reasons. 2- Compilation happens in the vast majority of the time as in a async process where the primitive is not advised anyway. That said I agree would be nice to fix this corner case in order to have the compiler producing reproducible elns. > In my opinion, we should instead: > > - always use optimized calls, unconditionally whether an advice exists > at the time of the compilation or not; Agreed > - ensure that native trampolines get installed when a primitive is > advised; Agreed > - ensure that advices are ruled out when native trampolines cannot be > installed during bootstrap. This would simply things so I like the idea, not sure it's acceptable tho. > > 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. Yep > 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": I think this is unrelared to this bug report. Probably we should have a dedicated bug report to discuss if this is a real bug o not. Thanks! Andrea