AtomicMarkableReference: Add pure Ruby implementation

[ianks]

This is the beginning of adding the primitive concurrent type AtomicMarkableReference. It is much like the AtomicReference, but contains a boolean marked value. It will be neccesary for implementing the recursive split-order (lock-free) hash table I plan to implement.

The code is pretty straight forward, and steals mostly from AtomicReference; there are some refactorings possible there but I did not feel comfortable diffing too much code to make that happen on my first PR.

Some notes/question about the design:

• I used Java's API as a guideline for implementing the API for this
• Like in Java, get returns an array of [@value, @marked], I am not a huge fan of returning an array like that for a public facing API, but since this is low level enough it might be justified. Plus it is rather easy to destructure -> markable_ref.get = val, mark. Do you think returning an array here is idiomatic-enough Ruby? Obviously, returning the acutal object would be the most OO, but there are obvious race conditions there so maybe we are an exception to the rule here. An alternative would be to create some sort of State object which represents the given state of the object at the time of return. Thoughts?
• I noticed none of the tests in atomic_markable_reference_spec attempted to create possible race conditions, etc; but there are specs in other places which rely on AtomicReference to work. Is this the way you guys have been testing the implementation in a threaded environment? Testing multithreaded implmentations is tough, as tests tend to fall short of covering the cases. Do you guys think I should add some tests to document in a threaded environment? Or would some sort of informal proof be better? Sorry about ramblng here; but if you guys could give me some better insight about testing in this lib in general I would be super appreciative!

Please go crazy on the feedback for improvments here! I would love to optimize this code in any way possible 😃

[pitr-ch]

@ianks first, let me welcome you to the group of concurrent-ruby contributors.

Couple of questions/comments/concerns:

• "returning the actual object would be the most OO, but there are obvious race conditions there" what race condition do you mean?
• I would suggest to implement this class by usage of AtomicReference, to avoid duplication. Using frozen array to store both values or an immutable class should be good enough.
• value, mark = ref.get and ref.set(value, mark) seems good to me.
• Do you know about other use-cases of this class?
• (updated) Sadly our test suite is not the best. Adding tests for raise conditions is good idea. We use other abstractions (like CountDownLatch, Event) freely in other tests since they are tested in other spec and assumed to be correct.

[jdantonio]

How about this? Struct.new(:value, :marked?)

Also, now that we've merged #274 can we put this under the Edge module, please? Thank you!

[ianks]

@pitr-ch

"returning the actual object would be the most OO, but there are obvious race conditions there" what race condition do you mean?

All I really meant was that by returning a reference to self, the user cannot know what the the values were are the exact time of updating, because another thread could concievably alter that AtomicMarkableReference after the get returns.

I would suggest to implement this class by usage of AtomicReference, to avoid duplication. Using frozen array to store both values or an immutable class should be good enough.

Maybe I am understanding this wrong, but AtomicMarkableReference needs to be mutable. Can you elaborate more on what you mean by this?

Keep in mind the main thing I need from this primitive is the compare_and_set(old_val, new_val, old_marked, new_marked). This effectively needs to update the two new values in one atomic operation (preferably without locks). In Java I believe this is done by having a marking bit on the reference; I was hoping to do a similar thing in the C extension.

Do you know about other use-cases of this class?

Yes. One very interesting structure you can build with this is a lock-free linked list (actually, this is the basis for the recursive split-order hash set 😃), Essentially, instead of physically removing nodes from a linked list, you logically remove a node by settings its mark to true, then have later adds or removes physically remove the item using compare_and_set. The compare_and_set becomes your linearization point and thus, no locks needed.

@jdantonio

How about this? Struct.new(:value, :marked?)

I like that. Maybe we could do something like this to give it some context to the user:

class AtomicMarkableReferenceState < Struct.new(:value, :marked); end

[pitr-ch]

race: ack, thanks

using AtomicReference: I meant something like this:

ref = AtomicReference.new
ref.compare_and_set([old_val, old_mark], [new_val, new_mark])

AtomicMarkableReference can hold AtomicReference internally and just wrap and unwrap the pair.
In java there is immutable class representing the pair.

use-case: Thanks for the example, lock-free linked list would be very useful too.

struct: You may also want to look at https://github.com/ruby-concurrency/concurrent-ruby/blob/synchronization/lib/concurrent/synchronization/immutable_struct.rb. Normal Struct may have issues with visibility.

[ianks]

ref = AtomicReference.new
ref.compare_and_set([old_val, old_mark], [new_val, new_mark])

I find this syntax somewhat counterintuitive and unidiomatic. I have never used a method in ruby with a signature similar to that. Althought I do like the idea of a ReferenceBooleanPair. I could possibly add support for that.

struct:

Nice. I will definitely use that.

[jdantonio]

Although not germane to the conversation, this syntax:

ref.compare_and_set([old_val, old_mark], [new_val, new_mark])

is very Erlang-ish

ref:compare_and_set({old_val, old_mark}, {new_val, new_mark}) ->
  {ok}.

But, sadly, not very Ruby-ish.

[pitr-ch]

Sorry, I've actually meant it to be used only in the implementation of MarkableReference. It would not be exposed anywhere. Using the immutable struct it would look as follows:

class MarkableReference < Synchronization::Object
  Pair = ImmutableStruct.with_fields :value, :mark
  def initialize(value, mark)
    @Reference = AtomicReference.new Pair[value, mark]
    ensure_ivar_visibility!
  end

  def compare_and_set(old_val, old_mark, new_val, new_mark)
    @Reference.compare_and_set Pair[old_val, old_mark], Pair[new_val, new_mark]
  end
  #...
end

[ianks]

@pitr-ch Unfortunately that creates the overhead of instantiating a struct object for every compare and set operation. Compare and set will be incredibly regular with the lock-free data structures, and it is unneccesary to do this much work to create a pair. Ideally, the compare_and_set can be accomplished with very little little logic (a bit mask for checking the mark, then a single XCHG assembly operation). Although I think for the pure ruby implementation using the SynchronizationObject should be perfect.

[ianks]

Also @pitr-ch, would it be best that I implement it with Synchronization object once that synchronization branch gets merged into master?

[pitr-ch]

@ianks yeah that would be great. I think you can start using the #284 now. There is only one failure on rbx which it's not related to the changes in the PR.

[jdantonio]

@ianks If the performance penalty of creating a new Struct on ever #get call is a concern (which is perfectly reasonable) then please return both values (as in 28f90ab). Standard Ruby allows multiple values to be retuned from a method. This isn't used very often, but it is standard Ruby.

ref = Concurrent::AtomicMarkableReference.new(10)
value, mark = ref.get

[jdantonio]

@ianks PS: Since returning multiple values from a Ruby method silently creates an array, there may be a performance hit there as well. We should run benchmark tests against both implementations.

[ianks]

I did some benchmarks of the performance properties of instantiating different object types in different interpreters.

Here is my methodology: https://gist.github.com/15362d1a28e24d57fe41

# ruby 2.2.2
                  real
StateClass    2.948580
StateStruct   2.996852
StateArray    1.369050

# rbx-2.5.3
                  real
StateClass    1.751999
StateStruct  20.588989
StateArray    0.964172

# jruby 1.7.19

                  real
StateClass    1.238175
StateStruct   1.401991
StateArray    0.509454

It seems across the board array is about 2-2.75x faster. rbx has a much slower struct implementation as of now, so I think that might rule out using a struct is this case. However the difference between class and array is somewhat small, and if it increased the usability enough I think it could warrant the performance hit. Although if performance is more important here, array might be the best way to go.

UPDATE: It seems RBX has fixed a bug in their struct impl as a result of these benchmarks; however it is still around 6x slower.

[chrisseaton]

You might want to report that performance to Rubinius - an order of magnitude slower has got to be a bug and I'm sure Struct should be exactly the same performance as the class. They'll be pleased as it's such a simple test case.

[pitr-ch]

👍 for array, I was advising the immutable struct because it ensures the visibility of its fields, not realizing that it is not necessary since read/write to atomic reference is volatile on all implementations. I am reasonably sure about MRI and JRuby. I am assuming Rbx too. get and set have memory barriers https://github.com/rubinius/rubinius/blob/master/vm/builtin/atomic.hpp#L26-L36, I could not decipher compare and swap but I think we can assume it's ok too.

@ianks it seems you know C/C++, you could verify: https://github.com/rubinius/rubinius/blob/master/vm/builtin/atomic.cpp#L23-L32, https://github.com/rubinius/rubinius/blob/b8b8cc4dcfd744e12c1360fab5fe4c66756febd7/vm/util/atomic.hpp#L95-L113

[ianks]

I updated the implementation to rely on reference pairs (value, boolean) instead on locks for AtomicMarkableReference.

@pitr-ch There was a mention in the SynchonizationObject doc about have fields which are auotmatically AtomicReferences, but I didn't have enough information to make that work on this go. If it is possible to do something like that, let me know so this can be more in line with the vision on the SynchronizationObject API.

[pitr-ch]

It should be the other way around, Concurrent::Edge::AtomicMarkableReference

[pitr-ch]

I was originally considering to add something like attr_volatile_with_cas but in the end I left it to usage of same pattern as you've used here with @Reference (final reference to a value with CAS operations) since it's more explicit, and it does not pollute the object's space.

[ianks]

Hmm, the tests are failing on JRuby for some reason. The interpreter is expecting 0 args for AtomicMarkableReference, when I have it defined as accepting 2 args with default values.

Also, as a side note to save some time, the bug exists by using the constant Concurrent::Edge::AtomicMarkableReference instead of described_class

     Failure/Error: subject { described_class.new(1000, true) }
     ArgumentError:
       wrong number of arguments calling `initialize` (2 for 0)
     # ./lib/concurrent/atomic/atomic_markable_reference.rb:7:in `initialize'
     # ./spec/concurrent/atomic/atomic_markable_reference_spec.rb:2:in `subject'
     # ./spec/concurrent/atomic/atomic_markable_reference_spec.rb:20:in `(root)'

I imagine this could be a JRuby bug, or maybe an issue with synchronization object? Has anyone experienced this before?

[jdantonio]

I'm in Detroit attending self.conference so I won't be able to look at this for a couple of days. I'll give you feedback as soon as I can.

[pitr-ch]

Could you also add API when it will just return false, raising and catching exception can expensive.

[pitr-ch]

Looks good. 👍 Thanks for working on this.

One idea though if you find it interesting. It seems to me that this class is a special case of more abstract class, let's name it AtomicArrayReference. Basically the AtomicArrayReference would not compare the arrays but its elements for equality (using :== for special cases). AtomicMarkableReference is then a special case of 2-element-AtomicArrayReference. It's just an idea for future improvement.

[ianks]

Thanks @pitr-ch! Could you take a look at the issue I was having with the JRuby error? You can see the failures on Travis. I am thinking of reporting it as a bug to JRuby, but I think it may be an issue with Synchronization Object.

[pitr-ch]

regarding the error, this should be super() so the args are not passed to parent initialize. I've mistakenly left old initialize(*args, &block) signature in ruby files (java extension has correct signature so it fails) so it was silently swallowing the args not using them.

[pitr-ch]

Fixed in 48bacc4

[ianks]

@pitr-ch let me know when that makes it into master so I can rebase.

[pitr-ch]

I think you do not need to wait for the fix to land in master, it can be merged earlier. The currently failing job is something else.

[ianks]

@pitr-ch I added the API for try_update_no_exception. I noticed in AtoimicReference there is a similar pattern, maybe we should make an API for that as well? In fact, we might want to consider making that the dafault behavior instead of raising an exception. I dont see much advantage to raising an exception in this case. Thoughts?

[pitr-ch]

yeah something like try_update -> false and try_update! -> exception would be nice. For both References. Doing the change on 0.8->0.9 is acceptable I think, we should probably add breaking changes part to changelog to communicate this more loudly though. @jdantonio thoughts?

[ianks]

@pitr-ch I updated the try_update API as per your suggestion. I tend to like not raising an exception by default. The only outstanding issue is that it differs from the AtomicReference API. We should considering deprecating the old AtomicReference try_update in favor of the: try_update -> false and try_update! -> exception before 1.0. @jdantonio, thoughts on this?

[jdantonio]

@ianks @pitr-ch I agree with not raising exceptions by default. We've been fairly consistent with explicitly using ! methods when exceptions are raised. I'm 👍 on the try_update and try_update! approach.

[ianks]

Any idea why I am getting this error on JRuby?

undefined method `_compare_and_set' for #<Concurrent::AtomicReference:0x213c812a>

[jdantonio]

Because that method doesn't exist on JRuby. The public method supported on all platforms is #compare_and_set. That's what you should be using. The underscore method is an internal implementation detail inherited from the original ruby-atomic gem.

[ianks]

Ok, Travis is happy now. Coveralls is angry though 😢 Let me know if there is anything more that needs to be done here.