Skip to content
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

remove use of parallel iterators except in batch methods #308

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

epwalsh
Copy link
Collaborator

@epwalsh epwalsh commented Jun 17, 2020

This is an alternative to #306. It simply removes the use of parallel iterators except within the batch methods (encode_batch, decode_batch). The result would be that the non-batch versions of encode/decode would be safe to use before and after forking.

Surprisingly, this actually improves performance in the encode benchmarks across the board by a HUGE margin in some cases. So this could be a good thing not just for Python safety, but for performance in general.

image

@epwalsh
Copy link
Collaborator Author

epwalsh commented Jun 17, 2020

#187

@n1t0
Copy link
Member

n1t0 commented Jun 29, 2020

We should probably do some more benchmarks for this. This is indeed surprising, but I guess it is highly dependant on the different use cases, and might not reflect the reality.

I was thinking about maybe having some way to limit the use of the parallel iterator only in cases where there are enough stuff to process. Maybe using the same method that we added in #311, while providing a minimum size to activate it for example. What do you think?

@epwalsh
Copy link
Collaborator Author

epwalsh commented Jun 29, 2020

I agree. I guess we could add some benchmarks that vary the size of the input sequence to see if there's an obvious cutoff where parallelization helps.

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.

None yet

2 participants