Actress merge

[pitr-ch]

continuation of #72, needs more work and discussions

• change Abstract class to module
• remove Algebrick dependency
• remove Atomic dependency
• cleanup
• split-files
• tests
• it deadlocks in rspec environment, 'no live threads'
• check all TODOs left in the code
• documentation
• merge

[jdantonio]

My main concern here is dependencies.

Since the beginning I've strictly avoided including any production dependencies. The gem has been entirely self-contained. There are several reasons for this. The main reason is that I've always viewed this gem as a toolkit, a foundation upon which other gems would be built. I didn't want to replicate Sidekiq, Celluloid, Akka, etc. Rather, I hoped to create a gem that could be used to build those other libraries more easily. Another reason is the viral nature of external dependencies. It always drives me crazy when I explicitly include one gem in a project and my Gemfile.lock bloats with a half-dozen or more dependencies. A final reason is that I have always secretly hoped that this gem could be incorporated directly into the Ruby standard library. Although I doubt that will ever happen, I've let that vision help drive the design.

I appreciate that the Atomic and Algebrick gems have useful capabilities, but including them crosses a line I've worked hard to avoid. I haven't looked at your code deeply enough to know how those gems are used but I'd like to explore ways we can reach a reasonable level of functionality for actors without including them. That may require us to build a companion gem for the more advanced actor functionality.

[pitr-ch]

I thought that this will be an matter for discussion hence the '?' after the point. :)

I understand and share your concerns about dependencies, but I think it's ok to include dependencies occasionally when needed if the gem has a good history and does not bring whole another mess of dependencies. Atomic does not have any and it would be very useful to have it in concurrent-ruby. I would also love for this gem to become standard for dealing with concurrency in Ruby (and I think it has a potential to do so) but it does not need to go to stdlib for that. Also AFAIK there is ongoing effort to gemify stdlib and remove as much as possible.

https://github.com/headius/ruby-atomic is implementation of AtomicReference, it has native implementations for several platforms using compare-and-swap instructions. I did some benchmarks it as fast as accessing instance variables but synchronized :), very handy. I would like to keep that. One additional note: It would also potentially allow to implement other primitives in concurrent-ruby without locks, I've experimented with lockless Future, I need to do some benchmarks if it is actually (noticeable) better.

Algebrick is ideal for message definition and message matching but it should not be forced on user. I would love though to have some example and link in documentation showing how it can be used combined. I'll remove the dep. on Algebrick.

[chrisseaton]

The atomic gem (the only other dependency) provides a single construct - AtomicReference, where we currently just have an AtomicFixnum and AtomicBoolean. MVar is also an atomic reference, but has more complicated semantics such as blocking that aren't always needed. The atomic gem also provides a finely tuned JRuby backend.

Perhaps Charles Nutter would be interested in merging his implementation into this gem? I think it would be minimal work, and the benefit would be that it would be part of a larger package that has lots of active maintenance. Charles has said a few times that he's interested in concurrent-ruby and believes Ruby needs a good set of concurrent primitives. Maybe we should approach him and ask him if he's interested?

[jdantonio]

We're very familiar with the Atomic gem (and @headius has been a great advocate of our gem). It's one of the gems I looked at when I began experimenting with the native implementations we will likely add in future versions. But @chrisseaton built lock-free atomic operations using software transactional memory and performed significant performance testing. Is there any reason we couldn't use Concurrent::atomically within our actors and achieve the same result, without adding an outside dependency?

@mighe @chrisseaton @lucasallan What are your thoughts regarding adding the Atomic gem as a dependency to our gem vs. remaining dependency-free?

[pitr-ch]

TVar is different from Atomic. I am not sure if they can be directly compared, in Clojure terms it is coordinated refs vs. uncoordinated atoms.

Refs are for Coordinated Synchronous access to Many Identities".
Atoms are for Uncoordinated synchronous access to a single Identity.

Atomic as a single uncoordinated reference is significantly simpler and probably faster than TVar. I think we should use TVar only when really needed. (It is scalable but it has overhead for simple operations.)

[mighe]

Is there any reason we couldn't use Concurrent::atomically within our actors and achieve the same result, without adding an outside dependency?

My only concern about STM is that inside every atomically block we should use only lock-free structure to avoid deadlocks or performance loss because everything is already synchronized (correct me if I'm wrong)... we could use them without any problem provided that inside actors we build everything upon STM.

Beside that, I like very very much concurrent-ruby independency and, as @chrisseaton just said, atomic gem provides only one low-level feature that we already have in some different flavour. In my opinion atomic could be merged inside concurrent-ruby if @headius agrees.
Anyway, I don't feel the urge to merge or use those two gems atomic, we could implement Actors using what we already have and optimize them in the future:

@pitr-ch the other our mates already know that every time I read something about lock-free structure in ruby I begin to be really really scared. Sometimes I'm really boring, but without a consistent, clear and well defined memory model, we should be as conservative as possible.
At the moment MRI memory model is undefined, jruby is defined but I'm not so sure it is "correct" from a ruby programmer perspective and rubinius is even worse: that is why we preferred lock based implementations.
Currently I'm not really concerned about performance, I prefer a safe and stable gem to a faster but potentially dangerous one.

[pitr-ch]

So far I did not see a need for STM although It may come later when it'll get more complicated. I am fine with implementing AtomicReference with locks. It is a useful concept well suited for this gem and we can optimize later with native implementations.

I've revisited the place I am using it and I was able to remove the dependency :) I guess the question of including Atomic or at least have AtomicReference is still open though.

[chrisseaton]

I think have a simple AtomicReference for now, then you can carry on with this merge, then we can implement a highly efficient version later, maybe by merging with the atomic gem.

I think you're right that STM is probably the wrong tool here - if you're only working with a single reference.

[schmurfy]

How will this be merged when completed, will it replace the current ActorContext ?

[jdantonio]

I agree with @mighe about performance. When I started the gem I focussed on creating a high-level set of abstractions that would be accessible to most Rubyists. Then @mighe and @chrisseaton began making those abstractions much more stable and efficient (as well as adding new abstractions). I feel this approached has worked very well. If we keep our focus on making a reliable, usable, and accessible Actor implementation (at which my initial implementation failed) we will have plenty of time to optimize later. A generic atomic variable like what @headius created would be a great addition, but the other atomic variables were the result of emergent design--solving the needs of our high-level abstractions. I'm comfortable continuing that evolution and staying dependency-free for now.

I'm intrigued by the thought of asking @headius to consider merging his gem into this one. Does anyone know him personally? I've only spoken to him over Twitter. I plan to attend Steel City Ruby in August where he'll be speaking. I hope to have a chance to buy him a coffee or beer then. I'll can ask him then.

[jdantonio]

@schmurfy It will likely replace ActorContext (since it hasn't been released) but we don't know yet. This is a spike. It's intended to fill the same role as ActorContext and is based on work @pitr-ch started in another gem. He has graciously offered to continue his work here. He and I share the same influences and ideas regarding how actors should work in Ruby. So we're exploring how best to integrate our work.

@schmurfy You've been a very strong supporter of my efforts with actors and have shared some great ideas that have helped shape our course. What are your thoughts on this approach for actors? How would this work for you if we went with actors based on this rather than ActorContext?

[schmurfy]

This definitely looks interesting, my question was just an attempt to try tracking what is happening and this is really nice to see so much work done around this gem, replacing ActorContext at this point is no an issue for me especially if the new one is better ^^

I read some of the previous issues/pull request leading to this, I will try to have a closer look and see if I can provide useful feedbacks on it.

[jdantonio]

@schmurfy Sounds good. Thanks again for taking such an active interest. I look forward to your feedback.

[schmurfy]

Here are my impressions after the first read:

type checking

I am really not a big fan of this in Ruby, if you don't have the right object you will get an error anyway so why add another operation ?

ask/tell

I like the names and they are easy to connect to what they do, I also like that we gain the fire and forget ability that was not in the current ActorContext, it would be nice though to have the equivalent of post! even if that's just a ask().value below.
While non blocking operations is clearly the best choice having the choice to use blocking operation out of the box is nice (especially in tests), what about "ask!" ?

There is one other operation that was lost: the ability to have a block being called on completion:

actor.ask(:something) do |ret|
  #...
end

If that's not too hard to implement it was a nice addition.

Actor hierarchy

I am a bit confused with its purpose, is this only for debug purpose only ?
or is it a way to implement a supervision tree ?

[jdantonio]

@schmurfy Thank you for the feedback.

Type checking will not be required in the receive method. Both @pitr-ch and I have implemented Erlang-style pattern matching in Ruby in outside gems and we both like this type of message dispatch, but we agree that this functionality can remain in external gems.

I agree with both of your suggestions regarding additional "post" functionality, and both are easy to implement. I'm comfortable with ask! as a blocking variation of ask. It's easy to implement and is probably safer for the user if we handle the blocking for them. I've always liked your suggestion to support a callback block on post (aka ask). Although Akka does't have equivalent functionality it is a very Ruby-ish approach (and it appeals to the functional programmer in me). I think we should implement both of those features.

I'll have to defer to @pitr-ch for the parent/child relationships. I assume, as you do, that this involves supervision. Akka implements supervision in a very similar manner. It's a very different approach than Erlang but it makes more sense in an OO language. I hadn't begun exploring supervision in the new ActorContext module so we haven't discussed it yet (except to agree that Erlang's rest_for_one strategy isn't something we want to include). I'm open to all suggestions and looking forward to hearing @pitr-ch's thoughts.

[schmurfy]

I think you should be able to use the RUBY_ENGINE constant for this.

[pitr-ch]

Ah, yeah I would, but I forgot to push a commit which is removing it. No instance variable is ever re-assigned outside of initializer so we are save, no need to synchronize access.

[headius]

I have no objections to merging atomic into concurrent-ruby. I'd really like to see concurrent-ruby become THE gem for concurrency utilities.

Note also thread_safe gem, which duplicates some collections in concurrent-ruby. Perhaps we should create a common github organization for these lower-level Ruby concurrency-related libraries?

[schmurfy]

Dependencies can be evil but that's not necessarily the case for me now that we have awesome tools like Bundler, in this case the atomic gem can surely be used outside of concurrent-ruby and merging it inside would force anyone wanting to use it to depend on concurrent-ruby.

A good example of evil dependency is when really small gem of 10/20 lines add activesupport as their dependencies just for one or two method... You end up with a dependency which is 20 times bigger than your gem for nothing (and as a bonus your code now takes 5 more seconds to load what is mostly dead code for you).

I like the idea of having an organization to host these low level gems though :)

PS: I know that the latest releases of activesupport give you more control on what is loaded but you can still end up loading the whole gem since there is a lot of interdependencies.

[pitr-ch]

Thanks for the feedback.

type checking - I find it extremely useful when developing new libs rapidly, it keeps me in check and helps me discover a lot of bugs right a way (no nil errors :). We can remove them if you like, when that goes to release.

ask! - I hesitate about the ask! (I like the name), it is a bad pattern to use those with actors. It can deadlock your app. Hm, but it'll be convenient for tests and probably other use-cases. So I've added it with waring in doc, that should work :)

ask callback - I am not sure about this. I've kept ivar as a param so observer can be added to the ivar itself, but I think this should not become a pattern, since it may not be a good idea to be able to easily inject arbitrary code for execution in Actor instances.

actor hierarchy - yeah I draw inspiration from Akka, for now it does not have any other purpose than nice organization and log output. I am planning to use it for supervision tree as you've guessed. Same reason applies for parent/child relationship tracking.

organization +1

[schmurfy]

The callback was a way to get an alternative to full fledged observers for simple cases, instead of this:

class DummyObserver  
  def update(time, value, reason)
    # ...
  end
end

iv = actor.ask()
iv.add_observer( DummyObserver.new )

You could use:

actor.ask(:something) do |time, value, reason|
  # ...
end

What is your concern with the callback, the fact that it capture the surrounding scope ? or is it something else ?
If used correctly both version of this code will act the same but the first is really tedious when you don't need a full class.

[pitr-ch]

IVar add_observer accepts block as well so it's not that bad:

actor.ask :something,
          Ivar.new.tap { |i| i.add_observer { |time, value, reason| } }

and if we change add_observer to return self instead the observer (which I think makes more sense) than:

actor.ask :something,
          Ivar.new.add_observer { |time, value, reason| p time, value, reason }

My concern is that it can bring down otherwise well tested and stable Actor. The notification of observers is executed on a thread given to the Actor inside the message processing block, which may lead to blocking or failing to process the message even if that would be fine without the observer. Ah I see now that is only true for the observers on IVar, there is a rescue block in original SimpleActorRef. I've wrongly assumed that it's using IVar observer as well. So the errors may not necessarily be an issue (if wrapped as in SimpleActorRef) but it can still break the actor by blocking. What is the use-case for the callbacks? We could also explore possibility of using Promise here which would handle the callbacks execution in child promises asynchronously.

[schmurfy]

to be honest I have no real use case to bring forward currently but as with "ask!" I strongly feel like that's something which could help in tests or quick prototyping.
In real code I will to think more about it and see if I can come up with a real use case.

PS: I completely missed that add_observer accepted a block.

[jdantonio]

This conversation has grown very long and covered several topics. I thank you all very much for your feedback, suggestions, and insight. To make the conversation easier to follow I've created three new issues. Please use those three threads to discuss the relevant topics. The three threads are:

• v0.6.0 Release Plan and Date
• New Actor Implementation Discussion and Roadmap
• v0.7.0, GitHub Organization, and Merging Atomic

[coveralls]

Coverage decreased (-0.46%) when pulling 130c049 on actress into ab03fae on master.

[pitr-ch]

Hey Guys,
I would say this is feature ready for merging. It is planned to be merged as experimental to 0.6 release of concurrent-ruby under namespace Actress (as discussed in #82). Remaining issues:

• more complete documentation
• fixing tests, they are intermittently failing (everywhere not just actress tests)

Even though, please review the PR, I've also changed few things outside of actress. I will continue with test fixing in the meantime.

[jdantonio]

👍 This is much better than the actor implementation I was working on. I think this will be a valuable addition to our gem. I am comfortable with merging.