[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#67005: 30.0.50; improve nadivce/comp/trampoline handling
From: |
Andrea Corallo |
Subject: |
bug#67005: 30.0.50; improve nadivce/comp/trampoline handling |
Date: |
Tue, 14 Nov 2023 03:31:09 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
Jens Schmidt <jschmidt4gnu@vodafonemail.de> 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 <monnier@iro.umontreal.ca> writes:
>>>>>> "AC" == Andrea Corallo <acorallo@gnu.org> 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
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/08
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/08
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/12
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/14
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/14
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/15
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/20
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/20
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/21
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Jens Schmidt, 2023/11/21
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/21
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Andrea Corallo, 2023/11/22
- bug#67005: 30.0.50; improve nadivce/comp/trampoline handling, Stefan Monnier, 2023/11/22