fixes to goops + light structs + 'u' slots

fixes to goops + light structs + 'u' slots

[Andy Wingo]

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

Hey folks,

Here are some patches that fix some goops <-> light structs <-> 'u'
slots interactions. See
http://article.gmane.org/gmane.lisp.guile.user/6371 for the basic idea
of where this goes. I'll write more later, but I think that these fixes
are obviously correct.

Let me know how this formatting comes out, or if there's a better way or
something.


[-- Attachment #2: 0001-respect-slot-allocation-e.g.-for-read-only-slot.patch --]
[-- Type: text/x-patch, Size: 3220 bytes --]

From fdaf1acd1af034ae6ac3f3dbeeb58bf4d8cca0ce Mon Sep 17 00:00:00 2001
From: Andy Wingo <wingo@unquote.local>
Date: Thu, 10 Apr 2008 01:23:06 +0200
Subject: [PATCH] respect slot allocation, e.g. for <read-only-slot>

	* libguile/goops.c (get_slot_value, set_slot_value): In the struct
	allocation case, don't poke the slots array directly -- we should
	go through struct-ref/struct-set! code so that we get the
	permissions and allocation ('u' versus 'p') correct.
---
 libguile/ChangeLog |    7 +++++++
 libguile/goops.c   |   11 +++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/libguile/ChangeLog b/libguile/ChangeLog
index e22c8f2..3e7b398 100644
--- a/libguile/ChangeLog
+++ b/libguile/ChangeLog
@@ -1,3 +1,10 @@
+2008-04-10  Andy Wingo  <wingo@pobox.com>
+
+	* libguile/goops.c (get_slot_value, set_slot_value): In the struct
+	allocation case, don't poke the slots array directly -- we should
+	go through struct-ref/struct-set! code so that we get the
+	permissions and allocation ('u' versus 'p') correct.
+
 2008-04-03  Ludovic Courtès  <ludo@gnu.org>
 
 	* inline.h (SCM_C_EXTERN_INLINE): New macro, addresses the
diff --git a/libguile/goops.c b/libguile/goops.c
index 8e398e3..a6769cd 100644
--- a/libguile/goops.c
+++ b/libguile/goops.c
@@ -1260,6 +1260,7 @@ slot_definition_using_name (SCM class, SCM slot_name)
 
 static SCM
 get_slot_value (SCM class SCM_UNUSED, SCM obj, SCM slotdef)
+#define FUNC_NAME "%get-slot-value"
 {
   SCM access = SCM_CDDR (slotdef);
   /* Two cases here:
@@ -1270,7 +1271,9 @@ get_slot_value (SCM class SCM_UNUSED, SCM obj, SCM slotdef)
    * we can just assume fixnums here.
    */
   if (SCM_I_INUMP (access))
-    return SCM_SLOT (obj, SCM_I_INUM (access));
+    /* Don't poke at the slots directly, because scm_struct_ref handles the
+       access bits for us. */
+    return scm_struct_ref (obj, access);
   else
     {
       /* We must evaluate (apply (car access) (list obj))
@@ -1287,6 +1290,7 @@ get_slot_value (SCM class SCM_UNUSED, SCM obj, SCM slotdef)
       return scm_eval_body (SCM_CLOSURE_BODY (code), env);
     }
 }
+#undef FUNC_NAME
 
 static SCM
 get_slot_value_using_name (SCM class, SCM obj, SCM slot_name)
@@ -1300,6 +1304,7 @@ get_slot_value_using_name (SCM class, SCM obj, SCM slot_name)
 
 static SCM
 set_slot_value (SCM class SCM_UNUSED, SCM obj, SCM slotdef, SCM value)
+#define FUNC_NAME "%set-slot-value"
 {
   SCM access = SCM_CDDR (slotdef);
   /* Two cases here:
@@ -1310,7 +1315,8 @@ set_slot_value (SCM class SCM_UNUSED, SCM obj, SCM slotdef, SCM value)
    * we can just assume fixnums here.
    */
   if (SCM_I_INUMP (access))
-    SCM_SET_SLOT (obj, SCM_I_INUM (access), value);
+    /* obey permissions bits via going through struct-set! */
+    scm_struct_set_x (obj, access, value);
   else
     {
       /* We must evaluate (apply (cadr l) (list obj value))
@@ -1331,6 +1337,7 @@ set_slot_value (SCM class SCM_UNUSED, SCM obj, SCM slotdef, SCM value)
     }
   return SCM_UNSPECIFIED;
 }
+#undef FUNC_NAME
 
 static SCM
 set_slot_value_using_name (SCM class, SCM obj, SCM slot_name, SCM value)
-- 
1.5.5-rc1.GIT


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-initialize-u-slots-to-0-not-SCM_UNPACK-SCM_GOOPS_.patch --]
[-- Type: text/x-patch, Size: 1714 bytes --]

From 687f8ab3f54170f04d31a5b18397238a92eea0b6 Mon Sep 17 00:00:00 2001
From: Andy Wingo <wingo@unquote.local>
Date: Thu, 10 Apr 2008 01:27:19 +0200
Subject: [PATCH] initialize 'u' slots to 0, not SCM_UNPACK(SCM_GOOPS_UNBOUND)

* goops.c (wrap_init): Initialize 'u' slots to 0, not some random
SCM value.
---
 libguile/ChangeLog |    5 ++++-
 libguile/goops.c   |    9 +++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/libguile/ChangeLog b/libguile/ChangeLog
index 3e7b398..cce8dbb 100644
--- a/libguile/ChangeLog
+++ b/libguile/ChangeLog
@@ -1,6 +1,9 @@
 2008-04-10  Andy Wingo  <wingo@pobox.com>
 
-	* libguile/goops.c (get_slot_value, set_slot_value): In the struct
+	* goops.c (wrap_init): Initialize 'u' slots to 0, not some random
+	SCM value.
+
+	* goops.c (get_slot_value, set_slot_value): In the struct
 	allocation case, don't poke the slots array directly -- we should
 	go through struct-ref/struct-set! code so that we get the
 	permissions and allocation ('u' versus 'p') correct.
diff --git a/libguile/goops.c b/libguile/goops.c
index a6769cd..abb96ab 100644
--- a/libguile/goops.c
+++ b/libguile/goops.c
@@ -1507,10 +1507,15 @@ static SCM
 wrap_init (SCM class, SCM *m, long n)
 {
   long i;
+  scm_t_bits slayout = SCM_STRUCT_DATA (class)[scm_vtable_index_layout];
+  const char *layout = scm_i_symbol_chars (SCM_PACK (slayout));
 
-  /* Set all slots to unbound */
+  /* Set all SCM-holding slots to unbound */
   for (i = 0; i < n; i++)
-    m[i] = SCM_GOOPS_UNBOUND;
+    if (layout[i*2] == 'p')
+      m[i] = SCM_GOOPS_UNBOUND;
+    else
+      m[i] = 0;
 
   return scm_double_cell ((((scm_t_bits) SCM_STRUCT_DATA (class))
 			   | scm_tc3_struct),
-- 
1.5.5-rc1.GIT


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-fix-struct-ref-and-struct-set-on-light-structs.patch --]
[-- Type: text/x-patch, Size: 2239 bytes --]

From dde5041ecc1ba5b7f3e1bf182b3f98107d5efbd5 Mon Sep 17 00:00:00 2001
From: Andy Wingo <wingo@unquote.local>
Date: Thu, 10 Apr 2008 01:32:14 +0200
Subject: [PATCH] fix struct-ref and struct-set! on "light" structs

* libguile/struct.c (scm_struct_ref, scm_struct_set_x): "Light" structs
have no hidden words (members of the SCM_STRUCT_DATA(x) array accessed
with negative indices). In that case, determine the number of fields
from the length of the struct layout descriptor. (Most GOOPS instances
are light structs.)
---
 libguile/ChangeLog |    6 ++++++
 libguile/struct.c  |   12 ++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/libguile/ChangeLog b/libguile/ChangeLog
index cce8dbb..0d91c6b 100644
--- a/libguile/ChangeLog
+++ b/libguile/ChangeLog
@@ -1,5 +1,11 @@
 2008-04-10  Andy Wingo  <wingo@pobox.com>
 
+	* struct.c (scm_struct_ref, scm_struct_set_x): "Light" structs
+	have no hidden words (members of the SCM_STRUCT_DATA(x) array
+	accessed with negative indices). In that case, determine the
+	number of fields from the length of the struct layout
+	descriptor. (Most GOOPS instances are light structs.)
+
 	* goops.c (wrap_init): Initialize 'u' slots to 0, not some random
 	SCM value.
 
diff --git a/libguile/struct.c b/libguile/struct.c
index c8d34a4..2d36303 100644
--- a/libguile/struct.c
+++ b/libguile/struct.c
@@ -659,7 +659,11 @@ SCM_DEFINE (scm_struct_ref, "struct-ref", 2, 0, 0,
 
   fields_desc = scm_i_symbol_chars (layout);
   layout_len = scm_i_symbol_length (layout);
-  n_fields = data[scm_struct_i_n_words];
+  if (SCM_STRUCT_VTABLE_FLAGS (handle) & SCM_STRUCTF_LIGHT)
+    /* no extra words */
+    n_fields = layout_len / 2;
+  else
+    n_fields = data[scm_struct_i_n_words];
   
   SCM_ASSERT_RANGE(1, pos, p < n_fields);
 
@@ -736,7 +740,11 @@ SCM_DEFINE (scm_struct_set_x, "struct-set!", 3, 0, 0,
 
   fields_desc = scm_i_symbol_chars (layout);
   layout_len = scm_i_symbol_length (layout);
-  n_fields = data[scm_struct_i_n_words];
+  if (SCM_STRUCT_VTABLE_FLAGS (handle) & SCM_STRUCTF_LIGHT)
+    /* no extra words */
+    n_fields = layout_len / 2;
+  else
+    n_fields = data[scm_struct_i_n_words];
 
   SCM_ASSERT_RANGE (1, pos, p < n_fields);
 
-- 
1.5.5-rc1.GIT

Re: fixes to goops + light structs + 'u' slots

[Ludovic Courtès]

Hi Andy!

Andy Wingo <wingo@pobox.com> writes:

> Here are some patches that fix some goops <-> light structs <-> 'u'
> slots interactions. See
> http://article.gmane.org/gmane.lisp.guile.user/6371 for the basic idea
> of where this goes. I'll write more later, but I think that these fixes
> are obviously correct.

Looks good to me, I applied them (with `git-am', which is a nice tool),
along with `NEWS' entries.

> From: Andy Wingo <wingo@unquote.local>

You forgot to set your email address.  :-)

> Subject: [PATCH] respect slot allocation, e.g. for <read-only-slot>
>
> 	* libguile/goops.c (get_slot_value, set_slot_value): In the struct
> 	allocation case, don't poke the slots array directly -- we should
> 	go through struct-ref/struct-set! code so that we get the
> 	permissions and allocation ('u' versus 'p') correct.

This one is of course correct, but I'm concerned about the performance
implications of all this permission checking.  `scm_struct_ref ()' is
not trivial, and it's not inlined, etc.  Maybe some preprocessing could
be made at vtable creation time so that we don't have to reinterpret the
whole layout string at each ref/set?

> * libguile/struct.c (scm_struct_ref, scm_struct_set_x): "Light" structs
> have no hidden words (members of the SCM_STRUCT_DATA(x) array accessed
> with negative indices). In that case, determine the number of fields
> from the length of the struct layout descriptor. (Most GOOPS instances
> are light structs.)

Is there a simple test case that reproduces this?  For instance, are
instances of <class> light structs?

Thanks,
Ludovic.

Re: fixes to goops + light structs + 'u' slots

[Andy Wingo]

Hey Ludovic,

Thanks for poking the patch!

On Thu 10 Apr 2008 23:33, ludo@gnu.org (Ludovic Courtès) writes:

>> From: Andy Wingo <wingo@unquote.local>
>
> You forgot to set your email address.  :-)

Whoops, thanks for the heads-up :)

>> Subject: [PATCH] respect slot allocation, e.g. for <read-only-slot>
>>
>> 	* libguile/goops.c (get_slot_value, set_slot_value): In the struct
>> 	allocation case, don't poke the slots array directly -- we should
>> 	go through struct-ref/struct-set! code so that we get the
>> 	permissions and allocation ('u' versus 'p') correct.
>
> This one is of course correct, but I'm concerned about the performance
> implications of all this permission checking.  `scm_struct_ref ()' is
> not trivial, and it's not inlined, etc.  Maybe some preprocessing could
> be made at vtable creation time so that we don't have to reinterpret the
> whole layout string at each ref/set?

Well, there are opportunities for improvement.

I have heard that in a well-done CLOS implementation, accessors are are
generally faster than slot-ref, because they can perform this class/slot
allocation calculation once, and have the method implementation just
dispatch using that table, compiling down to struct-ref instead of
slot-ref. So working this into accessors and method compilation would be
one way to do this.

That would also avoid the linear search of the class' slot definition
list that happens on every ref/set!.

Another option would be, as you say, storing more information in the
classes. But on the other hand what we currently have strikes a fine
balance, and is extensible via compute-get-n-set. I don't see GOOPS
objects suddenly becoming faster than structs themselves. But perhaps
benchmarking is really the way to go here.

>> * libguile/struct.c (scm_struct_ref, scm_struct_set_x): "Light" structs
>> have no hidden words (members of the SCM_STRUCT_DATA(x) array accessed
>> with negative indices). In that case, determine the number of fields
>> from the length of the struct layout descriptor. (Most GOOPS instances
>> are light structs.)
>
> Is there a simple test case that reproduces this?  For instance, are
> instances of <class> light structs?

Well, with the old code:

(define-class <foo> () (bar #:init-keyword #:bar))
(define x (make <foo> #:bar 1))

In theory, the struct has one slot, 'bar, stored in slot 0 of its
struct.

(struct? x) => #t

But:

(struct-ref x 0) => undefined

Because struct-ref reads `nfields' from one of the hidden words.

For some crazy reason, only instances with 1 or more slots are set to be
"light", so be sure to define a slot when you are testing this. Also,
only pure instances will be light: instances of <class> are classes, so
they will not be light.

To be honest I do not understand the light/non-light thing; to me all
instances should be what is called "light" and all vtables should also
be "light", but hold the necessary information in normal (i.e.,
non-negatively indexed) slots of the struct. It seems the difference
only exists because historically things were one way, and the better way
did not have time to fully supplant the old.

Cheers,

Andy
-- 
http://wingolog.org/

Re: fixes to goops + light structs + 'u' slots

[Ludovic Courtès]

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

Hi Andy,

Andy Wingo <wingo@pobox.com> writes:

> I have heard that in a well-done CLOS implementation, accessors are are
> generally faster than slot-ref, because they can perform this class/slot
> allocation calculation once, and have the method implementation just
> dispatch using that table, compiling down to struct-ref instead of
> slot-ref. So working this into accessors and method compilation would be
> one way to do this.

Yeah.  More pragmatically, things like SRFI-9 accessors could be
implemented as applicable SMOBS:

  http://thread.gmane.org/gmane.lisp.guile.devel/6716

> Another option would be, as you say, storing more information in the
> classes. But on the other hand what we currently have strikes a fine
> balance, and is extensible via compute-get-n-set. I don't see GOOPS
> objects suddenly becoming faster than structs themselves. But perhaps
> benchmarking is really the way to go here.

Yes.  Since `struct-ref' and `struct-set!' are in C, the optimization I
had in mind consists in using a compact C-friendly representation of the
layout string, along with an (additional) integer in struct's "hidden"
fields describing the size of that layout array.

However, the fact that layout strings are used internally is exposed and
built upon, e.g., in `make-record-type', which builds vtables using
`make-struct'.  Maybe that's not something we should worry about in 1.9?

> Well, with the old code:
>
> (define-class <foo> () (bar #:init-keyword #:bar))
> (define x (make <foo> #:bar 1))
>
> In theory, the struct has one slot, 'bar, stored in slot 0 of its
> struct.
>
> (struct? x) => #t
>
> But:
>
> (struct-ref x 0) => undefined
>
> Because struct-ref reads `nfields' from one of the hidden words.

Thanks, I added the attached tests for that.

> For some crazy reason, only instances with 1 or more slots are set to be
> "light", so be sure to define a slot when you are testing this. Also,
> only pure instances will be light: instances of <class> are classes, so
> they will not be light.

Hmm, after applying them, I noticed that they use two slots instead of
one.  But for sure, they yield a segfault when I undo your patch (commit
4650d115020924e8da5547d4c346cbe5cd01029e).  Do you think the segfault is
another problem?

> To be honest I do not understand the light/non-light thing; to me all
> instances should be what is called "light" and all vtables should also
> be "light", but hold the necessary information in normal (i.e.,
> non-negatively indexed) slots of the struct. It seems the difference
> only exists because historically things were one way, and the better way
> did not have time to fully supplant the old.

At first sight, I'd be in favor of having all instances as non-light,
because they have a "size" field that's useful.

Another interesting possibility would be to remove support for tail
arrays, since they don't appear to be used (not even in the test suite),
and they're probably broken, and they make the code more complex.

Thanks,
Ludo'.


[-- Attachment #2: The tests --]
[-- Type: text/x-patch, Size: 2243 bytes --]

diff --git a/test-suite/ChangeLog b/test-suite/ChangeLog
index fa169bc..518e53f 100644
--- a/test-suite/ChangeLog
+++ b/test-suite/ChangeLog
@@ -1,3 +1,10 @@
+2008-04-13  Ludovic Courtès  <ludo@gnu.org>
+
+	* tests/goops.test (defining classes)[interaction with
+	`struct-ref', interaction with `struct-set!']: New test.  Checks
+	the interaction of `struct-ref' with "light structs", fixed on
+	2008-04-10 (commit 4650d115020924e8da5547d4c346cbe5cd01029e).
+
 2008-04-06  Ludovic Courtès  <ludo@gnu.org>
 
 	* standalone/test-asmobs-lib.c, standalone/test-conversion.c,
diff --git a/test-suite/tests/goops.test b/test-suite/tests/goops.test
index 8ed697c..e4c2df9 100644
--- a/test-suite/tests/goops.test
+++ b/test-suite/tests/goops.test
@@ -1,6 +1,6 @@
 ;;;; goops.test --- test suite for GOOPS                      -*- scheme -*-
 ;;;;
-;;;; Copyright (C) 2001,2003,2004, 2006 Free Software Foundation, Inc.
+;;;; Copyright (C) 2001,2003,2004, 2006, 2008 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This program is free software; you can redistribute it and/or modify
 ;;;; it under the terms of the GNU General Public License as published by
@@ -148,7 +148,31 @@
 			  #t)
 			(lambda args
 			  #f)))
-    ))
+
+    (pass-if "interaction with `struct-ref'"
+       (eval '(define-class <class-struct> ()
+                (foo #:init-keyword #:foo)
+                (bar #:init-keyword #:bar))
+             (current-module))
+       (eval '(let ((x (make <class-struct>
+                         #:foo 'hello
+                         #:bar 'world)))
+                (and (struct? x)
+                     (eq? (struct-ref x 0) 'hello)
+                     (eq? (struct-ref x 1) 'world)))
+             (current-module)))
+
+     (pass-if "interaction with `struct-set!'"
+       (eval '(define-class <class-struct-2> ()
+                (foo) (bar))
+             (current-module))
+       (eval '(let ((x (make <class-struct-2>)))
+                (struct-set! x 0 'hello)
+                (struct-set! x 1 'world)
+                (and (struct? x)
+                     (eq? (struct-ref x 0) 'hello)
+                     (eq? (struct-ref x 1) 'world)))
+             (current-module)))))
 
 (with-test-prefix "defining generics"
 

Re: fixes to goops + light structs + 'u' slots

[Mikael Djurfeldt]

2008/4/13, Ludovic Courtès <ludo@gnu.org>:
> > I have heard that in a well-done CLOS implementation, accessors are are
>  > generally faster than slot-ref, because they can perform this class/slot
>  > allocation calculation once, and have the method implementation just
>  > dispatch using that table, compiling down to struct-ref instead of
>  > slot-ref. So working this into accessors and method compilation would be
>  > one way to do this.
>
> Yeah.  More pragmatically, things like SRFI-9 accessors could be
>  implemented as applicable SMOBS:
>
>   http://thread.gmane.org/gmane.lisp.guile.devel/6716

I think it's a good idea to read and understand the code before
planning changes.  At least given the state of Guile at the point when
GOOPS accessors were implemented, the current imlementation was
superior to both a slot-ref-based and a SMOB-based implementation.
This is because dispatching a smob took longer time for the evaluator
than interpreting the current special form-based solution
(@assert-bound-ref).  Note that the current implementation *does*
compile in the slot position into this form and indeed *has* the
property which Andy mentions above.  Also, SMOB-based accessors would
be more heavyweight with regard to class creation and memory
consumption than the current solution.

In any case, *please* always benchmark changes like this against previous code.

Re: fixes to goops + light structs + 'u' slots

[Ludovic Courtès]

Hi,

"Mikael Djurfeldt" <mikael@djurfeldt.com> writes:

> 2008/4/13, Ludovic Courtès <ludo@gnu.org>:

>> Yeah.  More pragmatically, things like SRFI-9 accessors could be
>>  implemented as applicable SMOBS:
>>
>>   http://thread.gmane.org/gmane.lisp.guile.devel/6716
>
> I think it's a good idea to read and understand the code before
> planning changes.  At least given the state of Guile at the point when
> GOOPS accessors were implemented, the current imlementation was
> superior to both a slot-ref-based and a SMOB-based implementation.
> This is because dispatching a smob took longer time for the evaluator
> than interpreting the current special form-based solution
> (@assert-bound-ref).  Note that the current implementation *does*
> compile in the slot position into this form and indeed *has* the
> property which Andy mentions above.  Also, SMOB-based accessors would
> be more heavyweight with regard to class creation and memory
> consumption than the current solution.

Sure.  Note that my remark and the referred thread dealt with struct and
SRFI-9 record accessors, not with GOOPS.

Thanks for your comment,
Ludovic.

Re: fixes to goops + light structs + 'u' slots

[Andy Wingo]

Hi Mikael!

On Sun 13 Apr 2008 21:09, "Mikael Djurfeldt" <mikael@djurfeldt.com> writes:

>> > I have heard that in a well-done CLOS implementation, accessors are are
>> > generally faster than slot-ref
>>
> Note that the current implementation *does*
> compile in the slot position into this form and indeed *has* the
> property which Andy mentions above.

I did not realize this. Thanks for mentioning it. I have shied away from
GOOPS internals in the past, but every time I have a brush with them I
learn something interesting.

What is your perspective regarding <foreign-slot>? I wrote a bit about
what I did recently in Guile-Gnome here:

   http://wingolog.org/archives/2008/04/11/allocate-memory-part-of-n

I define a class with a foreign slot like this:

guile> (use-modules (oop goops))
guile> (define-class <foo> () (bar #:class <foreign-slot>))
guile> (define x (make <foo>))
guile> (slot-set! x 'bar 45)
guile> (slot-set! x 'bar 450000000000000000000)

Backtrace:
In current input:
   6: 0* [slot-set! #<<foo> b7dbaa60> bar {450000000000000000000}]

<unnamed port>:6:1: In procedure slot-set! in expression (slot-set! x (quote bar) ...):
<unnamed port>:6:1: Value out of range 0 to 4294967295: 450000000000000000000
ABORT: (out-of-range)

Is this designed to work? It seems that all is still not right,
@slot-ref (only used in accessors, not in slot-ref) accesses the slot as
a SCM regardless of whether is is a 'u' slot or a 'p' slot. I suppose
that part of the dispatch/compilation needs to be made more
sophisticated.

>  Also, SMOB-based accessors would
> be more heavyweight with regard to class creation and memory
> consumption than the current solution.

Regarding memory consumption. Currently, structs are double-cells: one
word for the vtable, one for the data, one empty, and one for the
"STRUCT_GC_CHAIN", used (please correct me) during GC to ensure that
structs are freed before their vtables.

This seems to be to be a waste of memory in instances, in that they
occupy 4 words when they only need 2. Is it not possible to avoid this?
I have puzzled over this for a number of hours, but have not really come
up with anything that seems workable, given our lazy incremental
sweeping. I suppose another bitvector for structs in the cards would
work; you could run through it at the end of marking, and mark all
structs' vtables.

> In any case, *please* always benchmark changes like this against previous code.

Will work on some benchmarking, I am very interested to see how some of
the method dispatch and compilation code works.

Andy

ps. I'm happy to see you around!
-- 
http://wingolog.org/

Re: fixes to goops + light structs + 'u' slots

[Mikael Djurfeldt]

2008/4/14, Andy Wingo <wingo@pobox.com>:
I> have shied away from GOOPS internals in the past, but every time I have
> a brush with them I learn something interesting.

You're very kind.  It's in large parts not easily readable code.

>  What is your perspective regarding <foreign-slot>? I wrote a bit about
>  what I did recently in Guile-Gnome here:
>
>    http://wingolog.org/archives/2008/04/11/allocate-memory-part-of-n

Sorry for not having time right now to look into this.

>  I define a class with a foreign slot like this:
>
>  guile> (use-modules (oop goops))
>  guile> (define-class <foo> () (bar #:class <foreign-slot>))
>  guile> (define x (make <foo>))
>  guile> (slot-set! x 'bar 45)
>  guile> (slot-set! x 'bar 450000000000000000000)
>
>  Backtrace:
>  In current input:
>    6: 0* [slot-set! #<<foo> b7dbaa60> bar {450000000000000000000}]
>
>  <unnamed port>:6:1: In procedure slot-set! in expression (slot-set! x (quote bar) ...):
>  <unnamed port>:6:1: Value out of range 0 to 4294967295: 450000000000000000000
>  ABORT: (out-of-range)
>
>  Is this designed to work? It seems that all is still not right,
>  @slot-ref (only used in accessors, not in slot-ref) accesses the slot as
>  a SCM regardless of whether is is a 'u' slot or a 'p' slot. I suppose
>  that part of the dispatch/compilation needs to be made more
>  sophisticated.

Right.  That part of the implementation is not finished.  The
compilation of accessors is in goops.scm.

Another part, which is also not finished (or wasn't when I left) is
parts of the metaobject protocol.  I had some specific ideas how the
MOP part should be completed, which I hope I left somewhere in the
workbook repository.

>  >  Also, SMOB-based accessors would
>  > be more heavyweight with regard to class creation and memory
>  > consumption than the current solution.
>
> Regarding memory consumption. Currently, structs are double-cells: one
>  word for the vtable, one for the data, one empty, and one for the
>  "STRUCT_GC_CHAIN", used (please correct me) during GC to ensure that
>  structs are freed before their vtables.
>
>  This seems to be to be a waste of memory in instances, in that they
>  occupy 4 words when they only need 2. Is it not possible to avoid this?
>  I have puzzled over this for a number of hours, but have not really come
>  up with anything that seems workable, given our lazy incremental
>  sweeping. I suppose another bitvector for structs in the cards would
>  work; you could run through it at the end of marking, and mark all
>  structs' vtables.

I wash my hands.  :-)  When I left, structs where two words.  Not that
I don't appreciate the new garbage collector.

>  > In any case, *please* always benchmark changes like this against previous code.
>
> Will work on some benchmarking, I am very interested to see how some of
>  the method dispatch and compilation code works.

Thanks!

>  ps. I'm happy to see you around!

Ahh, I'm sorry that I'm actually only a ghost.  Have to focus on other
stuff right now, but I'm still a Guile user!

Re: fixes to goops + light structs + 'u' slots

[Andy Wingo]

Hi Mikael,

On Tue 15 Apr 2008 00:16, "Mikael Djurfeldt" <mikael@djurfeldt.com> writes:

> 2008/4/14, Andy Wingo <wingo@pobox.com>:
>> Regarding memory consumption. Currently, structs are double-cells: one
>>  word for the vtable, one for the data, one empty, and one for the
>>  "STRUCT_GC_CHAIN", used (please correct me) during GC to ensure that
>>  structs are freed before their vtables.
>
> I wash my hands.  :-)  When I left, structs where two words.

commit 08c880a36746289330f3722522960ea21fe4ddc8
Author: Mikael Djurfeldt <djurfeldt@nada.kth.se>
Date:   Wed Aug 9 18:29:31 2000 +0000

    * struct.c (scm_make_struct, scm_make_vtable_vtable): Structs
    handles are now double cells; Initialize SCM_STRUCT_GC_CHAIN to
    0.
    (scm_struct_gc_init, scm_free_structs): New GC C hooks.
    (scm_struct_prehistory): Install them.

It is natural for our memory to fade over this much time ;-)

But if at any point something sparks in your brain to figure out a way
around the GC chain, I'd certainly be interested. Otherwise we could put
that empty third word to good use.

Andy
-- 
http://wingolog.org/

Re: fixes to goops + light structs + 'u' slots

[Mikael Djurfeldt]

2008/4/19, Andy Wingo <wingo@pobox.com>:
> > I wash my hands.  :-)  When I left, structs where two words.
> commit 08c880a36746289330f3722522960ea21fe4ddc8
>  Author: Mikael Djurfeldt <djurfeldt@nada.kth.se>
>
>  It is natural for our memory to fade over this much time ;-)
>
>  But if at any point something sparks in your brain to figure out a way
>  around the GC chain, I'd certainly be interested. Otherwise we could put
>  that empty third word to good use.

Sigh... Well, apparently I'm the very author of this GC chain thing
you are talking to me about.  The only thing I can say is that there
was probably a reason at the time to do this.  Of course, when looking
at it now, it doesn't look like good design.  In fact, and to be
honest, I've never had the impression that structs are good design
either.  The GC chain mostly looks like a bug fix, doesn't it?  The
problem is that you don't always have time and resources to redesign
the whole thing.  Most changes I've done to Guile have been done for
the use of Guile in my work.  The Right thing to do is probably to
throw out structs and design new GOOPS objects, something I  wanted to
do from the start.  Also, when considering GC-related
things---remember that a lot of design decisions have been made
against the garbage collector as it looked like three revisions ago,
or something like that.  The current GC is a very different beast.

Also, I've seen your GOOPS todo.  It's nice to see your willingness to
continue development on GOOPS.  Unfortunately, I won't have the time
to help.  Just don't be too quick to throw things out.  Some code
needs to be replaced, but then, again, some code has thought behind
it.  Since you say that you couldn't find information in the workbook,
I'll try to dig up my ideas regarding the PURE_GENERIC flag and the
apply-generic MOP.  The extended-generic stuff is a way to get
insulation between two modules A and B which both import a module C.
Otherwise I think I should shut up.

Oh, and please *don't* try to compile methods at macro expansion time.
 GOOPS method compilation is based on the crazy idea to have one
compiled method per combination of argument types.  The idea is that
this won't give you a combinatorial explosion if you compile lazily,
waiting until the first application for a certain combination.   This
was an experiment.  Apparently, it works well enough in practise.  The
*big* possibility is that since each compiled method in the method
cache has typed arguments, there is the opportunity for *very*
interesting optimizations, such as elimination of a lot of the type
dispatch (e.g. that in accessors and in other calls to generics).

Re: fixes to goops + light structs + 'u' slots

[Andy Wingo]

Hi,

On Sun 20 Apr 2008 01:07, "Mikael Djurfeldt" <mikael@djurfeldt.com> writes:

> Well, apparently I'm the very author of this GC chain thing you are
> talking to me about. The only thing I can say is that there was
> probably a reason at the time to do this.

The reason is simple: you need a class to know how to free an instance.
If a class and all of its remaining instances are all unmarked and thus
sweepable, we need to sweep instances before classes.

The particular strategy for dealing with this (i.e. GC chains) is ugly,
however.

> In fact, and to be honest, I've never had the impression that structs
> are good design either.

They're not in many aspects (heavy/light/entity representations, tail
args), but the fundamentals are OK I think: word-vectors that are
interpreted by other word-vectors (denoted "vtables").

> The Right thing to do is probably to throw out structs and design new
> GOOPS objects, something I wanted to do from the start.

You might be right; OTOH we could just weed out those anachronistic
parts of structs, and orthogonalize the struct interface, making structs
contain the minimum necessary to support GOOPS objects and SRFI-9
records.

> Also, I've seen your GOOPS todo.  It's nice to see your willingness to
> continue development on GOOPS.  Unfortunately, I won't have the time
> to help.  Just don't be too quick to throw things out.

ACK. I'll be mailing patches to the list, so if you have time, take a
look and carp if things aren't going well.

> Otherwise I think I should shut up.

If it's for your own reasons, fine, but your insights are appreciated.

> Oh, and please *don't* try to compile methods at macro expansion time.
>  GOOPS method compilation is based on the crazy idea to have one
> compiled method per combination of argument types.  The idea is that
> this won't give you a combinatorial explosion if you compile lazily,
> waiting until the first application for a certain combination.   This
> was an experiment.  Apparently, it works well enough in practise.  The
> *big* possibility is that since each compiled method in the method
> cache has typed arguments, there is the opportunity for *very*
> interesting optimizations, such as elimination of a lot of the type
> dispatch (e.g. that in accessors and in other calls to generics).

Interesting!

Well, off to hack, or something. Thanks for the note.

Andy
-- 
http://wingolog.org/

Re: fixes to goops + light structs + 'u' slots

[Mikael Djurfeldt]

2008/4/14, Andy Wingo <wingo@pobox.com>:
>  Is this designed to work? It seems that all is still not right,
>  @slot-ref (only used in accessors, not in slot-ref) accesses the slot as
>  a SCM

Right, the special form is @slot-ref, not @assert-bound-ref as I
stated previously.

Re: fixes to goops + light structs + 'u' slots

[Andy Wingo]

On Sun 13 Apr 2008 21:09, "Mikael Djurfeldt" <mikael@djurfeldt.com> writes:

> At least given the state of Guile at the point when
> GOOPS accessors were implemented, the current imlementation was
> superior to both a slot-ref-based and a SMOB-based implementation.
[...]
> In any case, *please* always benchmark changes like this against previous code.

So, I was curious about this. I made four very simple tests:

accessor set:  (set! (bar instance) 3)
accessor ref:  (bar instance)
slot-set!:     (slot-set! instance 'bar 3)
slot-ref:      (slot-ref instance 'bar)

Where instance is defined as:
(define-class <cls> () (bar #:accessor bar))
(define instance (make <cls>))

I then ran the tests in that order in the order given, within this loop:

    (time (do ((i 1 (1+ i))) ((> i 1000000))  ... ))

That's the time macro from (ice-9 time).

These are my results on my Core2 Duo laptop, fixed to 800 mhz, against
the latest guile 1.8 and 1.6. The "baseline" is the do loop without any
test at all:

                   user time
               1.6          1.8
--------------+----------------------
baseline      |1.67s        1.97s
accessor set  |2.66s        3.90s
accessor ref  |2.61s        3.79s
slot-set!     |2.58s        3.80s
slot-ref      |2.23s        3.15s

I then ran accessor ref tests on objects that necessarily had their slots
bound, and thus would go through @assert-bound-ref:

(define-class <cls2> () (baz #:accessor baz #:init-value 3))
(define instance2 (make <cls2>))

                   user time
               1.6          1.8
--------------+----------------------
accessor ref  |2.22s        3.00s

Note the scale is that a single ref translates to 1 or 2 microseconds,
and this with the clock scaled back significantly. By way of comparison,
a 1000000-time ref loop in python took 0.52s, a set loop took 0.59s, and
a noop loop took 0.18s.

Conclusions:

  (1) 1.8 is slower than 1.6. (We knew that.)
  (2) The speed ordering is the same on 1.8 and 1.6:
      slot-ref < accessor ref
      slot-set! < accessor set
  (3) If the determination can be made that the slot will never be
      unbound, and we compile to the @assert-bound-ref case, then
      accessor refs are indeed faster than slot-ref.
  (4) In this particular test, guile is slower than python

I would speculate, Mikael, that it is case (3) that you are recalling.

Andy
-- 
http://wingolog.org/

Re: fixes to goops + light structs + 'u' slots

[Mikael Djurfeldt]

2008/4/16, Andy Wingo <wingo@pobox.com>:
> On Sun 13 Apr 2008 21:09, "Mikael Djurfeldt" <mikael@djurfeldt.com> writes:
>  I then ran accessor ref tests on objects that necessarily had their slots
>  bound, and thus would go through @assert-bound-ref:
>
>   (3) If the determination can be made that the slot will never be
>       unbound, and we compile to the @assert-bound-ref case, then
>       accessor refs are indeed faster than slot-ref.
> [...]
>  I would speculate, Mikael, that it is case (3) that you are recalling.

Right, although it is, in fact, @slot-ref which is the special form (I
said "@assert-bound-ref" by mistake).  Try the same benchmark
accessing the third or fourth slot.

Regarding Guile vs Python: Python is byte-compiled.  We once had a
byte-compiler "guile-vm" which could have been merged into Guile.
Maybe it's still possible with some work.

Best regards,
Mikael

Re: fixes to goops + light structs + 'u' slots

[Mikael Djurfeldt]

2008/4/16, Mikael Djurfeldt <mikael@djurfeldt.com>:
> 2008/4/16, Andy Wingo <wingo@pobox.com>:
>
> > On Sun 13 Apr 2008 21:09, "Mikael Djurfeldt" <mikael@djurfeldt.com> writes:
>
> >  I then ran accessor ref tests on objects that necessarily had their slots
>  >  bound, and thus would go through @assert-bound-ref:
>  >
>
> >   (3) If the determination can be made that the slot will never be
>  >       unbound, and we compile to the @assert-bound-ref case, then
>  >       accessor refs are indeed faster than slot-ref.
>
> > [...]
>
> >  I would speculate, Mikael, that it is case (3) that you are recalling.
>
>
> Right, although it is, in fact, @slot-ref which is the special form (I
>  said "@assert-bound-ref" by mistake).  Try the same benchmark
>  accessing the third or fourth slot.

To be more clear: It is only for bound slots that the accessor can be
compiled down to the special form @slot-ref which was significantly
faster than other tested access methods, using that version of Guile
on the type hardware which was available then, at the time goops was
developed.  It is new to me to see such small differences in timing.
Maybe it is also worth rolling up the loop a bit so that you have a
sequence of accesses in the loop body?