search

rmagick / rmagick

Ruby bindings for ImageMagick
https://rmagick.github.io/
MIT License
696 stars 140 forks source link

Release the Global VM Lock on ImageList operations #244

Closed thedrow closed 5 years ago

thedrow commented 8 years ago

I just started working on it and this PR doesn't clean resources correctly when killing a thread. It is functionally equivalent to the original code except the fact that other ruby threads are now able to run in parallel.

thedrow commented 8 years ago

Turns out that rm_ensure_result, rm_split and rm_check_exception require the GVL. I'll adjust the code accordingly.

carsonreinke commented 8 years ago

FYI, this changes the Ruby support since rb_thread_call_without_gvl is not available on 1.8.7 and is experimental in <2.0.

thedrow commented 8 years ago

@carsonreinke I didn't know that. I don't think that supporting very old Ruby versions that are no longer maintained is more important than new features. What do you guys think?

ioquatix commented 8 years ago

Ruby < 2.0.0 is no longer relevant IMHO.

mockdeep commented 8 years ago

I think we should cut a major version dropping support for 1.8.7, at the very least. Could go either way on 1.9.3. Would love to switch to the new hash syntax, though...

linduxed commented 8 years ago

IIRC we had a discussion somewhere about dropping support for deprecated Ruby versions, which would be 1.8.7 and 1.9.3. I can't find the issue/PR at the moment.

While I understand that there are some rare users of this gem that are still on deprecated Ruby versions, I say the following:

Since the later deprecated Ruby version (1.9.3) had its support dropped more than a year ago (February 23rd, 2015), it's high time to move on. I'm not even going to talk about 1.8.7.

vassilevsky commented 8 years ago

I agree with dropping old Ruby versions. In fact, the develop branch already has a commit that does it:

https://github.com/rmagick/rmagick/commits/develop

Feel free to redirect this PR to develop, as CONTRIBUTING.md suggests ;) We will release it as 3.0.

thedrow commented 8 years ago

@vassilevsky No problem. Will do. Would you guys care to fix the build? Right now it has all sorts of failures that prevents me from figuring out if my changes still work. Another question, do you guys prefer that rm_ensure_result, rm_split and rm_check_exception will acquire the GVL again if an error occurs or to release the GVL earlier?

vassilevsky commented 7 years ago

Tests are passing in master and develop now.

thedrow commented 7 years ago

I don't need this anymore. Can someone build off my work to make this happen?

vassilevsky commented 5 years ago

I've pulled your branch into the main repo and created #304 from it. We'll see.