Merge #2954

2954: optimize mapping conversion for dict r=davidhewitt a=davidhewitt

Equivalent of #2944 for dicts -> mapping.

The benchmark diff is not as significant as in #2944. Still something like 80ns -> 2ns on my machine.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

benches/bench_dict.rs

@@ -1,7 +1,7 @@
 use criterion::{criterion_group, criterion_main, Bencher, Criterion};
 
-use pyo3::prelude::*;
 use pyo3::types::IntoPyDict;
+use pyo3::{prelude::*, types::PyMapping};
 use std::collections::{BTreeMap, HashMap};
 
 fn iter_dict(b: &mut Bencher<'_>) {
@@ -63,6 +63,19 @@ fn extract_hashbrown_map(b: &mut Bencher<'_>) {
     });
 }
 
+fn mapping_from_dict(b: &mut Bencher<'_>) {
+    Python::with_gil(|py| {
+        const LEN: usize = 100_000;
+        let dict = (0..LEN as u64)
+            .map(|i| (i, i * 2))
+            .into_py_dict(py)
+            .to_object(py);
+        b.iter(|| {
+            let _: &PyMapping = dict.extract(py).unwrap();
+        });
+    });
+}
+
 fn criterion_benchmark(c: &mut Criterion) {
     c.bench_function("iter_dict", iter_dict);
     c.bench_function("dict_new", dict_new);
@@ -72,6 +85,8 @@ fn criterion_benchmark(c: &mut Criterion) {
 
     #[cfg(feature = "hashbrown")]
     c.bench_function("extract_hashbrown_map", extract_hashbrown_map);
+
+    c.bench_function("mapping_from_dict", mapping_from_dict);
 }
 
 criterion_group!(benches, criterion_benchmark);

newsfragments/2954.changed.md

@@ -0,0 +1 @@
+Optimize `PyMapping` conversion for `dict` inputs.

src/types/mapping.rs

@@ -3,12 +3,8 @@
 use crate::err::{PyDowncastError, PyErr, PyResult};
 use crate::once_cell::GILOnceCell;
 use crate::type_object::PyTypeInfo;
-use crate::types::{PyAny, PySequence, PyType};
+use crate::types::{PyAny, PyDict, PySequence, PyType};
-use crate::{
+use crate::{ffi, AsPyPointer, IntoPyPointer, Py, PyNativeType, PyTryFrom, Python, ToPyObject};
-    ffi, AsPyPointer, IntoPy, IntoPyPointer, Py, PyNativeType, PyTryFrom, Python, ToPyObject,
-};
-
-static MAPPING_ABC: GILOnceCell<PyResult<Py<PyType>>> = GILOnceCell::new();
 
 /// Represents a reference to a Python object supporting the mapping protocol.
 #[repr(transparent)]
@@ -119,17 +115,14 @@ impl PyMapping {
     }
 }
 
-fn get_mapping_abc(py: Python<'_>) -> Result<&PyType, PyErr> {
+static MAPPING_ABC: GILOnceCell<Py<PyType>> = GILOnceCell::new();
+
+fn get_mapping_abc(py: Python<'_>) -> PyResult<&PyType> {
     MAPPING_ABC
-        .get_or_init(py, || {
+        .get_or_try_init(py, || {
-            Ok(py
+            py.import("collections.abc")?.getattr("Mapping")?.extract()
-                .import("collections.abc")?
-                .getattr("Mapping")?
-                .downcast::<PyType>()?
-                .into_py(py))
         })
-        .as_ref()
+        .map(|ty| ty.as_ref(py))
-        .map_or_else(|e| Err(e.clone_ref(py)), |t| Ok(t.as_ref(py)))
 }
 
 impl<'v> PyTryFrom<'v> for PyMapping {
@@ -139,11 +132,15 @@ impl<'v> PyTryFrom<'v> for PyMapping {
     fn try_from<V: Into<&'v PyAny>>(value: V) -> Result<&'v PyMapping, PyDowncastError<'v>> {
         let value = value.into();
 
-        // TODO: surface specific errors in this chain to the user
+        // Using `is_instance` for `collections.abc.Mapping` is slow, so provide
-        if let Ok(abc) = get_mapping_abc(value.py()) {
+        // optimized case dict as a well-known mapping
-            if value.is_instance(abc).unwrap_or(false) {
+        if PyDict::is_type_of(value)
-                unsafe { return Ok(value.downcast_unchecked()) }
+            || get_mapping_abc(value.py())
-            }
+                .and_then(|abc| value.is_instance(abc))
+                // TODO: surface errors in this chain to the user
+                .unwrap_or(false)
+        {
+            unsafe { return Ok(value.downcast_unchecked()) }
         }
 
         Err(PyDowncastError::new(value, "Mapping"))