Skip to content

GH-91095: Specialize calls to normal python classes #93221

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

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented May 25, 2022

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

I get the rough idea of this, and to save some time for the other reviewers, here's how it goes:

Frames before a Python class instantiation
__main__ -> NULL

Frames when a Python class is instantiated:
__main__ -> shim -> __init__ -> NULL
(Shim is responsible for replacing None returned by __init__ with self. It contains a special instruction EXIT_INIT_CHECK to verify the None.)

I'm not confident that I can catch any subtle errors with this much code. Let's test this with buildbots.

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit e70711a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 27, 2022
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

The code LGTM. But whether the perf improvement is worth the +300 lines of code I don't feel so sure. You'll have to decide that for yourself.

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 7, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit b75b1aa 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 7, 2022
@@ -84,6 +84,7 @@ def pseudo_op(name, op, real_ops):
def_op('UNARY_NOT', 12)

def_op('UNARY_INVERT', 15)
def_op('EXIT_INIT_CHECK', 16)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be added to the opcodes list in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not. It shouldn't appear in any disassembly or traceback.
Once we have some sort of assembler, I want to replace

    EXIT_INIT_CHECK
    RETURN_VALUE

with

    POP_JUMP_IF_NOT_NONE error
    RETURN_VALUE
error:
    PUSH_NULL
    LOAD_CONST TypeError
    LOAD_CONST "__init__() should not return a value"
    CALL 1
    RAISE_VARARGS 1

So I expect it to removed before 3.12beta.

@markshannon
Copy link
Member Author

There were too many conflicts after the many changes to the interpreter.
Closing in favour of #99331

@markshannon markshannon deleted the specialize-calls-to-normal-python-classes branch September 26, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants