Skip to content

GH-91375: Port _asyncio static types to heap types and module state#99122

Merged
kumaraditya303 merged 27 commits into
python:mainfrom
kumaraditya303:isolate-asyncio
Nov 29, 2022
Merged

GH-91375: Port _asyncio static types to heap types and module state#99122
kumaraditya303 merged 27 commits into
python:mainfrom
kumaraditya303:isolate-asyncio

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Nov 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

erlend-aasland and others added 22 commits November 5, 2022 12:51
@erlend-aasland @kumaraditya303
Establish global state stub
12942bc
@kumaraditya303
Convert FutureIterType to heap type
378330c
@erlend-aasland @kumaraditya303
Convert PyRunningLoopHolder_Type to heap type
4db4e7f
@erlend-aasland @kumaraditya303
Convert TaskStepMethWrapper_Type to heap type
319df11
@erlend-aasland @kumaraditya303
Convert TaskType to heap type
46793d0
@erlend-aasland @kumaraditya303
Convert FutureType to heap type
1109b16
@erlend-aasland @kumaraditya303
Add types to state
d382cc1
@erlend-aasland @kumaraditya303
Put imports into module state
b7c1952
@erlend-aasland @kumaraditya303
Put remaining global into module struct
a147e87
@erlend-aasland @kumaraditya303
Pass state as argument to ENSURE_FUTURE_ALIVE
3938201
@erlend-aasland @kumaraditya303
Add helpers to get asyncio state from defining class and module def
3b4baba
@erlend-aasland @kumaraditya303
Prepare for module state, batch 1
06e8216
@erlend-aasland @kumaraditya303
Prepare for module state, batch 2
8a7ac68
@erlend-aasland @kumaraditya303
Prepare for module state, batch 3
2e51638
@erlend-aasland @kumaraditya303
Prepare for module state, batch 4
7ebf1e9
@erlend-aasland @kumaraditya303
Convert to module state
54d4900
@erlend-aasland @kumaraditya303
WIP
7772c82
@kumaraditya303
wip
a24f12c
@kumaraditya303
make it work!
bf0cb43
@kumaraditya303
skip test
9bd1a56
@kumaraditya303
buildbottest
d539152
@kumaraditya303
Revert "buildbottest"
83d1b54 
This reverts commit d539152.
@bedevere-bot bedevere-bot mentioned this pull request Nov 5, 2022
Closed
@kumaraditya303 kumaraditya303 added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 5, 2022
@bedevere-bot

bedevere-bot commented Nov 5, 2022

Copy link
Copy Markdown

🤖 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
erlend-aasland
erlend-aasland reviewed Nov 6, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.)

Comment thread Modules/_asynciomodule.c Outdated
Comment thread Modules/_asynciomodule.c Outdated
@kumaraditya303

kumaraditya303 commented Nov 7, 2022

Copy link
Copy Markdown
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

erlend-aasland commented Nov 7, 2022

Copy link
Copy Markdown
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

kumaraditya303 commented Nov 7, 2022

Copy link
Copy Markdown
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

erlend-aasland commented Nov 7, 2022

Copy link
Copy Markdown
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

ericsnowcurrently commented Nov 7, 2022

Copy link
Copy Markdown
Member

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

@erlend-aasland

erlend-aasland commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

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

Great, thanks!

@ericsnowcurrently ericsnowcurrently self-assigned this Nov 21, 2022
gvanrossum
gvanrossum reviewed Nov 22, 2022
View reviewed changes

@gvanrossum gvanrossum 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.

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

kumaraditya303 commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

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

Fixed merge conflict.

@erlend-aasland

erlend-aasland commented Nov 22, 2022

Copy link
Copy Markdown
Contributor

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

@kumaraditya303

kumaraditya303 commented Nov 26, 2022

Copy link
Copy Markdown
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

kumaraditya303 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

Merged, thanks @erlend-aasland

@kumaraditya303 kumaraditya303 mentioned this pull request Nov 29, 2022
Closed
@kristjanvalur kristjanvalur mentioned this pull request Nov 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

@ericsnowcurrently ericsnowcurrently

@erlend-aasland erlend-aasland

Labels

skip news topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kumaraditya303 @bedevere-bot @erlend-aasland @ericsnowcurrently @gvanrossum