-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PERF/TYP: typing cast in __getitem__ gives quite some overhead #44624
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
PERF/TYP: typing cast in __getitem__ gives quite some overhead #44624
Conversation
wow. ive been assuming that the casts are negligible. if that wrong, then there are probably a lot of small improvements to be made by avoiding them across the codebase. On the other hand, most of this is in the Union:
So at least in this case we can get most of the benefit by just doing the Union once at the module level |
Another (maybe easier) solution is to just put the type in double quotes: result = cast(
"Union[DatetimeLikeArrayT, DTScalarOrNaT]", super().__getitem__(key)
) I did a grep exercise, and that is what is done elsewhere in the code. The only other place I could find (via grep) that we need to fix this is in |
@jorisvandenbossche want to change the annotation to a string and call this a win? |
@jorisvandenbossche can you also make a similar change in |
Yeah, I had actually searched for other casts in the /arrays submodule, and this categorical one is the only other cast with a composite type (or that is not predefined). Since that's not typically a hot path, I didn't change it, but of course also can't harm, so pushed that change now anyway. |
While doing some unrelated profiling, I noticed typing overhead in
DatetimeArray.__getitem__
. Isolating that here, there is a typingcast(..)
call in that method, and removing that as done in the PR, I see the following impact:(I did this timing several times back and forth on a computer with no other activity at that moment, and the difference was stable)
Personally, I wouldn't have expected such a relative big impact of a typing cast. Because if that's the case, we should be very careful with using that in "hot paths".
cc @jbrockmendel @simonjayhawkins @Dr-Irv