bug#67005: 30.0.50; improve nadivce/comp/trampoline handling

[Andrea Corallo <acorallo@gnu.org>]

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