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

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

Merged
merged 4 commits into from
Mar 6, 2022

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Mar 4, 2022

Objects/typeobject.c Outdated Show resolved Hide resolved
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

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>

Objects/typeobject.c Show resolved Hide resolved
@bedevere-bot
Copy link

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

@corona10
Copy link
Member

corona10 commented Mar 5, 2022

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

Co-Authored-By: Dong-hee Na <[email protected]>
@Fidget-Spinner
Copy link
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
Copy link
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
Copy link
Member

corona10 commented Mar 5, 2022

@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
Copy link
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
Copy link
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
Copy link
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.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM
Feel free to merge this PR :)

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

@corona10 thanks for the reviews!

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