Skip to content

use workers pool in feature-extractor#193

Merged
smacker merged 3 commits intosrc-d:masterfrom
smacker:pool
Feb 7, 2019
Merged

use workers pool in feature-extractor#193
smacker merged 3 commits intosrc-d:masterfrom
smacker:pool

Conversation

@smacker
Copy link
Copy Markdown
Contributor

@smacker smacker commented Feb 7, 2019

Feature extraction is CPU bound task.
With increase of grpc workers it hits GIL.

@smacker smacker requested a review from se7entyse7en February 7, 2019 14:14
Feature extraction is CPU bound task.
With increase of grpc workers it hits GIL.

Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
Copy link
Copy Markdown
Contributor Author

smacker commented Feb 7, 2019

Performance testing:

Server started with 30 grpc workers.
In multiprocessing mode it had 5 workers in the pool.

3000 calls with 30 concurrent requests:
without multiprocessing: finished in 39.666251747s
with multiprocessing: finished in 23.718774715s

Signed-off-by: Maxim Sukharev <max@smacker.ru>
}

def __init__(self, pool):
super(Service, self)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you that this is working? This should be:

Suggested change
super(Service, self)
super(Service, self).__init__()

Maybe it works because in the service_pb2_grpc.FeatureExtractorServicer class the init is actually a noop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♀️ fixed

while True:
time.sleep(_ONE_DAY_IN_SECONDS)
except KeyboardInterrupt:
pool.terminate()
Copy link
Copy Markdown

@se7entyse7en se7entyse7en Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be called for exceptions different from KeyboardInterrupt. Actually I don't know whether it is going to leak some resources. Maybe you could use this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All sub-processes are created using fork. Operation system is going to take care of cleaning up when the server exits. So it's not a big deal. Also, we run feature-extractor in the container and any exit would cause restart of the container which would clean-up everything.

I don't think we need to improve termination here as long as we don't have any real issue with how it works right now.

Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker smacker merged commit 443779d into src-d:master Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants