FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-46921: Vectorcall support for `super()` by Fidget-Spinner · Pull Request #31687 · python/cpython · GitHub

/ cpython Public

bpo-46921: Vectorcall support for super() - #31687

Merged
Fidget-Spinner merged 4 commits into
python:mainfrom
Fidget-Spinner:super_vectorcall
Mar 6, 2022
ConversationCommits (4)ChecksFiles changed
Merged

bpo-46921: Vectorcall support for super()#31687
Fidget-Spinner merged 4 commits into
python:mainfrom
Fidget-Spinner:super_vectorcall

Conversation

Fidget-Spinner commented Mar 4, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

corona10 reviewed Mar 4, 2022
View reviewed changes
Comment thread Objects/typeobject.c Outdated
Use _PyArg_NoKwnames instead
896eb36 
Co-Authored-By: Dong-hee Na <donghee.na@python.org>
corona10 reviewed Mar 5, 2022
View reviewed changes
Comment thread Objects/typeobject.c
corona10 requested changes Mar 5, 2022
View reviewed changes

corona10 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The current PR does not check the number of arguments.
Please add the unit test for this also :)

AS-IS

>>> super(int, int, int)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: super() takes at most 2 arguments (3 given)

PR

>>> super(int, int, int)
<super: <class 'int'>, NULL>

Comment thread Objects/typeobject.c

bedevere-bot commented Mar 5, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

corona10 commented Mar 5, 2022

Copy link
Copy Markdown
Member

See my benchmark: https://bugs.python.org/msg414577
It looks like worth applying the vector call :)

Address code review
0bbe8d6 
Co-Authored-By: Dong-hee Na <donghee.na@python.org>

Fidget-Spinner commented Mar 5, 2022

Copy link
Copy Markdown
Member Author

@corona10 thanks for taking the time to benchmark this and for the extremely useful suggestions too. I forgot all the cool argument checking helpers we have since I'm a little rusty.

If you're interested, there's the monster GH-30992 too where I measured >2X speedup. But it's very complex and I don't have high hopes for it being merged.

sweeneyde commented Mar 5, 2022

Copy link
Copy Markdown
Member

I wonder how much a free list would help? I'd bet super objects typically have short lifetimes and not many are alive at once.

corona10 commented Mar 5, 2022

Copy link
Copy Markdown
Member

@Fidget-Spinner

Please add the unit test for this also :)

As I wrote, please add the test for checking TypeError when the given number of arguments are greater equal than 3 :)

Fidget-Spinner commented Mar 5, 2022

Copy link
Copy Markdown
Member Author

@Fidget-Spinner

Please add the unit test for this also :)

As I wrote, please add the test for checking TypeError when the given number of arguments are greater equal than 3 :)

🤦 my bad, I missed that. Thanks again.

I added one more test since I noticed it wasn't covered in the test suite.

Fidget-Spinner commented Mar 5, 2022

Copy link
Copy Markdown
Member Author

I wonder how much a free list would help? I'd bet super objects typically have short lifetimes and not many are alive at once.

There will probably be some improvement versus relying on CPython's obmalloc "free list". My final goal is to not need any super object at all though :). BTW, are you able to guesstimate how much more complexity we need for a super free list? If it isn't too complex, it's likely more worth it than my cached superinstruction overkill implementation.

sweeneyde commented Mar 5, 2022

Copy link
Copy Markdown
Member

I wonder how much a free list would help? I'd bet super objects typically have short lifetimes and not many are alive at once.

There will probably be some improvement versus relying on CPython's obmalloc "free list". My final goal is to not need any super object at all though :). BTW, are you able to guesstimate how much more complexity we need for a super free list? If it isn't too complex, it's likely more worth it than my cached superinstruction overkill implementation.

I think it's (relatively) straightforward, see floatobject.c for an example. The actual free list gets attached to _PyInterpreterState there.

corona10 approved these changes Mar 6, 2022
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM
Feel free to merge this PR :)

Fidget-Spinner merged commit 602024e into python:main Mar 6, 2022
Fidget-Spinner deleted the super_vectorcall branch March 6, 2022 06:21

Fidget-Spinner commented Mar 6, 2022

Copy link
Copy Markdown
Member Author

@corona10 thanks for the reviews!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

corona10 corona10 approved these changes
markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL