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. |
11 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
NicolasHug commentedMay 21, 2020
•
edited
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: