Clojure

Create volatile box for managing state

Details

  • Type: Enhancement Enhancement
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Motivation:

Clojure needs a faster variant of Atom for managing state inside transducers. That is, Atoms do the job, but they provide a little too much capability for the purposes of transducers. Specifically the compare and swap semantics of Atoms add too much overhead. Therefore, it was determined that a simple volatile ref type would work to ensure basic propagation of its value to other threads and reads of the latest write from any other thread. While updates are subject to race conditions, access is controlled by JVM guarantees.

Solution overview: Create a concrete type in Java, akin to clojure.lang.Box, but volatile inside supports IDeref, but not watches etc.

API:

(volatile! x) ;;ctor
(vreset! vol newval) ;;like reset
(vswap! vol f args) ;;same shape as swap!, but MACRO over vreset!

Patch: volatile3.diff

Screened by: fogus

  1. volatile2.diff
    28/Aug/14 9:04 AM
    7 kB
    Alex Miller
  2. volatile3.diff
    29/Aug/14 4:50 PM
    7 kB
    Alex Miller

Activity

Hide
Alex Miller added a comment - - edited

Dumb benchmark before/after...

java -cp target/classes -Xmx512m -server clojure.main
(def t (take 1000000))
(def v (doall (range 1000000)))
(defn bench [t v]
  (time (into [] t v)))
(dotimes [_ 30] (bench t v))

before - 29-32 ms after warmup
after - 22-23 ms after warmup

Show
Alex Miller added a comment - - edited Dumb benchmark before/after...
java -cp target/classes -Xmx512m -server clojure.main
(def t (take 1000000))
(def v (doall (range 1000000)))
(defn bench [t v]
  (time (into [] t v)))
(dotimes [_ 30] (bench t v))
before - 29-32 ms after warmup after - 22-23 ms after warmup
Hide
Alex Miller added a comment -

From Stu H elsewhere:

Three questions:
1) Should we keep volatile? in the public API?
2) Should we work in terms of IVolatile interface (guessing no)
3) Do we need a CLJS version of these APIs?

Show
Alex Miller added a comment - From Stu H elsewhere: Three questions: 1) Should we keep volatile? in the public API? 2) Should we work in terms of IVolatile interface (guessing no) 3) Do we need a CLJS version of these APIs?
Hide
Alex Miller added a comment -

1. We have many tickets requesting predicates over types that are "internal" and generally I find these to be helpful. They also can help in making core more portable to cljs (maybe those fns would fall back to atoms in cljs?).
2. We have tickets requesting the equivalent of this for IAtom (CLJ-803) etc. I don't think an interface adds any value to us here though. There seems to be some requests for this kind of passthrough interface from tooling as a decoupling point. Not putting my finger on those discussions but I know I've heard this, maybe on the mailing list.
3. I think yes if that allows us to be more efficient than whatever is being done now. Not obvious to me.

Show
Alex Miller added a comment - 1. We have many tickets requesting predicates over types that are "internal" and generally I find these to be helpful. They also can help in making core more portable to cljs (maybe those fns would fall back to atoms in cljs?). 2. We have tickets requesting the equivalent of this for IAtom (CLJ-803) etc. I don't think an interface adds any value to us here though. There seems to be some requests for this kind of passthrough interface from tooling as a decoupling point. Not putting my finger on those discussions but I know I've heard this, maybe on the mailing list. 3. I think yes if that allows us to be more efficient than whatever is being done now. Not obvious to me.
Hide
Nicola Mometto added a comment -

Why is vswap! a macro?

Show
Nicola Mometto added a comment - Why is vswap! a macro?
Hide
Max Penet added a comment -

the vswap! macro is probably for performance reasons (the main motivation of this code to begin with), to avoid using apply or unrolling tons of arities

Show
Max Penet added a comment - the vswap! macro is probably for performance reasons (the main motivation of this code to begin with), to avoid using apply or unrolling tons of arities
Hide
Nicola Mometto added a comment -

If that is the only reason, why can't it be a regular fn + :inline metadata?

Show
Nicola Mometto added a comment - If that is the only reason, why can't it be a regular fn + :inline metadata?
Hide
Jozef Wagner added a comment -

why the bang in the name of volatile! function? If the reason is to warn users that this is an 'expert only' stuff, I suggest to use a verbose name instead, e.g. volatile-reference. (This will also be consistent with approach chosen in the names of volatile-mutable and unsynchronized-mutable hints.)

Show
Jozef Wagner added a comment - why the bang in the name of volatile! function? If the reason is to warn users that this is an 'expert only' stuff, I suggest to use a verbose name instead, e.g. volatile-reference. (This will also be consistent with approach chosen in the names of volatile-mutable and unsynchronized-mutable hints.)
Hide
Rich Hickey added a comment -

Can you please lift the with-meta stuff out of the syntax-quote?
Actually, if volatile! ctor returned a type-hinted value that extra hinting might not even be needed. Let's do both for now.

Also the type hint on the volatile? arg makes no sense - it's a predicate asking if something is a volatile.

Show
Rich Hickey added a comment - Can you please lift the with-meta stuff out of the syntax-quote? Actually, if volatile! ctor returned a type-hinted value that extra hinting might not even be needed. Let's do both for now. Also the type hint on the volatile? arg makes no sense - it's a predicate asking if something is a volatile.
Hide
Alex Miller added a comment -

Made changes as requested.

Show
Alex Miller added a comment - Made changes as requested.
Hide
Fogus added a comment -

I downloaded the patch and applied to latest master. I ran the isolated tests and the full test suite and also ensured that the patch didn't add any reflection warnings. I then modified the ticket description to add a little more context and motivation (for future readers). The code is straight-forward and clean.

Show
Fogus added a comment - I downloaded the patch and applied to latest master. I ran the isolated tests and the full test suite and also ensured that the patch didn't add any reflection warnings. I then modified the ticket description to add a little more context and motivation (for future readers). The code is straight-forward and clean.
Hide
Alex Miller added a comment -

Updated to volatile3.diff to address offline comment from Rich.

Show
Alex Miller added a comment - Updated to volatile3.diff to address offline comment from Rich.

People

Vote (0)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: