-
-
Notifications
You must be signed in to change notification settings - Fork 38
[FEATURE] Makes entries being loaded in parallel #74
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
Conversation
This patch is mainly inspired from the work of @inrinder (see `2bbc2dae`). You may notice an execution up to twice as fast. This new behavior could be disabled with the new `parallel_loading` configuration option. Co-authored-by: Michael Bromilow <12384431+ingrinder@users.noreply.github.com>
Since we're executing I/O-heavy work, should we use |
You're actually right 👍 executor = cm_stack.enter_context(
- ThreadPoolExecutor(max_workers=((os.cpu_count() or 1) * 5))
+ ThreadPoolExecutor(max_workers=min(len(enabled_entries), (os.cpu_count() or 1) + 4))
) Reference : python/cpython#13618 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried that limiting threads based on CPU count could slow down execution for the longer-running entries, however after some testing (and fun playing with matplotlib! 😆) on an older Core 2 Duo machine it looks like it's just fine 👍.
Just a moment - just noticed an (admittedly stupid) edge case we should deal with.
...maybe *someone* just wants the logo?
Nice catch ! Thanks 🙂 |
This patch is mainly inspired from the work of @ingrinder (see
2bbc2dae
).You may notice an execution up to twice as fast.
This behavior could be disabled with the new
parallel_loading
configuration option.See #68.
How has this been tested ?
Locally & test cases.
Types of changes :
Checklist :
[ ] [IF NEEDED] I have updated the test cases (which pass) accordingly ;