-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize optional and keyword arguments (calls to procedures using these become twice as fast) #465
base: master
Are you sure you want to change the base?
Conversation
PS: a simple grep shows that more than 60 lines of code in STklos use optional and keyword arguments, and I suppose that for some of them would the speedup would be really nice. For example (selected output from grep), (define (delete x l :optional (comp equal?))
(define (delete! x l :optional (comp equal?))
(define (hash-table-update! hash key func :optional (thunk #f thunk?))
(define (hash-table-get ht key :optional (default #f default?))
(define (member x y :optional (compar %equiv?)) (member-simple x y compar))
(define (assoc x y :optional (compar %equiv?)) (assoc-simple x y compar))
(define (mutex-lock! mtx :optional timeout (thread (current-thread)))
(define (mutex-unlock! mtx :optional condv timeout) |
For reference, this was my idea for (define (rewrite-params-and-body method? formals body)
(let* ((params (parse-parameter-list method? formals))
(req (car params))
(opt (or (cadr params) '()))
(key (or (caddr params) '()))
(rest (cadddr params)))
(let* ((opt-names (map car opt))
(opt-tests (filter symbol? (map caddr opt)))
(key-names (map make-keyword (map car key)))
(key-tests (filter symbol? (map caddr key))))
(values (if (or opt key)
;; We have a :optional or a :key keyword
(let ((new-formals(append req
opt-names
opt-tests
key-names
key-tests
(if rest (list rest) '()))))
(cons new-formals body))
;; "Normal" lambda
(begin (if rest
(if (null? req)
(set! req rest)
(set-cdr! (last-pair req) rest)))
`(,req ,@body)))
opt key)))) But it needs to return |
Hi @jrpellegrini, This work is really interesting. I have already thought to a similar thing before (it is quite similar to the way Bigloo implement DSSSL keywords). However, I gave up since I didn't find a good way to use it when you are in a multi files context. When you want to call About macros, the lambda used can be an extended lambda, so it should not be a problem: stklos> (define-macro (foo a b :optional x) `(list ',a ',b ',x))
;; foo
stklos> (foo x y)
(x y #f)
stklos> (foo x y z)
(x y z) |
I'm not sure I understand -- do you mean when I create a procedure ;; file A.stk
(define (g a b :key (e 100) (f 200)) (list a b e f)) ;; file B.stk
(include "A.stk")
(display (g 1 2 :f 10)) If (The code I was working on would keep the defaults inside the closure object, so the VM only needs to check what's missing, and reorder the keyword arguments.) |
As to the compiled file, I suppose it would work if the compiler issues the |
So, since the closure object knows its optional and keyword arguments, no bookkeeping is necessary, I suppose. (no?) |
In fact, I was thinking of something like ;; file A.stk
(define (g a b :key (e 100) (f 200)) (list a b e f)) and ;; file B.stk
(require "A")
(display (g 1 2 :f 10)) Id Furthermore, we'll need some kind of static analysis even if everything is a single file: (define (f a b) (list a b))
(define (g a b :key (e 100) (f 200)) (list a b e f))
(set! f g)
(f 1 2) ;; Here we must compile a call such as (f 1 2 100 200) And static analysis can be not sufficient, since the value of (when (foo) (set! f g))
(f 1 2) ;; do we call original f or original g? In fact, I think that the thing you try to do can be done at compile time using macros. For the example you gave before we can implement: (define x 0)
(define y 0)
(define-macro (G a b
:optional (c 10 c?) (d 20)
:key (e -1 e?) (f -2))
(define (%real-G a b ;; mandatory
c d ;; optionals
c? ;; tests for optionals
e f ;; keyword arguments
e? ;; tests for keywords
r) ;; rest list
(set! x (+ x c f))
(set! y (+ y e d)))
`(,%real-G ,a ,b ,c ,d ,c? ,e ,f ,e? '()))
(time (repeat 10_000_000
(G 0 0 2 :e 1 :f -2)))
(print (list x y)) Here the call that must be generated is determined at compile time (and it can be in another file as macros) So, a possible way to implement faster extended functions could be (perhaps) to use a new type of object which is macro expanded when used in a call and compiled as actually if passed as a parameter or when we cannot prove that that we can do an efficient call in sequences such as It is a bit complicated, and we would need two implementations for the extended function (one normal lambda and one which correspond to the actual extended lambda). I need to think more about it, but it should work. I'm not sure to have been clear. |
Oh, but we don't :) If the procedure was compiled, then the compiler generated something like
If it was not compiled, then when the source is evaluated the optionals and keywords will be read... Your example: (when (foo) (set! f g))
(f 1 2) ;; do we call original f or original g? would still work, because This is the change to struct closure_obj {
stk_header header;
int16_t arity;
uint16_t code_size;
SCM formals;
SCM opt_args; /* a list of default values for optional arguments */ <= here
SCM key_args; /* an assoc list; keyword -> default */ <= and here
SCM env;
... (etc)
};
The VM (in This is what my implementation currently does:
I think I didn't understand yet the problem. About the macro implementation,
By storing the arguments and defaults in the closure, we can pass it along freely, and the VM will always be able to decode it. I thought about using macros, but then thought that maybe with procedures it would be easier, because the |
I don't understand how you can compile the call of (define (foo)
(bar 1 2))
(define (bar x y :optional z :key (a 100) (b 200))
(list x y z a b r))
No problem for the VM since at runtime you have the closure. At compile time, the closure may not exist. |
I thought of working as if |
07c9289
to
c9fa01f
Compare
I think I got it almost working. There is a sequence of six commits in this PR. The last one is a temporary proof-of-concept change to the compiler, which introduces a new special form You can define a ;; A.stk
(define f
(Lambda (a :optional (b 10 b?) :rest r :key (c 20 c?))
(list a '-- b b? '-- c c? '-- r))) Compile ;; B.stk
(load "A.ostk")
(define g f)
(format #t "result: ~w~%" (g -1 :c -2)) If you load This is because the
But I noticed that there is some small glitch somewhere (the test value is being used instead of the arg value in one specific situation). Still working on this |
c9fa01f
to
bf934dc
Compare
It seems to work properly. But if I turn this |
I've resolved the conflicts for merging, but I'll have to check again the working of this. Keeping information (including keyword and optional args) in the plist is great! Anyway -- when the procedure is created, these CLOSURE_OPT_ARGS(z) = STk_nil;
CLOSURE_KEY_ARGS(z) = STk_nil; were done in the C code. I've just removed them, and this is a reminder for myself that the equivalent using plists needs to be put there. |
3558964
to
95d8393
Compare
I adapted the code to using plists. So... This is the state of this PR:
|
95d8393
to
20a1a95
Compare
I think the only problem with this PR now is that I don't know how to bootstrap it... Everything works, but generating the boot file crashes. |
20a1a95
to
dd90d04
Compare
@egallesio -- is there some special trick to bootstrap STkos when you change the bytecode in a fundamental way, like this? (It changes the way procedures with optionals & keywords work). I thought of going a radical route and try to temporarily change all opts&keys from the core STklos code, then generate the boot images, and then change back the procedures. Is there another way? |
Not really; You need to rewrite some code using the new feature, test, peak some other code and use the new feature, test and so on. When you have all base code converted, you're' done (work by small steps, otherwise it'll break, and you'll don't know why). For now, I have seen that
(define f1
(Lambda (a :optional (b 10 b?) :rest r :key (c 20 c?))
(list a '-- b b? '-- c c? '-- r)))
(define f2
(lambda (a :optional (b 10 b?) :rest r :key (c 20 c?))
(list a '-- b b? '-- c c? '-- r)))
stklos> (f1 1 2)
(1 -- 2 #t -- 20 #f -- (2))
stklos> (f2 1 2)
(1 -- 2 #t -- 20 #f -- ())
stklos> Anyway, it seems promising (even if I don't see improvements as big as you). In any case, I will not try to integrate this PR for this release, since it changes too many things. |
Of course, it's a big change, I wouldn't expect it to be included that soon. Ok, I'll check the problems that you have described! |
If you have time, and want to, it would be great. |
==================================================== = = I. Closures now remember optional and keyword args = ==================================================== - The procedure structure holds two more items in its plist: opt_args and key_args. These are assoc lists for the optional and keyword arguments. Examples: ( (a 10 a-given?) (b 20 #f) ) ;; for optionals ( (:x -2 x-given?) (:y -3 #f) ) ;; for keywords - Include a new VM instruction, CLOSURE_SET_OPT_KEY, with two arguments. We set both optionals nad keywords with the same VM instruction, so as to not waste one new instruction on this -- both would be called at the same point (closure creation) anyway. - Also add %procedure-optionals and %procedure-keys. ======================================================= = = II. Add %%set-procedure-optionals-and-keys and others = ======================================================= 1) Temporarily add a procedure to set optional and keyword arguments of procedures. This is used for debugging, and may be deleted later. Note: the format for both optionals and keywords is ( (name value test) ...) ALL the three must be specified. 2) Add %%set-procedure-arity! 3) %procedure-arity also returns # of optionals and key args stklos> (%procedure-keys g) ((#:e -1 e?)) stklos> (%procedure-optionals g) ((c 10 c?) (d 20)) stklos> (%procedure-arity g) -3 2 1 ======================================= = = 3. VM: process optionals and keywords = ======================================= The adjust_arity function now processes optional and keyword arguments: If arity < 0, and the mandatory arguments were passed, then 1. The list of optionals in the procedure is processed sequentially, and the user-supplied values are pushed onto the stack. 2. If any optional arguments are still missing, their defaults are taken from the procedure definition and pushed. 3. The list of optionals is again processed sequentially. This time, we check for each argument wether the user wants a test for that argument. If the user wanted a test and the argument was passed, we push #t. If the user wanted a test and the value was not passed, we push #f 4. If the closure had optionals but no keywords, *and* there were more arguments passed, we signal an error. 5. A pointer ptr is on the current arg list position. We process the keyword list sequentially, calling STk_key_get on ptr to push either the user supplied or the default value. 6. We again run the keyword list in order to push the test values for keyword arguments. So if g is defined as (define (g a b :optionals (c 10 c?) (d 20) :rest r :key (e -1 e?) (f -2 #f))) (list a b c c? d e e? f r)) The stack will be used *as if* g was defined like (define g (a b c d c? e f e? r) (list a b c c? d e e? f r)) Note that the tests c? and e? are not adjacent to their argument (but this is not visible to the user). Also, the keyword list is kept in the closure as an assoc list already using the keywords. That is, it is not ( (e -1 e?) (f -2 #f) ) but rather ( (:e -1 e?) (:f -2 #f) ). The compiler will take care of this. We use the debugging procedures %%set-procedure-arity! and %%set-procedure-optionals-and-keys! to experiment with this without disrupting the behavior of the rest of STklos: stklos> (define (g a b c d c? e f e? r) (list a b c c? d e e? f r)) ;; g stklos> (%%set-procedure-arity! g -3) stklos> (%%set-procedure-optionals-and-keys! g '( (c 10 c?) (d 20 #f) ) '( (:e -1 e?) (:f -2 #f) ) ) stklos> (g 1 2) (1 2 10 #f 20 -1 #f -2 ()) stklos> (g 1 2 3) (1 2 3 #t 20 -1 #f -2 (3)) stklos> (g 1 2 3 4) (1 2 3 #t 4 -1 #f -2 (3 4)) stklos> (g 1 2 3 4 :e 5) (1 2 3 #t 4 5 #t -2 (3 4 #:e 5)) stklos> (g 1 2 3 4 :f 6) (1 2 3 #t 4 -1 #f 6 (3 4 #:f 6)) stklos> (g 1 2 3 4 :f 6 :e 5) (1 2 3 #t 4 5 #t 6 (3 4 #:f 6 #:e 5))
* Add alternative versions of: - compile-user-lambda - rewrite-params-and-body - extended-lambda->lambda - compile-lambda These versions have the Lambda spelled with a capital "L", except for "rewrite-params-and-body", whose alternative version is (temporarily) x:rewrite-params-and-body. The way these new procedures work is this: instead of just passing around the parameter list, they will pass four values: . the parameter list . the optional list . the keyword list . the arity - Add a Lambda special form (capital L), which generates code for the new optional and keyword parameter code. - Document parse-parameter-list and compute-arity a bit more. There are NO functional changes to standard STklos. The changes are only visible when one uses the new Lambda form: (define f (Lambda (a :optional (b 10 b?) :rest r :key c d) (list a b c d r)))
:rest should hold the parameters *after* the optional parameters, not including them... Fixes this: ``` (define f1 (Lambda (a :optional (b 10 b?) :rest r :key (c 20 c?)) (list a '-- b b? '-- c c? '-- r))) (define f2 (lambda (a :optional (b 10 b?) :rest r :key (c 20 c?)) (list a '-- b b? '-- c c? '-- r))) stklos> (f1 1 2) (1 -- 2 #t -- 20 #f -- (2)) stklos> (f2 1 2) (1 -- 2 #t -- 20 #f -- ()) ```
dd90d04
to
5b01c55
Compare
I believe this one is fixed... Will work on the others later. |
He he. These two are related. An |
It is now similar to compile-user-lambda, wxcept that it treats :optional and :key differently.
I believe I've fixed all those problems. I'll see if I can make STklos bootstrap with the changes later this week. |
(%procedure-name (lambda () "y" :doc 1)) => "doc" ;; should have been "y" |
A rest list was being appended even when the parameter list had no opts, keys and was proper.
e68429a
to
1e37b08
Compare
Hi @egallesio !
What
While looking at the code generated for optional and keyword arguments, I thought that perhaps they could be optimzied. I did a small experiment, and it's close to actually becoming ready. These are the commits I have made, in order:
%%set-procedure-optionals-and-keys!
so we can test these procedures that know their optionals and keyword arguments. This is a temporary hack only to build the procedures with the new optionals and keyword fields.%%set-procedure-arity!
, to also change the arity of procedures. This is a temporary hack only to build the procedures with the correct arity.adjust_arity
becomesadjust_stack_get_arity
, because it really reorganizes the stack, putting the arguments in proper order. For the usually-created lambdas, no changes. But if the closure being called has akey
oropt
field different fromSTk_nil
(and that only happens if we explicitly use the procedures in steps 2 and 3), then those fields are used. I have tried to thoroughly document this in comments in the VM code.%procedure-arity
so that it returns three values: arity as usual; number of optional arguments; number of keyword arguments (this isn't strictly necessary, but I thought it'd be nice.Why?
The benefits are speed (it's almost twice as fast to use the new in-VM version of opts-and-keys parsing); and also convenience :) because then we'd be able to ask
%procedure-optionals
...How do I build a procedure that uses the new parsing?
The last step would be to change the compiler, but that seems harder than what I thought, so I decided to show what I've done so far.
and so on.
Rudimentary benchmarks
I have tried some very rudimentary timings:
Do we keep trying? :)
Do you think it would be worth changing the compiler in order to take advantage of this? I came up with a new version of
rewrite-params-and-body
but it didn't work properly.