From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Federico Tedin Newsgroups: gmane.emacs.devel Subject: Re: Patch to remove a bit of duplicated code in eval.c Date: Fri, 17 Sep 2021 22:27:47 +0200 Message-ID: References: <87h7ekxkb1.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="30396"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Sep 17 22:28:41 2021 Return-path: Envelope-to: ged-emacs-devel@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 1mRKTQ-0007h1-V8 for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Sep 2021 22:28:40 +0200 Original-Received: from localhost ([::1]:54504 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mRKTP-0006E9-L1 for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Sep 2021 16:28:39 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:59734) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mRKSn-0005ZK-FF for emacs-devel@gnu.org; Fri, 17 Sep 2021 16:28:01 -0400 Original-Received: from mail-io1-xd30.google.com ([2607:f8b0:4864:20::d30]:41956) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mRKSl-00065s-QD for emacs-devel@gnu.org; Fri, 17 Sep 2021 16:28:01 -0400 Original-Received: by mail-io1-xd30.google.com with SMTP id j18so13635325ioj.8 for ; Fri, 17 Sep 2021 13:27:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TwtN5FonIWC9tLBPMBX9y2Xrka7WiNUb5zzxDGa/myM=; b=eHCV6oAdiIrAcgl61HEh1WHKgRjZVdKrLIjJbFg0anzqoFnbnCW/d4ONNkRHO0+pVI KbPKVu81Il/oKY/WWeUBUN+LnZ0ojCkwPbugX/HYgTMGTD19DdCP0Vc+ZVLxZX1GrtUB SPDTYOyOb1JOBIlMmnmDukMBeNYXkq/Dho9VSKo3xSfkP/xs2CFpAk0k7ZEDZtS6BQgt /c9Kb5vE6OF9UGe2ICZW36TR8qx/JkwETJnuwXJvgTncSq86tkGl11CNI3in2BRf9wjg ML/GvR8D1yI60s9b3S9v2WO/Rmcz9k09gFk7BMLl/fsjUSnf4jD4sGuFcftPcaUEXifb tndQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TwtN5FonIWC9tLBPMBX9y2Xrka7WiNUb5zzxDGa/myM=; b=udyJ9WH30rsYE1HtYsl6JMmaZkm4O/dX4eOesxXEjPxCE+A11Nh5M9xXSmHLssPbW/ o9Dc2bJczay3M3wYkrGHoYrfrsvrUyboATotve0parnIjHMtuTpozhu3eXQNs1EZAtVe EQa7myAomTe8WnWJaNPnq2ILW6cwrRA4yHauP2cPTa0iR829gCDr1sAgnAu/GbZcK8bu VIrwauv/GQf2noLnvv6q+njh1V0+DMxm5d13DMe5XQmU9LbPTrNvVfV5WFCfrJ1vbvCk BqT32u06PuzBtRuq4euUZOpLpUmuwKbh5qs32WwpJSrL7jzS2MsJuHsMsiBQzajnkLiM 0UjA== X-Gm-Message-State: AOAM531m5d1MPfAN06wMSTZBSO42aQMLv3Cptfcv1lwcicGoPUH1liyI N2EjSAxI8VusnCRMNG9MmQa69xolN1Pg78q+CyM= X-Google-Smtp-Source: ABdhPJzO4zl9ujUUfsPuDrKdPiHa1QOZRzKRNZhFoh6uoDfDtvwfq4R6LwlhYKhVzUXYn2nOTj/9noDeHfmyWqr8xRU= X-Received: by 2002:a02:9699:: with SMTP id w25mr434226jai.143.1631910477994; Fri, 17 Sep 2021 13:27:57 -0700 (PDT) In-Reply-To: Received-SPF: pass client-ip=2607:f8b0:4864:20::d30; envelope-from=federicotedin@gmail.com; helo=mail-io1-xd30.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:274935 Archived-At: (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 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!