FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-40077: Convert _elementtree types to heap types by erlend-aasland · Pull Request #23428 · python/cpython · GitHub

/ cpython Public

bpo-40077: Convert _elementtree types to heap types - #23428

Closed
erlend-aasland wants to merge 14 commits into
python:masterfrom
erlend-aasland:bpo-40077/elementtree
Closed

bpo-40077: Convert _elementtree types to heap types#23428
erlend-aasland wants to merge 14 commits into
python:masterfrom
erlend-aasland:bpo-40077/elementtree

Conversation

erlend-aasland commented Nov 20, 2020
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

erlend-aasland changed the title bpo40077: Convert _elementtree types to heap types bpo-40077: Convert _elementtree types to heap types Nov 20, 2020

erlend-aasland commented Nov 20, 2020
edited
Loading

Copy link
Copy Markdown
Contributor Author

@corona10 would you mind taking a look at this, when I've sorted out the CI failures?

erlend-aasland marked this pull request as draft November 20, 2020 20:31
Comment thread Modules/_elementtree.c Outdated

tiran commented Nov 21, 2020

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.

Comment thread Modules/_elementtree.c Outdated

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 24, 2020

erlend-aasland commented Jan 11, 2021
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor Author

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.

erlend-aasland marked this pull request as ready for review January 11, 2021 09:10
erlend-aasland requested a review from tiran January 11, 2021 09:14
github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jan 12, 2021

erlend-aasland commented Feb 1, 2021
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor Author

FYI, all of these types follow the GC protocol, as described in bpo-42972.

(cc. @vstinner @shihai1991)

vstinner commented Feb 1, 2021

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.

erlend-aasland marked this pull request as draft February 1, 2021 22:23

Copy link
Copy Markdown
Contributor Author

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:

  1. 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
  2. (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?

Copy link
Copy Markdown
Contributor Author

FYI, PEP-687 was just accepted.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL