[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Quite sneaky bug in SRFI-9
From: |
Ludovic Courtès |
Subject: |
Re: Quite sneaky bug in SRFI-9 |
Date: |
Fri, 11 Mar 2011 14:55:26 +0100 |
User-agent: |
Gnus/5.110013 (No Gnus v0.13) Emacs/23.3 (gnu/linux) |
Hi Andreas,
Andreas Rottmann <address@hidden> writes:
> Hi! After being puzzled for some time, I found the root cause for an
> issue with my code was actually a quite serious bug in SRFI-9's
> define-inlinable' (here we go again -- but this time it's an actual
> bug, I promise ;-)): The current implementation of `define-inlinable'
> duplicates actual parameters passed to the macros it generates, for
> example (note the `(find-next-solution! s 10000)' expression):
> scheme@(guile-user)> (tree-il->scheme (macroexpand '(solution?
> (find-next-solution! s 10000))))
> $6 = (if ((@@ (srfi srfi-9) struct?) (find-next-solution! s 10000))
> ((@@ (srfi srfi-9) eq?) ((@@ (srfi srfi-9)
> struct-vtable) (find-next-solution! s 10000))
> (@@ (dorodango solver) solution))
> #f)
>
Ouch, good catch!
> - It's certainly not as efficient as the current, broken implementation.
> Ideally, the expansion would be a `let' block instead of the
> `call-with-values' invocation.
Yes, that would be nice. This patch seems to do the trick:
diff --git a/module/srfi/srfi-9.scm b/module/srfi/srfi-9.scm
index fad570b..54bc8a8 100644
--- a/module/srfi/srfi-9.scm
+++ b/module/srfi/srfi-9.scm
@@ -64,6 +64,12 @@
(cond-expand-provide (current-module) '(srfi-9))
+(define-syntax letify
+ (syntax-rules ()
+ ((_ (var ...) (val ...) body ...)
+ (let ((var val) ...)
+ body ...))))
+
(define-syntax define-inlinable
;; Define a macro and a procedure such that direct calls are inlined, via
;; the macro expansion, whereas references in non-call contexts refer to
@@ -80,15 +86,17 @@
(syntax-case x ()
((_ (name formals ...) body ...)
(identifier? #'name)
- (with-syntax ((proc-name (make-procedure-name #'name)))
+ (with-syntax ((proc-name (make-procedure-name #'name))
+ ((args ...) (generate-temporaries #'(formals ...))))
#`(begin
(define (proc-name formals ...)
body ...)
(define-syntax name
(lambda (x)
(syntax-case x ()
- ((_ formals ...)
- #'(begin body ...))
+ ((_ args ...)
+ #'(letify (formals ...) (args ...)
+ (begin body ...)))
(_
(identifier? x)
#'proc-name))))))))))
I can’t think of any way to do it more elegantly.
> Has anyone done benchmarks of the benefits of the current (broken)
> SRFI-9 accessors and predicates vs. regular procedures on real code?
> Given the argument duplication issue, it might be the case that
> `define-inlinable' actually slowed things down :-).
I benchmarked it indirectly via ‘vlist-cons’ et al. in ‘vlists.bm’.
The bonus is that there are special opcodes for ‘struct-vtable’,
‘make-struct’, and ‘struct-ref’. So when you inline constructors and
accessors, you also get to use directly those opcodes, which makes quite
a difference.
I would like to keep this property. Not doing this gives an incentive
to using lists everywhere since ‘car’, ‘pair?’, etc. are open-coded.
Thanks!
Ludo’.