Use concurrent futures to reduce blocking IO #37
Conversation
Especially the queue inspection can be currently very slow, because a transport to the message broker needs to be opened and messages are exchanged. Especially in multi queue setup these operations should happen in a concurrent manner.
|
Hi @ryanhiebert I hope you like it. It should give this beauty a couple extra horse powers. It currently takes use 3200ms to process this view. I will give you an update on how the performance improves with concurrent IO. |
|
Sorry for all the commits, I was so lazy I coded it in a browser :P |
|
/ping @ryanhiebert |
There was a problem hiding this comment.
It looks really good to me, so I'm just thinking through to try and see if there are possible unintended consequences. Thank you for your work on this, and for reminding me about it.
Have you tried this code in your own application yet? I'd love some verification that it's working for somebody before I merge and release it.
| install_requires=['six'], | ||
| install_requires=[ | ||
| 'six', | ||
| 'futures; python_version == "2.7"', |
There was a problem hiding this comment.
I didn't know about this syntax. This is cool, and helped me find this article: https://hynek.me/articles/conditional-python-dependencies/
hirefire/procs/__init__.py
Outdated
| with ThreadPoolExecutor() as executor: | ||
| # Execute all procs in parallel to avoid blocking IO | ||
| # especially celery which needs to open a transport to AMQP. | ||
| return list(executor.map(_run, procs.items())) |
There was a problem hiding this comment.
Is there any downside to this? Are the app and connection used by Celery thread-safe? We use the same app that celery does to get the connection, etc. I don't want to end up flooding the RabbitMQ server with connections, and I'm just trying to think of other possible downsides. I'm always slow to use threads, because I don't use them very often and I don't have a lot of practice with thinking through possible issues with them.
There was a problem hiding this comment.
That is a good point. I recently had an issue with connection stacking regarding the DB even if the DB was not used, see revsys/django-health-check#182
I checked both amqp and librabbitmq which uses rabbitmq-c which seem to be thread safe.
The DB issue only happens in configuration edge cases, but I am happy to add the bit that ensures they are closed if you want me to.
There was a problem hiding this comment.
Does that mean that you have not yet used this branch in production? How will the affect other Procs, for queues other than Celery?
There was a problem hiding this comment.
The only other Proc we use uses Redis. I can not vouch that this will not have any side effects on other third party packages.
We are currently not using it on production, I will setup a staging environment tho and stress test the endpoint.
Can you assign the PR to me, that way I won't for get. I am going on vacation on Friday. I don't know if I can manage before I leave.
There was a problem hiding this comment.
@codingjoe : Have you been able to test it in a staging or production environment?
|
Perhaps this needs to be a setting, so that if we find that there are bugs with race conditions, etc, it will only affect those who have opted into using this feature. It does seems like a pretty cool thing to do. |
|
I'd really like this to be behind a setting. @codingjoe : are you willing to add such a setting, so that we can be sure that things won't break for existing users, even if there's some weirdness in one of the backend types that isn't compatible with threads? |
|
That's a great idea. I'll do that. I will be disabled by default to ensure backwards compatibility. |
|
I have Slack periodically reminding me about this, so it doesn't fall off my radar, so I'm passing that reminder along to you, @codingjoe . Do you have an idea of when you might be able to look at it, so I don't keep bugging you when you know you won't be able to get to it yet? |
|
@ryanhiebert I am on DjangoCon now, have plenty time to pick up my pending contributions. I'll work on it right now. |
|
@ryanhiebert do you want this to be a Django setting? It's tricky, since it's implemented in the core library. I might need to inherit some parts in the contrib section to ensure it's working in Django properly. |
|
@ryanhiebert ok, this should do it. BTW, I say the the custom json encoder is not really needed anywhere since you don't encode datetimes. You might want to consider dropping it. |
|
Oh I also took the liberty to use a |
|
ping @ryanhiebert |
|
I'm sorry that I haven't responded on this. The design looks great. It turns out this is going to break my own usage of the library, because in my $WORK code I use that It looks really good, despite the hangup that has been slowing me down, and I think it's the right way to go. |
|
@ryanhiebert little anecdote: This change + disabling worker pool inspection increased performance on our site by 206x from >3s to <15ms ;) |
|
Sweet! The worker inspection is what really kills the time, so that doesn't surprise me in the least. I'm not sure, though, that you necessarily gained much just from this change. Problem is that I don't get a reliable number, that won't shut down running tasks, unless I use the worker inspection, so its kinda a non-starter to do that for my use-cases. |
|
Yes, that is right. We run many queues tho. So concurrently did cut it by 10x |
Especially the queue inspection can be currently very slow,
because a transport to the message broker needs to be opened
and messages are exchanged. Especially in multi queue setup
these operations should happen in a concurrent manner.