bug#29866: 27.0.50; cl-loop: Calculate the array length just once

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Tino Calancha]

X-Debbugs-CC: monnier@iro.umontreal.ca

It looks sensible to calculate the array length just once,
instead of recalculate it on each iteration:
--8<-----------------------------cut here---------------start------------->8---
commit 1175db5cf09eb39e7e70703f38578d31b0dd9398
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Thu Dec 28 00:56:32 2017 +0900

    cl-loop: Calculate the array length just once
    
    * lisp/emacs-lisp/cl-macs.el (cl--parse-loop-clause):
    Dont calculate the array length on each iteration (Bug#29866).

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index f5311041cc..f671aea399 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1317,11 +1317,13 @@ cl--parse-loop-clause
 
 	       ((memq word '(across across-ref))
 		(let ((temp-vec (make-symbol "--cl-vec--"))
+                      (temp-len (make-symbol "--cl-len--"))
 		      (temp-idx (make-symbol "--cl-idx--")))
 		  (push (list temp-vec (pop cl--loop-args)) loop-for-bindings)
+		  (push (list temp-len `(length ,temp-vec)) loop-for-bindings)
 		  (push (list temp-idx -1) loop-for-bindings)
 		  (push `(< (setq ,temp-idx (1+ ,temp-idx))
-                            (length ,temp-vec))
+                            ,temp-len)
                         cl--loop-body)
 		  (if (eq word 'across-ref)
 		      (push (list var `(aref ,temp-vec ,temp-idx))

--8<-----------------------------cut here---------------end--------------->8---


In GNU Emacs 27.0.50 (build 32, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-12-28 built on calancha-pc
Repository revision: da94ea92bc3ba6c236b394c00e6bbb725131a149
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9.3 (stretch)

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

> X-Debbugs-CC: monnier@iro.umontreal.ca
>
> It looks sensible to calculate the array length just once,
> instead of recalculate it on each iteration:
I have checked after my nap for more optimizations like that.
The patch below pass the tests.
As long as the length of the seq be an invariant (is it?), then
it must be OK to store it in `temp-seq'.
--8<-----------------------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index f671aea399..d70cf7d664 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -1338,6 +1338,7 @@ cl--parse-loop-clause
 				    (error "Expected `of'"))))
 		      (seq (cl--pop2 cl--loop-args))
 		      (temp-seq (make-symbol "--cl-seq--"))
+	              (temp-len (make-symbol "--cl-len--"))
 		      (temp-idx
                        (if (eq (car cl--loop-args) 'using)
                            (if (and (= (length (cadr cl--loop-args)) 2)
@@ -1348,16 +1349,19 @@ cl--parse-loop-clause
 		  (push (list temp-seq seq) loop-for-bindings)
 		  (push (list temp-idx 0) loop-for-bindings)
 		  (if ref
-		      (let ((temp-len (make-symbol "--cl-len--")))
+                      (progn
 			(push (list temp-len `(length ,temp-seq))
 			      loop-for-bindings)
 			(push (list var `(elt ,temp-seq ,temp-idx))
 			      cl--loop-symbol-macs)
 			(push `(< ,temp-idx ,temp-len) cl--loop-body))
+                    ;; Evaluate seq length just if needed, that is, when seq is not a cons.
+                    (push (list temp-len (or (consp seq) `(length ,temp-seq)))
+			  loop-for-bindings)
 		    (push (list var nil) loop-for-bindings)
 		    (push `(and ,temp-seq
 				(or (consp ,temp-seq)
-                                    (< ,temp-idx (length ,temp-seq))))
+                                    (< ,temp-idx ,temp-len)))
 			  cl--loop-body)
 		    (push (list var `(if (consp ,temp-seq)
                                          (pop ,temp-seq)

--8<-----------------------------cut here---------------end--------------->8---

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Lars Ingebrigtsen]

Tino Calancha <tino.calancha@gmail.com> writes:

> As long as the length of the seq be an invariant (is it?), then
> it must be OK to store it in `temp-seq'.

I can't see that the Common Lisp standard specifies whether it's
invariant or not...

http://www.ai.mit.edu/projects/iiip/doc/CommonLISP/HyperSpec/Body/sec_6-1-2-1.html

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Stefan Monnier]

> X-Debbugs-CC: monnier@iro.umontreal.ca

Thanks.

> It looks sensible to calculate the array length just once,
> instead of recalculate it on each iteration:

I tend to agree, but I don't know very much about the intended semantics
of Common Lisp's `loop` macro, nor about cl-loop's implementation (tho
I admit I did mess with it without being really sure what I was doing).


        Stefan


> --8<-----------------------------cut here---------------start------------->8---
> commit 1175db5cf09eb39e7e70703f38578d31b0dd9398
> Author: Tino Calancha <tino.calancha@gmail.com>
> Date:   Thu Dec 28 00:56:32 2017 +0900
>
>     cl-loop: Calculate the array length just once
>     
>     * lisp/emacs-lisp/cl-macs.el (cl--parse-loop-clause):
>     Dont calculate the array length on each iteration (Bug#29866).
>
> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index f5311041cc..f671aea399 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -1317,11 +1317,13 @@ cl--parse-loop-clause
>  
>  	       ((memq word '(across across-ref))
>  		(let ((temp-vec (make-symbol "--cl-vec--"))
> +                      (temp-len (make-symbol "--cl-len--"))
>  		      (temp-idx (make-symbol "--cl-idx--")))
>  		  (push (list temp-vec (pop cl--loop-args)) loop-for-bindings)
> +		  (push (list temp-len `(length ,temp-vec)) loop-for-bindings)
>  		  (push (list temp-idx -1) loop-for-bindings)
>  		  (push `(< (setq ,temp-idx (1+ ,temp-idx))
> -                            (length ,temp-vec))
> +                            ,temp-len)
>                          cl--loop-body)
>  		  (if (eq word 'across-ref)
>  		      (push (list var `(aref ,temp-vec ,temp-idx))
>
> --8<-----------------------------cut here---------------end--------------->8---
>
>
> In GNU Emacs 27.0.50 (build 32, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
>  of 2017-12-28 built on calancha-pc
> Repository revision: da94ea92bc3ba6c236b394c00e6bbb725131a149
> Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
> System Description: Debian GNU/Linux 9.3 (stretch)

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Tino Calancha]

On Wed, 27 Dec 2017, Stefan Monnier wrote:

>> X-Debbugs-CC: monnier@iro.umontreal.ca
>
> Thanks.
>
>> It looks sensible to calculate the array length just once,
>> instead of recalculate it on each iteration:
>
> I tend to agree, but I don't know very much about the intended semantics
> of Common Lisp's `loop` macro, nor about cl-loop's implementation (tho
> I admit I did mess with it without being really sure what I was doing).
While studying this code I found easier to hack if we use gensym's
instead of make-symbol; otherwise, the code creates several symbols with
the same printed representation "--cl-var--".  For example, with 
current code you can read expansions with something like:

(let* ((--cl-var-- 'foo)
        (--cl-var-- 'bar))

I prefer to read:

(let* ((--cl-var--1 'foo)
        (--cl-var--2 'bar))

Do you have any preference in these cases?

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Drew Adams]

> While studying this code I found easier to hack if we use gensym's
> instead of make-symbol; otherwise, the code creates several symbols with
> the same printed representation "--cl-var--".  For example, with
> current code you can read expansions with something like:
> 
> (let* ((--cl-var-- 'foo)
>        (--cl-var-- 'bar))

That's the same variable, `--cl-var--', bound
first to `foo' and then to `bar'.

> I prefer to read:
> 
> (let* ((--cl-var--1 'foo)
>        (--cl-var--2 'bar))

That's two different variables.

> Do you have any preference in these cases?

As presented here, they are not equivalent.
Check the actual code to see if the second
can, in fact, be used in place of the first.

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Noam Postavsky]

Drew Adams <drew.adams@oracle.com> writes:

>> While studying this code I found easier to hack if we use gensym's
>> instead of make-symbol; otherwise, the code creates several symbols with
>> the same printed representation "--cl-var--".  For example, with
>> current code you can read expansions with something like:
>> 
>> (let* ((--cl-var-- 'foo)
>>        (--cl-var-- 'bar))
>
> That's the same variable, `--cl-var--', bound
> first to `foo' and then to `bar'.

That's a printout using print-circle and/or print-gensym set to nil.  If
those were set to t, you would get something that could be properly read
back as lisp code, e.g.:

    (let* ((#1=#:--cl-var-- 'foo)
           (#2=#:--cl-var-- 'bar))
       (frobnicate #1# #2#))

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Drew Adams]

> >> (let* ((--cl-var-- 'foo)
> >>        (--cl-var-- 'bar))
> >
> > That's the same variable, `--cl-var--', bound
> > first to `foo' and then to `bar'.
> 
> That's a printout using print-circle and/or print-gensym set to nil.  If
> those were set to t, you would get something that could be properly read
> back as lisp code, e.g.:
> 
>     (let* ((#1=#:--cl-var-- 'foo)
>            (#2=#:--cl-var-- 'bar))
>        (frobnicate #1# #2#))

Ah, thanks.  That wasn't clear.  Sorry for the noise.

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Stefan Monnier]

> While studying this code I found easier to hack if we use gensym's
> instead of make-symbol; otherwise, the code creates several symbols with
> the same printed representation "--cl-var--".  For example, with current
> code you can read expansions with something like:

I always (setq print-gensym t print-circle t)


        Stefan

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Tino Calancha]

On Wed, 27 Dec 2017, Stefan Monnier wrote:

>> While studying this code I found easier to hack if we use gensym's
>> instead of make-symbol; otherwise, the code creates several symbols with
>> the same printed representation "--cl-var--".  For example, with current
>> code you can read expansions with something like:
>
> I always (setq print-gensym t print-circle t)
Thank you all for th eexplanations.
Wouldn't be those good defaults?  I don't see any advantage, rather than 
confusion for the current defaults.  I even replaced locally all those 
make-symbol with gensym to follow the expansions without get crazy.

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Tino Calancha]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> X-Debbugs-CC: monnier@iro.umontreal.ca
>
> Thanks.
>
>> It looks sensible to calculate the array length just once,
>> instead of recalculate it on each iteration:
>
> I tend to agree, but I don't know very much about the intended semantics
> of Common Lisp's `loop` macro, nor about cl-loop's implementation (tho
> I admit I did mess with it without being really sure what I was doing).
Ok, let's do it.
Pushed to master branch as commit 'cl-loop: Calculate the array length just once'
(bfca19e475c01f13dbacc7f8b7bb1aecf46cb7e4)

bug#29866: 27.0.50; cl-loop: Calculate the array length just once

[Noam Postavsky]

tags 29866 - patch
tags 29866 + wontfix
quit

Tino Calancha <tino.calancha@gmail.com> writes:

>  		(let ((temp-vec (make-symbol "--cl-vec--"))
> +                      (temp-len (make-symbol "--cl-len--"))
>  		      (temp-idx (make-symbol "--cl-idx--")))
>  		  (push (list temp-vec (pop cl--loop-args)) loop-for-bindings)
> +		  (push (list temp-len `(length ,temp-vec)) loop-for-bindings)
>  		  (push (list temp-idx -1) loop-for-bindings)

I reverted this because it breaks with 'and' because the bindings are
made with 'let' rather than 'let*' (see Bug#40727).  I'm marking wontfix
because I don't intend to anything more about it, but I wouldn't oppose
another attempt that didn't have this problem (I think it would require
extensive code changes though).