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-91375: Port _asyncio static types to heap types and module state #99122

Merged
merged 27 commits into from
Nov 29, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Nov 5, 2022

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 83d1b54 🤖

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 Nov 5, 2022
@kumaraditya303 kumaraditya303 added topic-asyncio 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 5, 2022
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 for filling in the missing pieces. Looks good!

We should be able to get rid of module_initialized now that we've got multi-phase init.
Also, the module_free(NULL) at the end of module_init() should now be removed.

(BTW, I added two nit suggestions to clean up some style mess introduced by me.)

Modules/_asynciomodule.c Outdated Show resolved Hide resolved
Modules/_asynciomodule.c Outdated Show resolved Hide resolved
@kumaraditya303
Copy link
Contributor Author

We should be able to get rid of module_initialized now that we've got multi-phase init.
Also, the module_free(NULL) at the end of module_init() should now be removed.

Yes, I thought I had done that but apparently not, might have been messed up in rebasing. Fixed now.

@erlend-aasland
Copy link
Contributor

Kumar: Do you think there's a chance of getting one of the other core devs to do a review? (Andrew or maybe Eric?)

@kumaraditya303
Copy link
Contributor Author

Do you think there's a chance of getting one of the other core devs to do a review? (Andrew or maybe Eric?)

Andrew is not active ATM because of the war, Eric is busy with the other stuff so I don't think he will have time for this.

@erlend-aasland
Copy link
Contributor

Andrew is not active ATM because of the war, Eric is busy with the other stuff so I don't think he will have time for this.

Yeah, I feared that.

Do you have a chance to check if there's a performance hit? (I'd be very surprised if there is one.)

@erlend-aasland erlend-aasland self-assigned this Nov 7, 2022
@ericsnowcurrently
Copy link
Member

I'm going to take a look, but it might not be immediately.

@erlend-aasland
Copy link
Contributor

I'm going to take a look, but it might not be immediately.

Great, thanks!

@ericsnowcurrently ericsnowcurrently self-assigned this Nov 21, 2022
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Wow, this goes on and on. I'm not sure if anyone could review it -- I'm sure I can't. I am tempted to just trust Erlend, Kumar, the compiler, and the unit test suite, and merge it.

However, there's a merge conflict that needs to be resolved first...

@kumaraditya303
Copy link
Contributor Author

However, there's a merge conflict that needs to be resolved first...

Fixed merge conflict.

@erlend-aasland
Copy link
Contributor

Most of the changes are trivial stuff like passing the module state around to various support and helper functions.

@kumaraditya303
Copy link
Contributor Author

I am thinking of merging this by next week so that it gets in the next alpha. @ericsnowcurrently If you are not able to review by then, I can do a follow up PR for your review if that's okay with you.

@kumaraditya303 kumaraditya303 merged commit 4cfc1b8 into python:main Nov 29, 2022
@kumaraditya303 kumaraditya303 deleted the isolate-asyncio branch November 29, 2022 10:07
@kumaraditya303
Copy link
Contributor Author

Merged, thanks @erlend-aasland

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.

5 participants