* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
@ 2017-12-27 16:10 Tino Calancha
2017-12-27 20:13 ` Tino Calancha
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Tino Calancha @ 2017-12-27 16:10 UTC (permalink / raw)
To: 29866; +Cc: monnier
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)
^ permalink raw reply related [flat|nested] 12+ messages in thread* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 16:10 bug#29866: 27.0.50; cl-loop: Calculate the array length just once Tino Calancha
@ 2017-12-27 20:13 ` Tino Calancha
2017-12-27 20:20 ` Lars Ingebrigtsen
2017-12-27 20:54 ` Stefan Monnier
2020-05-06 2:21 ` Noam Postavsky
2 siblings, 1 reply; 12+ messages in thread
From: Tino Calancha @ 2017-12-27 20:13 UTC (permalink / raw)
To: 29866; +Cc: monnier
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---
^ permalink raw reply related [flat|nested] 12+ messages in thread* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 16:10 bug#29866: 27.0.50; cl-loop: Calculate the array length just once Tino Calancha
2017-12-27 20:13 ` Tino Calancha
@ 2017-12-27 20:54 ` Stefan Monnier
2017-12-27 21:36 ` Tino Calancha
2018-01-07 15:39 ` Tino Calancha
2020-05-06 2:21 ` Noam Postavsky
2 siblings, 2 replies; 12+ messages in thread
From: Stefan Monnier @ 2017-12-27 20:54 UTC (permalink / raw)
To: Tino Calancha; +Cc: 29866
> 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)
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 20:54 ` Stefan Monnier
@ 2017-12-27 21:36 ` Tino Calancha
2017-12-28 1:27 ` Drew Adams
2017-12-28 2:39 ` Stefan Monnier
2018-01-07 15:39 ` Tino Calancha
1 sibling, 2 replies; 12+ messages in thread
From: Tino Calancha @ 2017-12-27 21:36 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 29866, 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?
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 21:36 ` Tino Calancha
@ 2017-12-28 1:27 ` Drew Adams
2017-12-28 1:49 ` Noam Postavsky
2017-12-28 2:39 ` Stefan Monnier
1 sibling, 1 reply; 12+ messages in thread
From: Drew Adams @ 2017-12-28 1:27 UTC (permalink / raw)
To: Tino Calancha, Stefan Monnier; +Cc: 29866
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-28 1:27 ` Drew Adams
@ 2017-12-28 1:49 ` Noam Postavsky
2017-12-28 1:52 ` Drew Adams
0 siblings, 1 reply; 12+ messages in thread
From: Noam Postavsky @ 2017-12-28 1:49 UTC (permalink / raw)
To: Drew Adams; +Cc: 29866, Stefan Monnier, Tino Calancha
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#))
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-28 1:49 ` Noam Postavsky
@ 2017-12-28 1:52 ` Drew Adams
0 siblings, 0 replies; 12+ messages in thread
From: Drew Adams @ 2017-12-28 1:52 UTC (permalink / raw)
To: Noam Postavsky; +Cc: 29866, Stefan Monnier, Tino Calancha
> >> (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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 21:36 ` Tino Calancha
2017-12-28 1:27 ` Drew Adams
@ 2017-12-28 2:39 ` Stefan Monnier
2017-12-28 17:21 ` Tino Calancha
1 sibling, 1 reply; 12+ messages in thread
From: Stefan Monnier @ 2017-12-28 2:39 UTC (permalink / raw)
To: Tino Calancha; +Cc: 29866
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-28 2:39 ` Stefan Monnier
@ 2017-12-28 17:21 ` Tino Calancha
0 siblings, 0 replies; 12+ messages in thread
From: Tino Calancha @ 2017-12-28 17:21 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 29866
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 20:54 ` Stefan Monnier
2017-12-27 21:36 ` Tino Calancha
@ 2018-01-07 15:39 ` Tino Calancha
1 sibling, 0 replies; 12+ messages in thread
From: Tino Calancha @ 2018-01-07 15:39 UTC (permalink / raw)
To: 29866-done
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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#29866: 27.0.50; cl-loop: Calculate the array length just once
2017-12-27 16:10 bug#29866: 27.0.50; cl-loop: Calculate the array length just once Tino Calancha
2017-12-27 20:13 ` Tino Calancha
2017-12-27 20:54 ` Stefan Monnier
@ 2020-05-06 2:21 ` Noam Postavsky
2 siblings, 0 replies; 12+ messages in thread
From: Noam Postavsky @ 2020-05-06 2:21 UTC (permalink / raw)
To: Tino Calancha; +Cc: 29866
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).
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-06 2:21 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-27 16:10 bug#29866: 27.0.50; cl-loop: Calculate the array length just once Tino Calancha
2017-12-27 20:13 ` Tino Calancha
2017-12-27 20:20 ` Lars Ingebrigtsen
2017-12-27 20:54 ` Stefan Monnier
2017-12-27 21:36 ` Tino Calancha
2017-12-28 1:27 ` Drew Adams
2017-12-28 1:49 ` Noam Postavsky
2017-12-28 1:52 ` Drew Adams
2017-12-28 2:39 ` Stefan Monnier
2017-12-28 17:21 ` Tino Calancha
2018-01-07 15:39 ` Tino Calancha
2020-05-06 2:21 ` Noam Postavsky
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).