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

gh-112069: Adapt set/frozenset methods to Argument Clinic #115112

Merged
merged 15 commits into from
Feb 8, 2024

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Feb 6, 2024

This is the first to step to making the set object thread-safe in free threading Python. See this PR for reference. As suggested, I am splitting the PR into one which first adds AC support and then a followup PR which adds thread safety.

@tomasr8
Copy link
Member Author

tomasr8 commented Feb 6, 2024

@erlend-aasland Is there a way to convert the number functions (like set_sub, etc.) or functions defined on the type itself (like set_repr) or do I leave them as is?

@erlend-aasland
Copy link
Contributor

@erlend-aasland Is there a way to convert the number functions (like set_sub, etc.) or functions defined on the type itself (like set_repr) or do I leave them as is?

You can only adapt PyMemberDef, PyGetSetDef, and Py_tp_new, Py_tp_init C functions. Eventually, we will add support for slots, but for now only __init__ and __new__ are supported.

@erlend-aasland
Copy link
Contributor

See #114258 for clinic support for slots.

Co-authored-by: AN Long <[email protected]>
@tomasr8
Copy link
Member Author

tomasr8 commented Feb 7, 2024

See #114258 for clinic support for slots.

Thanks for the context! So if I understand correctly some of the tp_ methods will become available in AC at some point. Should I postpone this PR until then or should we merge it now and then revisit the remaining methods later?

@tomasr8 tomasr8 marked this pull request as ready for review February 7, 2024 12:16
@erlend-aasland
Copy link
Contributor

Thanks for the context!

Anytime :)

So if I understand correctly some of the tp_ methods will become available in AC at some point.

That's the plan, yes.

Should I postpone this PR until then or should we merge it now and then revisit the remaining methods later?

We can adapt the slots later.

Objects/setobject.c Outdated Show resolved Hide resolved
Objects/setobject.c Outdated Show resolved Hide resolved
Objects/setobject.c Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland marked this pull request as draft February 8, 2024 13:53
@erlend-aasland
Copy link
Contributor

@tomasr8, I marked this as a draft while you are working on updating it; mark is as ready for review when you are ready for a new round of reviews.

Objects/setobject.c Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland changed the title gh-112069: Convert set/frozenset methods to Argument Clinic gh-112069: Adapt set/frozenset methods to Argument Clinic Feb 8, 2024
Co-authored-by: Erlend E. Aasland <[email protected]>
@erlend-aasland erlend-aasland marked this pull request as ready for review February 8, 2024 15:19
Co-authored-by: Erlend E. Aasland <[email protected]>
@erlend-aasland
Copy link
Contributor

The help() diff between main and this PR is now:

--- main.txt	2024-02-08 16:23:15
+++ pr.txt	2024-02-08 16:23:38
@@ -77,7 +77,7 @@
  |      Return value^self.
  |
  |  __sizeof__(self, /)
- |      S.__sizeof__() -> size of S in memory, in bytes
+ |      S.__sizeof__() -> size of S in memory, in bytes.
  |
  |  __sub__(self, value, /)
  |      Return self-value.
@@ -114,17 +114,18 @@
  |  intersection_update(self, /, *others)
  |      Update the set, keeping only elements found in it and all others.
  |
- |  isdisjoint(self, object, /)
+ |  isdisjoint(self, other, /)
  |      Return True if two sets have a null intersection.
  |
- |  issubset(self, object, /)
+ |  issubset(self, other, /)
  |      Report whether another set contains this set.
  |
- |  issuperset(self, object, /)
+ |  issuperset(self, other, /)
  |      Report whether this set contains another set.
  |
  |  pop(self, /)
  |      Remove and return an arbitrary set element.
+ |
  |      Raises KeyError if the set is empty.
  |
  |  remove(self, object, /)
@@ -228,7 +229,7 @@
  |      Return value^self.
  |
  |  __sizeof__(self, /)
- |      S.__sizeof__() -> size of S in memory, in bytes
+ |      S.__sizeof__() -> size of S in memory, in bytes.
  |
  |  __sub__(self, value, /)
  |      Return self-value.
@@ -245,13 +246,13 @@
  |  intersection(self, /, *others)
  |      Return a new set with elements common to the set and all others.
  |
- |  isdisjoint(self, object, /)
+ |  isdisjoint(self, other, /)
  |      Return True if two sets have a null intersection.
  |
- |  issubset(self, object, /)
+ |  issubset(self, other, /)
  |      Report whether another set contains this set.
  |
- |  issuperset(self, object, /)
+ |  issuperset(self, other, /)
  |      Report whether this set contains another set.
  |
  |  symmetric_difference(self, other, /)

There's one case of an added punctuation, and a couple of cases where the new docstring now aligns better with the docs (using other iso. object). IMO, this is acceptable.

@erlend-aasland
Copy link
Contributor

BTW, generated clinic code excluded, the diff is now 1 file changed, 259 insertions(+), 157 deletions(-); nice!

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! (I'll land this after you've run make regen-clinic)

@tomasr8
Copy link
Member Author

tomasr8 commented Feb 8, 2024

Thanks a lot for your help!

@erlend-aasland erlend-aasland merged commit ed1a8da into python:main Feb 8, 2024
35 checks passed
@tomasr8 tomasr8 deleted the setobject-argument-clinic branch February 8, 2024 16:50
@colesbury
Copy link
Contributor

Thanks @tomasr8 and @erlend-aasland!

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
skirpichev added a commit to skirpichev/cpython that referenced this pull request Oct 28, 2024
Current patch partially address issue, working in case when all
arguments either positional-only or vararg.

This also converts gcd(), lcm() and hypot() functions of the math module
to the Argument Clinic.  Fix issue python#101123.

Objects/setobject.c and Modules/gcmodule.c adapted.  This fixes slight
performance regression for set methods, introduced by
python#115112:

| Benchmark            | ref    | patch                |
|----------------------|:------:|:--------------------:|
| set().update(s1, s2) | 354 ns | 264 ns: 1.34x faster |

Benchmark code:
```py
import pyperf
s1, s2 = {1}, {2}
runner = pyperf.Runner()
runner.bench_func('set().update(s1, s2)', set().update, s1, s2)
```
erlend-aasland pushed a commit that referenced this pull request Oct 31, 2024
…#126064)

Avoid temporary tuple creation when all arguments either positional-only
or vararg.

Objects/setobject.c and Modules/gcmodule.c adapted. This fixes slight
performance regression for set methods, introduced by gh-115112.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants