On Wed, Jul 23, 2025 at 04:32:22PM +0200, Peter Bex via Chicken-hackers wrote:
> The benchmark suite seems mostly unaffected (see the attached file
> benchmark-comparison.txt), but it improves a few programs' performance.
> Notably kernwyk-cat.  It's a cool example because it's a simple
> program which illustrates the concept well:
> 
> (define (catport in out)
>   (let ((x (read-char in)))
>     (if (not (eof-object? x))
>         (begin
>          (write-char x out)   ;; closure container
>          (catport in out))))) ;; closure user

As I was lying awake in bed at night, this example crossed my mind
again.  It occurred to me that this is actually a "safe" example, where
the closure does not even get mutated.  In such cases, we can reuse
closures without worry of observing such mutations, so it's even safe
to have the users or containers escape or allow re-use when containers
contain multiple users.

Implementing this was mostly straightforward: I added an additional pass
after merging the tricky sharing closures.  See the attached patch.
It works fine, but unfortunately it seems to have no effect on the
benchmarks at all.

I think it might be wortwhile to apply anyway, considering the benchmark
that *did* get affected by the original patch would have been fine with
the "safe" version too.

I've added this to the shared-closures branch (which I've also rebased
onto master).

Cheers,
Peter
>From d58fdb2e91ff921afb64cb09aea9377d0144ef58 Mon Sep 17 00:00:00 2001
From: Peter Bex <pe...@more-magic.net>
Date: Mon, 28 Jul 2025 14:54:45 +0200
Subject: [PATCH] Allow safe reuse of way more closures

The original problem was where growing nested closures would be copied
over and over and over, so a lot of care needed to be taken in order
to know if the closure could be shared or not to avoid problems where
the mutations could be observed.

But closures for where there is no mutation at all it's much simpler
because they're immutable.  This happens in cases where the nested
closures simply happen to close over exactly the same variables as
their containing closure.  This means we can re-use such closures
where one closure creates multiple other closures, and where the
closures escape.
---
 core.scm | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/core.scm b/core.scm
index b78483de..9ef88f84 100644
--- a/core.scm
+++ b/core.scm
@@ -2395,6 +2395,12 @@
                       (= 1 nreferences) )
               (quick-put! plist 'collapsable #t) ) ) )
 
+         (and-let* ((val (or local-value value))
+                    ((eq? '##core#lambda (node-class val)))
+                   ((rassoc sym callback-names eq?)))
+           (let ((lparams (node-parameters val)))
+             (db-put! db (first lparams) 'callback #t)))
+
         ;; If it has a known value that is a procedure, and if the number of 
call-sites is equal to the
         ;;  number of references (does not escape), then make all formal 
parameters 'unused which are
         ;;  never referenced or assigned (if no rest parameter exist):
@@ -2558,6 +2564,12 @@
                   lss)
         (remove (lambda (x) (hash-table-ref seen x)) ls)))
 
+    (define (symbolset= ls . lss)
+      (every (lambda (lst)
+               (and (null? (symbolset-difference ls lst))
+                    (null? (symbolset-difference lst ls))))
+             lss))
+
     (define (test sym item) (db-get db sym item))
 
     (define (register-customizable! var id)
@@ -2736,6 +2748,72 @@
          (else (concatenate (map (lambda (n) (merge-shareable n 
shared-closure)) subs)) ) ) ))
 
 
+    ;; Merge "reusable" closures.  The "shareable" closures above
+    ;; require great care to be taken because variables are mutated
+    ;; into the container closure by the user closure where they're
+    ;; introduced, which should not be observable.  However, closures
+    ;; where the full set of variables is already known may freely be
+    ;; reused any number of times because they're immutable.
+    ;; NOTE: Unboxing of non-escaping vars is not implemented yet.
+    (define (merge-reusable n reusable-closure)
+      (let ((subs (node-subexpressions n))
+           (params (node-parameters n)) )
+       (case (node-class n)
+
+         ((quote ##core#undefined ##core#provide ##core#proc ##core#primitive)
+          #f)
+
+         ((##core#lambda ##core#direct_lambda)
+          (##sys#decompose-lambda-list
+           (third params)
+           (lambda (vars argc rest)
+             (let* ((id (first params))
+                     (this-closure (or (test id 'shared-closure) (test id 
'captured-variables) '()))
+                     (sharing-mode (test id 'sharing-mode))
+                     ;; Callbacks may not be reused (see TODO in 
analyze-expression)
+                     (is-callback? (test id '##compiler#callback-lambda))
+                     ;; We don't want existing containers or users' shared 
closures to be reused by contained closures.
+                     ;; However, we do allow containers to be moved "up front" 
if there are other closures that
+                     ;; share the same variables (which means there are no 
mutating assignments to closed-over vars).
+                     (container-needed? (merge-reusable (first subs) (and (not 
sharing-mode)
+                                                                          (not 
is-callback?)
+                                                                          
this-closure))))
+                (cond ((and reusable-closure
+                            (not is-callback?)
+                            ;; The closure must match exactly with the 
reusable container
+                            ;; Note that if the closure is already a 
container, we compare
+                            ;; against its shared closure.  This is safe to do 
because
+                            ;; if they're the same, no variables are updated 
via mutation.
+                            (symbolset= reusable-closure this-closure)
+                            ;; Minimum shared closure size - don't want to 
share a single var, it's extra indirection
+                            (> (length this-closure) 1))
+                       ;; Reset captured vars.  This closure only captures the 
container
+                       (db-put! db id 'closure-size 1)
+                       (db-put! db id 'captured-variables '())
+                       (db-put! db id 'sharing-mode 'user)
+                       (set! sharing-users (add1 sharing-users))
+                       ;; If this closure was already marked as a container by 
merge-reusable-closures,
+                       ;; we turn it into a user.  Adjust counter to reflect 
this.
+                       (when (eq? sharing-mode 'container)
+                         (set! sharing-containers (sub1 sharing-containers)))
+                       ;; We'd like a container to be created in the creating 
closure
+                       #t)
+
+                      ;; If this closure cannot be turned into a sharing user, 
we might need to
+                      ;; turn it into a container if any of its subclosures 
were turned into a user.
+                      (container-needed?
+                       (db-put! db id 'sharing-mode 'container)
+                       (db-put! db id 'shared-closure this-closure)
+                       (set! sharing-containers (add1 sharing-containers))
+                       ;; Its parent does not become a container
+                       #f)
+
+                      ;; No reuse :'(
+                      (else #f))))))
+
+         (else (any (lambda (n) (merge-reusable n reusable-closure)) subs) ) ) 
))
+
+
     ;; Create explicit closures:
     (define (transform n crefvar closure)
       (let ((subs (node-subexpressions n))
@@ -2986,6 +3064,7 @@
       (debugging 'o "customizable procedures" customizable))
     (debugging 'p "closure conversion merging of shareables phase...")
     (merge-shareable node #f)
+    (merge-reusable node #f)
     (unless (and (zero? sharing-containers)
                  (zero? sharing-users)) ;; Users should always be zero if 
containers is (but paranoia prevails, helps w/ debugging)
       (debugging 'o "shared closure containers" sharing-containers)
-- 
2.49.0

Reply via email to