[PATCH] Findirect_function

[PATCH] Findirect_function

[Manuel Giraud]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

Hi,

Maybe it is bike shedding territory (and maybe I'm missing something)
but here are two patches for Findirect_function and its usage. "make
check" is the same and so far it works for me™.

Best regards,

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-simplify-return-case-for-Findirect_function.patch --]
[-- Type: text/x-patch, Size: 805 bytes --]

From 300db08a4d66814000e2d6eb373f931c37913954 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 14 Jan 2022 11:28:28 +0100
Subject: [PATCH 1/2] simplify return case for Findirect_function.

---
 src/data.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/data.c b/src/data.c
index f287c38fcd..65babc49e9 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2380,10 +2380,8 @@ DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 2, 0,
   if (SYMBOLP (result) && !NILP (result)
       && (result = XSYMBOL (result)->u.s.function, SYMBOLP (result)))
     result = indirect_function (result);
-  if (!NILP (result))
-    return result;
 
-  return Qnil;
+  return result;
 }
 \f
 /* Extract and set vector and string elements.  */
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-use-Findirect_function-more.patch --]
[-- Type: text/x-patch, Size: 1939 bytes --]

From 99658ecb6860185e0b3fdb8932de3a8b6f7e0f69 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Fri, 14 Jan 2022 11:29:10 +0100
Subject: [PATCH 2/2] use Findirect_function more.

---
 src/eval.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 5514583b6a..f72eb6f67a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2668,14 +2668,10 @@ DEFUN ("apply", Fapply, Sapply, 1, MANY, 0,
   numargs += nargs - 2;
 
   /* Optimize for no indirection.  */
-  if (SYMBOLP (fun) && !NILP (fun)
-      && (fun = XSYMBOL (fun)->u.s.function, SYMBOLP (fun)))
-    {
-      fun = indirect_function (fun);
-      if (NILP (fun))
-	/* Let funcall get the error.  */
-	fun = args[0];
-    }
+  fun = Findirect_function(fun, Qt);
+  if (NILP (fun))
+    /* Let funcall get the error.  */
+    fun = args[0];
 
   if (SUBRP (fun) && XSUBR (fun)->max_args > numargs
       /* Don't hide an error by adding missing arguments.  */
@@ -3069,10 +3065,7 @@ DEFUN ("funcall", Ffuncall, Sfuncall, 1, MANY, 0,
  retry:
 
   /* Optimize for no indirection.  */
-  fun = original_fun;
-  if (SYMBOLP (fun) && !NILP (fun)
-      && (fun = XSYMBOL (fun)->u.s.function, SYMBOLP (fun)))
-    fun = indirect_function (fun);
+  fun = Findirect_function(original_fun, Qt);
 
   if (SUBRP (fun) && !SUBR_NATIVE_COMPILED_DYNP (fun))
     val = funcall_subr (XSUBR (fun), numargs, args + 1);
@@ -3388,13 +3381,7 @@ DEFUN ("func-arity", Ffunc_arity, Sfunc_arity, 1, 1, 0,
  retry:
 
   /* Optimize for no indirection.  */
-  function = original;
-  if (SYMBOLP (function) && !NILP (function))
-    {
-      function = XSYMBOL (function)->u.s.function;
-      if (SYMBOLP (function))
-	function = indirect_function (function);
-    }
+  function = Findirect_function(original, Qt);
 
   if (CONSP (function) && EQ (XCAR (function), Qmacro))
     function = XCDR (function);
-- 
2.34.1


[-- Attachment #4: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

Re: [PATCH] Findirect_function

[Mattias Engdegård]

14 jan. 2022 kl. 11.40 skrev Manuel Giraud <manuel@ledu-giraud.fr>:

> Maybe it is bike shedding territory (and maybe I'm missing something)

Not at all, patches are always welcome!

> but here are two patches for Findirect_function and its usage. "make
> check" is the same and so far it works for me™.

The first change is straightforward:

> @@ -2380,10 +2380,8 @@ DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 2, 0,
>    if (SYMBOLP (result) && !NILP (result)
>        && (result = XSYMBOL (result)->u.s.function, SYMBOLP (result)))
>      result = indirect_function (result);
> -  if (!NILP (result))
> -    return result;
>  
> -  return Qnil;
> +  return result;

Good! See if you can find more instances of this unfortunate pattern. Even if it makes no difference for the generated code (very likely), it obfuscates the source code.

The other changes are less obvious since they may introduce extra costs; some of these code paths are fairly hot. In addition, I'm giving part of the interpreter an overhaul so perhaps we could drop them for the time being. Do you agree?

Re: [PATCH] Findirect_function

[Manuel Giraud]

Mattias Engdegård <mattiase@acm.org> writes:

> 14 jan. 2022 kl. 11.40 skrev Manuel Giraud <manuel@ledu-giraud.fr>:
>
>> Maybe it is bike shedding territory (and maybe I'm missing something)
>
> Not at all, patches are always welcome!

Thanks.

[...]

> Good! See if you can find more instances of this unfortunate
> pattern. Even if it makes no difference for the generated code (very
> likely), it obfuscates the source code.

Ok, I'll look for this kind of pattern.

> The other changes are less obvious since they may introduce extra
> costs; some of these code paths are fairly hot. In addition, I'm
> giving part of the interpreter an overhaul so perhaps we could drop
> them for the time being. Do you agree?

Yes it is one more C function call. Sure you could drop it!
-- 
Manuel Giraud

Re: [PATCH] Findirect_function

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Fri, 14 Jan 2022 11:40:02 +0100
> 
> Maybe it is bike shedding territory (and maybe I'm missing something)
> but here are two patches for Findirect_function and its usage. "make
> check" is the same and so far it works for me™.

Do we care about the additional cost a function call?  funcall
and apply are quite hot spots in Emacs, AFAIK.

Re: [PATCH] Findirect_function

[Andreas Schwab]

On Jan 14 2022, Manuel Giraud wrote:

> diff --git a/src/data.c b/src/data.c
> index f287c38fcd..65babc49e9 100644
> --- a/src/data.c
> +++ b/src/data.c
> @@ -2380,10 +2380,8 @@ DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 2, 0,
>    if (SYMBOLP (result) && !NILP (result)
>        && (result = XSYMBOL (result)->u.s.function, SYMBOLP (result)))
>      result = indirect_function (result);
> -  if (!NILP (result))
> -    return result;
>  
> -  return Qnil;
> +  return result;
>  }

Looks like a leftover from commit 60f8214e97.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Findirect_function

[Manuel Giraud]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Jan 14 2022, Manuel Giraud wrote:
>
>> diff --git a/src/data.c b/src/data.c
>> index f287c38fcd..65babc49e9 100644
>> --- a/src/data.c
>> +++ b/src/data.c
>> @@ -2380,10 +2380,8 @@ DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 2, 0,
>>    if (SYMBOLP (result) && !NILP (result)
>>        && (result = XSYMBOL (result)->u.s.function, SYMBOLP (result)))
>>      result = indirect_function (result);
>> -  if (!NILP (result))
>> -    return result;
>>  
>> -  return Qnil;
>> +  return result;
>>  }
>
> Looks like a leftover from commit 60f8214e97.

I see it is when Findirect_function could not signal an error
anymore. While here, could we remove the 'noerror' argument?
-- 
Manuel Giraud

Re: [PATCH] Findirect_function

[Stefan Monnier]

>    /* Optimize for no indirection.  */
> -  if (SYMBOLP (fun) && !NILP (fun)
> -      && (fun = XSYMBOL (fun)->u.s.function, SYMBOLP (fun)))
> -    {
> -      fun = indirect_function (fun);
> -      if (NILP (fun))
> -	/* Let funcall get the error.  */
> -	fun = args[0];
> -    }
> +  fun = Findirect_function(fun, Qt);
> +  if (NILP (fun))
> +    /* Let funcall get the error.  */
> +    fun = args[0];

This should remove the corresponding comment.
And I think I'd only be willing to make such a change if it comes with
some benchmarking showing that this optimization does not actually make
a difference.


        Stefan

Re: [PATCH] Findirect_function

[Manuel Giraud]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>    /* Optimize for no indirection.  */
>> -  if (SYMBOLP (fun) && !NILP (fun)
>> -      && (fun = XSYMBOL (fun)->u.s.function, SYMBOLP (fun)))
>> -    {
>> -      fun = indirect_function (fun);
>> -      if (NILP (fun))
>> -	/* Let funcall get the error.  */
>> -	fun = args[0];
>> -    }
>> +  fun = Findirect_function(fun, Qt);
>> +  if (NILP (fun))
>> +    /* Let funcall get the error.  */
>> +    fun = args[0];
>
> This should remove the corresponding comment.

You mean the /* Optimize… */ comment, right?

> And I think I'd only be willing to make such a change if it comes with
> some benchmarking showing that this optimization does not actually make
> a difference.

Do you happen to have some hints on how to do benchmarking in emacs? 😅
-- 
Manuel Giraud

Re: [PATCH] Findirect_function

[Stefan Monnier]

>> This should remove the corresponding comment.
> You mean the /* Optimize… */ comment, right?

Yes.

>> And I think I'd only be willing to make such a change if it comes with
>> some benchmarking showing that this optimization does not actually make
>> a difference.
> Do you happen to have some hints on how to do benchmarking in emacs? 😅

There's the `elisp-benchmarks` package on GNU ELPA, and another standard
benchmark is to measure the time for Emacs to compile all its
`.el` files.

The `elisp-benchmarks` would welcome more benchmarks, so I'd encourage
you to write some of your own and contribute them there ;-)


        Stefan

Re: [PATCH] Findirect_function

[Manuel Giraud]

[-- Attachment #1: Type: text/plain, Size: 4086 bytes --]

Hi,

So here are the results of `elisp-benchmarks-run' on one "emacs -Q"
master and one patched with the attached patch:

master:
* Results

  | test               | non-gc avg (s) | gc avg (s) | gcs avg | tot avg (s) | tot avg err (s) |
  |--------------------+----------------+------------+---------+-------------+-----------------|
  | inclist            |          28.16 |       0.00 |       0 |       28.16 |            0.80 |
  | fibn-rec           |          12.89 |       0.00 |       0 |       12.89 |            1.05 |
  | listlen-tc         |          12.68 |       0.00 |       0 |       12.68 |            0.47 |
  | pack-unpack        |           0.28 |       0.21 |       5 |        0.49 |            0.03 |
  | pidigits           |          32.88 |      45.67 |     211 |       78.55 |            0.67 |
  | map-closure        |          16.46 |       0.00 |       0 |       16.46 |            0.74 |
  | nbody              |           5.05 |      17.50 |     416 |       22.56 |            0.94 |
  | dhrystone          |          14.55 |       0.00 |       0 |       14.55 |            2.48 |
  | pack-unpack-old    |           0.75 |       0.47 |      11 |        1.22 |            0.16 |
  | bubble             |           8.54 |       9.90 |     234 |       18.44 |            1.61 |
  | inclist-type-hints |          28.42 |       0.00 |       0 |       28.42 |            0.55 |
  | bubble-no-cons     |          20.45 |       0.05 |       1 |       20.50 |            1.34 |
  | fibn               |          20.79 |       0.00 |       0 |       20.79 |            1.38 |
  | fibn-tc            |          11.13 |       0.00 |       0 |       11.13 |            0.41 |
  | pcase              |          21.63 |       0.00 |       0 |       21.63 |            0.25 |
  | flet               |          20.92 |       0.00 |       0 |       20.92 |            0.11 |
  |--------------------+----------------+------------+---------+-------------+-----------------|
  | total              |         255.58 |      73.80 |     878 |      329.38 |            4.10 |

patched:
* Results

  | test               | non-gc avg (s) | gc avg (s) | gcs avg | tot avg (s) | tot avg err (s) |
  |--------------------+----------------+------------+---------+-------------+-----------------|
  | inclist            |          27.89 |       0.00 |       0 |       27.89 |            0.20 |
  | fibn-rec           |          11.29 |       0.00 |       0 |       11.29 |            0.20 |
  | listlen-tc         |          12.71 |       0.00 |       0 |       12.71 |            0.98 |
  | pack-unpack        |           0.26 |       0.25 |      10 |        0.51 |            0.04 |
  | pidigits           |          32.27 |      45.05 |     382 |       77.32 |            1.37 |
  | map-closure        |          16.71 |       0.00 |       0 |       16.71 |            1.15 |
  | nbody              |           4.82 |      19.54 |     752 |       24.37 |            1.46 |
  | dhrystone          |          13.38 |       0.00 |       0 |       13.38 |            0.60 |
  | pack-unpack-old    |           0.72 |       0.51 |      20 |        1.23 |            0.08 |
  | bubble             |           8.12 |      10.19 |     400 |       18.30 |            0.92 |
  | inclist-type-hints |          28.40 |       0.00 |       0 |       28.40 |            0.69 |
  | bubble-no-cons     |          20.75 |       0.08 |       3 |       20.83 |            2.06 |
  | fibn               |          20.04 |       0.00 |       0 |       20.04 |            1.90 |
  | fibn-tc            |          11.02 |       0.00 |       0 |       11.02 |            1.03 |
  | pcase              |          22.31 |       0.00 |       0 |       22.31 |            2.14 |
  | flet               |          21.81 |       0.00 |       0 |       21.81 |            2.18 |
  |--------------------+----------------+------------+---------+-------------+-----------------|
  | total              |         252.51 |      75.61 |    1568 |      328.13 |            5.13 |

It seems that we are in the same ballpark.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-unused-argument-to-Findirect_function.patch --]
[-- Type: text/x-patch, Size: 5782 bytes --]

From c0a250642dc998c35e8a3d471198fb13ff5bb54f Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Mon, 17 Jan 2022 11:16:09 +0100
Subject: [PATCH] Remove unused argument to Findirect_function.

Use Findirect_function more where possible.
---
 src/callint.c |  2 +-
 src/data.c    |  8 +++-----
 src/doc.c     |  2 +-
 src/eval.c    | 36 ++++++++++--------------------------
 src/keymap.c  |  2 +-
 5 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/src/callint.c b/src/callint.c
index ce77c893f4..f046d53e95 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -327,7 +327,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
   /* If SPECS is not a string, invent one.  */
   if (! STRINGP (specs))
     {
-      Lisp_Object funval = Findirect_function (function, Qt);
+      Lisp_Object funval = Findirect_function (function);
       uintmax_t events = num_input_events;
       Lisp_Object input = specs;
       /* Compute the arg values using the user's expression.  */
diff --git a/src/data.c b/src/data.c
index f287c38fcd..173f0fb54f 100644
--- a/src/data.c
+++ b/src/data.c
@@ -2365,13 +2365,13 @@ indirect_function (register Lisp_Object object)
   return hare;
 }
 
-DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 2, 0,
+DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 1, 0,
        doc: /* Return the function at the end of OBJECT's function chain.
 If OBJECT is not a symbol, just return it.  Otherwise, follow all
 function indirections to find the final function binding and return it.
 Signal a cyclic-function-indirection error if there is a loop in the
 function chain of symbols.  */)
-  (register Lisp_Object object, Lisp_Object noerror)
+  (register Lisp_Object object)
 {
   Lisp_Object result;
 
@@ -2380,10 +2380,8 @@ DEFUN ("indirect-function", Findirect_function, Sindirect_function, 1, 2, 0,
   if (SYMBOLP (result) && !NILP (result)
       && (result = XSYMBOL (result)->u.s.function, SYMBOLP (result)))
     result = indirect_function (result);
-  if (!NILP (result))
-    return result;
 
-  return Qnil;
+  return result;
 }
 \f
 /* Extract and set vector and string elements.  */
diff --git a/src/doc.c b/src/doc.c
index 0b12eb154d..3c8bb19eb6 100644
--- a/src/doc.c
+++ b/src/doc.c
@@ -325,7 +325,7 @@ DEFUN ("documentation", Fdocumentation, Sdocumentation, 1, 2, 0,
 					raw);
     }
 
-  Lisp_Object fun = Findirect_function (function, Qnil);
+  Lisp_Object fun = Findirect_function (function);
   if (NILP (fun))
     xsignal1 (Qvoid_function, function);
   if (CONSP (fun) && EQ (XCAR (fun), Qmacro))
diff --git a/src/eval.c b/src/eval.c
index 5514583b6a..a8c69f43ee 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2344,7 +2344,7 @@ DEFUN ("autoload-do-load", Fautoload_do_load, Sautoload_do_load, 1, 3, 0,
     return Qnil;
   else
     {
-      Lisp_Object fun = Findirect_function (funname, Qnil);
+      Lisp_Object fun = Findirect_function(funname);
 
       if (!NILP (Fequal (fun, fundef)))
 	error ("Autoloading file %s failed to define function %s",
@@ -2667,15 +2667,10 @@ DEFUN ("apply", Fapply, Sapply, 1, MANY, 0,
 
   numargs += nargs - 2;
 
-  /* Optimize for no indirection.  */
-  if (SYMBOLP (fun) && !NILP (fun)
-      && (fun = XSYMBOL (fun)->u.s.function, SYMBOLP (fun)))
-    {
-      fun = indirect_function (fun);
-      if (NILP (fun))
-	/* Let funcall get the error.  */
-	fun = args[0];
-    }
+  fun = Findirect_function(fun);
+  if (NILP (fun))
+    /* Let funcall get the error.  */
+    fun = args[0];
 
   if (SUBRP (fun) && XSUBR (fun)->max_args > numargs
       /* Don't hide an error by adding missing arguments.  */
@@ -3008,7 +3003,7 @@ FUNCTIONP (Lisp_Object object)
 {
   if (SYMBOLP (object) && !NILP (Ffboundp (object)))
     {
-      object = Findirect_function (object, Qt);
+      object = Findirect_function(object);
 
       if (CONSP (object) && EQ (XCAR (object), Qautoload))
 	{
@@ -3068,11 +3063,7 @@ DEFUN ("funcall", Ffuncall, Sfuncall, 1, MANY, 0,
 
  retry:
 
-  /* Optimize for no indirection.  */
-  fun = original_fun;
-  if (SYMBOLP (fun) && !NILP (fun)
-      && (fun = XSYMBOL (fun)->u.s.function, SYMBOLP (fun)))
-    fun = indirect_function (fun);
+  fun = Findirect_function(original_fun);
 
   if (SUBRP (fun) && !SUBR_NATIVE_COMPILED_DYNP (fun))
     val = funcall_subr (XSUBR (fun), numargs, args + 1);
@@ -3387,14 +3378,7 @@ DEFUN ("func-arity", Ffunc_arity, Sfunc_arity, 1, 1, 0,
 
  retry:
 
-  /* Optimize for no indirection.  */
-  function = original;
-  if (SYMBOLP (function) && !NILP (function))
-    {
-      function = XSYMBOL (function)->u.s.function;
-      if (SYMBOLP (function))
-	function = indirect_function (function);
-    }
+  function = Findirect_function(original);
 
   if (CONSP (function) && EQ (XCAR (function), Qmacro))
     function = XCDR (function);
@@ -3936,9 +3920,9 @@ get_backtrace_starting_at (Lisp_Object base)
 
   if (!NILP (base))
     { /* Skip up to `base'.  */
-      base = Findirect_function (base, Qt);
+      base = Findirect_function(base);
       while (backtrace_p (pdl)
-             && !EQ (base, Findirect_function (backtrace_function (pdl), Qt)))
+             && !EQ (base, Findirect_function(backtrace_function (pdl))))
         pdl = backtrace_next (pdl);
     }
 
diff --git a/src/keymap.c b/src/keymap.c
index ed69b1c427..7be0e3c155 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1614,7 +1614,7 @@ current_minor_maps (Lisp_Object **modeptr, Lisp_Object **mapptr)
 	      }
 
 	    /* Get the keymap definition--or nil if it is not defined.  */
-	    temp = Findirect_function (XCDR (assoc), Qt);
+	    temp = Findirect_function(XCDR (assoc));
 	    if (!NILP (temp))
 	      {
 		cmm_modes[i] = var;
-- 
2.34.1


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

Re: [PATCH] Findirect_function

[Stefan Monnier]

> It seems that we are in the same ballpark.

Indeed, it seems it's even faster!


        Stefan

Re: [PATCH] Findirect_function

[Stefan Monnier]

> From c0a250642dc998c35e8a3d471198fb13ff5bb54f Mon Sep 17 00:00:00 2001
> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Mon, 17 Jan 2022 11:16:09 +0100
> Subject: [PATCH] Remove unused argument to Findirect_function.
>
> Use Findirect_function more where possible.
> ---
>  src/callint.c |  2 +-
>  src/data.c    |  8 +++-----
>  src/doc.c     |  2 +-
>  src/eval.c    | 36 ++++++++++--------------------------
>  src/keymap.c  |  2 +-
>  5 files changed, 16 insertions(+), 34 deletions(-)

Oh, the patch probably needs an entry etc/NEWS to say that the second
arg (which was declared obsolete in Emacs-25) was now removed.


        Stefan

Re: [PATCH] Findirect_function

[Po Lu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Oh, the patch probably needs an entry etc/NEWS to say that the second
> arg (which was declared obsolete in Emacs-25) was now removed.

Maybe not yet, since it was only obsoleted in Emacs 25.
Thanks.