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

Type annotation of _reevaluate_occupancy_worker #4398

Merged
merged 7 commits into from
Jan 7, 2021

Conversation

jakirkham
Copy link
Member

Does some light type annotation of _reevaluate_occupancy_worker to improve the Cython code generated for this function. This cuts down on the amount of time spent in this method when determining what tasks to steal.

@mrocklin
Copy link
Member

mrocklin commented Jan 4, 2021

+1

@jakirkham jakirkham force-pushed the opt_reev_occ_wrk branch 5 times, most recently from 97f2ecc to 4279b44 Compare January 5, 2021 00:05
There is no need to retrieve this value when we already have it. So just
go ahead and return the variable directly.
This way if it is `None`, we can check that and handle it much faster
(both in Python and Cython/C).
Hopefully should order computation to avoid a round off error.
This should speed up mathematical operations with these variables.
@jakirkham jakirkham force-pushed the opt_reev_occ_wrk branch 2 times, most recently from bec92c2 to 49db986 Compare January 5, 2021 19:38
@jacobtomlinson
Copy link
Member

The CI failures here appear to be unrelated, they are the same on master.

@jakirkham
Copy link
Member Author

The CI failures here appear to be unrelated, they are the same on master.

Submitted PR ( #4405 ) to just bump the timeout. Not sure if that is acceptable or if we would rather do something else.

@jakirkham
Copy link
Member Author

Any other thoughts here? 🙂

@jacobtomlinson
Copy link
Member

Nope LGTM

@jacobtomlinson jacobtomlinson merged commit 8f33b9e into dask:master Jan 7, 2021
@jakirkham jakirkham deleted the opt_reev_occ_wrk branch January 7, 2021 16:59
@jakirkham
Copy link
Member Author

Thanks all! 😄

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.

3 participants