Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove celluloid and use pure Ruby threads #222

Merged
merged 1 commit into from
Oct 24, 2013

Conversation

sethvargo
Copy link
Contributor

This removes the celluloid pieces (I think I got all of them) and uses pure Ruby threads to handle parallelism. I've noticed a significant speed increase and I think there's a little less indirection in this approach. Thoughts?

/cc @schisamo

@ghost ghost assigned sethvargo Oct 23, 2013
@fnichol
Copy link
Contributor

fnichol commented Oct 23, 2013

I think you're reading my mind here 😄

I love me some celluloid but all we're really doing here is running some stuff at the same time that shouldn't interact with each other.

I'll give this a more thorough look over in the morning. Thank you!

@schisamo
Copy link
Contributor

👍 this LGTM. Always nice to simplify a code base and remove dependencies!

@damm
Copy link

damm commented Oct 23, 2013

Please pardon the dust on this question.

@sethvargo So switching from Celluloid to Ruby Threads was faster between most versions of Ruby? 1.8.7 works faster even?

@sethvargo
Copy link
Contributor Author

@damm I no longer have Ruby 1.8.7 installed on my system as it's been EOL for many months now. The overall time decreased. I don't know if that was due to Celluloid's boot time or the pure thread implementation.

@damm
Copy link

damm commented Oct 24, 2013

May have been end of life; but still the default ruby version on Mac's up until Mavericks :( Which is why I mentioned it.

I imagine this will make it easier to support test-kitchen using native threads...

@sethvargo
Copy link
Contributor Author

I'm fairly certain that Celluloid doesn't work on Ruby 1.8, so this could potentially allow Test Kitchen to run on Ruby 1.8. That being said, it's not recommended that you use your system Ruby for development, and I don't think test kitchen promises Ruby 1.8 support. I'm also not sure what you're arguing against. Could you please elaborate?

@lamont-granquist
Copy link
Contributor

pretty sure that this patch can only improve the ruby 1.8.7 situation:

% gem install celluloid
Fetching: timers-1.1.0.gem (100%)
Successfully installed timers-1.1.0
Fetching: celluloid-0.15.2.gem (100%)
ERROR:  Error installing celluloid:
    celluloid requires Ruby version >= 1.9.2.

@sethvargo
Copy link
Contributor Author

@fnichol have you had a chance to review this?

@damm
Copy link

damm commented Oct 24, 2013

There was no issue or argument for or against; it was a question @sethvargo and that was if you had tested it against 1.8.7 and you had not.

Thanks!

lamont-granquist added a commit that referenced this pull request Oct 24, 2013
Remove celluloid and use pure Ruby threads
@lamont-granquist lamont-granquist merged commit 5f65689 into master Oct 24, 2013
@sethvargo sethvargo deleted the remove_celluloid branch October 24, 2013 19:37
@fnichol fnichol mentioned this pull request Oct 29, 2013
@fnichol
Copy link
Contributor

fnichol commented Oct 29, 2013

@sethvargo Yes, looking nice and simple!

@damm Thankfully I made Ruby 1.9+ a requirement with Test Kitchen from the start so there is a better chance that we're good here. In a way this will make using Test Kitchen as a library a bit simpler. If someone wanted to wrap/delegate an Instance with Celluloid actors there won't be much code to add.

@test-kitchen test-kitchen locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants