slowness in guile 1.8

slowness in guile 1.8

[Andy Wingo]

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

Hello guile devs,

I am doing some profiling with guile-gnome and guile 1.8. It is
significantly slower than 1.6. One reason in guile-gnome is the thinking
behind e.g. this change from 2004:

2004-12-22  Marius Vollmer  <marius.vollmer@uni-dortmund.de>

	* boot-9.scm (module-make-local-var!): When creating a new
	variable, initialize it to the value of any imported variable with
	the given name.  This allows code like (define round round) to
	work as expected.

(There were previous changes when duplicate handlers were introduced in
2003 by Mikael.)

This scheme code gets called whenever a variable is defined. The fact
that guile goes through module-variable and module binders, etc etc, to
compute what a value would have been *and then we throw it away*, is
quite wasteful. I'm attaching a statprof log from loading (gnome gtk);
out of 6.2 seconds loading the module (!), fully 3.8 of them were inside
module-variable. Of course profiling interpretation is not that simple,
but I think the trace shows a significant regression from 1.6, where I
was able to get (gnome gtk) loading down to less than 2 seconds, which
is still too much.

In this particular case the solution would be to compute the value
before computing the variable in which to store it. Computing the
variable in which to store is then O(1) in the number of modules in the
system, as it doesn't have to search the use list.

Any thoughts on this? Sometimes I think I'm just ranting to an empty
room ;)

Cheers,

Andy.
-- 
http://wingolog.org/

[-- Attachment #2: gnome-gtk-statprof --]
[-- Type: text/plain, Size: 6397 bytes --]

guile> (with-statprof #:hz 100 (resolve-interface '(gnome gtk)))
%     cumulative   self             
time   seconds     seconds      name
 16.55      1.13      1.03  hash-for-each
 12.41      3.81      0.77  module-search
 11.38      1.24      0.71  %record-type-check
 10.69      2.18      0.66  module-local-variable
  5.17      0.94      0.32  module-obarray-ref
  5.17      0.66      0.32  module-obarray
  5.17      0.32      0.32  record-type-descriptor
  3.79      0.56      0.24  eq?
  2.76      4.06      0.17  module-make-local-var!
  2.76      0.60      0.17  module-binder
  2.41      0.60      0.15  module-uses
  2.07      6.20      0.13  primitive-load
  1.72      1.62      0.11  for-each
  1.03      0.13      0.06  catch
  1.03      0.06      0.06  cache-hashval
  0.69      4.21      0.04  dynamic-call
  0.69      0.21      0.04  module-ensure-local-variable!
  0.69      0.09      0.04  module-obarray-set!
  0.69      0.06      0.04  vector-set!
  0.34      0.15      0.02  module-add!
  0.34      0.11      0.02  module-modified
  0.34      0.11      0.02  add-method!
  0.34      0.09      0.02  vector-map
  0.34      0.09      0.02  module-ref
  0.34      0.06      0.02  module-call-observers
  0.34      0.04      0.02  module-observers
  0.34      0.02      0.02  module-weak-observers
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  not
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  module-for-each
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  gtype-class->type
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  hash-set!
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  make-module
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.34      0.02      0.02  loop
  0.00      6.20      0.00  resolve-module
  0.00      6.20      0.00  try-load-module
  0.00      6.20      0.00  dynamic-wind
  0.00      6.20      0.00  try-module-autoload
  0.00      6.20      0.00  load-file
  0.00      6.20      0.00  resolve-interface
  0.00      6.20      0.00  primitive-eval
  0.00      6.20      0.00  save-module-excursion
  0.00      6.20      0.00  with-fluid*
  0.00      5.67      0.00  process-use-modules
  0.00      4.77      0.00  map
  0.00      4.75      0.00  apply
  0.00      3.81      0.00  module-variable
  0.00      3.29      0.00  load-file
  0.00      1.18      0.00  module-use-interfaces!
  0.00      1.15      0.00  process-duplicates
  0.00      0.98      0.00  load-file
  0.00      0.47      0.00  process-define-module
  0.00      0.43      0.00  module-export!
  0.00      0.36      0.00  load-file
  0.00      0.34      0.00  load-file
  0.00      0.30      0.00  loop
  0.00      0.24      0.00  initialize
  0.00      0.24      0.00  make-instance
  0.00      0.19      0.00  %gw-module-binder
  0.00      0.19      0.00  gtype->class
  0.00      0.17      0.00  gobject-class-set-properties!
  0.00      0.15      0.00  method-cache-install!
  0.00      0.15      0.00  memoize-method!
  0.00      0.13      0.00  cache-try-hash!
  0.00      0.13      0.00  hashed-method-cache-insert!
  0.00      0.13      0.00  get-direct-supers
  0.00      0.09      0.00  %gw-module-binder
  0.00      0.09      0.00  load-file
  0.00      0.09      0.00  gobject-type-get-properties
  0.00      0.09      0.00  make-class
  0.00      0.09      0.00  defined?
  0.00      0.06      0.00  loop
  0.00      0.06      0.00  cons*
  0.00      0.06      0.00  logand
  0.00      0.06      0.00  module-public-interface
  0.00      0.04      0.00  load-file
  0.00      0.04      0.00  init-gobject-class
  0.00      0.04      0.00  especify-metaclass!
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  compute-new-list-of-methods
  0.00      0.02      0.00  hash-fold
  0.00      0.02      0.00  beautify-user-module!
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  list
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  init-gobject-class
  0.00      0.02      0.00  compile-method
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  init-gobject-class
  0.00      0.02      0.00  init-gobject-class
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  module-use!
  0.00      0.02      0.00  load-file
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  next-method?
  0.00      0.02      0.00  init-gobject-class
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  init-gobject-class
  0.00      0.02      0.00  init-gobject-class
  0.00      0.02      0.00  compute-entry-with-cmethod
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  class-of
  0.00      0.02      0.00  slot-set!
  0.00      0.02      0.00  memv
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  loop
  0.00      0.02      0.00  compute-slot-accessors
  0.00      0.02      0.00  find-duplicate
  0.00      0.02      0.00  loop
---
Sample count: 290
Total time: 6.2 seconds (9/10 seconds in GC)

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

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

Re: slowness in guile 1.8

[Ludovic Courtès]

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

Hello Guilers!

Andy Wingo <wingo@pobox.com> writes:

> 2004-12-22  Marius Vollmer  <marius.vollmer@uni-dortmund.de>
>
> 	* boot-9.scm (module-make-local-var!): When creating a new
> 	variable, initialize it to the value of any imported variable with
> 	the given name.  This allows code like (define round round) to
> 	work as expected.

(See also: http://cvs.savannah.gnu.org/viewvc/guile/guile-core/ice-9/boot-9.scm?root=guile&r1=1.341&r2=1.342)

Andy and I discussed this issue on IRC.

It turns out that my recent changes in the module system (allowing for
lazy duplicate binding checks) revert this change, for the reasons
mentioned by Andy ("superfluous" overhead).

However, in doing so it breaks `(define round round)' ("unbound
variable").  Inverting the order of the calls to `scm_sym2var ()' and
`scm_eval_car ()' in `scm_m_define ()' so that it first evaluates the
value, and then creates the variable and assigns it the just-evaluated
value fixes the problem.

Alas, it breaks the following test in `syntax.test':

  (pass-if "binding is created before expression is evaluated"
    (= (eval '(begin
                (define foo
                  (begin
                    (set! foo 1)
                    (+ foo 1)))
                foo)
             (interaction-environment))
       2))

This test case illustrates the fact that _internal_ defines are
equivalent to `letrec' (Section 5.2.2); top-level defines should behave
similarly for new variables (Section 5.2.1).

For top-level defines as in `(define round round)', the rule is that
`define' is equivalent to `set!' when the variable is already bound
(Section 5.2.1).  This justifies the change made by Marius to
`module-make-local-var!' (above).

Consequently:

  * `module-make-local-var!' cannot be changed in 1.8;

  * `module-make-local-var!' in HEAD must be reverted to its previous
    definition (attached patch).

This is _very_ unfortunate performance-wise, as Andy noted, but I don't
have any better idea ATM.   :-(

Thanks,
Ludovic.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Reverting `module-make-local-var!' in HEAD --]
[-- Type: text/x-patch, Size: 1660 bytes --]

--- orig/ice-9/boot-9.scm
+++ mod/ice-9/boot-9.scm
@@ -1511,10 +1511,22 @@
 	       (module-modified m)
 	       b)))
 
-      ;; Create a new local variable.
-      (let ((local-var (make-undefined-variable)))
-        (module-add! m v local-var)
-        local-var)))
+      ;; 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.  This is required by Section 5.2.1 of R5RS which
+      ;; says that `define' is equivalent to `set!' when V is already bound
+      ;; (of course here we respect module boundaries so this is not exactly
+      ;; a `set!').
+      (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)))
 
 ;; module-ensure-local-variable! module symbol
 ;;

--- orig/test-suite/tests/modules.test
+++ mod/test-suite/tests/modules.test
@@ -118,7 +118,16 @@
               (map module-variable
                    (map resolve-interface mods)
                    syms)
-              locals))))
+              locals)))
+
+  (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.
+      (eval '(define round round) m)
+      (eq? (module-ref m 'stat) stat))))
 
 
 \f


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

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

Re: slowness in guile 1.8

[Andy Wingo]

Hey Ludovic,

Thanks for looking at this!

On Fri, 2007-05-25 at 20:12 +0200, Ludovic Courtès wrote:
> Alas, it breaks the following test in `syntax.test':

I'm not sure what "it" is in this case; I assume you mean the fix to
module-make-local-var!.

>   (pass-if "binding is created before expression is evaluated"
>     (= (eval '(begin
>                 (define foo
>                   (begin
>                     (set! foo 1)
>                     (+ foo 1)))
>                 foo)
>              (interaction-environment))
>        2))
> 
> This test case illustrates the fact that _internal_ defines are
> equivalent to `letrec' (Section 5.2.2); top-level defines should behave
> similarly for new variables (Section 5.2.1).

I don't know what you are trying to say here; top-level defines do not
"behave similarly" to letrec. R5RS says in section 5.2.1:

        If <variable> is not bound, however, then the definition will
        bind <variable> to a new location before performing the
        assignment, whereas it would be an error to perform a `set!' on
        an unbound variable.  

The new variable should be created before the assignment, but _not
necessarily before evaluation of the rhs_.

> For top-level defines as in `(define round round)', the rule is that
> `define' is equivalent to `set!' when the variable is already bound
> (Section 5.2.1).  This justifies the change made by Marius to
> `module-make-local-var!' (above).

Only if foo is already bound, in the case you gave, is the test
syntactically valid.

I think the test is bogus. MzScheme, Gambit, Scheme48, and Guile 1.6
think so too. You have snatched defeat from the jaws of victory ;-)

Suggest removing the test.

Cheers,

Andy.
-- 
http://wingolog.org/


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

Re: slowness in guile 1.8

[Andy Wingo]

Replying to myself,

On Sat, 2007-05-26 at 12:49 +0200, Andy Wingo wrote:
> On Fri, 2007-05-25 at 20:12 +0200, Ludovic Courtès wrote:
> >   (pass-if "binding is created before expression is evaluated"
> >     (= (eval '(begin
> >                 (define foo
> >                   (begin
> >                     (set! foo 1)
> >                     (+ foo 1)))
> >                 foo)
> >              (interaction-environment))
> >        2))

This test case was added by Dirk in on 26 April 2004:

http://cvs.savannah.gnu.org/viewvc/guile/guile-core/test-suite/tests/syntax.test?root=guile&view=log#rev1.33

The commit message is interesting. It is also when he reordered the
lines in scm_m_define. I do think that its intention fails though; to me
the above is not valid Scheme.

Regards,

Andy.
-- 
http://wingolog.org/


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

Re: slowness in guile 1.8

[Ludovic Courtès]

Hi Andy!

Andy Wingo <wingo@pobox.com> writes:

> On Fri, 2007-05-25 at 20:12 +0200, Ludovic Courtès wrote:
>> Alas, it breaks the following test in `syntax.test':
>
> I'm not sure what "it" is in this case; I assume you mean the fix to
> module-make-local-var!.

"It" means "the reordering of `eval_car' and `sym2var' in `eval.c'".

>>   (pass-if "binding is created before expression is evaluated"
>>     (= (eval '(begin
>>                 (define foo
>>                   (begin
>>                     (set! foo 1)
>>                     (+ foo 1)))
>>                 foo)
>>              (interaction-environment))
>>        2))
>> 
>> This test case illustrates the fact that _internal_ defines are
>> equivalent to `letrec' (Section 5.2.2); top-level defines should behave
>> similarly for new variables (Section 5.2.1).
>
> I don't know what you are trying to say here; top-level defines do not
> "behave similarly" to letrec. R5RS says in section 5.2.1:
>
>         If <variable> is not bound, however, then the definition will
>         bind <variable> to a new location before performing the
>         assignment, whereas it would be an error to perform a `set!' on
>         an unbound variable.  
>
> The new variable should be created before the assignment, but _not
> necessarily before evaluation of the rhs_.

Oh, right, there's a subtle difference here, so your interpretation may
well be valid, indeed.

> I think the test is bogus.

Actually, no: the test does a `define' _within_ the body of `begin', so
I *think* this qualifies as an internal define, and internal defines are
equivalent to `letrec' (Section 5.2.2).  So the test is equivalent to:

  (letrec ((foo (begin
                  (set! foo 1)
                  (+ foo 1))))
    foo)

And this is valid (and does actually work in all the previously
mentioned implementations except SCM).

IOW, `scm_m_define ()' must be refined to distinguish between internal
defines and top-level defines.

Needs some more thought now...

Thanks!

Ludovic.



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

Re: slowness in guile 1.8

[Ludovic Courtès]

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

Hi,

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

> Actually, no: the test does a `define' _within_ the body of `begin', so
> I *think* this qualifies as an internal define, and internal defines are
> equivalent to `letrec' (Section 5.2.2).

I was wrong: `define' within `begin' does not qualify as an "internal
define", so the test "binding is created before expression is evaluated"
was incorrect (or over-specified compared to R5RS[*]).

After further discussion with Andy, I committed the patch below to HEAD.
It inverts the order of expression evaluation and `scm_sym2var' in
`scm_m_define ()' (which is concerned only with top-level defines),
thereby fixing the `(define round round)' case.  At the same time, it
breaks the aforementioned test from `syntax.test', but there's nothing
wrong with that.

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?

Thanks,
Ludovic.

[*] FWIW, the wording for `define' in the newly-released R5.93RS
    (Section 9.3.1) is the same as that of R5RS (Section 5.2.1).



[-- Attachment #2: The fix for `(define round round)' in HEAD --]
[-- Type: text/x-patch, Size: 3796 bytes --]

--- orig/libguile/ChangeLog
+++ mod/libguile/ChangeLog
@@ -1,3 +1,9 @@
+2007-05-26  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-05-05  Ludovic Courtès  <ludo@chbouib.org>
 
 	Implemented lazy duplicate binding handling.


--- orig/libguile/eval.c
+++ mod/libguile/eval.c
@@ -1209,10 +1209,11 @@
   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)
 {
@@ -1222,9 +1223,9 @@
     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-05-26  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-05-09  Ludovic Courtès  <ludo@chbouib.org>
 
 	* tests/srfi-19.test ((current-time time-tai) works): Use `time?'.


--- 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))




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

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

Re: slowness in guile 1.8

[Andy Wingo]

Hi,

On Sat, 2007-05-26 at 16:45 +0200, Ludovic Courtès wrote:
> I committed the patch below to HEAD.

Great!

> 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)))

This code has only "worked" for guile 1.8 anyways. It did not work in
1.6, so I don't see an issue in changing this behavior.

Cheers,

Andy.
-- 
http://wingolog.org/


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

Re: slowness in guile 1.8

[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