-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
chore: optimize dictionary access in strip_padding numpy #3994
chore: optimize dictionary access in strip_padding numpy #3994
Conversation
include/pybind11/numpy.h
Outdated
auto name = spec[0].cast<pybind11::str>(); | ||
auto spec_fo = spec[1].cast<tuple>(); | ||
for (auto kv_item : field_dict) { | ||
auto name = reinterpret_borrow<pybind11::str>(kv_item.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this replaces obj.cast<pybind11::str>()
— which does type checking — with reinterpret_borrow<pybind11::str>
— which blindly assumes kv_item.first
is a str
. If that's true (?), and there is some upstream oversight/bug/API change, this could lead to a crash (relatively good) or a silent failure and time-consuming debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwgk Okay, I'll revert that part of the change. the dict_iterator returns an iterator of a std::pair which doesn't have a clean way of handling the steal / borrow distinction. I think you can call cast<> to a pyobject directly on it, but I don't know how it handles that code path to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwgk I've reverted to the older dict api.
Description
Further optimize strip_padding by making construction of field_descriptors faster by adding a ctor, using emplace, and trying to use the native C++ APIs for iterating through the dictionary.
Suggested changelog entry:
* optimize dictionary access in ``strip_padding`` for numpy