[PATCH] primitive resolution for public refs

[PATCH] primitive resolution for public refs

[BT Templeton]

* module/language/tree-il/primitives.scm (resolve-primitives!): Resolve
  public module-refs to primitives.
---
 module/language/tree-il/primitives.scm |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/module/language/tree-il/primitives.scm b/module/language/tree-il/primitives.scm
index 3c6769d..1dcab20 100644
--- a/module/language/tree-il/primitives.scm
+++ b/module/language/tree-il/primitives.scm
@@ -265,13 +265,15 @@
                                (module-variable mod name)))
                (lambda (name) (make-primitive-ref src name))))
        ((<module-ref> src mod name public?)
-        ;; for the moment, we're disabling primitive resolution for
-        ;; public refs because resolve-interface can raise errors.
-        (let ((m (and (not public?) (resolve-module mod))))
-          (and m 
-               (and=> (hashq-ref *interesting-primitive-vars*
-                                 (module-variable m name))
-                      (lambda (name) (make-primitive-ref src name))))))
+        (and=> (and=> (resolve-module mod)
+                      (if public?
+                          module-public-interface
+                          identity))
+               (lambda (m)
+                 (and=> (hashq-ref *interesting-primitive-vars*
+                                   (module-variable m name))
+                        (lambda (name)
+                          (make-primitive-ref src name))))))
        ((<call> src proc args)
         (and (primitive-ref? proc)
              (make-primcall src (primitive-ref-name proc) args)))
-- 
1.7.9.1

Re: [PATCH] primitive resolution for public refs

[Noah Lavine]

Hello,

Are you sure this is correct? What if someone calls the module-set!
function on that module between the definition and use of whatever
function you're compiling?

I agree very strongly that we need to be able to do this sort of
optimization, but I think we might need some sort of caching mechanism
to do it right, where the cache of compiled function bodies is
invalidated by module-set!.

Noah

On Mon, Mar 12, 2012 at 12:42 PM, BT Templeton <bpt@hcoop.net> wrote:
> * module/language/tree-il/primitives.scm (resolve-primitives!): Resolve
>  public module-refs to primitives.
> ---
>  module/language/tree-il/primitives.scm |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/module/language/tree-il/primitives.scm b/module/language/tree-il/primitives.scm
> index 3c6769d..1dcab20 100644
> --- a/module/language/tree-il/primitives.scm
> +++ b/module/language/tree-il/primitives.scm
> @@ -265,13 +265,15 @@
>                                (module-variable mod name)))
>                (lambda (name) (make-primitive-ref src name))))
>        ((<module-ref> src mod name public?)
> -        ;; for the moment, we're disabling primitive resolution for
> -        ;; public refs because resolve-interface can raise errors.
> -        (let ((m (and (not public?) (resolve-module mod))))
> -          (and m
> -               (and=> (hashq-ref *interesting-primitive-vars*
> -                                 (module-variable m name))
> -                      (lambda (name) (make-primitive-ref src name))))))
> +        (and=> (and=> (resolve-module mod)
> +                      (if public?
> +                          module-public-interface
> +                          identity))
> +               (lambda (m)
> +                 (and=> (hashq-ref *interesting-primitive-vars*
> +                                   (module-variable m name))
> +                        (lambda (name)
> +                          (make-primitive-ref src name))))))
>        ((<call> src proc args)
>         (and (primitive-ref? proc)
>              (make-primcall src (primitive-ref-name proc) args)))
> --
> 1.7.9.1
>
>

Re: [PATCH] primitive resolution for public refs

[BT Templeton]

Noah Lavine <noah.b.lavine@gmail.com> writes:

> Hello,
>
> Are you sure this is correct? What if someone calls the module-set!
> function on that module between the definition and use of whatever
> function you're compiling?
>
> I agree very strongly that we need to be able to do this sort of
> optimization, but I think we might need some sort of caching mechanism
> to do it right, where the cache of compiled function bodies is
> invalidated by module-set!.

I don't think I've changed the correctness wrt `module-set!' and the
like -- the current, private-refs-only optimization already exhibits the
problem you describe:

| scheme@(guile-user)> (define old-car car)
| scheme@(guile-user)> (module-set! (current-module)
|                                   'car
|                                   (lambda (x) (format #t "car ~A~%" x) (old-car x)))
| scheme@(guile-user)> ,x (lambda (x) ((@@ (guile) car) x))
| [...]
| Disassembly of #<procedure 1d23ae0 at <current input>:4:0 (x)>:
|   
|   0    (assert-nargs-ee/locals 1)      ;; 1 arg, 0 locals
|   2    (local-ref 0)                   ;; `x'
|   4    (car)
|   5    (return)

I'm not certain it's otherwise correct; I just added a workaround for
the specific issue mentioned in the comment.

> On Mon, Mar 12, 2012 at 12:42 PM, BT Templeton <bpt@hcoop.net> wrote:
>> * module/language/tree-il/primitives.scm (resolve-primitives!): Resolve
>>  public module-refs to primitives.
>> ---
>>  module/language/tree-il/primitives.scm |   16 +++++++++-------
>>  1 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/module/language/tree-il/primitives.scm b/module/language/tree-il/primitives.scm
>> index 3c6769d..1dcab20 100644
>> --- a/module/language/tree-il/primitives.scm
>> +++ b/module/language/tree-il/primitives.scm
>> @@ -265,13 +265,15 @@
>>                                (module-variable mod name)))
>>                (lambda (name) (make-primitive-ref src name))))
>>        ((<module-ref> src mod name public?)
>> -        ;; for the moment, we're disabling primitive resolution for
>> -        ;; public refs because resolve-interface can raise errors.
>> -        (let ((m (and (not public?) (resolve-module mod))))
>> -          (and m
>> -               (and=> (hashq-ref *interesting-primitive-vars*
>> -                                 (module-variable m name))
>> -                      (lambda (name) (make-primitive-ref src name))))))
>> +        (and=> (and=> (resolve-module mod)
>> +                      (if public?
>> +                          module-public-interface
>> +                          identity))
>> +               (lambda (m)
>> +                 (and=> (hashq-ref *interesting-primitive-vars*
>> +                                   (module-variable m name))
>> +                        (lambda (name)
>> +                          (make-primitive-ref src name))))))
>>        ((<call> src proc args)
>>         (and (primitive-ref? proc)
>>              (make-primcall src (primitive-ref-name proc) args)))
>> --
>> 1.7.9.1

-- 
Inteligenta persono lernas la lingvon Esperanton rapide kaj facile.
Esperanto estas moderna, kultura lingvo por la mondo. Simpla, fleksebla,
belsona, Esperanto estas la praktika solvo de la problemo de universala
interkompreno. Lernu la interlingvon Esperanton!

Re: [PATCH] primitive resolution for public refs

[Noah Lavine]

Hello,

> I don't think I've changed the correctness wrt `module-set!' and the
> like -- the current, private-refs-only optimization already exhibits the
> problem you describe:
>
> | scheme@(guile-user)> (define old-car car)
> | scheme@(guile-user)> (module-set! (current-module)

I think (current-module) should be (guile) here. Otherwise I think you
will change 'car in (guile-user) and not affect (guile). But I have no
doubt that this bug is real.

> |                                   'car
> |                                   (lambda (x) (format #t "car ~A~%" x) (old-car x)))
> | scheme@(guile-user)> ,x (lambda (x) ((@@ (guile) car) x))
> | [...]
> | Disassembly of #<procedure 1d23ae0 at <current input>:4:0 (x)>:
> |
> |   0    (assert-nargs-ee/locals 1)      ;; 1 arg, 0 locals
> |   2    (local-ref 0)                   ;; `x'
> |   4    (car)
> |   5    (return)
>
> I'm not certain it's otherwise correct; I just added a workaround for
> the specific issue mentioned in the comment.

Yes, you're right.

That just raises the question, though: what's the right thing to do? I
realized that you could argue this is part of making a closure,
because it closes over the current value of variables in other
modules. That doesn't match the behavior of regular closures, though:
they just close over storage locations, so they can still be affected
by external set!'s.

Noah

Re: [PATCH] primitive resolution for public refs

[BT Templeton]

Noah Lavine <noah.b.lavine@gmail.com> writes:

> Hello,
>
>> I don't think I've changed the correctness wrt `module-set!' and the
>> like -- the current, private-refs-only optimization already exhibits the
>> problem you describe:
>>
>> | scheme@(guile-user)> (define old-car car)
>> | scheme@(guile-user)> (module-set! (current-module)
>
> I think (current-module) should be (guile) here. Otherwise I think you
> will change 'car in (guile-user) and not affect (guile). But I have no
> doubt that this bug is real.

You're probably thinking of `module-define!'; `module-set!' mutates the
variable object directly (whether it's imported from another module or
not), so it works as written, modifying both (@ (guile-user) car) and (@
(guile) car).

>> I'm not certain it's otherwise correct; I just added a workaround for
>> the specific issue mentioned in the comment.
>
> Yes, you're right.
>
> That just raises the question, though: what's the right thing to do? I
> realized that you could argue this is part of making a closure,
> because it closes over the current value of variables in other
> modules. That doesn't match the behavior of regular closures, though:
> they just close over storage locations, so they can still be affected
> by external set!'s.

If the (guile) module and its variables were immutable by default, the
optimization would be correct. Perhaps Guile could provide something
similar to Common Lisp package locks:
<http://www.sbcl.org/manual/Package-Locks.html>

-- 
Inteligenta persono lernas la lingvon Esperanton rapide kaj facile.
Esperanto estas moderna, kultura lingvo por la mondo. Simpla, fleksebla,
belsona, Esperanto estas la praktika solvo de la problemo de universala
interkompreno. Lernu la interlingvon Esperanton!

Re: [PATCH] primitive resolution for public refs

[Ludovic Courtès]

Hi BT,

BT Templeton <bpt@hcoop.net> skribis:

> * module/language/tree-il/primitives.scm (resolve-primitives!): Resolve
>   public module-refs to primitives.

What about resolving private module-refs too?  Here’s the motivation:

  scheme@(guile-user)> (use-modules(ice-9 match))
  scheme@(guile-user)> ,expand (match x ((_ ...) #t))
  $4 = (let ((v-356 x))
    (let ((failure-360
            (lambda ()
              ((@@ (ice-9 match) error)
               'match
               "no matching pattern"))))
      (let ((_-414 v-356))
        (if ((@@ (ice-9 match) list?) _-414)   ;;; <-- here
          #t
          (failure-360)))))

Also, here’s a test case for you to add in tree-il.test:  ;-)

  (pass-if-peval resolve-primitives
    ;; Module-refs that resolve to primitives are opened.
    (apply (@@ (guile) car) (const (1 2)))
    (const 1))

Could you provide an updated patch?

Ludo’.

Re: [PATCH] primitive resolution for public refs

[BT Templeton]

* module/language/tree-il/primitives.scm (resolve-primitives!): Resolve
  public module-refs to primitives.

* test-suite/tests/tree-il.test: New tests for primitive resolution.
---
 module/language/tree-il/primitives.scm |   16 +++++++++-------
 test-suite/tests/tree-il.test          |   10 +++++++++-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/module/language/tree-il/primitives.scm b/module/language/tree-il/primitives.scm
index 3c6769d..1dcab20 100644
--- a/module/language/tree-il/primitives.scm
+++ b/module/language/tree-il/primitives.scm
@@ -265,13 +265,15 @@
                                (module-variable mod name)))
                (lambda (name) (make-primitive-ref src name))))
        ((<module-ref> src mod name public?)
-        ;; for the moment, we're disabling primitive resolution for
-        ;; public refs because resolve-interface can raise errors.
-        (let ((m (and (not public?) (resolve-module mod))))
-          (and m 
-               (and=> (hashq-ref *interesting-primitive-vars*
-                                 (module-variable m name))
-                      (lambda (name) (make-primitive-ref src name))))))
+        (and=> (and=> (resolve-module mod)
+                      (if public?
+                          module-public-interface
+                          identity))
+               (lambda (m)
+                 (and=> (hashq-ref *interesting-primitive-vars*
+                                   (module-variable m name))
+                        (lambda (name)
+                          (make-primitive-ref src name))))))
        ((<call> src proc args)
         (and (primitive-ref? proc)
              (make-primcall src (primitive-ref-name proc) args)))
diff --git a/test-suite/tests/tree-il.test b/test-suite/tests/tree-il.test
index 4dbe310..9ce91e7 100644
--- a/test-suite/tests/tree-il.test
+++ b/test-suite/tests/tree-il.test
@@ -1598,7 +1598,15 @@
    (lambda _
      (lambda-case
       (((x y) #f #f #f () (_ _))
-       _)))))
+       _))))
+
+  (pass-if-peval
+   ((@ (guile) car) '(1 2))
+   (const 1))
+
+  (pass-if-peval
+   ((@@ (guile) car) '(1 2))
+   (const 1)))
 
 
-- 
1.7.9.1


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

> Hi BT,
>
> BT Templeton <bpt@hcoop.net> skribis:
>
>> * module/language/tree-il/primitives.scm (resolve-primitives!): Resolve
>>   public module-refs to primitives.
>
> What about resolving private module-refs too?  Here’s the motivation:

That was already supported; this patch just additionally adds support
for public refs.

> Also, here’s a test case for you to add in tree-il.test:  ;-)
>
>   (pass-if-peval resolve-primitives
>     ;; Module-refs that resolve to primitives are opened.
>     (apply (@@ (guile) car) (const (1 2)))
>     (const 1))
>
> Could you provide an updated patch?

Yes, see above for a version that includes tests.

-- 
Inteligenta persono lernas la lingvon Esperanton rapide kaj facile.
Esperanto estas moderna, kultura lingvo por la mondo. Simpla, fleksebla,
belsona, Esperanto estas la praktika solvo de la problemo de universala
interkompreno. Lernu la interlingvon Esperanton!

Re: [PATCH] primitive resolution for public refs

[Ludovic Courtès]

Hi BT,

The patch doesn’t apply currently on stable-2.0.  Could you send an
updated one, or apply it yourself?

Thanks,
Ludo’.

Re: [PATCH] primitive resolution for public refs

[BT Templeton]

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

> Hi BT,
>
> The patch doesn’t apply currently on stable-2.0.  Could you send an
> updated one, or apply it yourself?

It was a patch for master. Committed to stable-2.0 in commit
a8004dcb4d7148ec66cbaa109a18715d757700eb

-- 
Inteligenta persono lernas la lingvon Esperanton rapide kaj facile.
Esperanto estas moderna, kultura lingvo por la mondo. Simpla, fleksebla,
belsona, Esperanto estas la praktika solvo de la problemo de universala
interkompreno. Lernu la interlingvon Esperanton!