bpo-1635741: port _elementtree to multi-phase init (PEP 489) by koubaa · Pull Request #23535 · python/cpython · GitHub

https://bugs.python.org/issue1635741

@vstinner @shihai1991 @corona10 please review

The patch looks like it was a lot of work. Thanks for investing your time to port _elementtree.

I'm a bit worried that the module state getter is going to affect performance negatively. The code has to use the slow _PyType_GetModuleByDef() API. In my patch for the _ssl module I added a module state pointer to instance structs. For example you could add state to typedef struct ElementObject and then reference the state with self->state. This would reduce the amount of get state calls and simplify the code.

What do you think? Is this a safe apprach?

The qualifier object is not self-explanatory. Every Python object is an object. This includes module objects and type objects. How about:

Suggested change

      
            get_elementtree_module_state_by_object(void *object)
          
            get_elementtree_module_state_by_instance(PyObject *self)

How about expend get_element_module_state() to receive self object or type object?

@tiran I tried using PyObject * and that gives a lot of compiler warnings (see somem below):

C:\Dev\OSS\cpython\cpython\Modules_elementtree.c(3749,74): warning C4133: 'function': incompatible types - from 'XMLParserObject *' to 'PyObject *' [C:\Dev\OSS\cpython\cpython\PCbuild_elementtree.vcxproj]
C:\Dev\OSS\cpython\cpython\Modules\clinic_elementtree.c.h(26,52): warning C4133: 'function': incompatible types - from 'ElementObject *' to 'PyObject *' [C:\Dev\OSS\cpython\cpython\PCbuild_elementtree.vcxproj]

@shihai1991 do you mean two methods, one for each?

yes. just my personal suggestion :)

Is this correct? AFAIK the argument clinic code has only access to type, not to self.

@tiran This is where I got from trial and error, see the implementation of _elementtree_Element_append in Modules/clinic/_elementtree.c.h

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

The patch looks like it was a lot of work. Thanks for investing your time to port _elementtree.

I'm a bit worried that the module state getter is going to affect performance negatively. The code has to use the slow _PyType_GetModuleByDef() API. In my patch for the _ssl module I added a module state pointer to instance structs. For example you could add state to typedef struct ElementObject and then reference the state with self->state. This would reduce the amount of get state calls and simplify the code.

What do you think? Is this a safe apprach?

@tiran I'm not sure if its safe but I understand it in principle. I don't know exactly the lifetime of instance structs when pickling. @vstinner thoughts on that approach?

Is there a benchmark I could try to measure the performance impact? _PyType_GetModuleByDef seems to only be slow if there is significant subclassing so I could try to whip up an example that tries to overdo that.

This PR introduces a ref. leak (looks similar to the ref. leak I had in PR #23428):

bash$ ./python.exe -m test -F -r -j1 -R 3:5 test_xml_etree_c
Using random seed 1742818
0:00:00 load avg: 1.54 Run tests in parallel using 1 child processes
0:00:14 load avg: 1.50 [  1/1] test_xml_etree_c failed
beginning 8 repetitions
12345678
........
test_xml_etree_c leaked [147, 147, 147, 147, 147] references, sum=735
test_xml_etree_c leaked [59, 59, 59, 59, 59] memory blocks, sum=295

== Tests result: FAILURE ==

1 test failed:
    test_xml_etree_c

Total duration: 14.8 sec
Tests result: FAILURE

@erlend-aasland oof sorry, I didn't realize you were working on this module too. Any tips to find the cause of the ref leak or which reference is leaking? I'm not too familiar with the diagnostics & telemetry available.

oof sorry, I didn't realize you were working on this module too.

No problem, you're fine! Feel free to take a look at my PR. I haven't been able to devote any free time to this the last month or so, anyway.

Any tips to find the cause of the ref leak or which reference is leaking? I'm not too familiar with the diagnostics & telemetry available.

Me neither. It kinda stopped when I encountered the ref. leak. I use python.exe -m test -F -r -j1 -R 3:5 test_<mod> to check for reference/memory leaks. I've tried playing with some of the -X options as well, but I haven't found a good way to debug ref leaks yet. I'll keep an eye on this PR to see what you find out :)

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

https://bugs.python.org/issue1635741 is closed. What is that status of this PR?

The change is still relevant, but should use a new issue number.

Moreover, the SC asked to put the conversion of static types to heap types on hold. @encukou and @erlend-aasland wrote https://peps.python.org/pep-0687/ which may unblock the situation but it's still a draft.

FYI, PEP-687 was just accepted.

Superseded by gh-101285

post _elementtree to multi-phase init

d5b1fb6

the-knights-who-say-ni added the CLA signed label Nov 28, 2020

bedevere-bot added the awaiting review label Nov 28, 2020

tiran requested changes Nov 28, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 28, 2020

visit/clear

8f2355a

github-actions Bot added the stale Stale PR or inactive for long period of time. label Jan 11, 2021

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 1, 2022

kylotan mannequin mentioned this pull request Sep 19, 2022

Py_Finalize() doesn't clear all Python objects at exit #44470

Closed

erlend-aasland closed this Mar 14, 2023

-              #define ET_STATE_GLOBAL \
-                  ((elementtreestate *) PyModule_GetState(PyState_FindModule(&elementtreemodule)))
+              static inline elementtreestate *
+              get_elementtree_module_state_by_object(void *object)

	get_elementtree_module_state_by_object(void *object)
	get_elementtree_module_state_by_instance(PyObject *self)

+              class _elementtree.Element "ElementObject *" "get_elementtree_module_state_by_object(self)->element_type"
+              class _elementtree.TreeBuilder "TreeBuilderObject *" "get_elementtree_module_state_by_object(self)->tree_builder_type"
+              class _elementtree.XMLParser "XMLParserObject *" "get_elementtree_module_state_by_object(self)->xml_parser_type"

bpo-1635741: port _elementtree to multi-phase init (PEP 489) by koubaa · Pull Request #23535 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-1635741: port _elementtree to multi-phase init (PEP 489) - #23535

bpo-1635741: port _elementtree to multi-phase init (PEP 489)#23535koubaa wants to merge 2 commits intopython:mainpython/cpython:mainfrom koubaa:bpo-1635741-elementtreekoubaa/cpython:bpo-1635741-elementtreeCopy head branch name to clipboard

Conversation

koubaa commented Nov 28, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koubaa commented Nov 28, 2020

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

tiran Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

koubaa Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

koubaa Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

shihai1991 Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tiran Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

koubaa Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 28, 2020

Uh oh!

koubaa commented Nov 28, 2020

Uh oh!

erlend-aasland commented Dec 1, 2020

Uh oh!

koubaa commented Dec 11, 2020

Uh oh!

erlend-aasland commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 11, 2021

Uh oh!

iritkatriel commented Apr 11, 2022

Uh oh!

vstinner commented Apr 19, 2022

Uh oh!

erlend-aasland commented Jun 27, 2022

Uh oh!

erlend-aasland commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bpo-1635741: port _elementtree to multi-phase init (PEP 489)#23535
koubaa wants to merge 2 commits into
python:mainfrom
koubaa:bpo-1635741-elementtree

koubaa commented Nov 28, 2020 •
edited by bedevere-bot
Loading

erlend-aasland commented Dec 11, 2020 •
edited
Loading