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

Pooled fibers #371

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@cheald
Contributor

cheald commented Jan 27, 2014

(Recreated based off of master now)

This adds support for a variant on TaskFiber which pools fibers for a given thread. This is primarily a triage patch for JRuby 1.7.5+ which isn't currently pooling the threads that back its fibers, resulting in a ridonkulous amount of CPU burn as it spins up a new thread per Fiber. See jruby/jruby#1443 for full details there.

This is the same fiber pattern used by rack-fiber_pool, among others. In my tests, it's completely eliminated the JRuby CPU thrashing, and actually improved performance of Celluloid under MRI, too.

Using this very unrigorous benchmark, I saw the following results:

TaskFiber TaskPooledFiber
2.0.0-p247 4.08 sec 2.79 sec
jruby-1.7.10 29.5 sec (not a typo) 5.04 sec

cheald added some commits Jan 27, 2014

Big refactor of the FiberPool; now maintains an internal pool, resume…
…s fibers to let them finish when trimming them. Fixes thread growth issues; fibers left unresumed on dead threads are still an issue under JRuby though. Will need a JRuby fix to address.
@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jan 30, 2014

Contributor

headius patched in thread pooling for fibers into JRuby, and I ran some quick rough performance tests with patched and unpatched versions of JRuby, with and without the fiber pool at jruby/jruby#1443. I'm seeing:

1.7.10 1.7.10-patched 1.7.0-fiber-pool 1.7.0-patched-fiber-pool
26.57 sec 12.09 sec 9.32 sec 9.01 sec

This would suggest that even when JRuby is fixed, it would be prudent to use an application-level fiber pool when possible.

Contributor

cheald commented Jan 30, 2014

headius patched in thread pooling for fibers into JRuby, and I ran some quick rough performance tests with patched and unpatched versions of JRuby, with and without the fiber pool at jruby/jruby#1443. I'm seeing:

1.7.10 1.7.10-patched 1.7.0-fiber-pool 1.7.0-patched-fiber-pool
26.57 sec 12.09 sec 9.32 sec 9.01 sec

This would suggest that even when JRuby is fixed, it would be prudent to use an application-level fiber pool when possible.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

@cheald not sure what happened here... why this collected dust.

Are you still using this code yourself? I will factor this into #512

Member

digitalextremist commented Apr 1, 2015

@cheald not sure what happened here... why this collected dust.

Are you still using this code yourself? I will factor this into #512

@digitalextremist digitalextremist added this to the 0.17.0 milestone Apr 1, 2015

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 1, 2015

Contributor

We are still using it with great success, yes.

Contributor

cheald commented Apr 1, 2015

We are still using it with great success, yes.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

I am very excited to test this myself @cheald and bring it in. So this effectively creates a third possible task class, correct; in your implementation it's an alternative to TaskFiber and TaskThread?

Member

digitalextremist commented Apr 1, 2015

I am very excited to test this myself @cheald and bring it in. So this effectively creates a third possible task class, correct; in your implementation it's an alternative to TaskFiber and TaskThread?

@digitalextremist digitalextremist modified the milestones: 0.16.5, 0.17.0 Apr 1, 2015

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

So far, this is amazing @cheald. I need to test it further but in my extremely complicated implementation it seems like it's removing issues I've been stuck with congestion-wise for years.

Member

digitalextremist commented Apr 1, 2015

So far, this is amazing @cheald. I need to test it further but in my extremely complicated implementation it seems like it's removing issues I've been stuck with congestion-wise for years.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

@tarcieri I think this is a separate gem -- celluloid-taskpooledfiber

Member

digitalextremist commented Apr 1, 2015

@tarcieri I think this is a separate gem -- celluloid-taskpooledfiber

@digitalextremist digitalextremist added the gem label Apr 1, 2015

@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Apr 1, 2015

Member

I'd like to get pools out of core entirely FWIW

Member

tarcieri commented Apr 1, 2015

I'd like to get pools out of core entirely FWIW

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

Agreed, #512 -- I'm saying this is another pool oriented gem.

Member

digitalextremist commented Apr 1, 2015

Agreed, #512 -- I'm saying this is another pool oriented gem.

@tarcieri

This comment has been minimized.

Show comment
Hide comment
@tarcieri

tarcieri Apr 1, 2015

Member

I'm all for multiple competing pool gems 😉

Including this one:

https://github.com/halorgium/celluloid-coordinator

Member

tarcieri commented Apr 1, 2015

I'm all for multiple competing pool gems 😉

Including this one:

https://github.com/halorgium/celluloid-coordinator

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

Right on. I have it coexisting well with the existing pool implementation, but it's different enough to where I wouldn't want to lump it into celluloid-pool

Member

digitalextremist commented Apr 1, 2015

Right on. I have it coexisting well with the existing pool implementation, but it's different enough to where I wouldn't want to lump it into celluloid-pool

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

That looks good also ( celluloid-coordinator ) -- I'll bring that over also. And I think I may have another myself that I call spans but that can hold a while probably.

Member

digitalextremist commented Apr 1, 2015

That looks good also ( celluloid-coordinator ) -- I'll bring that over also. And I think I may have another myself that I call spans but that can hold a while probably.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

Yeah, #478 is looking really good right about now.

Member

digitalextremist commented Apr 1, 2015

Yeah, #478 is looking really good right about now.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 1, 2015

Contributor

Sorry, I've been out all evening. Yes, this is simply an alternate Task type. If it's desired, I can publish it as a gem; shouldn't be too hard.

Contributor

cheald commented Apr 1, 2015

Sorry, I've been out all evening. Yes, this is simply an alternate Task type. If it's desired, I can publish it as a gem; shouldn't be too hard.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

Dinged you on LinkedIn in on the topic, I was going to ask if you'd mind being attributed and I'd cut the gem for you either stand-alone or in celluloid-extras so it can be actively maintained and kept current with changes as they happen in the API ( unless you are prepared to do that ). There are several pieces of Celluloid being decoupled so it'd be in good company.

Member

digitalextremist commented Apr 1, 2015

Dinged you on LinkedIn in on the topic, I was going to ask if you'd mind being attributed and I'd cut the gem for you either stand-alone or in celluloid-extras so it can be actively maintained and kept current with changes as they happen in the API ( unless you are prepared to do that ). There are several pieces of Celluloid being decoupled so it'd be in good company.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 1, 2015

Contributor

Oh, that's fine. I just pushed https://github.com/cheald/celluloid-task-pooled-fiber but I'm happy to nuke it if it's already in motion to be made available somewhere.

Contributor

cheald commented Apr 1, 2015

Oh, that's fine. I just pushed https://github.com/cheald/celluloid-task-pooled-fiber but I'm happy to nuke it if it's already in motion to be made available somewhere.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 1, 2015

Contributor

BTW, if you want, I'm happy to just give you the commit bit on that repo, too. Whatever's easiest - we're definitely going to be using TaskPooledFiber for a long while, so I will have a vested interest in keeping it maintained. :)

Contributor

cheald commented Apr 1, 2015

BTW, if you want, I'm happy to just give you the commit bit on that repo, too. Whatever's easiest - we're definitely going to be using TaskPooledFiber for a long while, so I will have a vested interest in keeping it maintained. :)

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Apr 1, 2015

Contributor

I went ahead and pushed https://rubygems.org/gems/celluloid-task-pooled-fiber - if this ends up getting rolled into another gem I'll just pull it.

Contributor

cheald commented Apr 1, 2015

I went ahead and pushed https://rubygems.org/gems/celluloid-task-pooled-fiber - if this ends up getting rolled into another gem I'll just pull it.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 1, 2015

Member

The way you've got it now will work out totally fine. I look forward to maintaining this together. I'm very impressed with its performance in my use so far. My own platform is like new, snappier than ever before. Glad this code came back to the foreground. Excited to see it widely used.

Member

digitalextremist commented Apr 1, 2015

The way you've got it now will work out totally fine. I look forward to maintaining this together. I'm very impressed with its performance in my use so far. My own platform is like new, snappier than ever before. Glad this code came back to the foreground. Excited to see it widely used.

@digitalextremist

This comment has been minimized.

Show comment
Hide comment
@digitalextremist

digitalextremist Apr 6, 2015

Member

This will be well taken care of, with your cooperation or approval of any decisions, in #549 once the new API changes land.

Member

digitalextremist commented Apr 6, 2015

This will be well taken care of, with your cooperation or approval of any decisions, in #549 once the new API changes land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment