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

[ludo@gnu.org (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"