Annotate check_idle_saturated for Cythonization
#4289
Conversation
Combine assignments and separate them from `if` blocks.
To make it easier to type `occ` later, define this as a non-`None` value, which is also clearly bogus (namely `-1.0`). That way we can still replace this value when it hasn't been otherwise set while also including a type that includes the default value.
jakirkham
force-pushed
the
cy_check_idle_saturated
branch
from
3b30189
to
92806bd
Compare
For now to just see what is possible, assign these attributes to local variables typed as `set`s. This will allow Cython to use the corresponding Python C APIs with these objects; thus, optimizing how they are handled. This saves us temporarily from trying to more generally Cythonize this class while exploring optimizations here.
As `self.idle` is actually a `SortedSet` instead of a `set`, we can't type it. So revert the typing of `idle`. Though leave all other `set`-based typing for other optimizations to be applied where possible. Also leave the assignment of `self.idle` to `idle` as this generates the attribute access code only once and we need this for both branches anyways.
jakirkham
force-pushed
the
cy_check_idle_saturated
branch
from
92806bd
to
d540c5c
Compare
|
The sortedcontainers issue is interesting. It is convenient for scheduling logic to keep that around, but I'll be curious to learn how much that hurts us long-term. The one other issue to consider is if we want to start including cython/ctypes level typing in the scheduler. This is still an experimental effort and so maybe a step backwards (readability gets mangled a bit). I think it's probably still ok because it'll be easy to strip out later on if we decide that to go in a different direction. I'm cc'ing @jcrist though, who I think is the person most likely to object to adding annotations like these. |
No objections from me. I find running mypy in CI a bit onerous, but adding annotations for optional cythonization seems totally fine. |
|
OK, in that case this looks good to me. @jakirkham I'm happy to merge this. I notice that you still have the WIP flag up so I'll wait a bit unless you want to give the go-ahead. Alternatively, please merge at will. |
jakirkham
changed the title
check_idle_saturated for Cythonizationcheck_idle_saturated for Cythonization
|
Thanks all! 😄 I think this is ready. May come back to this function after we have typed some of the |
In our profiling of the scheduler, we identified to main transitions that took a good chunk of time. These are
transition_processing_memoryandtransition_waiting_processing. The former takes slightly longer than the other, but both are easily 2x slower than any transition that follows them. Both of them directly or indirectly make a call tocheck_idle_saturated. While this is not necessarily the worst bottleneck for either of them, it does stick out on the callgraph and stands a good chance of improving both transitions runtimes at once. Additionallycheck_idle_saturatedincludes a fair bit of code that simply crunches numbers and does not touch Python objects as much. It also isn't as dependent on the Cythonization of other classes as other functions in the profile are. So this makes it easier to Cythonize this piece of code without needing to touch too much other code.Here we go through and annotate the local variables with types. Also we assign non-local variables accessed through attributes to local variables, which we type. Additionally we changed default argument values to be more friendly with C-style types. Initially we tried to type
self.idle. However asself.idleis asortedcontainers.SortedSet, this didn't work (as we need an actual Pythonsetto type it). So we left this as untyped.Should add as a good chunk of the time in
check_idle_saturatedis just spent doingws.status == Status.closed, we still need PR ( #4270 ) to cutdown on the time spent in this method.