Re: [PATCH v2] Add atomic-box-update! function to (ice-9 atomic)

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3325 bytes --]



Op 22-08-2023 om 12:59 schreef Andrew Tropin:
> 
> * module/ice-9/atomic.scm (atomic-box-update!): New variable.
> ---
> Changes since v1. Use single-argument proc to avoid potential perfomance
> problems cause of call to apply.
> 
>   module/ice-9/atomic.scm | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/module/ice-9/atomic.scm b/module/ice-9/atomic.scm
> index 2a8af901d..6bfa2e8ee 100644
> --- a/module/ice-9/atomic.scm
> +++ b/module/ice-9/atomic.scm
> @@ -25,7 +25,8 @@
>               atomic-box-ref
>               atomic-box-set!
>               atomic-box-swap!
> -            atomic-box-compare-and-swap!))
> +            atomic-box-compare-and-swap!
> +            atomic-box-update!))
>   
>   (eval-when (expand load eval)
>     (load-extension (string-append "libguile-" (effective-version))
> @@ -36,3 +37,14 @@
>     (add-interesting-primitive! 'atomic-box-set!)
>     (add-interesting-primitive! 'atomic-box-swap!)
>     (add-interesting-primitive! 'atomic-box-compare-and-swap!))
> +
> +(define (atomic-box-update! box proc)
> +  "Atomically updates value of BOX to (PROC BOX-VALUE), returns new
> +value.

* , returns new value -> and returns the new value

While descriptive works, for consistency with other atomics 
documentation, this imperative procedure needs to be documented in the 
imperative mood:

Atomically update the value of BOX to (PROC BOX-VALUE) and return the 
new value.

Alternatively, you can adjust the other documentation of atomics to the 
indicative mood and preserve the original docstring (except for the 
grammar correction mentioned in the beginning).

   PROC may be called multiple times, and thus PROC should be
> +free of side effects." > +  (let loop ()
> +    (let* ((old-value (atomic-box-ref box))
> +           (new-value (proc old-value)))
> +      (if (eq? old-value (atomic-box-compare-and-swap! box old-value new-value))
> +          new-value
> +          (loop)))))

This can be optimised, by using the return value of CAS as the new old 
value instead of calling atomic-box-ref again:

(let loop ((old-value (atomic-box-ref box)))
   (let* ((new-value (proc new-value))
          (new-old-value (atomic-box-compare-and-swap! box old-value 
new-value)))
     (if (eq? new-old-value old-value)
         new-value
         (loop new-old-value))))

Maybe there is some concurrency weirdness that can cause slower 
iterations to reduce the number of iterations (*); I'm assuming this 
isn't the case here.  But if it is, in fact, the case here, and the goal 
is to exploit this effect, I would think it's better to explicitly 
implement the back-off.

(I haven't benchmarked any of this, I'm purely going by the number of 
operations.)

(*) E.g., from Wikipedia: 
https://en.wikipedia.org/w/index.php?title=Compare-and-swap&oldid=1141385700

[...] Instead of immediately retrying after a CAS operation fails, 
researchers have found that total system performance can be improved in 
multiprocessor systems—where many threads constantly update some 
particular shared variable—if threads that see their CAS fail use 
exponential backoff—in other words, wait a little before retrying the 
CAS.[4]

Best regards,
Maxime Devos.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]