Implement swap! - Githubissues

technomancy commented 11 years ago

The functionality around boxes that ships with Racket is pretty low-level. It would be great if the equivalent of clojure.core/swap! were available; that way you could treat boxes more functionally. (Rather than providing an old and new value, swap! simply accepts a function to call on the old value and uses the return value as the new value.)

Since box-cas! is new in 5.3 it would be great if this could fall back to set-box! if necessary. One drawback here would be that it would not be guaranteed to be atomic on older versions of Racket. We may be able to work around this with a locking scheme; not sure whether it's worth the hassle.

I can take a shot at implementing this if you think it's a good idea.

greghendershott commented 11 years ago

@technomancy I'm not familiar with the use of clojure.core/swap!, much less how it compres/contrasts with box-cas!. So probably I shouldn't be the one to take a crack at it.

Also, I don't know enough to say whether you might prefer to release this as its own small library for Racket. Is the most interesting thing about swap! its "Clojure-ness" (in which case here might be a good home for it) or its "swap!-ness" (in which case it might be better elsewhere)? I don''t know enough to have an opinion. What do you think?

greghendershott commented 10 years ago

I was reviewing open issues and noticed this. Reading it again, today, it sounds like I was discouraging you from adding this. That wasn't my intention. If you're still interested in submitting this, I'd be delighted to take it. Also I think it would be fine to do the easier implementation requiring 5.3+ and using box-cas!.

qerub commented 10 years ago

Is the most interesting thing about swap! its "Clojure-ness" (in which case here might be a good home for it) or its "swap!-ness" (in which case it might be better elsewhere)?

Atoms (and thereby swap!) are such a fundamental piece of Clojure that they score high on the "Clojure-ness" scale IMHO.

OTOH, all multi-threaded Racket programs are potential users of swap! and there's nothing Clojure-specific about it.

It isn't worth creating a new library for a single simple function so my suggestion is that we incorporate swap! in Rackjure and if Racket proper ever incorporates it we deprecate our implementation.

qerub commented 10 years ago

Here's a first stab at an implementation BTW:

(define (swap! box f . args)
  (let loop ()
    (let* ([old (unbox box)]
           [new (apply f old args)])
      (if (box-cas! box old new)
          new
          (loop)))))

qerub commented 10 years ago

For reference, here's the Clojure implementation: https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Atom.java#L75

Racket boxes don't have validators or watchers, hence the absence of that in my implementation. (Thought: Maybe we should bring this over too?)

soegaard commented 10 years ago

Racket has chaperones. http://docs.racket-lang.org/reference/chaperones.html?q=guardians#(def._((quote._~23~25kernel)._chaperone-box))

qerub commented 10 years ago

@soegaard: Thanks! No need to reimplement that then.

@greghendershott: What do you think of my suggestion? Would you accept a patch (with tests, of course) that added box-swap!?

greghendershott commented 10 years ago

@qerub Yes I would. Thanks!

qerub commented 10 years ago

I have a patch ready at https://github.com/qerub/rackjure/tree/box-swap now, but it triggers some bug in Racket 5.3.6 so lets wait with merging. (Work with -current, thanks for checking @soegaard!)

qerub commented 10 years ago

OK to merge with me now; the bug is fixed in the latest stable release of Racket (5.92).

greghendershott commented 10 years ago

@qerub merged -- thanks!

greghendershott commented 10 years ago

@qerub Could you please take a look at https://travis-ci.org/greghendershott/rackjure/jobs/17768289 ?

raco test: (submod "./utils.rkt" test)
+: contract violation
  expected: number?
  given: '(1)
  argument position: 2nd
  other arguments...:
   3000
  context...:
   /home/travis/build/greghendershott/rackjure/rackjure/utils.rkt:19:0: box-swap!
   /home/travis/build/greghendershott/rackjure/rackjure/utils.rkt:36:4: thunk
   /home/travis/build/greghendershott/rackjure/rackjure/utils.rkt: [running body]
   /usr/racket/collects/compiler/commands/test.rkt:29:10: for-loop
   f8
   /usr/racket/collects/compiler/commands/../../racket/private/map.rkt:53:19: loop
   /usr/racket/collects/compiler/commands/test.rkt: [running body]
   /usr/racket/collects/raco/raco.rkt: [running body]
   /usr/racket/collects/raco/main.rkt: [running body]

The tests failed:

after I pushed an unrelated commit
only for one build (Racket 5.3.6)
but with an error that doesn't seem 5.3.6-specific
and the tests have succeeded (even for 5.3.6) before

My guess would be that there's some intermittent bug -- that will cause the tests to fail if run long enough. What do you think?

qerub commented 10 years ago

That looks exactly like the Racket bug that made me want to postpone the merge. The bug is not deterministic, hence the previous successful build(s)… To be honest, I should have thought more about this before okaying the merge. Here's the most important fact: box-swap! won't work for sure with some/all versions before 5.92. What do you think about disabling box-swap! (in some sane way) in < 5.92 and adding 5.92 to your continuous integration machinery?

greghendershott commented 10 years ago

Ah. Got it.

I have a commit on a topic branch: https://github.com/greghendershott/rackjure/commit/a5ba5f4f30516b435a303a6b3196f7dd63535a96

Bit of a detour to twiddle my Travis script for 5.92, but it's working now. Passes.

Will merge to master shortly.

qerub commented 10 years ago

I attached a note here: https://github.com/greghendershott/rackjure/commit/a5ba5f4f30516b435a303a6b3196f7dd63535a96#commitcomment-5205500

LGTM otherwise. :)

greghendershott / rackjure

Implement swap! #6