Improve performance in ActiveStorage::Service::MirrorService
#51740
Conversation
| @@ -30,6 +30,13 @@ def self.build(primary:, mirrors:, name:, configurator:, **options) # :nodoc: | |||
|
|
|||
| def initialize(primary:, mirrors:) | |||
| @primary, @mirrors = primary, mirrors | |||
| @executor = Concurrent::ThreadPoolExecutor.new( | |||
There was a problem hiding this comment.
Can you do a performance benchmarking?
There was a problem hiding this comment.
Sure! I'll add performance benchmark and request review to you.
There was a problem hiding this comment.
@akhilgkrishnan
This is performance benchmark using GCS. In my opinion, there is a performance improvement when looking at real time.
-
repeat time: 10
-
Used service: Google Cloud Storage (region: ap-northeast-3)
-
Used image size: 512kb
-
primary: 1, mirrors: 2
| Task | Real Time | User Time | System Time | Total Time |
|---|---|---|---|---|
| Parallel | 0.453 | 0.055 | 0.015 | 0.070 |
| Sequential | 0.979 | 0.045 | 0.011 | 0.056 |
- primary: 1, mirrors: 1
| Task | Real Time | User Time | System Time | Total Time |
|---|---|---|---|---|
| Parallel | 0.380 | 0.053 | 0.020 | 0.074 |
| Sequential | 0.624 | 0.033 | 0.008 | 0.041 |
There was a problem hiding this comment.
@akhilgkrishnan Could you please review when you get a chance? 😄
There was a problem hiding this comment.
@akhilgkrishnan Gentle ping 🙏
There was a problem hiding this comment.
Sorry @heka1024 for the late response. This looks good to me. Lets wait for someone from core to review.
heka1024
force-pushed
the
parallel-upload-in-mirror-service
branch
from
00febe1 to
ecfbb5e
Compare
…ervice#delete` and `ActiveStorage::Service::MirrorService#delete_prefixed`
heka1024
force-pushed
the
parallel-upload-in-mirror-service
branch
from
ecfbb5e to
3f2258a
Compare
| service.public_send method, *args | ||
| tasks = each_service.collect do |service| | ||
| Concurrent::Promise.execute(executor: @executor) do | ||
| service.public_send method, *args |
There was a problem hiding this comment.
I don't think we can guarantee the downstream services are thread-safe, and this could have breaking consequences.
Even the built-in services, I'm not sure, but if the developer is using a custom service they may not realize that it needs to be thread-safe.
I'm also not sure introducing a thread-pool here is cost-effective, we should measure it under more realistic load. Also given the defaults you set, makes me think the best approach is to make this opt-in and configurable.
That FIXME has existed since the beginning (19a5191), so there are many many applications that are accepting the current performance footprint (albeit room for improvement here). Was there a reason you wanted to add this, or just looking for TODOs to work on?
There was a problem hiding this comment.
@zzak Actually, I found this TODO while reading the code and worked on it. I haven't experienced performance issues in my application.
I think there are two options:
- Make this configurable and keep the default as the non-threaded version.
- Close this PR and remove the TODO comment.
What do you think?
There was a problem hiding this comment.
I like to think of opening PRs sometimes as asking a question. Let's consider this PR the form of "should mirror service use threads?" and see what feedback we get.
I would wait for any more feedback before changing it like option 1, I was just thinking of possible scenarios.
If you don't hear back after a while feel free to reach out.
Thanks for your PR!
There was a problem hiding this comment.
I think we should just assume all services are thread-safe. Rails is entirely thread-safe, and we should push the community to be.
Motivation / Background
This Pull Request has been created to remove
FIXMEcomment ofActiveStorage::Service::MirrorService.Detail
This Pull Request use thread pool to parallelize
deleteanddelete_prefixedinActiveStorage::Service::MirrorService.Additional information
If this is a nit performance boost than complexity, how about remove comment?
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]