Skip to content
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

Add PyDict_GetItem method to PyDict type binding #3964

Open
MarshalX opened this issue Mar 16, 2024 · 3 comments
Open

Add PyDict_GetItem method to PyDict type binding #3964

MarshalX opened this issue Mar 16, 2024 · 3 comments

Comments

@MarshalX
Copy link

This will be helpful in micro optimizations where we don't care about errors and we are okay to get None.

The current implementation of get_item uses PyDict_GetItemWithError which creates a little overhead while taking Python errors.

pyo3/src/types/dict.rs

Lines 431 to 438 in dcba984

match unsafe {
ffi::PyDict_GetItemWithError(dict.as_ptr(), key.as_ptr())
.assume_borrowed_or_opt(py)
.map(Borrowed::to_owned)
} {
some @ Some(_) => Ok(some),
None => PyErr::take(py).map(Err).transpose(),
}

The overhead could be pretty displayed using flame graph:
image

The right side with PyErr::take could gone

The solution is to add new method get_item_TBD which will use PyDict_GetItem instead

@davidhewitt
Copy link
Member

For some context, I changed this in #3330. I removed the "infallible" version but perhaps that was too aggressive and we can bring it back with an awkward name and a strong recommendation not to use it except in performance sensitive cases.

It's possible there are ways that we could make error handling overhead lighter, those would need exploration.

@davidhewitt
Copy link
Member

Possibly relevant: in 3.13 we will have PyDict_GetItemRef which will be faster because error and not-present cases will be distinguishable in the return code. https://docs.python.org/dev/c-api/dict.html#c.PyDict_GetItemRef

So a cleaner option might be to just optimise this method on 3.13 once available.

@davidhewitt
Copy link
Member

Also for freethreaded Python PyDict_GetItemRef is strongly recommended to avoid race conditions, which is a reason not to expose an API wrapping PyDict_GetItem as it is not thread safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants