GeezerVison is showing github.com — bpo-40077: Convert _elementtree types to heap types by erlend-aasland · Pull Request #23428 · python/cpython
Conversation
erlend-aasland
commented
Nov 20, 2020
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland
changed the title
bpo40077: Convert _elementtree types to heap types
bpo-40077: Convert _elementtree types to heap types
Nov 20, 2020
|
@corona10 would you mind taking a look at this, when I've sorted out the CI failures?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Tests are failing because subinterpreter re-initialized etree types. You either have to add an initialized guard to type initialization or move the types to module state completely.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Tests are failing because subinterpreter re-initialized etree types. You either have to add an initialized guard to type initialization or move the types to module state completely.
Ah, thanks, @tiran! I modified the CREATE_TYPE macro with a simple guard. Moving to module state (and multi-phase init) will also be a large PR, so for the reviewers convenience, I think it's best to keep those changes separate.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 30 days with no activity.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland
commented
Jan 11, 2021
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Ref. leaks are now fixed (commit 9210a36):
$ ./python.exe -m test -R 3:5 test_xml_etree test_xml_etree_c ([bpo-40077](https://bugs.python.org/issue40077)/elementtree)cpython.git
0:00:00 load avg: 1.25 Run tests sequentially
0:00:00 load avg: 1.25 [1/2] test_xml_etree
beginning 8 repetitions
12345678
........
0:00:03 load avg: 1.39 [2/2] test_xml_etree_c
beginning 8 repetitions
12345678
........
== Tests result: SUCCESS ==
All 2 tests OK.
Total duration: 12.4 sec
Tests result: SUCCESS
Rebased onto master & force-pushed; hope that's ok. Ready for review, @tiran.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland
commented
Feb 1, 2021
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I dislike the temporary situation where we create new heap types but don't clear them at exit.
Would it be possible to write a first PR to convert the _elementtree extension to multiphase init? And maybe also prepare the code for having per module types (having a module state)? (same PR or a different PR) For example, create a state which contains pointers to the static types, and ensure that static types are no longer referenced directly.
|
shihai1991 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
I dislike the temporary situation where we create new heap types but don't clear them at exit.
I agree.
Would it be possible to write a first PR to convert the _elementtree extension to multiphase init? And maybe also prepare the code for having per module types (having a module state)? (same PR or a different PR) For example, create a state which contains pointers to the static types, and ensure that static types are no longer referenced directly.
No problem. I'll create a new PR for multiphase init (bpo 1635741). If it's not too large a change, I'll include module state as well.
|
shihai1991 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Would it be possible to write a first PR to convert the _elementtree extension to multiphase init?
I did some fiddling with this today, and the problem is that we've already got a (partial) module state, and it often uses PyState_FindModule (via the ET_STATE_GLOBAL macro) to look up the module, in order to fetch the state. Since we can't use PyState_FindModule with multi-phase init, there are two solutions, as I see it:
- Fetch the state using
_PyType_GetModuleByDef and PyType_GetModule, but that requires converting the static types to heap types => the diff explodes => difficult to review
- (Temporary) convert the existing module state to a global static state, and modify
ET_STATE_GLOBAL and get_elementtree_state to return a pointer to the static state. Pro: small PR, easy to review. Con: A small step backwards, module state wise.
What do you think?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
tiran
Awaiting requested review from tiran
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|