`extract::<i64>()` failure is extremely slow

[samuelcolvin]

Bug Description

extracting an int is extremely slow when it fails.

Benchmarks:

test extract_int_downcast_fail    ... bench:           1 ns/iter (+/- 0)
test extract_int_downcast_success ... bench:           7 ns/iter (+/- 0)
test extract_int_extract_fail     ... bench:         114 ns/iter (+/- 8) 😱 
test extract_int_extract_success  ... bench:           7 ns/iter (+/- 0)
test extract_str_downcast_fail    ... bench:           1 ns/iter (+/- 0)
test extract_str_downcast_success ... bench:           3 ns/iter (+/- 0)
test extract_str_extract_fail     ... bench:          23 ns/iter (+/- 0)
test extract_str_extract_success  ... bench:           4 ns/iter (+/- 0)

I'm suggesting this difference is big enough to count as a bug, but happy to be corrected.

Steps to Reproduce

Benchmark Code

#[bench]
fn extract_str_extract_success(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let s = PyString::new(py, "Hello, World!") as &PyAny;

        bench.iter(|| {
            let v = black_box(s).extract::<&str>().unwrap();
            black_box(v);
        });
    });
}

#[bench]
fn extract_str_extract_fail(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let d = PyDict::new(py) as &PyAny;

        bench.iter(|| {
            match black_box(d).extract::<&str>() {
                Ok(v) => panic!("should err {}", v),
                Err(e) => black_box(e),
            }
        });
    });
}

#[bench]
fn extract_str_downcast_success(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let s = PyString::new(py, "Hello, World!") as &PyAny;

        bench.iter(|| {
            let py_str = black_box(s).downcast::<PyString>().unwrap();
            let v = py_str.to_str().unwrap();
            black_box(v);
        });
    });
}

#[bench]
fn extract_str_downcast_fail(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let d = PyDict::new(py) as &PyAny;

        bench.iter(|| {
            match black_box(d).downcast::<PyString>() {
                Ok(v) => panic!("should err {}", v),
                Err(e) => black_box(e),
            }
        });
    });
}

#[bench]
fn extract_int_extract_success(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let int_obj: PyObject = 123.into_py(py);
        let int = int_obj.as_ref(py);

        bench.iter(|| {
            let v = black_box(int).extract::<i64>().unwrap();
            black_box(v);
        });
    });
}


#[bench]
fn extract_int_extract_fail(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let d = PyDict::new(py) as &PyAny;

        bench.iter(|| {
            match black_box(d).extract::<i64>() {
                Ok(v) => panic!("should err {}", v),
                Err(e) => black_box(e),
            }
        });
    });
}

#[bench]
fn extract_int_downcast_success(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let int_obj: PyObject = 123.into_py(py);
        let int = int_obj.as_ref(py);

        bench.iter(|| {
            let py_int = black_box(int).downcast::<PyInt>().unwrap();
            let v = py_int.extract::<i64>().unwrap();
            black_box(v);
        });
    });
}


#[bench]
fn extract_int_downcast_fail(bench: &mut Bencher) {
    Python::with_gil(|py| {
        let d = PyDict::new(py) as &PyAny;

        bench.iter(|| {
            match black_box(d).downcast::<PyInt>() {
                Ok(v) => panic!("should err {}", v),
                Err(e) => black_box(e),
            }
        });
    });
}

Backtrace

na

Your operating system and version

MacOS

Your Python version (python --version)

Python 3.10.5

Your Rust version (rustc --version)

rustc 1.68.0-nightly (4781233a7 2023-01-16)

Your PyO3 version

0.18.3

How did you install python? Did you use a virtualenv?

pyenv

Additional Info

No response

[adamreichold]

I think the reason is that for str, we just downcast to PyString and call .to_str() on that, i.e. we support a narrow set of types:

/// Allows extracting strings from Python objects.
/// Accepts Python `str` and `unicode` objects.
impl<'source> FromPyObject<'source> for &'source str {
    fn extract(ob: &'source PyAny) -> PyResult<Self> {
        ob.downcast::<PyString>()?.to_str()
    }
}

For i64 however, we call PyNumber_Index which apparently tries much harder:

impl<'source> FromPyObject<'source> for $rust_type {
    fn extract(ob: &'source PyAny) -> PyResult<$rust_type> {
        let ptr = ob.as_ptr();
        unsafe {
            let num = ffi::PyNumber_Index(ptr);
            if num.is_null() {
                Err(PyErr::fetch(ob.py()))
            } else {
                let result = err_if_invalid_value(ob.py(), !0, $pylong_as_ll_or_ull(num));
                ffi::Py_DECREF(num);
                result
            }
        }
    }
}

As your benchmarks do, you can .downcast::<PyInt>() first before extract to avoid PyNumber_Index trying at all. Other than that, I don't see how we can make extract::<i64>() without regressing the set of allowed source types. In summary, I think this is another case of #2278 which mainly needs someone finding the time to write all these trade-offs down.

[adamreichold]

For what its worth, I did try replacing PyErr::fetch by directly constructing a PyDowncastError and this did not change things, so the time is spent in PyNumber_Index.

So an upstream bug report against CPython as it should be possible to reject PyDict fast enough might improve the situation. Maybe PyNumber_Index stringifies the dict and tries to parse that? 🤷🏽‍♀️

[adamreichold]

But it does not seem to do anything crazy, just checking whether the already is a PyLong and then if it has the __index__ slot filled: https://github.com/python/cpython/blob/2b7027d0b2ee2e102a24a0da27d01b8221f9351c/Objects/abstract.c#L1443

[samuelcolvin]

I tried using py.None() instead of PyDict and the performance is the same - just to say, there's nothing special about PyDict.

[adamreichold]

As written above this is just what supporting the __index__ semantics costs and can be avoided via downcast if the trade-off is known, hence I would prefer to close this as a duplicate of #2278 to keep the issue backlog somewhat manageable.

[samuelcolvin]

Thanks, I think that's fair.

[samuelcolvin]

At the risk of labouring this point, the same behaviour is seen with floats:

test extract_float_downcast_fail     ... bench:           2 ns/iter (+/- 0)
test extract_float_downcast_success  ... bench:           3 ns/iter (+/- 0)
test extract_float_extract_fail      ... bench:          96 ns/iter (+/- 5)
test extract_float_extract_success   ... bench:           3 ns/iter (+/- 0)

This is slightly surprising since I wouldn't expect floats to crop up as much in indexes etc.

[adamreichold]

This is slightly surprising since I wouldn't expect floats to crop up as much in indexes etc.

This is not PyNumber_Index, it is PyFloat_AsDouble, i.e. https://github.com/python/cpython/blob/72c3d2e105f120f6f2bce410699b34dac4e948fd/Objects/floatobject.c#L289, which might call into __index__ or __float__.

Python is still a dynamic language that needs to check type objects, chase pointers to structures full of function pointers and call these function pointers at runtime. At 3.2 GHz, we are talking around 300 cycles for 100 ns which in my humble opinion seems somewhat reasonable for that kind of complexity.

[davidhewitt]

I wonder if this falls into the theme of narrow vs wide conversions which has come up a few times? Is there a theoretical path in these cases which avoids the __index__ conversions and therefore only accepts exactly int or float (respectively for i64 and f64)?

We could define newtypes Exact<i64> (bikesheddable) with these optimized FromPyObject conversions, perhaps. Or introduce proc macro options to specify narrow or wide conversions.

[adamreichold]

We could define newtypes Exact (bikesheddable) with these optimized FromPyObject conversions, perhaps. Or introduce proc macro options to specify narrow or wide conversions.

We could, but one can already avoid the costs using and do narrow conversions using downcast (PyNumber_Index is fast when called on PyInt/Long). The main problem here IMHO is discoverability, just knowing these trade-offs exist and how to make the choice.

[samuelcolvin]

The main problem here IMHO is discoverability, just knowing these trade-offs exist and how to make the choice.

agreed.

Worth noting that with float, it's not quite as simple as PyFloat::is_type_of(x) at least for us because we want Decimals to be coerced to floats as well as floats - this is covered by extract, but obviously not by is_type_of / is_instance.

Do you know if there's any way to extract floats, including decimals without using a plain extract::<f64>()? I tried importing Decimal, storing it in a GILOnceCell and doing an is_instance check, but it was slower.