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

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

Closed

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Nov 20, 2020

@erlend-aasland 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
Copy link
Contributor Author

erlend-aasland commented Nov 20, 2020

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

@erlend-aasland erlend-aasland marked this pull request as draft November 20, 2020 20:31
Modules/_elementtree.c Outdated Show resolved Hide resolved
@tiran
Copy link
Member

tiran commented Nov 21, 2020

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.

@erlend-aasland
Copy link
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.

Modules/_elementtree.c Outdated Show resolved Hide resolved
@github-actions
Copy link

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

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 24, 2020
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 11, 2021

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 erlend-aasland marked this pull request as ready for review January 11, 2021 09:10
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Jan 12, 2021
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 1, 2021

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

(cc. @vstinner @shihai1991)

@vstinner
Copy link
Member

vstinner commented Feb 1, 2021

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.

@erlend-aasland
Copy link
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 erlend-aasland marked this pull request as draft February 1, 2021 22:23
@erlend-aasland
Copy link
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?

@erlend-aasland
Copy link
Contributor Author

FYI, PEP-687 was just accepted.

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