Re: slowness in guile 1.8

[ludo@chbouib.org (Ludovic Courtès)]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Hi,

ludo@chbouib.org (Ludovic Courtès) writes:

> For 1.8, I'm pretty much inclined to commit a similar patch, i.e., where
> `module-make-local-var!' and `scm_m_define' are copied from HEAD.  This
> would break code that does things like:
>
>   (define foo (begin (set! foo 1) (+ foo 1)))
>
> but I think it's reasonable to break such code (which relies on
> non-R5RS-compliant behavior anyway), especially given the performance
> gain we get in return.  What do you think?

I committed the attached patch to 1.8.

I added an "Incompatible Changes" category in `NEWS'.  This sounds a bit
frightening, but the description hopefully emphasizes the
conservativeness of the change.  Please do improve the wording or
presentation if you find it inappropriate.

Thanks,
Ludovic.


[-- Attachment #2: The patch against `module-make-local-var!' in 1.8 --]
[-- Type: text/x-patch, Size: 13128 bytes --]

--- orig/ChangeLog
+++ mod/ChangeLog
@@ -1,3 +1,7 @@
+2007-06-13  Ludovic Courtès  <ludo@chbouib.org>
+
+	* NEWS: Mention top-level define incompatible change.
+
 2007-06-12  Ludovic Courtès  <ludo@chbouib.org>
 
 	* NEWS: Mention `inet-ntop' bug fix.
--- orig/NEWS
+++ mod/NEWS
@@ -12,6 +12,16 @@ Changes in 1.8.2 (since 1.8.1):
 ** set-program-arguments
 ** make-vtable
 
+* Incompatible changes
+
+** The body of a top-level `define' no longer sees the binding being created
+
+In a top-level `define', the binding being created is no longer visible
+from the `define' body.  This breaks code like
+"(define foo (begin (set! foo 1) (+ foo 1)))", where `foo' is now
+unbound in the body.  However, such code was not R5RS-compliant anyway,
+per Section 5.2.1.
+
 * Bugs fixed
 
 ** Fractions were not `equal?' if stored in unreduced form.
--- orig/ice-9/ChangeLog
+++ mod/ice-9/ChangeLog
@@ -1,3 +1,9 @@
+2007-06-13  Ludovic Courtès  <ludo@chbouib.org>
+
+	* boot-9.scm (module-make-local-var!): Simplified.  No need to
+	check for the value of a same-named imported binding since the
+	newly created variable is systematically assigned afterwards.
+
 2007-01-04  Kevin Ryde  <user42@zip.com.au>
 
 	* boot-9.scm (top-repl): Check (defined? 'SIGBUS) before using that
--- orig/ice-9/boot-9.scm
+++ mod/ice-9/boot-9.scm
@@ -1515,19 +1515,10 @@
 	       (module-modified m)
 	       b)))
 
-      ;; No local variable yet, so we need to create a new one.  That
-      ;; new variable is initialized with the old imported value of V,
-      ;; if there is one.
-      (let ((imported-var (module-variable m v))
-	    (local-var (or (and (module-binder m)
-				((module-binder m) m v #t))
-			   (begin
-			     (let ((answer (make-undefined-variable)))
-			       (module-add! m v answer)
-			       answer)))))
-	(if (and imported-var (not (variable-bound? local-var)))
-	    (variable-set! local-var (variable-ref imported-var)))
-	local-var)))
+      ;; Create a new local variable.
+      (let ((local-var (make-undefined-variable)))
+        (module-add! m v local-var)
+        local-var)))
 
 ;; module-ensure-local-variable! module symbol
 ;;
--- orig/libguile/ChangeLog
+++ mod/libguile/ChangeLog
@@ -1,3 +1,9 @@
+2007-06-13  Ludovic Courtès  <ludo@chbouib.org>
+
+	* eval.c (scm_m_define): Updated comment.  Changed order for value
+	evaluation and `scm_sym2var ()' call, which is perfectly valid per
+	R5RS.  This reverts the change dated 2004-04-22 by Dirk Herrmann.
+
 2007-06-12  Ludovic Courtès  <ludo@chbouib.org>
 
 	* socket.c (scm_inet_ntop): In the `AF_INET' case, declare `addr4'
--- orig/libguile/eval.c
+++ mod/libguile/eval.c
@@ -1213,10 +1213,11 @@ canonicalize_define (const SCM expr)
   return expr;
 }
 
-/* According to section 5.2.1 of R5RS we first have to make sure that the
- * variable is bound, and then perform the (set! variable expression)
- * operation.  This means, that within the expression we may already assign
- * values to variable: (define foo (begin (set! foo 1) (+ foo 1)))  */
+/* According to Section 5.2.1 of R5RS we first have to make sure that the
+   variable is bound, and then perform the `(set! variable expression)'
+   operation.  However, EXPRESSION _can_ be evaluated before VARIABLE is
+   bound.  This means that EXPRESSION won't necessarily be able to assign
+   values to VARIABLE as in `(define foo (begin (set! foo 1) (+ foo 1)))'.  */
 SCM
 scm_m_define (SCM expr, SCM env)
 {
@@ -1226,9 +1227,9 @@ scm_m_define (SCM expr, SCM env)
     const SCM canonical_definition = canonicalize_define (expr);
     const SCM cdr_canonical_definition = SCM_CDR (canonical_definition);
     const SCM variable = SCM_CAR (cdr_canonical_definition);
+    const SCM value = scm_eval_car (SCM_CDR (cdr_canonical_definition), env);
     const SCM location
       = scm_sym2var (variable, scm_env_top_level (env), SCM_BOOL_T);
-    const SCM value = scm_eval_car (SCM_CDR (cdr_canonical_definition), env);
 
     if (SCM_REC_PROCNAMES_P)
       {
--- orig/test-suite/ChangeLog
+++ mod/test-suite/ChangeLog
@@ -1,3 +1,11 @@
+2007-06-13  Ludovic Courtès  <ludo@chbouib.org>
+
+	* tests/syntax.test (top-level define)[binding is created before
+	expression is evaluated]: Moved to "internal define", using `let'
+	instead of `begin'.  The test was not necessarily valid for
+	top-level defines, according to Section 5.2.1 or R5RS.
+	[redefinition]: New.
+
 2007-06-12  Ludovic Courtès  <ludo@chbouib.org>
 
 	* tests/socket.test: Renamed module to `(test-suite test-socket)'.
--- orig/test-suite/tests/syntax.test
+++ mod/test-suite/tests/syntax.test
@@ -725,15 +725,16 @@
 
 (with-test-prefix "top-level define"
 
-  (pass-if "binding is created before expression is evaluated"
-    (= (eval '(begin
-                (define foo
-                  (begin
-                    (set! foo 1)
-                    (+ foo 1)))
-                foo)
-             (interaction-environment))
-       2))
+  (pass-if "redefinition"
+    (let ((m (make-module)))
+      (beautify-user-module! m)
+
+      ;; The previous value of `round' must still be visible at the time the
+      ;; new `round' is defined.  According to R5RS (Section 5.2.1), `define'
+      ;; should behave like `set!' in this case (except that in the case of
+      ;; Guile, we respect module boundaries).
+      (eval '(define round round) m)
+      (eq? (module-ref m 'round) round)))
 
   (with-test-prefix "currying"
 
@@ -780,6 +781,17 @@
                   (eq? 'c (a 2) (a 5))))
           (interaction-environment)))
 
+  (pass-if "binding is created before expression is evaluated"
+    ;; Internal defines are equivalent to `letrec' (R5RS, Section 5.2.2).
+    (= (eval '(let ()
+                (define foo
+                  (begin
+                    (set! foo 1)
+                    (+ foo 1)))
+                foo)
+             (interaction-environment))
+       2))
+
   (pass-if "internal defines with begin"
     (false-if-exception
      (eval '(let ((a identity) (b identity) (c identity))

* added files

--- /dev/null
+++ mod/{arch}/guile-core/guile-core--cvs-head/guile-core--cvs-head--0/lcourtes@laas.fr--2006-libre/patch-log/patch-53
@@ -0,0 +1,28 @@
+Revision: guile-core--cvs-head--0--patch-53
+Archive: lcourtes@laas.fr--2006-libre
+Creator: Ludovic Court`es <ludovic.courtes@laas.fr>
+Date: Sat May 26 16:22:22 CEST 2007
+Standard-date: 2007-05-26 14:22:22 GMT
+Modified-files: libguile/eval.c
+    test-suite/tests/syntax.test
+New-patches: lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-27
+    lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-28
+    lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-29
+    lcourtes@laas.fr--2006-libre/guile-core--cvs-head--0--patch-53
+    lcourtes@laas.fr--2006-libre/guile-core--devo--0--patch-19
+    lcourtes@laas.fr--2006-libre/guile-core--devo--0--patch-20
+Summary: Allow for `(define round round)'.
+Keywords: module-make-local-var! scm_m_define
+
+Patches applied:
+
+ * lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7  (patch 27-29)
+
+   - variable lookup: Allow for `(define round round)'.
+   - Reverted the earlier patch and properly fixed `(define round round)'.
+   - Merge from lcourtes@laas.fr--2006-libre/guile-core--devo--0
+
+ * guile-core--devo--0  (patch 19-20)
+
+   - variable lookup: Allow for `(define round round)'.
+   - Reverted the earlier patch and properly fixed `(define round round)'.
--- /dev/null
+++ mod/{arch}/guile-core/guile-core--cvs-head/guile-core--cvs-head--0/lcourtes@laas.fr--2006-libre/patch-log/patch-54
@@ -0,0 +1,15 @@
+Revision: guile-core--cvs-head--0--patch-54
+Archive: lcourtes@laas.fr--2006-libre
+Creator: Ludovic Court`es <ludovic.courtes@laas.fr>
+Date: Sat May 26 16:29:54 CEST 2007
+Standard-date: 2007-05-26 14:29:54 GMT
+Modified-files: libguile/ChangeLog test-suite/ChangeLog
+New-patches: lcourtes@laas.fr--2006-libre/guile-core--cvs-head--0--patch-54
+Summary: Updated ChangeLogs wrt. previous patch.
+Keywords: 
+
+Patches applied:
+
+ * lcourtes@laas.fr--2005-libre/guile-core--cvs--1.7--patch-40
+   Avoid C++-style casts in `numbers.c' (Mike Gran).
+
--- /dev/null
+++ mod/{arch}/guile-core/guile-core--devo/guile-core--devo--0/lcourtes@laas.fr--2006-libre/patch-log/patch-19
@@ -0,0 +1,17 @@
+Revision: guile-core--devo--0--patch-19
+Archive: lcourtes@laas.fr--2006-libre
+Creator: Ludovic Courtes <ludovic.courtes@laas.fr>
+Date: Sat May 26 16:12:14 CEST 2007
+Standard-date: 2007-05-26 14:12:14 GMT
+Modified-files: ice-9/boot-9.scm
+    test-suite/tests/modules.test
+New-patches: lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-27
+    lcourtes@laas.fr--2006-libre/guile-core--devo--0--patch-19
+Summary: variable lookup: Allow for `(define round round)'.
+Keywords: redefinition scm_m_define
+
+* ice-9/boot-9.scm (module-make-local-var!): Reverted to its previous
+  definition, i.e., look for a same-named imported var and use its value.
+
+* test-suite/tests/modules.test (redefinition): Added a test case that
+  previously failed.
--- /dev/null
+++ mod/{arch}/guile-core/guile-core--devo/guile-core--devo--0/lcourtes@laas.fr--2006-libre/patch-log/patch-20
@@ -0,0 +1,26 @@
+Revision: guile-core--devo--0--patch-20
+Archive: lcourtes@laas.fr--2006-libre
+Creator: Ludovic Courtes <ludovic.courtes@laas.fr>
+Date: Sat May 26 16:13:57 CEST 2007
+Standard-date: 2007-05-26 14:13:57 GMT
+Modified-files: ice-9/boot-9.scm libguile/eval.c
+    test-suite/tests/modules.test
+    test-suite/tests/syntax.test
+New-patches: lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-28
+    lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-29
+    lcourtes@laas.fr--2006-libre/guile-core--devo--0--patch-20
+Summary: Reverted the earlier patch and properly fixed `(define round round)'.
+Keywords: module-make-local-var!
+
+* ice-9/boot-9.scm (module-make-local-var!): Reverted to the previous
+  value, i.e., to not check for same-named imported variables.
+
+* libguile/eval.c (scm_m_define): Updated comment.  Changed order for
+  value evaluation and `scm_sym2var ()' call, which is perfectly valid
+  per R5RS.
+
+* test-suite/tests/modules.test (foundations)[redefinition]: Removed.
+
+* test-suite/tests/syntax.test (top-level define)[binding is created
+  before expression is evaluated]: Moved to "internal define".
+  [redefinition]: New.
--- /dev/null
+++ mod/{arch}/guile-core/guile-core--devo/guile-core--devo--1.7/lcourtes@laas.fr--2005-mobile/patch-log/patch-27
@@ -0,0 +1,16 @@
+Revision: guile-core--devo--1.7--patch-27
+Archive: lcourtes@laas.fr--2005-mobile
+Creator: Ludovic Courtes <ludovic.courtes@laas.fr>
+Date: Fri May 25 22:47:29 CEST 2007
+Standard-date: 2007-05-25 20:47:29 GMT
+Modified-files: ice-9/boot-9.scm
+    test-suite/tests/modules.test
+New-patches: lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-27
+Summary: variable lookup: Allow for `(define round round)'.
+Keywords: redefinition scm_m_define
+
+* ice-9/boot-9.scm (module-make-local-var!): Reverted to its previous
+  definition, i.e., look for a same-named imported var and use its value.
+
+* test-suite/tests/modules.test (redefinition): Added a test case that
+  previously failed.
--- /dev/null
+++ mod/{arch}/guile-core/guile-core--devo/guile-core--devo--1.7/lcourtes@laas.fr--2005-mobile/patch-log/patch-28
@@ -0,0 +1,24 @@
+Revision: guile-core--devo--1.7--patch-28
+Archive: lcourtes@laas.fr--2005-mobile
+Creator: Ludovic Courtes <ludovic.courtes@laas.fr>
+Date: Sat May 26 16:07:17 CEST 2007
+Standard-date: 2007-05-26 14:07:17 GMT
+Modified-files: ice-9/boot-9.scm libguile/eval.c
+    test-suite/tests/modules.test
+    test-suite/tests/syntax.test
+New-patches: lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-28
+Summary: Reverted the earlier patch and properly fixed `(define round round)'.
+Keywords: module-make-local-var!
+
+* ice-9/boot-9.scm (module-make-local-var!): Reverted to the previous
+  value, i.e., to not check for same-named imported variables.
+
+* libguile/eval.c (scm_m_define): Updated comment.  Changed order for
+  value evaluation and `scm_sym2var ()' call, which is perfectly valid
+  per R5RS.
+
+* test-suite/tests/modules.test (foundations)[redefinition]: Removed.
+
+* test-suite/tests/syntax.test (top-level define)[binding is created
+  before expression is evaluated]: Moved to "internal define".
+  [redefinition]: New.
--- /dev/null
+++ mod/{arch}/guile-core/guile-core--devo/guile-core--devo--1.7/lcourtes@laas.fr--2005-mobile/patch-log/patch-29
@@ -0,0 +1,14 @@
+Revision: guile-core--devo--1.7--patch-29
+Archive: lcourtes@laas.fr--2005-mobile
+Creator: Ludovic Courtes <ludovic.courtes@laas.fr>
+Date: Sat May 26 16:10:20 CEST 2007
+Standard-date: 2007-05-26 14:10:20 GMT
+New-patches: lcourtes@laas.fr--2005-mobile/guile-core--devo--1.7--patch-29
+    lcourtes@laas.fr--2006-libre/guile-core--devo--0--patch-18
+Summary: Merge from lcourtes@laas.fr--2006-libre/guile-core--devo--0
+
+Patches applied:
+
+ * lcourtes@laas.fr--2006-libre/guile-core--devo--0  (patch 18)
+
+   - Fixed SRFI-19's `time-process'.  Reported by Scott Shedden.


[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel