-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Optimize valid_workers
#4329
Optimize valid_workers
#4329
Conversation
97512a3
to
27639d3
Compare
distributed/scheduler.py
Outdated
|
||
if not valid_workers and not ts._loose_restrictions and self.workers: | ||
if valid_workers == set() and not ts._loose_restrictions and self.workers: |
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'm surprised that it makes sense to create an empty set each time.
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.
Yeah I think you are right; we can do better. Will push a fix.
In [4]: %timeit s == set()
91.3 ns ± 0.747 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [5]: %timeit s is not None and not s
37.3 ns ± 0.373 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
(tested with s = set()
)
27639d3
to
d259df2
Compare
+1 |
d259df2
to
868b15f
Compare
This works better if we want to type `s` as `set`. Also this is a more typical default value when initializing a variable unlike `True`.
868b15f
to
b2eaf60
Compare
+1 |
Thanks Matt! 😄 |
Annotates
valid_workers
for Cython optimization. In particular ensure it always returns aset
orNone
(in case all workers are viable instead ofTrue
as before). Also use unique variables when they have already been assigned or if new results may have different types than before. This makes it easier to annotate these variables and for Cython to then optimize usage thereof.