Re: Patch to remove a bit of duplicated code in eval.c

[Federico Tedin]

(Apologies if there's any strange formatting, can't find this email on
Gnus and had to reply with the web client)

On Fri, Sep 17, 2021 at 7:11 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > @@ -3081,11 +2978,52 @@ DEFUN ("funcall", Ffuncall, Sfuncall, 1, MANY, 0,
> >  }
> >
> >
> > +static Lisp_Object
> > +apply_subr (struct Lisp_Subr *subr, Lisp_Object args, ptrdiff_t count)
> > +{
>
> I think this definition deserves a comment explaining at least what is
> `count` (the other two are fairly self-explanatory, but not that one).

Good point.

> > +  Lisp_Object *arg_vector;
> > +  Lisp_Object tem;
> > +  USE_SAFE_ALLOCA;
> > +
> > +  ptrdiff_t numargs = list_length (args);
> > +
> > +  if (subr->max_args != UNEVALLED)
> > +    {
> > +      Lisp_Object args_left = args;
> > +      SAFE_ALLOCA_LISP (arg_vector, numargs);
> > +
> > +      for (ptrdiff_t i = 0; i < numargs; i++)
> > +     {
> > +       tem = Fcar (args_left);
> > +       args_left = Fcdr(args_left);
> > +       tem = eval_sub(tem);
>
> [ Be careful to remember to put a space before the open parens.  ]

Will do!

> >  Lisp_Object
> > -funcall_subr (struct Lisp_Subr *subr, ptrdiff_t numargs, Lisp_Object *args)
> > +funcall_subr (struct Lisp_Subr *subr, ptrdiff_t numargs, Lisp_Object 
> > *args, bool unevalled_ok)
> >  {
>
> I'm not very happy with this.
> Everywhere else in Emacs, the name "funcall" means we're calling
> a *function* and not a special form.  I think we'd be better off keeping
> `funcall_subr` unchanged and use "something else" when `+apply_subr`
> needs to handle a special form (aka `UNEVALLED`).
>
> That will also make it obvious that the patch does not slow down
> execution of bytecode at all (which does use `funcall_subr` but
> not `eval_sub`).

That is a great idea. I was not very satisfied with the change to
'funcall_subr' either. Fitting the change entirely in 'apply_subr'
should not be too difficult.

> > My concerns now are:
> > 1) Could I have broken anything without realizing it, since this is such
> > a central function in Lisp code evaluation? Everything seems to be
> > compiling fine (without warnings) and so far I haven't had any crashes.
>
> I haven't looked in enough details to be sure, but in principle it
> should be OK since it re-uses the well-tested `funcall_subr` code.
>
> > 2) I removed a comment that made reference to Bug#21245, but it seems
> > like it makes sense since the variable it refers to is no longer needed.
>
> That removal looks good, thanks.
>
> > 3) Have I maybe made Emacs slower by always using SAFE_ALLOCA_LISP for
> > the subroutine arguments (instead of only for 'max_args=MANY')?
>
> It might slightly slow down execution of interpreted code, but
> interpreted code should not be performance critical (after all, if
> speed matters, the answer is to byte-compile the code).  You can try and
> measure the slowdown in the following way:
>
>     rm src/*.pdmp lisp/**/*.elc
>     (cd src; make bootstrap-emacs.pdmp)
>     rm lisp/**/*.elc
>     (cd lisp; time make emacs-lisp/macroexp.elc)
>
> The important part is to time the `make emacs-lisp/macroexp.elc`.
> The three lines before it only serve to get to a state where we have
> a working Emacs executable with no bytecode at all (so the compilation
> of `macroexp.el` takes a long while because all the code is
> interpreted).

Thanks for the tip on measuring execution speed. I think I might be
able to undo this particular change though, since I didn't realize
that if max_args!=MANY,
then there's the possibility of just using a Lisp_Object args[8],
which is what the code that I removed did (I commented this in my
reply to Eli). So this change
was unnecessary.

Appreciate the feedback!