update `extract_argument` to use Bound APIs by davidhewitt · Pull Request #3708 · PyO3/pyo3

davidhewitt · 2023-12-28T21:36:20Z

Builds upon #3681 to move our internal argument extraction mechanisms off the GIL Pool.

Only the last commit is new, and I don't think this is worth reviewing right until #3681 is in, after which I'll rebase and tidy this a bit further. Pushing now purely because I think this should hopefully demonstrate a tantalising performance speedup which we're so close to merging! 😂

codspeed-hq · 2023-12-28T22:06:42Z

CodSpeed Performance Report

Merging #3708 will improve performances by 13.02%

_{Comparing davidhewitt:extract-arg2 (101563c) with main (a15e4b1)}

Summary

⚡ 2 improvements
✅ 65 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`davidhewitt:extract-arg2`	Change
⚡	`test_simple_args_kwargs_rs`	35.3 µs	31.9 µs	+10.64%
⚡	`test_simple_kwargs_rs`	35 µs	31 µs	+13.02%

davidhewitt · 2023-12-28T22:47:52Z

Hmm I'll investigate later why this hasn't yielded the speedup I've seen in #3606, there's likely some other part I need to merge first...

davidhewitt · 2023-12-29T08:00:37Z

Ah, I think it's PyDict::new_bound which is missing, I'll open a PR for that once we've merged the feature proposed in #3707

davidhewitt · 2024-02-03T21:23:21Z

The performance regressions with the ordered_X benchmarks are genuine and look like they're caused because part of the proc macros go through &PyCell<T> GIL Ref machinery, and the compatibility implementation of FromPyObject::extract_bound is hurting this.

Probably the right approach is to get this PR merged (well, needs #3784 reviewed and merged first) and also #3792. Then I think there will be enough pieces in place to deal with updating the proc macro internals in a follow-up.

davidhewitt · 2024-02-05T13:38:05Z

This is getting closer to being reviewable, but I think first it was worth splitting out #3801 and #3802.

davidhewitt

Whew, I think this is finally ready to review and I'll deal with the PyRef/PyCell changes separately.

src/instance.rs

src/err/mod.rs

davidhewitt · 2024-02-17T00:49:28Z

pytests/src/pyfunctions.rs

+type Any<'py> = Bound<'py, PyAny>;
+type Dict<'py> = Bound<'py, PyDict>;
+type Tuple<'py> = Bound<'py, PyTuple>;


Clippy was warning about type complexity in the return value tuples without defining these aliases.

davidhewitt · 2024-02-17T00:51:13Z

src/types/string.rs

    #[cfg(any(Py_3_10, not(Py_LIMITED_API)))]
    #[allow(clippy::wrong_self_convention)]
-    fn to_str(self) -> PyResult<&'a str> {
+    pub(crate) fn to_str(self) -> PyResult<&'a str> {


Used in keyword argument extraction from borrowed keyword names.

davidhewitt · 2024-02-17T00:59:31Z

src/types/dict.rs

+impl<'py> Bound<'py, PyDict> {
+    /// Iterates over the contents of this dictionary without incrementing reference counts.
+    ///
+    /// # Safety
+    /// It must be known that this dictionary will not be modified during iteration.
+    pub(crate) unsafe fn iter_borrowed<'a>(&'a self) -> BorrowedDictIter<'a, 'py> {
+        BorrowedDictIter::new(self)
+    }
+}


The purpose of adding this crate-private API was that I wanted to keep keyword argument processing using Borrowed for both the "fastcall" and default calling conventions. As far as I am aware, argument processing can safely process a kwargs dict in this way because it's the only referee of the kwargs dict.

davidhewitt · 2024-02-17T08:02:10Z

pyo3-macros-backend/src/method.rs

+                        let result = #call;
+                        result


These changes to return result on a separate line were caused by the fact that #call creates a borrow on _args and _kwargs, which now have Drop implementations due to being Bound<PyTuple> and Option<Bound<PyDict>> (if *args, or **kwargs are accepted).

So to satisfy the borrow checker we had to split the expression like this.

I think given our decision in macro code is to trend away from a single giant expression anyway, and it'll help me in #3847 where I need to do this too, this seemed a reasonable adjustment to me.

src/impl_/extract_argument.rs

LilyFirefly

I spotted one nit.

src/err/mod.rs

Co-authored-by: Lily Foote <code@lilyf.org>

Icxolu · 2024-02-17T11:47:04Z

src/impl_/extract_argument.rs

-        if let Some(kwnames) = py.from_borrowed_ptr_or_opt::<PyTuple>(kwnames) {
+
+        // Safety: kwnames is known to be a pointer to a tuple, or null
+        let kwnames: &'py Option<Bound<'py, PyTuple>> = std::mem::transmute(&kwnames);


Maybe we want to create a Bound::ref_from_ptr_or_opt to avoid std::mem::transmute?

In the end I adjusted the iter_borrowed() implementations to allow calling them from a Borrowed instance, and then I was able to use Borrowed::from_ptr methods in this code here. I think it's fine to use Borrowed instead of ref_from_ptr here because we can control that pesky borrowed lifetime 'a without worrying about user code, unlike the macros.

src/impl_/extract_argument.rs

davidhewitt · 2024-02-19T09:06:22Z

Thanks for the reviews! I've removed the PyArg newtype following #3858, which has made this implementation a fair bit nicer on the macro side. I'll try to find time to tidy the rest up later.

LilyFirefly

Looking good! Just a few nitpicks.

src/impl_/extract_argument.rs

src/types/dict.rs

Icxolu

I spotted a few Borrowed::from_ptr calls in proc macro generated code. It might be nice to replace them with BoundRef if possible, for a bit of extra safety. Otherwise LGTM!

pyo3-macros-backend/src/method.rs

pyo3-macros-backend/src/params.rs

pyo3-macros-backend/src/pymethod.rs

davidhewitt · 2024-02-27T19:05:42Z

Thanks both, I'll aim to tidy this up tomorrow 👍

Co-authored-by: Lily Foote <code@lilyf.org>

davidhewitt · 2024-02-28T19:36:17Z

Thanks both, think we're finally there on this one!

MarshalX · 2024-02-28T20:31:47Z

Hmm I'll investigate later why this hasn't yielded the speedup I've seen in #3606, there's likely some other part I need to merge first...

What is the latest result btw?

davidhewitt · 2024-02-28T20:35:01Z

#3708 (comment), but I think it's quite likely that parts of the speedup already got merged now, given how many times this got split up.

I will probably analyze again some time and see if there's any further tricks to apply here, but for now I think this is good.

davidhewitt force-pushed the extract-arg2 branch from 18226c6 to 6668785 Compare December 31, 2023 08:07

davidhewitt added the CI-skip-changelog Skip checking changelog entry label Dec 31, 2023

davidhewitt force-pushed the extract-arg2 branch 3 times, most recently from 3555727 to 5dc78b7 Compare December 31, 2023 22:38

davidhewitt force-pushed the extract-arg2 branch 3 times, most recently from 36189d4 to 1fe867b Compare February 3, 2024 07:09

davidhewitt force-pushed the extract-arg2 branch 2 times, most recently from befcd39 to 5591f9b Compare February 5, 2024 13:26

This was referenced Feb 5, 2024

add PyStringMethods::encode_utf8 #3801

Merged

add PyBackedStr and PyBackedBytes #3802

Merged

davidhewitt force-pushed the extract-arg2 branch from 5591f9b to dcd20f4 Compare February 12, 2024 23:29

davidhewitt mentioned this pull request Feb 14, 2024

allow from_py_with on function args to take a fn(&Bound) -> PyResult #3837

Merged

davidhewitt force-pushed the extract-arg2 branch 2 times, most recently from 1acb54d to 2f956f4 Compare February 17, 2024 00:40

update extract_argument to use Bound APIs

1301ee1

davidhewitt force-pushed the extract-arg2 branch from 2f956f4 to 1301ee1 Compare February 17, 2024 00:56

davidhewitt commented Feb 17, 2024

View reviewed changes

davidhewitt marked this pull request as ready for review February 17, 2024 01:00

davidhewitt mentioned this pull request Feb 17, 2024

New Bound API implementation tracking issue #3684

Closed

63 tasks

tidy up borrow in macros expression

7cb8e99

davidhewitt commented Feb 17, 2024

View reviewed changes

update trybuild output

2d07582

davidhewitt commented Feb 17, 2024

View reviewed changes

src/impl_/extract_argument.rs Outdated Show resolved Hide resolved

LilyFirefly reviewed Feb 17, 2024

View reviewed changes

src/err/mod.rs Outdated Show resolved Hide resolved

more concise form for DowncastError::new

9a599c3

Co-authored-by: Lily Foote <code@lilyf.org>

Icxolu reviewed Feb 17, 2024

View reviewed changes

davidhewitt mentioned this pull request Feb 18, 2024

add Bound API constructors from borrowed pointers #3858

Merged

davidhewitt added 2 commits February 19, 2024 08:36

Merge branch 'main' into extract-arg2

6badf8e

use Borrowed instead of newtype

22f960f

davidhewitt mentioned this pull request Feb 20, 2024

Tidy some usage of py.from_borrowed_ptr and py.from_borrowed_ptr_or_opt #3877

Merged

davidhewitt force-pushed the extract-arg2 branch from 6490983 to 31616ed Compare February 20, 2024 08:49

use Borrowed::from_ptr methods in extract_argument

89913e8

davidhewitt force-pushed the extract-arg2 branch from 31616ed to 89913e8 Compare February 20, 2024 08:49

davidhewitt added 2 commits February 26, 2024 23:19

Merge branch 'main' into extract-arg2

56260a8

update UI tests

9ec7084

LilyFirefly approved these changes Feb 27, 2024

View reviewed changes

src/impl_/extract_argument.rs Outdated Show resolved Hide resolved

src/impl_/extract_argument.rs Outdated Show resolved Hide resolved

src/impl_/extract_argument.rs Outdated Show resolved Hide resolved

src/types/dict.rs Show resolved Hide resolved

Icxolu approved these changes Feb 27, 2024

View reviewed changes

pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved

pyo3-macros-backend/src/params.rs Outdated Show resolved Hide resolved

pyo3-macros-backend/src/pymethod.rs Outdated Show resolved Hide resolved

davidhewitt and others added 3 commits February 28, 2024 07:08

avoid double-negative #[cfg] clauses

306aa81

Co-authored-by: Lily Foote <code@lilyf.org>

Merge branch 'main' into extract-arg2

108f1e7

review: LilyFoote, Icxolu feedback

101563c

LilyFirefly approved these changes Feb 28, 2024

View reviewed changes

Icxolu approved these changes Feb 28, 2024

View reviewed changes

davidhewitt added this pull request to the merge queue Feb 28, 2024

Merged via the queue into PyO3:main with commit 8a12970 Feb 28, 2024

davidhewitt deleted the extract-arg2 branch February 28, 2024 20:29

Conversation

davidhewitt commented Dec 28, 2023

Uh oh!

codspeed-hq bot commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #3708 will improve performances by 13.02%

Summary

Benchmarks breakdown

Uh oh!

davidhewitt commented Dec 28, 2023

Uh oh!

davidhewitt commented Dec 29, 2023

Uh oh!

davidhewitt commented Feb 3, 2024

Uh oh!

davidhewitt commented Feb 5, 2024

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidhewitt Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

davidhewitt Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

davidhewitt Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

davidhewitt Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LilyFirefly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Icxolu Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

davidhewitt Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidhewitt commented Feb 19, 2024

Uh oh!

LilyFirefly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Icxolu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidhewitt commented Feb 27, 2024

Uh oh!

davidhewitt commented Feb 28, 2024

Uh oh!

MarshalX commented Feb 28, 2024

Uh oh!

davidhewitt commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codspeed-hq bot commented Dec 28, 2023 •

edited

Loading