-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
Cython and memviews creation #17299
Comments
Shall we pin this issue (at least to easily come back to the discussion)? |
Sounds like cython/cython#3617 might be a fix for some of it? |
yeah, I updated the comment |
FYI I don't think cython/cython#3617 will really help with speed here - it just makes
equivalent (speed-wise) to
You'd still be better off using your second version if you really need the best performance. |
Slicing memoryviews also comes with some more operations to setup new memoryviews. Even if there's little interactions with the CPython interpreter, this might still be costly, especially if slicing is used in Generally, I think creating memoryview might be costly in |
See the reasons here: scikit-learn#17299
See the reasons here: scikit-learn#17299
Not an issue, just a Cython-related PSA that we need to keep in mind when reviewing PRs:
We shouldn't create 1d views for each sample, this is slow:
do this instead, or use pointers, at least for now:
This is valid for any pattern that generates lots of views so looping over features might not be a good idea either if we expect lots of features.
There might be a "fix" in cython/cython#2227 / cython/cython#3617
The reason is that there's a significant overhead when creating all these 1d views, which comes from Cython internal ref-counting (details at cython/cython#2987). In the hist-GBDT prediction code, this overhead amounts for more than 30% of the runtime so it's not negligible.
Note that:
prange
used to generate some additional Python interactions, but this was fixed in cython/cython@794d21d and backported to Cython 0.29CC @scikit-learn/core-devs
The text was updated successfully, but these errors were encountered: