GNU bug report logs - #36139
[PATCH] Make better use of the switch op in cond forms

Previous Next

Package: emacs;

Reported by: Mattias Engdegård <mattiase <at> acm.org>

Date: Sat, 8 Jun 2019 15:16:02 UTC

Severity: wishlist

Tags: patch

Done: Mattias Engdegård <mattiase <at> acm.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36139 in the body.
You can then email your comments to 36139 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Sat, 08 Jun 2019 15:16:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Mattias Engdegård <mattiase <at> acm.org>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sat, 08 Jun 2019 15:16:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: bug-gnu-emacs <at> gnu.org
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: [PATCH] Make better use of the switch op in cond forms
Date: Sat, 8 Jun 2019 16:40:07 +0200
[Message part 1 (text/plain, inline)]
Currently, `cond' forms will only use a switch op under very specific circumstances, which limits their potential utility: every condition must be a scalar comparison (eq, eql or equal) between a variable and a constant; the same variable and the same test function must be used everywhere. These restrictions also limit the generation of switch ops from `pcase' and `cl-case' constructs.

In actual `cond' forms, scalar comparisons are mixed with multi-value ones (memq, memql, member); different comparisons are mixed, since the type of values can vary at runtime. There are also non-switch comparisons thrown into the same `cond' for convenience, typically at the beginning or end. Occasionally, there are even multiple switch-like sequences against different variables in the same cond.

Attached is a set of patches which gradually eliminate many of these restrictions. Each patch makes a fixed and hopefully easily-reviewed improvement. Short overview:

* 0001-Compile-list-member-functions-in-cond-to-switch.patch

Allow memq, memql and member to be used in switch generation, since these are idiomatically used for multiple values with the same body. They are also used by `pcase' and `cl-case'. For example,

 (cond ((eq x 'a) 11)
       ((memq x '(b c d)) 22))

will generate a single switch as if the code had been

 (cond ((eq x 'a) 11)
       ((eq x 'b) 22)
       ((eq x 'c) 22)
       ((eq x 'd) 22))

- that is, the byte code for the body (22) will be generated thrice.

* 0002-Share-identical-switch-clause-bodies.patch

An improvement of the above to share the body byte code between all values of the same `cond' clause. This means that the switch jump table is no longer an injective mapping.

* 0003-Tighter-pcase-or-pattern-member-function-selection.patch

Make `pcase' use the most specific comparison function (memq instead of memql, etc) in each case, depending on the values.
This patch is technically independent of the other ones, but improves code generation for `pcase'.

* 0004-Compile-cond-with-heterogeneous-tests-into-switch.patch

Allow switch generation with a mixture of eq, eql, equal, memq, memql and member.

* 0005-Compile-any-subsequence-of-cond-clauses-to-switch.patch

Generalise the code to produce switch ops for any switch-like part of a `cond' form. These switches can use different variables, and there can be any number of non-switch clauses before, after and between the switch clauses.

Performance: The patch set is loosely monotonous with the assumptions already present, in the sense that if the current code generator does not produce slower code in any case, nor is it expected to do so after the patches have been applied. In practice, micro-benchmarks naturally show the expected gains, but I haven't found much real-world code that is easy enough to benchmark. Some unpublished tree-traversal code improved about 7 %.

[0001-Compile-list-member-functions-in-cond-to-switch.patch (application/octet-stream, attachment)]
[0002-Share-identical-switch-clause-bodies.patch (application/octet-stream, attachment)]
[0003-Tighter-pcase-or-pattern-member-function-selection.patch (application/octet-stream, attachment)]
[0004-Compile-cond-with-heterogeneous-tests-into-switch.patch (application/octet-stream, attachment)]
[0005-Compile-any-subsequence-of-cond-clauses-to-switch.patch (application/octet-stream, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Sat, 08 Jun 2019 15:40:02 GMT) Full text and rfc822 format available.

Message #8 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Mattias Engdegård <mattiase <at> acm.org>,
 36139 <at> debbugs.gnu.org
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: RE: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Sat, 8 Jun 2019 08:38:54 -0700 (PDT)
How does this kind of thing work with an enclosing `cl-flet' or similar that redefines `memq' or whatever?  Or in a file that redefines `memq' or whatever using `defun' or similar?

(But I guess the same problem exists in the current code, wrt `eq' etc.?)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Sun, 09 Jun 2019 08:39:01 GMT) Full text and rfc822 format available.

Message #11 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 36139 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Sun, 9 Jun 2019 10:38:25 +0200
8 juni 2019 kl. 17.38 skrev Drew Adams <drew.adams <at> oracle.com>:
> 
> How does this kind of thing work with an enclosing `cl-flet' or similar that redefines `memq' or whatever?  Or in a file that redefines `memq' or whatever using `defun' or similar?

`cl-flet' works by macro-expansion, not by lexical rebinding of the function value, so it should be reasonably safe.
monotonic



Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Mon, 10 Jun 2019 15:40:01 GMT) Full text and rfc822 format available.

Message #14 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> gmail.com
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Mattias Engdegård <mattiase <at> acm.org>,
 36139 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Mon, 10 Jun 2019 11:38:57 -0400
Drew Adams <drew.adams <at> oracle.com> writes:

> Or in a file that redefines `memq' or whatever using `defun' or similar?
>
> (But I guess the same problem exists in the current code, wrt `eq' etc.?)

Redefining eq, equal, memq, or member with defun or advice is already
unreliable because they are translated to byte codes.  eql and memql are
not, so this patchset (specifically, the last 2 patches, I think) would
make the situation a bit worse for those functions, in that it would
prevent defun/advice override for eql and memql from applying in cond
forms.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 11 Jun 2019 11:13:01 GMT) Full text and rfc822 format available.

Message #17 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 36139 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 11 Jun 2019 13:12:26 +0200
10 juni 2019 kl. 17.38 skrev npostavs <at> gmail.com:
> 
> Redefining eq, equal, memq, or member with defun or advice is already
> unreliable because they are translated to byte codes.  eql and memql are
> not, so this patchset (specifically, the last 2 patches, I think) would
> make the situation a bit worse for those functions, in that it would
> prevent defun/advice override for eql and memql from applying in cond
> forms.

`eql' is already recognised for switch generation in cond forms today. More generally, is redefinition of such fundamental built-ins really a serious concern? The compiler assumes standard semantics for plenty of functions, byte-codes or not. This is also mentioned in the manual.






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 11 Jun 2019 11:26:02 GMT) Full text and rfc822 format available.

Message #20 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 36139 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 11 Jun 2019 07:25:05 -0400
Mattias Engdegård <mattiase <at> acm.org> writes:

> 10 juni 2019 kl. 17.38 skrev npostavs <at> gmail.com:
>> 
>> Redefining eq, equal, memq, or member with defun or advice is already
>> unreliable because they are translated to byte codes.  eql and memql are
>> not, so this patchset (specifically, the last 2 patches, I think) would
>> make the situation a bit worse for those functions, in that it would
>> prevent defun/advice override for eql and memql from applying in cond
>> forms.
>
> `eql' is already recognised for switch generation in cond forms
> today.

Oh, I wasn't aware of that.  Somehow I had the impression it was only
for `eq'.

> More generally, is redefinition of such fundamental built-ins
> really a serious concern?

Not in my opinion, I just wanted to mention it for completeness.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 18 Jun 2019 12:47:01 GMT) Full text and rfc822 format available.

Message #23 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: 36139 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Noam Postavsky <npostavs <at> gmail.com>, Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 18 Jun 2019 14:46:43 +0200
Anything I can do to make the changes easier to review? All tests pass, bootstrap works, and so on; I'm using them daily.

The patches could be reviewed and committed piecemeal. Patch 3 (pcase) may be a good start since it's a small independent improvement.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 18 Jun 2019 18:50:02 GMT) Full text and rfc822 format available.

Message #26 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 18 Jun 2019 14:48:44 -0400
> * lisp/emacs-lisp/bytecomp.el (byte-compile-cond-jump-table-info):
> Expand `memq', `memql' and `member' to their corresponding
> equality tests.
> ---
>  lisp/emacs-lisp/bytecomp.el | 46 +++++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> index 38cce14fd6..8f089119de 100644
> --- a/lisp/emacs-lisp/bytecomp.el
> +++ b/lisp/emacs-lisp/bytecomp.el
> @@ -4117,23 +4117,39 @@ byte-compile-cond-jump-table-info
>                              (byte-compile-cond-vars (cadr condition) (cl-caddr condition))))
>                      (obj1 (car-safe vars))
>                      (obj2 (cdr-safe vars))
> -                    (body (cdr-safe clause)))
> +                    (body (cdr-safe clause))
> +                    equality)
>                 (unless prev-var
>                   (setq prev-var obj1))
> -               (unless prev-test
> -                 (setq prev-test test))
> -               (if (and obj1 (memq test '(eq eql equal))
> -                        (eq test prev-test)
> -                        (eq obj1 prev-var))
> -                   ;; discard duplicate clauses
> -                   (unless (assoc obj2 cases test)
> -                     (push (list obj2 body) cases))
> -                 (if (and (macroexp-const-p condition) condition)
> -		     (progn (push (list byte-compile--default-val
> -					(or body `(,condition)))
> -				  cases)
> -                            (throw 'break t))
> -                   (setq ok nil)
> +               (cond
> +                ((and obj1 (memq test '(eq eql equal))
> +                      (eq obj1 prev-var)
> +                      (or (not prev-test) (eq test prev-test)))
> +                 (setq prev-test test)
> +                 ;; discard duplicate clauses
> +                 (unless (assoc obj2 cases test)
> +                   (push (list obj2 body) cases)))
> +
> +                ((and obj1 (memq test '(memq memql member))
> +                      (eq obj1 prev-var)
> +                      (listp obj2)
> +                      (setq equality (cdr (assq test '((memq   . eq)
> +                                                       (memql  . eql)
> +                                                       (member . equal)))))
> +                      (or (not prev-test) (eq equality prev-test)))
> +                 (setq prev-test equality)
> +                 ;; Expand to individual equality tests.
> +                 (dolist (elem obj2)
> +                   (unless (assoc elem cases equality)
> +                     (push (list elem (or body `(',(funcall test elem obj2))))
> +                           cases))))
> +
> +                ((and (macroexp-const-p condition) condition)
> +		 (push (list byte-compile--default-val
> +                             (or body `(,condition)))
> +		       cases)
> +                 (throw 'break t))
> +                (t (setq ok nil)
>                     (throw 'break nil))))))
>           (list (cons prev-test prev-var) (nreverse cases)))))

This patch on its own is problematic because of the code-increase it
can introduce.  So I'd suggest merging it with the other one.


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 18 Jun 2019 18:57:01 GMT) Full text and rfc822 format available.

Message #29 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 18 Jun 2019 14:56:44 -0400
> Allow switch generation with a mixture of eq, eql, equal, memq, memql and member.

AFAIK all of those give the same result (in practice) as using
equal/member: I'm having a hard time imagining an eq/eql/memq/memql test
against a constant which behaves differently from equal/member except
for those that can simply always return nil (e.g. (eq x "toto") can
always return nil since there's no way the caller of this code can make
sure x is really the same string object as the "toto" generated by the
compiler).

So I think we should "standardize" on equal/member and mostly disregard
the eq/eql/equal difference (except maybe for emitting a warning when
comparing against something where `equal` doesn't give the same result).


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 18 Jun 2019 19:04:02 GMT) Full text and rfc822 format available.

Message #32 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 18 Jun 2019 15:03:01 -0400
> * lisp/emacs-lisp/pcase.el (pcase--u1):
> Use the most specific of `memq', `memql' and `member' in or-patterns
> with constant cases.  This improves performance and may help the byte-code
> compiler generate a switch.
> * test/lisp/emacs-lisp/pcase-tests.el (pcase-tests-member):
> Add mixed-type or-pattern test cases.
> ---
>  lisp/emacs-lisp/pcase.el            | 15 ++++++++-------
>  test/lisp/emacs-lisp/pcase-tests.el |  6 ++++--
>  2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
> index a644453a94..ae2cf8eb02 100644
> --- a/lisp/emacs-lisp/pcase.el
> +++ b/lisp/emacs-lisp/pcase.el
> @@ -785,25 +785,26 @@ pcase--u1
>     ((eq 'or (caar matches))
>      (let* ((alts (cdar matches))
>             (var (if (eq (caar alts) 'match) (cadr (car alts))))
> -           (simples '()) (others '()) (memql-ok t))
> +           (simples '()) (others '()) (mem-fun 'memq))
>        (when var
>          (dolist (alt alts)
>            (if (and (eq (car alt) 'match) (eq var (cadr alt))
>                     (let ((upat (cddr alt)))
>                       (eq (car-safe upat) 'quote)))
>                (let ((val (cadr (cddr alt))))
> -                (unless (or (integerp val) (symbolp val))
> -                  (setq memql-ok nil))
> -                (push (cadr (cddr alt)) simples))
> +                (cond ((integerp val)
> +                       (when (eq mem-fun 'memq)
> +                         (setq mem-fun 'memql)))
> +                      ((not (symbolp val))
> +                       (setq mem-fun 'member)))
> +                (push val simples))
>              (push alt others))))
>        (cond
>         ((null alts) (error "Please avoid it") (pcase--u rest))
>         ;; Yes, we can use `memql' (or `member')!
>         ((> (length simples) 1)
>          (pcase--u1 (cons `(match ,var
> -                                 . (pred (pcase--flip
> -                                          ,(if memql-ok #'memql #'member)
> -                                          ',simples)))
> +                                 . (pred (pcase--flip ,mem-fun ',simples)))
>                           (cdr matches))
>                     code vars
>                     (if (null others) rest

LGTM.  The other direction is to just always use `member`
and speed up the implementation of `member` by testing the type of
the first arg and dispatch to memq/memql when possible.


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 18 Jun 2019 19:07:01 GMT) Full text and rfc822 format available.

Message #35 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 18 Jun 2019 15:06:12 -0400
> Allow any mixture of `eq', `eql' and `equal', `memq', `memql' and
> `member' in a switch-like `cond' to be compiled into a single switch.

LGTM,


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Tue, 18 Jun 2019 19:20:02 GMT) Full text and rfc822 format available.

Message #38 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Tue, 18 Jun 2019 15:19:25 -0400
> A single `cond' form can how be compiled to any number of switch ops,
> interspersed with non-switch conditions in arbitrary ways.

It can also be compiled to a bunch of switch ops only, right?
(e.g. if it starts with a switch on `x` and then is followed by
a switch on `y`)

> +    (and (> (length cases) 1)

I think this `1` deserves a comment (IIRC it's the number of cases
above which using a switch is expected to be faster than a sequence of
tests).

> +        ;; Since `byte-compile-body' might increase `byte-compile-depth'
> +        ;; by 1, not preserving its value will cause it to potentially
> +        ;; increase by one for every clause body compiled, causing
> +        ;; depth/tag conflicts or violating asserts down the road.
> +        ;; To make sure `byte-compile-body' itself doesn't violate this,
> +        ;; we use `cl-assert'.
> +        (byte-compile-body body byte-compile--for-effect)
> +        (cl-assert (or (= byte-compile-depth init-depth)
> +                       (= byte-compile-depth (1+ init-depth))))

IIRC the depth is altered depending on byte-compile--for-effect (if
byte-compile--for-effect is non-nil when entering the function but nil
afterwards, depth should be identical, and it should be increased by
1 otherwise), so we should be able to tighten this assertion to replace
the `or` with an `if`.

Other than that, the patch looks fine to me.


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Wed, 19 Jun 2019 09:26:02 GMT) Full text and rfc822 format available.

Message #41 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Wed, 19 Jun 2019 11:25:45 +0200
18 juni 2019 kl. 20.48 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
> 
> This patch on its own is problematic because of the code-increase it
> can introduce.  So I'd suggest merging it with the other one.

Thanks, I merged and pushed those two patches as one.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Wed, 19 Jun 2019 09:31:02 GMT) Full text and rfc822 format available.

Message #44 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Wed, 19 Jun 2019 11:30:00 +0200
18 juni 2019 kl. 21.03 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
> 
> LGTM.  The other direction is to just always use `member`
> and speed up the implementation of `member` by testing the type of
> the first arg and dispatch to memq/memql when possible.

Thank you. I ended up pushing this patch on the grounds that it took care of unfinished business in pcase (it did try to select which function to use, just didn't go all the way). Speeding up `member' is, of course, useful on its own.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Wed, 19 Jun 2019 09:31:03 GMT) Full text and rfc822 format available.

Message #47 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Wed, 19 Jun 2019 11:30:27 +0200
18 juni 2019 kl. 21.06 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
> 
>> Allow any mixture of `eq', `eql' and `equal', `memq', `memql' and
>> `member' in a switch-like `cond' to be compiled into a single switch.
> 
> LGTM,

Thank you, pushed.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Wed, 19 Jun 2019 10:16:01 GMT) Full text and rfc822 format available.

Message #50 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Wed, 19 Jun 2019 12:14:58 +0200
18 juni 2019 kl. 21.19 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
> 
>> A single `cond' form can how be compiled to any number of switch ops,
>> interspersed with non-switch conditions in arbitrary ways.
> 
> It can also be compiled to a bunch of switch ops only, right?
> (e.g. if it starts with a switch on `x` and then is followed by
> a switch on `y`)

Correct, and good catch. I rephrased the commit message.

>> +    (and (> (length cases) 1)
> 
> I think this `1` deserves a comment (IIRC it's the number of cases
> above which using a switch is expected to be faster than a sequence of
> tests).

Agreed, but the condition comes from the existing code (bytecomp.el:4180) where the number isn't motivated further either. I just assumed it was chosen with at least some care.

The ability to include multi-value cases in the switch makes the condition a conservative choice: if it is a good decision for single-value cases, it is definitely valid for multiple values per case.

I added a comment stating the intent, but it's not a lot more than restating the Lisp in English.

>> +        ;; Since `byte-compile-body' might increase `byte-compile-depth'
>> +        ;; by 1, not preserving its value will cause it to potentially
>> +        ;; increase by one for every clause body compiled, causing
>> +        ;; depth/tag conflicts or violating asserts down the road.
>> +        ;; To make sure `byte-compile-body' itself doesn't violate this,
>> +        ;; we use `cl-assert'.
>> +        (byte-compile-body body byte-compile--for-effect)
>> +        (cl-assert (or (= byte-compile-depth init-depth)
>> +                       (= byte-compile-depth (1+ init-depth))))
> 
> IIRC the depth is altered depending on byte-compile--for-effect (if
> byte-compile--for-effect is non-nil when entering the function but nil
> afterwards, depth should be identical, and it should be increased by
> 1 otherwise), so we should be able to tighten this assertion to replace
> the `or` with an `if`.

I'll do that in a separate change then, because it seems to be orthogonal to my changes.
Brief experiments seem to indicate that the

        (byte-compile-body body byte-compile--for-effect)

call does not seem to alter byte-compile--for-effect, but it does increase depth by 1 iff byte-compile--for-effect is non-nil.

> Other than that, the patch looks fine to me.

Thanks for the review! Pushed to master.






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#36139; Package emacs. (Wed, 19 Jun 2019 14:04:01 GMT) Full text and rfc822 format available.

Message #53 received at 36139 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139 <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Wed, 19 Jun 2019 16:03:43 +0200
[Message part 1 (text/plain, inline)]
18 juni 2019 kl. 21.03 skrev Stefan Monnier <monnier <at> iro.umontreal.ca>:
> 
> LGTM.  The other direction is to just always use `member`
> and speed up the implementation of `member` by testing the type of
> the first arg and dispatch to memq/memql when possible.

Here is a patch that does some cheap static optimisations: equal/eql and member/memql become eq and memq if constant symbols are involved. (Works for me, bootstraps fine, etc.)

I considered doing the same for equal->eql and member->memql. It turns out that equal is faster than eql for numbers, since eql doesn't have its own byte op. For the same reason, member is faster than memql for lists of up to 5 elements (on this machine). While we could reduce equal to eql for constant lists > 5 elements of only symbols and numbers, it's perhaps more trouble than it's worth.

[0001-Strength-reduce-equal-eql-member-and-memql.patch (application/octet-stream, attachment)]

Reply sent to Mattias Engdegård <mattiase <at> acm.org>:
You have taken responsibility. (Fri, 28 Jun 2019 20:52:01 GMT) Full text and rfc822 format available.

Notification sent to Mattias Engdegård <mattiase <at> acm.org>:
bug acknowledged by developer. (Fri, 28 Jun 2019 20:52:02 GMT) Full text and rfc822 format available.

Message #58 received at 36139-done <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 36139-done <at> debbugs.gnu.org
Subject: Re: bug#36139: [PATCH] Make better use of the switch op in cond forms
Date: Fri, 28 Jun 2019 22:51:02 +0200
19 juni 2019 kl. 16.03 skrev Mattias Engdegård <mattiase <at> acm.org>:
> 
> Here is a patch that does some cheap static optimisations: equal/eql and member/memql become eq and memq if constant symbols are involved. (Works for me, bootstraps fine, etc.)

After dithering a bit and testing alternatives, I pushed a tweaked version of that patch; there seemed to be little reason not to.





bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 27 Jul 2019 11:24:08 GMT) Full text and rfc822 format available.

This bug report was last modified 5 years and 322 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.